1.16.5 Is InventoryCloseEvent safe from player packet manipulation?

Discussion in 'Spigot Plugin Development' started by ToasterlordVEVO, Jun 16, 2021.

  1. I'm not sure if this is the right thread to be posting this on, but it seemed close enough. Apologies as I'm new to the whole thing.

    But I'm working on a plugin and normally I'd have some sort of complex system that checks to see if the inventory is valid through checking if it matches with another one (it only works for GUIs meant for clickable items though). No time to explain it though since it's both very verbose and incredibly irrelevant.

    However, unfortunately, I'm unable to do that, as the inventory I found this "issue" with is meant to have items add or removed by the player, which is obviously unpredictable. So, the only way to truly check if the inventory was valid (if they were using the correct inventory for the close event) was through checking the title.

    This brought a problem, as checking the title alone could allow for the player to cheat the system by placing down a chest or something and renaming it so the fake inventory contains a matching title to the real one. This was an easy fix though and was through the use of a list to keep track on which players had their inventories open.

    But then, I soon afterward realized it brought another problem. I'm not sure if this is accurate, but reading online about it, I noticed that the inventory close event only fires when the player sends a packet to the server. And, since the player is the one essentially controlling the event, it leaves a big question:

    What would happen if the player somehow manipulated their packets and hid the fact that they closed the inventory, then when they open the fake inventory and apply the changes, it will act as if they're still on the previous one?

    I know this post is rambly, and to be honest, in my case it isn't much of an issue to worry about (since all it'll do is override the inventory data with stuff that they owned to begin with). But it's still a concerning (theoretical) security hole I noticed that could potentially be an issue for some more complicated and vulnerable projects in the future.

    Unless of course there's some other things I didn't read about. Apologies, again, I'm still rather new and basically started 2 days ago lol.
     
  2. I think you can use inventory libs like SmartInventory by Minuskube, this prevent duplication, ect... And implements lot of methods very useful to create an inventory. You just can create inventory with some lines
     
  3. And, if inventory is closed for the server, all actions he will do in inventory will be cancelled
     
  4. Just make a list of the open inventories (or a map) and when the close inventory event is called check if the view's top inventory is in it - this way you always know if it's a "correct" inventory and don't have to rely on volatile stuff like the inventory's name.
     
    • Like Like x 1