search

samolego / GolfIV

An anti-exploit attempt for Fabric
https://modrinth.com/mod/golfiv
MIT License
49 stars 13 forks source link

Removing an illegal item from survival mode, or right clicking to equip armor from your hand, ghost clears the NBT data of all other items in your inventory #39

Closed CalXee closed 3 years ago

CalXee commented 3 years ago

For some reason, removing an illegal item clears all other item's NBT data. Quite annoying. You have to click on it again in order for it to be back to normal.

Ampflower commented 3 years ago

I believe as part of finding KJP12#2, this is caused by the ItemInventoryKickPatch class overzealously removing NBT from various items that needs them to be able to render properly, notably here: https://github.com/samolego/GolfIV/blob/705f6a1a1cfbb9e3c92cd149eb0b1c4e2d826383/src/main/java/org/samo_lego/golfiv/event/S2CPacket/ItemInventoryKickPatch.java#L34-L45

A patch that may work would be to selectively whitelist some NBT structures for various items as deemed fit.

samolego commented 3 years ago

Humm, perhaps even adding packet size check before applying it ...

Ampflower commented 3 years ago

Another thing I have noticed while working on patching this is that this feature tends to break creative inventories due to the odd property of creative inventories being able to spawn any and all items at will, which can also break any items that has NBT bound to them. An example would be like #40, where you had a potion in your inventory, you relog, and now the potion is just the uncraftable kind. Doesn't matter what the potion was.

samolego commented 3 years ago

That's a different thing I'd say, as this here is just visual problem. Creative items are like survival ones, are checked for the allowed nbt and then just get other nbt removed.

Ampflower commented 3 years ago

In survival's case, yes, it is just visual, but for creative, it can end up replacing the inventory in its entirety silently, even if nothing should be replaced or legalised to begin with. It's not as much of a problem when it's more patched out, but can still show through some, say if you had a chest or shulker with items in it in your inventory, the tag may get sanitised out silently due to the creative client resyncing the inventory with the server, making it lose any items it would've had before.

One way I'm thinking of to work around creative's inventory oddity is to have some kind of config or condition where creative can get a pass as long as it can fit in the packet if you want it to be more strict to survival. If you'd like, I can open a separate issue to discuss a bit more in depth on this in specific or to have a tracking ticket, since it's kinda an issue on its own that happens to be caused by the same code.

samolego commented 3 years ago

I agree, creating a separate issue would be better.