Skip to content

Fix losing a item in ItemFrame upon interacting it on protected region you do not have access#3088

Merged
hakusaro merged 4 commits intoPryaxis:general-develfrom
NightKLP:general-devel
May 14, 2025
Merged

Fix losing a item in ItemFrame upon interacting it on protected region you do not have access#3088
hakusaro merged 4 commits intoPryaxis:general-develfrom
NightKLP:general-devel

Conversation

@NightKLP
Copy link
Contributor

@NightKLP NightKLP commented Apr 3, 2025

this is the best thing i could come up with

  • i tested it in my main server
  • where spamming it doesn't dupe
  • you do not lose your item modifier/prefix
  • it is a common issue when players lose their item which cause by a itemframe
  • if there's better ways than this i don't know honestly

return a item when a item frame was interacted in protected region you do not have permission with
@Arthri
Copy link
Contributor

Arthri commented Apr 3, 2025

Thanks for the pull request! I have a few suggestions, if you don't mind

@lost-werewolf
Copy link
Contributor

Why not just use the UpdateItemDrop (90) packet? In DropItemInstanced method from NPC.cs, the item slot is set to be unusable for 54000 frames after spawned, and then the item is set to inactive right after sending UpdateItemDrop. This eliminates sending a second packet afterwards and also replicates vanilla behavior for locally instanced items.
Of course, you need to send this packet to only the player who's being rejected.

@bartico6
Copy link
Collaborator

bartico6 commented Apr 3, 2025

Why not just use the UpdateItemDrop (90) packet? In DropItemInstanced method from NPC.cs, the item slot is set to be unusable for 54000 frames after spawned, and then the item is set to inactive right after sending UpdateItemDrop. This eliminates sending a second packet afterwards and also replicates vanilla behavior for locally instanced items. Of course, you need to send this packet to only the player who's being rejected.

Repeatedly rightclicking a blocked item frame would fill up the locked out slots (54000 frames is 900 seconds or 15 minutes) and allow a malicious client to prevent item drops from functioning on the server at all by locking out all slots.

@lost-werewolf
Copy link
Contributor

Why not just use the UpdateItemDrop (90) packet? In DropItemInstanced method from NPC.cs, the item slot is set to be unusable for 54000 frames after spawned, and then the item is set to inactive right after sending UpdateItemDrop. This eliminates sending a second packet afterwards and also replicates vanilla behavior for locally instanced items. Of course, you need to send this packet to only the player who's being rejected.

Repeatedly rightclicking a blocked item frame would fill up the locked out slots (54000 frames is 900 seconds or 15 minutes) and allow a malicious client to prevent item drops from functioning on the server at all by locking out all slots.

I see, thank you for the insight.
The unusable timer could perhaps be reduced to just a few seconds or even just one second with little to no impact on behavior as it is likely the client would pick up the item as soon as it's spawned on them. Not sure if this would be the best, as you said - a malicious client could indeed spam the item frame to temporarily disable item drops, but significantly reducing the timer could remedy this if it's set to something very low (perhaps just half a second - a second).

NightKLP and others added 2 commits April 4, 2025 12:48
Co-authored-by: Arthri <41360489+Arthri@users.noreply.github.com>
Co-authored-by: Arthri <41360489+Arthri@users.noreply.github.com>
@NightKLP NightKLP requested a review from Arthri April 4, 2025 04:51
ACaiCat added a commit to UnrealMultiple/TShock-Beta that referenced this pull request May 4, 2025
@hakusaro
Copy link
Member

hakusaro commented May 9, 2025

@Arthri @ACaiCat is this one okay to merge in your opinion?

@ACaiCat
Copy link
Member

ACaiCat commented May 11, 2025

@Arthri @ACaiCat is this one okay to merge in your opinion?

Tested.It can work as intended.

@hakusaro hakusaro enabled auto-merge May 14, 2025 09:46
@hakusaro hakusaro merged commit 7728b60 into Pryaxis:general-devel May 14, 2025
8 checks passed
@hakusaro
Copy link
Member

Can someone update the wiki for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants