Fix signed chat state de-sync caused by execute run say
#222
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
`SignableCommand#of` goes down the command parse results to find signed arguments which will form part of the signed chat state. This allows commands like `/say` to have their messages be signed by the client. When running through `/execute` (or any other similar command) however, the message shouldn't be signed, as the command may be run by another player, making the signature on the message meaningless. To prevent this, `SignableCommand` stops searching when a command node redirection loops back to the root command node (where it might go to a command like `/say`). This allows other commands which use redirection to still retain signed arguments, while disallowing `/execute` and similar commands. However, because of NeoForge's client commands system, the root command node of the dispatcher and the root command node pointed by `/execute run` for redirection are different, causing the stop check above to never trigger, thus causing the client to sign the arguments. As the server disregards the signatures, this now causes a de-sync between the signed chat state of the client and the server; thus, the client will be kicked by the server upon the next chat message, when it realizes a de-sync happened. To fix this, we generalize the check from being the original root command node, to being *any* RootCommandNode. This retains compatibility with the original vanilla check, while allowing for our custom root command nodes to also count for the check. Fixes neoforged#186
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.
This PR fixes #186 by generalizing the check in
SignableCommand#ofto account for any root command node, rather than only the original/dispatcher's root command node.SignableCommand#ofgoes down the command parse results to find signed arguments which will form part of the signed chat state. This allows commands like/sayto have their messages be signed by the client.When running through
/execute(or any other similar command) however, the message shouldn't be signed, as the command may be run by another player, making the signature on the message meaningless.To prevent this,
SignableCommandstops searching when a command node redirection loops back to the root command node (where it might go to a command like/say). This allows other commands which use redirection to still retain signed arguments, while disallowing/executeand similar commands.However, because of NeoForge's client commands system, the root command node of the dispatcher and the root command node pointed by
/execute runfor redirection are different, causing the stop check above to never trigger, thus causing the client to sign the arguments.As the server disregards the signatures, this now causes a de-sync between the signed chat state of the client and the server; thus, the client will be kicked by the server upon the next chat message, when it realizes a de-sync happened.
To fix this, we generalize the check from being the original root command node, to being any RootCommandNode. This retains compatibility with the original vanilla check, while allowing for our custom root command nodes to also count for the check.