Fix unsigned commands detected as signed #1082
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm dubious on the nature of this given the current issues where having where cancelling anything ends up busting the chain more or less, even proxy commands appear to be doing so, yet I thought we had that handled just fine... |
|
Yes, the current implementation has more issues and chat signing and the chain doesn't work at all when I'm using velocity as a proxy. I would prefer some proper fix to allow chat and command cancellation but until someone had the time this workaround should work fine. |
electronicboy
approved these changes
Oct 12, 2023
BlackBaroness
added a commit
to PvPClashnet/Velocity-CommandExecuteEvent
that referenced
this pull request
Oct 24, 2023
* Add version information for 1.20.1 (PaperMC#1021) * bump adventure to 4.14.0 (PaperMC#1034) * check if a plugin has a executor service (PaperMC#1038) * check if a plugin has an executor service * checkstyle * feat: add TabList#addEntries (PaperMC#987) * [ci skip] Replaced weired i with i in javadocs (PaperMC#1057) In this little patch I replaced an i which caused my build process to crash with an i * Do not track plugin channels registered per-player on the proxy (PaperMC#591) We don't need to track this information since Velocity uses the JoinGame packet, which is about as good of a server rejoin mechanism we're likely to get in vanilla Minecraft. * fix PaperMC#1062 * 1.20.2 Support (PaperMC#1088) Co-authored-by: RednedEpic <[email protected]> Co-authored-by: Gero <[email protected]> * Actually send plugin message registration to backend servers I don't see where this was ever done, and don't see how plugin messaging could of ever worked, at least within the confines of CB and co, given the fact that we never seemed to be sending this to the backend? * appease checkstyle, move back to older fix placement * Change packet decode logging prompt * Catch throwables when loading plugins (PaperMC#1098) * Several improvements and fixes for 1.20.2 (PaperMC#1097) * Send LoginAcknowledged immediately * Resend player list header/footer after backend server switched to config state * Fix clearHeaderAndFooter not clearing fields in ConnectedPlayer * Clear boss bars, header/footer, tab list when switching client to config state * Send client settings in config state * Fix unsigned commands detected as signed (PaperMC#1082) * fix: commands flagged as signed without signed arguments * feat: improve error message for illegal protocol state. * acknowledge seen messages to server when running proxy commands (PaperMC#1100) * Implement ComponentLogger (PaperMC#1022) * Log the protocol phase in case of trying to obtain a packet id not existing in the phase (PaperMC#1107) * Bump netty to 4.1.100.Final (PaperMC#1067) --------- Co-authored-by: Pantera (Mad_Daniel) <[email protected]> Co-authored-by: chris <[email protected]> Co-authored-by: Aaron <[email protected]> Co-authored-by: powercas_gamer <[email protected]> Co-authored-by: Groldi <[email protected]> Co-authored-by: Andrew Steinborn <[email protected]> Co-authored-by: Emmanuel Lampe <[email protected]> Co-authored-by: Paul <[email protected]> Co-authored-by: RednedEpic <[email protected]> Co-authored-by: Gero <[email protected]> Co-authored-by: Shane Freeder <[email protected]> Co-authored-by: Joo200 <[email protected]> Co-authored-by: Adrian <[email protected]>
BlackBaroness
added a commit
to influct/Velocity
that referenced
this pull request
Oct 24, 2023
* 1.20.2 Support (PaperMC#1088) Co-authored-by: RednedEpic <[email protected]> Co-authored-by: Gero <[email protected]> * Actually send plugin message registration to backend servers I don't see where this was ever done, and don't see how plugin messaging could of ever worked, at least within the confines of CB and co, given the fact that we never seemed to be sending this to the backend? * appease checkstyle, move back to older fix placement * Change packet decode logging prompt * Catch throwables when loading plugins (PaperMC#1098) * Several improvements and fixes for 1.20.2 (PaperMC#1097) * Send LoginAcknowledged immediately * Resend player list header/footer after backend server switched to config state * Fix clearHeaderAndFooter not clearing fields in ConnectedPlayer * Clear boss bars, header/footer, tab list when switching client to config state * Send client settings in config state * Fix unsigned commands detected as signed (PaperMC#1082) * fix: commands flagged as signed without signed arguments * feat: improve error message for illegal protocol state. * acknowledge seen messages to server when running proxy commands (PaperMC#1100) * Implement ComponentLogger (PaperMC#1022) * Log the protocol phase in case of trying to obtain a packet id not existing in the phase (PaperMC#1107) * Bump netty to 4.1.100.Final (PaperMC#1067) --------- Co-authored-by: Paul <[email protected]> Co-authored-by: RednedEpic <[email protected]> Co-authored-by: Gero <[email protected]> Co-authored-by: Shane Freeder <[email protected]> Co-authored-by: Joo200 <[email protected]> Co-authored-by: Adrian <[email protected]> Co-authored-by: Pantera (Mad_Daniel) <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem
Using the proxy to intercept commands is sometimes required and should work with the api and the CommandExecuteEvent.
Currently every denied command will result into a disconnect with the message of a invalid protocol state. Even if the command is not signed and only has none or unsigned arguments.
The solution
To solve this problem I debugged the issue and found the reason: The salt is set for unsigned commands even if the command itself is not signed. This will cause a wrong return value for
SessionPlayerCommand#isSignedand theSessionCommandHandlerwill not cancel the event correctly.I changed the implmentation of
isSignedand added some useful debug information in case of denied signed messages.