Shift+right click a SignShop Sign breaks the shop working (Even if its blocked from editing!)

TomLewis commented 2 years ago

Description of Issue

Using Signshop https://www.spigotmc.org/resources/signshop.10997/ Anyone can shift+right click anyone's signshop sign and break it with shift+right click, even if they are "blocked" from doing so by not having trust permissions.

I created the same issue in SignShop as Im not sure which plugin is broken here. https://github.com/wargamer/SignShop/issues/148

Version Information

[21:15:47] [Render thread/INFO]: [CHAT] --------------------------------------------------
[21:15:47] [Render thread/INFO]: [CHAT] CMI: 9.2.3.2 BungeeCord CMIB  MySQL-> 9.2.3.4
[21:15:47] [Render thread/INFO]: [CHAT] CMILib: 1.2.3.2 -> 1.2.3.3
[21:15:47] [Render thread/INFO]: [CHAT] Server: Purpur 1.18.2-R0.1-SNAPSHOT
[21:15:47] [Render thread/INFO]: [CHAT] CMI economy: Disabled Vault: 1.7.3-b131 CMI Chat: Disabled 
[21:15:47] [Render thread/INFO]: [CHAT] Modules -> 31 enabled 25 disabled: spawnerProximity, customMessages, mirror, tablist, moneyCheque, skin, noTarget, elytraBoost, elytraLaunch, coloredArmor, votifier, shulkerBackpack, attachedCommands, spawnerCharge, namePlates, homeInteractions, hpBossBar, afk, anvilRenameColor, dynamicSigns, playerChatTag, durabilityLoss, flightCharge, disabledEnchants, worldLimits
[21:15:47] [Render thread/INFO]: [CHAT] --------------------------------------------------

Errors

No response

Relevant Config Sections

No response

Relevant Plugins

No response

Agreements

[X] My server is supported by CMI.
[X] My version of CMI at the time of this report is up to date.
[X] I have searched the github and asked around before making this report.

weaves7 commented 2 years ago

All my testing was done with CMI V9.2.3.4 I have tried canceling all PlayerInteractEvents on signs with various listener priorities and CMI always opens the SignUI. I also tried canceling all SignChangeEvents and the SignUI is still opened and that is when the sign glitches out, usually because an internal color code becomes an alternate color code. The CMI onSignInteractShift() method does not appear to respect already cancelled interactions. There is also a place in that method where a SignChangeEvent is called and the result is never checked. It looks like a following if statement was meant to check it but it checks blockPlaceEvent.isCancelled() instead. This was already done with an actual CMIEvent.placeSignEvent a bit before. I believe what has happened is either @Zrips forgot to respect already cancelled interactions and/or he meant to check for canceled SignChangeEvent but forgot to change the name after copying a NCP method.

TomLewis commented 2 years ago

@weaves7 what an amazing deep dive, fantastic reply, thank you. Hopefully this helps Zrips resolve it!

dan28000 commented 2 years ago

do you have it in right place in config?

to blacklist edit of sign it needs to be in

Signs:
  editBlackList:
  - '<text>'

text would be line that is same for all shops for blocking also make sure you don't have bypass perms

weaves7 commented 2 years ago

Although it does look like adding the SignShop signs to the blacklist could help and would keep a sign owner from accidentally editing one, I don't think that the sign edit feature should be ignoring cancelled sign interact and edit events. I also don't believe that allowing these sign edits were Zrips' intentions either. @TomLewis It may be a good interim fix to add signshop signs to the blacklist though.

Zrips commented 2 years ago

This will be fixed with next update. Thanks for reporting and determining cause of it.

TomLewis commented 2 years ago

As a heads up, the same behavior happens with /se command, hopefully these two systems both use the same function to fix both!

Thanks for the upcoming fix!

Zrips / CMI