fix: getstorage talkaction

[arrudaqs]

Description

Fix the getstorage talkaction used by server admins, which helps imensely with quest debugging.

Behaviour

Actual

Whenever the talkaction /getstorage Player, storagename is called, the server receives a string (storage name) or a number (storage key) as parameters, the key works fine, but the name should not be a string, it should be converted to the actual global storage variable type.

Expected

Given the admin prepares the /getstorage talkaction to be sent;
When the admin sends the storage parameter as a string e.g (/getstorage GOD, Storage.Quest.U8_0.TheIceIslands.Questline);
Then the /getstorage talkaction accepts the string parameter correctly.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested

• Tested the talkaction before my fix, all string typed parameters returned -1 (e.g Storage.Quest.U8_0.BarbarianTest.Questline returned -1)
• Tested the talkaction after my fix, all string typed parameters returned their actual value (e.g Storage.Quest.U8_0.BarbarianTest.Questline returned 8)

Test Configuration:

• Server Version: 3.1.2
• Client: 13.40
• Operating System: Windows 10

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I checked the PR checks reports

[arrudaqs]

@dudantas please have a look 🤭

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[dudantas]

Using loadstring here introduces potential security and performance risks, especially if the input (split[2]) is user-provided or not sanitized. A safer and more efficient approach would be to directly attempt converting the value to a number or treating it as a string key if the conversion fails. This would eliminate the need for dynamically executed code and improve both the safety and clarity of the implementation.

Example alternative:

local storageKey = tonumber(split[2]) or split[2]
local storageValue = target:getStorageValue(storageKey)
self:sendTextMessage(MESSAGE_EVENT_ADVANCE, "The storage with id: " .. split[1] .. " is: " .. storageValue .. ".")

This ensures the same functionality without the risks associated with loadstring.

[dudantas]

Please, address what I said in the previous comment, as it is very critical.

[kaleohanopahala]

Nah...
This changes seems unsafe to me.
Could you test this?

function Player.getStorageValueTalkaction(self, param)
    -- Sanity check for parameters
    if not HasValidTalkActionParams(self, param, "Usage: /getstorage <playername>, <storage key or name>") then
        return true
    end

    local split = param:split(",")
    if not split[2] then
        self:sendCancelMessage("Insufficient parameters.")
        return true
    end

    local target = Player(split[1]:trim())
    if not target then
        self:sendCancelMessage("A player with that name is not online.")
        return true
    end

    -- Storage key Validation
    local storageKey = tonumber(split[2]) or split[2]:trim()
    if not storageKey then
        self:sendCancelMessage("Invalid storage key or name.")
        return true
    end

    -- Get the storage key
    local storageValue = target:getStorageValue(storageKey)
    if storageValue == nil then
        self:sendTextMessage(MESSAGE_EVENT_ADVANCE, "The storage with id: " .. split[2] .. " does not exist or is not set for player " .. target:getName() .. ".")
    else
        self:sendTextMessage(MESSAGE_EVENT_ADVANCE, "The storage with id: " .. split[2] .. " from player " .. target:getName() .. " is: " .. storageValue .. ".")
    end

    return true
end

local storageGet = TalkAction("/getstorage")

function storageGet.onSay(player, words, param)
    return player:getStorageValueTalkaction(param)
end

storageGet:separator(" ")
storageGet:groupType("gamemaster")
storageGet:register()