Added a client side player name sanitation #8137
Conversation
|
This sanitization will need to be done by the server when the client connects or changes their name. If somebody sends a name with a miniyaml-breaking character then the client will crash before it gets a chance to sanitize it. |
| var forbiddenNames = new string[] { "Open", "Closed", "Spectator" }; | ||
|
|
||
| var clean = dirty; | ||
| if (string.IsNullOrEmpty(dirty) || forbiddenNames.Contains(clean) || clean.Contains(" AI")) |
There was a problem hiding this comment.
Foo AIBar will be flagged as dirty.
Perhaps EndsWith() would be better, if we must limit this.
There was a problem hiding this comment.
A simpler and more robust solution would be to check against the AI names directly.
There was a problem hiding this comment.
The TD and D2K bots don't have "AI" in their names.
Mailaender
force-pushed
the
sanitize-player-name
branch
from
11dfb00
to
854ce0d
Compare
|
I now also added a game server side player name sanitizing during client validation from the handshake and added a server name check as requested by @obrakmann. The rules are similar. |
|
|
||
| var clean = dirty; | ||
| if (string.IsNullOrEmpty(dirty)) | ||
| clean = ""; |
There was a problem hiding this comment.
This will probably need to have some non-empty default value (Newbie is the default player name, so probably that), otherwise the sanitation in this case doesn't really work.
There was a problem hiding this comment.
This will be replaced by Newbie and OpenRA Game (default server name) in the functions below.
There was a problem hiding this comment.
Wait a moment, you are right. My ordering of the checks is wrong. Entering # will allow an empty name.
Mailaender
force-pushed
the
sanitize-player-name
branch
from
854ce0d
to
f4d052c
Compare
|
Optimized it again after the function split for server name checks. |
|
This changes LobbyUtils.cs at the same lines as #8118. Edit: Also SettingsLogic.cs. |
|
The most important place to do the sanitization is LobbyLogic.IntepretCommand (the "name" handler). Please add it here too. A more polished client-side implementation would prevent the bad characters from being entered in the text fields in the first place, and then rely on the server-side implementation to deal with any hacked clients. |
|
Also please don't forget:
... which will return an empty string. |
Mailaender
force-pushed
the
sanitize-player-name
branch
from
f4d052c
to
edca755
Compare
|
Well spotted. Updated. |
| server.SendMessage("{0} is now known as {1}.".F(client.Name, s)); | ||
| client.Name = s; | ||
| var sanitizedName = OpenRA.Settings.SanitizedPlayerName(s); | ||
| Log.Write("server", "Player@{0} is now known as {1}.", conn.Socket.RemoteEndPoint, sanitizedName); |
There was a problem hiding this comment.
Could you add a check if the name changed really?
If I try to change abcdefg30 to abcdefg30# the # is removed and abcdefg30 is now known as abcdefg30 appears.
|
Otherwise 👍 / ✅ |
|
Good idea. Done. |
|
Looks good to me. Thanks! 👍 |
Added a client side player name sanitation
Closes #3200
Closes #3332
Closes #7739
Fixes #8031