Major Vulnerability
blablubbabcDEV opened this issue ยท 3 comments
Migrated from: https://dev.bukkit.org/projects/shopkeepers/issues/430
Originally posted by mel_instagibson (Jul 13, 2016):
What steps will reproduce the problem?
- create a shop and sell it to a player
- set protect chest to true
- now wait until the shopkeepers refresh (when they teleport in midair) and try to access another players shopkeeper chest, it will work and you can rob himSo as i said above other people are able to open chests that belong to other shopkeeper owners when the "refresh" happens. they can just click the chest in that very second the villagers teleport midair and he can take all the items out.this makes the plugin entirely useless for my needs :/
Originally commented by blablubbabc (Jul 13, 2016):
I haven't tested it yet, but I assume that this might be caused by the refreshing which happens whenever the world gets saved by the minecraft server:
I unload/deactivate all shopkeepers temporarily while it is saving the world, so that the shopkeeper entities don't get stored in the chunk data, fixing some entity duplication issue which could otherwise occur occasionally.
Currently it iterates over all active shopkeepers when checking if a chest is protected.
If this is the issue I can see at least 4 possible solutions:
Either checking all (even 'currently inactive / unloaded') shopkeepers when testing if a chest is protected: Slightly worse in performance if you have lots of mostly inactive shopkeepers, but this would also 'fix' the (though unrealistic) scenario for player shops that the chunk of the chest is loaded but the (usually nearby) shopkeeper isn't loaded/active because it is in a different (ex. neighboring) unloaded chunk (usually nearby chunks are however always loaded when a player tries to access a chest, so this isn't that large of an issue currently).
Only despawn and then respawn the entities, without 'simulating a chunk unload' for the shopkeepers / without completely deactivating them: Clean, but requires quite some more implementation effort because spawning is currently tightly bound to the activation-state.
Reimplementing how it checks if a chest is protected, in a way that it considers all shopkeepers, but isn't affected by a decrease in performance the more shopkeepers you have, regardless if active or inactive.
Temporarily (1 tick) disabling all chest opening all together on the server every time the world was saved: Simple to implement.
I will probably look into implementing option 3 tomorrow.
Edited Jul 13, 2016