search

John-Paul-R / Essential-Commands

Configurable, permissions-backed utility commands for Fabric servers (tpa, home, warp, spawn, back, nick, rtp)
MIT License
105 stars 33 forks source link

TeleportAskHere command fail to work on player with special name #255

Open tadshi opened 10 months ago

tadshi commented 10 months ago

Hello!

I'm using 0.33.2-mc1.20 in my tiny server with my friends and went into a "bug" these days.

One of my friend took "xxx#1"(I'm not sure whether this name is legal as we're playing in offline mode) as his ID and his /tpahere request to other players cannot be comfirmed. The error message showed his ID was not recognized by some command due to the sharp mark "#" in his name.

I guess this problem is caused by this line in the code which directly concat player name with raw string to form a command.

(Although I'm not familiar with mc commands, I'm a little curious about whether non-admin player can exploit this e.g. use a strange player name for injection and send a tpahere request to server admin so as to execute some op command)

John-Paul-R commented 10 months ago

Hiya, thanks for the report!

"xxx#1"(I'm not sure whether this name is legal as we're playing in offline mode) as his ID

According to the Java Profile Names MC FAQ section, this name is not legal. Relevant excerpt:

Accepted characters:

  • A-Z (upper and lower case)
  • 0-9
  • The only special character accepted is _ (underscore)

Because of this, I suspect that the default name parser that MC commands use (which this command uses as well) will reject such a name. (For example, does /whisper xxx#1 work? -- I'd expect not.)

If a vanilla command does work with this name, I'd be very interested to know.

(Although I'm not familiar with mc commands, I'm a little curious about whether non-admin player can exploit this e.g. use a strange player name for injection and send a tpahere request to server admin so as to execute some op command)

I suspect not. My understanding of MC command parsing is that each space-separated string is evaluated as a separate token (with the exception of things like "Greedy String" argument types, which are not used here). Because of this behavior, in this example (/tpaccept <playerNameArgument>), any user input will be evaluated as part of the playerNameArgument (or, as additional, invalid arguments -- in the case where the user input contains one or more spaces), within the context of that tpaccept. I'm not aware of any way to abruptly end a command and start another within the same command string, so I think this would render injection attacks impossible here.

tadshi commented 10 months ago

Oh, thanks a lot for your detailed response!

According to the Java Profile Names MC FAQ section, this name is not legal. Relevant excerpt:

Oh, sorry for that. Seems the id is illegal. I have not made a thorough research on the website.

Because of this, I suspect that the default name parser that MC commands use (which this command uses as well) will reject such a name. (For example, does /whisper xxx#1 work? -- I'd expect not.) If a vanilla command does work with this name, I'd be very interested to know.

Well, actually we have not ever tested /whisper -- but there are some vanilla commands which work together with that wierd ID, if enough care is taken. If the ID xxx#1 is quoted with a pair of ", then commands like /effect, /tp, /gamemode will functions properly (e.g. /tp xxx#1 will throw an error while /tp "xxx#1" will tp me to the player). So I once considered that adding a pair of quote before concatenating can be a simple fix, but now it may not be the case.

Interestingly though, commands like /op and /deop will not work on that player under most circumtances and I have to use @a to select that player. Probably because those are server-side only commands?

I suspect not. My understanding of MC command parsing is that each space-separated string is evaluated as a separate token (with the exception of things like "Greedy String" argument types, which are not used here). ...

Oh I see that. As long as the context of one command would not be changed into another one during argument parsing (e.g. semicolon in bash script), this implementation should be safe. I also did a little research and find no way to exploit this.

So to sum up, the name xxx#1 is not officially supported by MC, at least modern MC Java Edition, so this issue become a feature request for unbound behavior rather than a bug report. As most commands from this modpack function so well, this one actually did not cause much trouble. Maybe we should close this?

John-Paul-R commented 9 months ago

Leaving this open, because I do admit it would be cool to have all possible nicknames supported by the server-side argument parser, and that's worth tracking. But I don't know if/when I will get to it.