Potential security & code quality issue

Krunegan / inv_manager

To use it, you must have the "inv_manager" privilege and type the command "/inv_edit <playername>" A formspec will be displayed on the screen showing both the player's main inventory. The formspec is automatically updated, allowing moderators to view and edit player inventory changes in real time.

Other

1 stars 0 forks source link

Potential security & code quality issue #4

Open appgurueu opened 1 year ago

appgurueu commented 1 year ago

Your on_player_receive_fields_handler does not check that the submitting player (still) has the necessary privilege. Cheaters can submit any form at any time, so this is a problem. In practice mod security will check form names against form submissions, so it probably won't be a problem (except for recently demoted admins, which is very much an edge case). But to be on the safe side you should do priv validation at the time of form submission (in on receive fields).
You're registering new callbacks every time you show a formspec. The callbacks will fill up; this is a memory leak and may cause bugs.

Krunegan commented 1 year ago

The player will now be verified to have the necessary privileges before taking any action.

About the callbacks every time the formspec is shown, I'm not sure how to fix it...

appgurueu commented 1 year ago

Well, you want a callback / closure-style handler approach. That is very reasonable; it's one of the two main features of fslib. If you were to implement something similar yourself, you could just have a handler which deals with all yourmod:yourformname events and dispatches based on the player name to a table of closures. You'd have to make sure to delete entries in this table when formspecs are closed or players leave to not leak memory.

Krunegan commented 3 months ago

I have reworked the mod, but I still haven't fixed the callback

appgurueu commented 3 months ago

Great :)

The "memory leak" is probably not very high priority.