ESOUI - attempt to access a private function 'UseItem' from insecure code

ESOUI (https://www.esoui.com/forums/index.php)

- General Authoring Discussion (https://www.esoui.com/forums/forumdisplay.php?f=174)

- - attempt to access a private function 'UseItem' from insecure code (https://www.esoui.com/forums/showthread.php?t=6617)

sirinsidiator

11/06/16 11:03 AM

attempt to access a private function 'UseItem' from insecure code

I finally took the time to investigate the old "attempt to access a private function 'UseItem' from insecure code" error.
The issue can be reliably reproduced when opening a container via the keybind, closing it and then reopening it again without moving the mouse away from the item while AdvancedFilters is active.
I picked the code apart and disabled it part by part, line by line and finally found that the issue happens when PLAYER_INVENTORY:UpdateList is called from an addon.
More precisly it happens when self:ApplySort is called inside that method. My guess is that ZO_ScrollList_Commit breaks the secure context for the keybind when it autoselects the inventory item entry.

Maybe ZOS could add a new method RequestPlayerInventoryUpdate or something which allows us to call this from a secure context.
Until a better solution is found, I suggest addon authors use the following method instead of directly calling PLAYER_INVENTORY:UpdateList.

Lua Code:

local function SafeUpdateList(object, ...)
    local isMouseVisible = SCENE_MANAGER:IsInUIMode()
 
    --if the mouse is visible, cycle its visibility to refresh the integrity of the control beneath it
    if isMouseVisible then HideMouse() end
 
    object:UpdateList(...)
 
    if isMouseVisible then ShowMouse() end
end
 
-- example usage for different inventories:
SafeUpdateList(PLAYER_INVENTORY, INVENTORY_BACKPACK)
SafeUpdateList(STORE_WINDOW)

Baertram

11/06/16 11:14 AM

Thanks for your time and the workaround.
Will implement this function into my inventory updating addons.

If I understand your workaround function right it won't update the inventory if the mouse is currently over any control of an item in the inventory?
I guess this would break addons like FCOItemSaver as you right click an item to show the context menu, choose an entry and afterwards the inventory will be updated. The mouse is above any inventory item after the context menu closes... And thus the inventory update won't happen anymore.

I'll test this if the control is the context menu, as the update fires, or the inventory line again.
And if you mark an item via keybinding it will definately be the inventory item control..

sirinsidiator

11/06/16 01:02 PM

You are right. It won't help in situations where you do need the mouse over an item.
Right now I do not have a better idea how to solve this, but I will think a bit more about it during the week.

merlight

11/06/16 07:32 PM

I thought this was generally known already. Remember we've discussed this in so many threads... and then wondered why it's so hard to find more info. Apparently there's no sane way to search addon comments.

Edit: and also no working way to link to a specific comment. You have to switch to the #comments tab by hand and then paste the link to the address bar :rolleyes:

Randactyl

11/06/16 08:54 PM

yes, merlight that is the current known workaround.

It also does not break if you simply double click to use the item rather than pressing the primary keybind (default: 'E').

Siri's solution pretty much only fixes the "rapid writ container opening" case and sets up an expected result that libFilters and addons which leverage it or their own inventory updating methods will no longer have issues. At least for libFilters, this now produces unexpected functionality when an addon keybind is used on an inventory slot.

Baertram has been working on a solution involving passing the "initiating control" as an optional argument to the updater and still updating the list if the initiator matches the current mouse over control (thereby negating siri's fix for another specific situation).

I'm still thinking about these proposed changes. Right now I'm somewhere around thinking it's adding unneeded complexity for us to a long standing API issue with two solid and easy user workarounds (moving cursor or double clicking to use).

There is a high possibility that addons implementing libFilters and a keybind can accomplish their tasks without a list update, as Siri suggested this morning. This would be ideal since then the library could implement the safe list update and addons could use better practice for their tasks.

I'm going to really dive into this on Tuesday, as I have a good deal of schoolwork to do tonight and tomorrow. In the meantime, please correct any of my assumptions and further work if it still tickles your fancy :)

sirinsidiator

11/07/16 06:44 AM

Quote:

Originally Posted by merlight (Post 28822)

I have to admit I never read the comments for that addon. The code was terrible enough. :p
Jokes aside, it looks like Randactyl has found an even better solution:

Lua Code:

PLAYER_INVENTORY:UpdateList(INVENTORY_BACKPACK)
HideMouse()
ShowMouse()

I would suggest to use this in the following way:

Lua Code:

local function SafePlayerInventoryUpdateList(inventoryType)
    PLAYER_INVENTORY:UpdateList(inventoryType)
    if SCENE_MANAGER:IsInUIMode() then
        HideMouse()
        ShowMouse()
    end
end

Btw. you should come and visit our gitter chat sometime. We discuss most problems over there and it would be awesome to have your input from time to time. ;)

ZOS_ChipHilseberg

11/07/16 09:04 AM

I'm not immediately clear on how refreshing the list triggers a UseItem. What is that code path?

sirinsidiator

11/07/16 10:11 AM

Quote:

Originally Posted by ZOS_ChipHilseberg (Post 28827)

I'm not immediately clear on how refreshing the list triggers a UseItem. What is that code path?

The refresh itself does not trigger UseItem, but when the user does not move the mouse and then presses the keybind for the primary action the error gets triggered because the secure context of the highlighted item has been violated due to the refresh from an addon.

This happens quite often when I try to open several crafting writ containers one after another. I usually press E to open them and R to take the content and won't move the mouse inbetween.

Randactyl

11/07/16 11:07 AM

@Chip

The surefire way to reproduce this is to

1. Install Advanced Filters version 1.3.3.0
2. Get a container in your inventory (writ box, PTS template item distribution, etc.)
3. Hover over the container and do not move the mouse at all
4. press 'E' or double-click to open the container inventory
5. Press alt to close the container inventory
6. Press 'E', not double-click, to open the container inventory again
7. Error displays

Between steps 5 and 6 is when an addon call to PLAYER_INVENTORY:UpdateList is triggered with the cursor over an item slot (thereby causing that control to become untrusted somehow)

My fix cycles the mouse's state after the inventory update. This apparently allows the moused over control's integrity to be re-verified.

merlight

11/07/16 03:52 PM

Disclaimer: I haven't tried running ESO since TG PTS, and am pulling stuff off a very unreliable medium.

Quote:

Originally Posted by ZOS_ChipHilseberg (Post 28827)

I'm not immediately clear on how refreshing the list triggers a UseItem. What is that code path?

AFAIR the culprit was that an inventory refresh triggers OnMouseExit+Enter on the item slot under mouse cursor.
Here: https://github.com/esoui/esoui/blob/...ates.lua#L1209
Keybinds are registered in OnMouseEnter, so if the refresh is invoked from an addon, they become "insecure". I think it should be possible to trigger this even without addons. Try

Lua Code:

/script ZO_ScrollList_Commit(ZO_PlayerInventoryBackpack)

while hovering over a container/consumable and then use it.

Quote:

Originally Posted by sirinsidiator (Post 28826)

I have to admit I never read the comments for that addon. The code was terrible enough. :p
Jokes aside, it looks like Randactyl has found an even better solution:

Lua Code:

PLAYER_INVENTORY:UpdateList(INVENTORY_BACKPACK)
HideMouse()
ShowMouse()

I'd HideMouse before, UpdateList, ShowMouse after. That'll avoid superfluous OnMouseExit+Enter in UpdateList.

Quote:

Originally Posted by Randactyl (Post 28829)

Between steps 5 and 6 is when an addon call to PLAYER_INVENTORY:UpdateList is triggered with the cursor over an item slot (thereby causing that control to become untrusted somehow)

It's not relevant, but I wonder why UpdateList is called from addon context when a container is opened/closed.

sirinsidiator

11/07/16 04:03 PM

Quote:

Originally Posted by merlight (Post 28835)

I'd HideMouse before, UpdateList, ShowMouse after. That'll avoid superfluous OnMouseExit+Enter in UpdateList.

That might be a nice optimization.

Quote:

Originally Posted by merlight (Post 28835)

It's not relevant, but I wonder why UpdateList is called from addon context when a container is opened/closed.

It's because the filters are updated on scene show, which happens when you close a container.

ZOS_ChipHilseberg

11/07/16 04:27 PM

Ah yeah, I see the problem. The closure we're passing in to the menu system is tainted after the addon does the commit. We could fix it by passing data in to the closure call instead of closing over it so we don't regenerate it every time we recreate the actions list. This would require modifying the menu system to accept some params to feed into the callback. I can put it on the list, but it's not a super quick fix.

sirinsidiator

11/07/16 04:41 PM

Sounds good. If you ever get to change the menu system, maybe you could also make it a bit easier for addons to change the behavior of an existing entry.

In the current system this is the simplest way I could come up with to change the primary action of the craft bag item actions for AGS' direct selling feature :D

Lua Code:

local oAddSlotAction = ZO_InventorySlotActions.AddSlotAction
 
local currentInventorySlot
ZO_PreHook("ZO_InventorySlot_DiscoverSlotActionsFromActionList", function(inventorySlot, slotActions) currentInventorySlot = inventorySlot end)
 
-- prepare AddSlotAction in order to redirect the action
ZO_InventorySlotActions.AddSlotAction = function(slotActions, actionStringId, actionCallback, actionType, visibilityFunction, options)
    if(actionStringId == SI_ITEM_ACTION_REMOVE_ITEMS_FROM_CRAFT_BAG and TRADING_HOUSE:IsAtTradingHouse()) then
        if(self:IsItemAlreadyBeingPosted(currentInventorySlot)) then
            actionStringId = SI_TRADING_HOUSE_REMOVE_PENDING_POST
            actionCallback = function()
                self:ClearPendingItem()
                ZO_InventorySlot_OnMouseEnter(currentInventorySlot)
            end
        else
            actionStringId = SI_TRADING_HOUSE_ADD_ITEM_TO_LISTING
            actionCallback = function() TryInitiatingItemPost(currentInventorySlot) end
        end
        actionType = "primary"
    end
    oAddSlotAction(slotActions, actionStringId, actionCallback, actionType, visibilityFunction, options)
end

All times are GMT -6. The time now is 04:55 AM.