bugfix(network): Deny players with invalid names from joining a LAN game room #1595
Conversation
GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPIhandlers.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameNetwork/LANAPIhandlers.cpp
Outdated
Show resolved
Hide resolved
| // should not be in a player name. It should also not consist of only space characters. | ||
| if (canJoin) | ||
| { | ||
| constexpr WideChar IllegalNameChars[] = L",:;|\f\n\r\t\v"; |
There was a problem hiding this comment.
Why is the pipe character disallowed? I created a network match and was able to match 2 players with pipes (before this change). The pipe character is popular to separate clan names.
There was a problem hiding this comment.
This one was included via a suggestion, any thoughts on this @roossienb?.
I think it would be OK to allow pipe for the parsing functionality at least - as far as I know it's ,:; that would mess up the string splitting.
There was a problem hiding this comment.
roossienb asked if pipe was a problem. It is not after testing it.
| // should not be in a player name. It should also not consist of only space characters. | ||
| if (canJoin) | ||
| { | ||
| constexpr WideChar IllegalNameChars[] = L",:;|\f\n\r\t\v"; |
There was a problem hiding this comment.
Instead of banning \f\n\r\t\v, how about banning all characters less than 32 ? That includes all control characters.
There was a problem hiding this comment.
Yes, that makes sense. As these are wide chars, are there other values or sequences beyond the traditional ASCII control characters that we should consider as well?
There was a problem hiding this comment.
I think this needs testing. I just tested Network lobby with nickname öäü and it triggered Asserts in Debug, but otherwise worked.
There was a problem hiding this comment.
Yes, testing is definitely needed here.
I was thinking of things like Surrogate pairs and Unicode special characters that will require a lot more validation/handling that what is proposed in this PR right now.
Should we limit names to only characters in the BMP for example? Require the string to be normalized to a specific normalization form?
| if (canJoin) | ||
| { | ||
| constexpr WideChar IllegalNameChars[] = L",:;|\f\n\r\t\v"; | ||
| const Bool containsIllegalChars = wcscspn(msg->name, IllegalNameChars); |
There was a problem hiding this comment.
Can simplify further to wcscspn(msg->name, L",:;|\f\n\r\t\v");
There was a problem hiding this comment.
Should also be... != wcslen(msg->name)
| reply.GameNotJoined.playerIP = senderIP; | ||
| canJoin = false; | ||
|
|
||
| DEBUG_LOG(("LANAPI::handleRequestJoin - join denied because of illegal characters in the player name.")); |
There was a problem hiding this comment.
This code only affects LAN lobby, correct? If so, does this need to be replicated anywhere in Online Lobby code?
There was a problem hiding this comment.
I haven't seen the online lobby code use the string representation of the GameInfo, which is the root of this problem. On the other hand I haven't looked really closely at the online lobby code either.
There might be 2 cases here:
- Excluding
;:,because it causes trouble for the string parsing. If the online lobby does not use string parsing it strictly does not need to be replicated, other than maybe for consistency across the two lobbies. - Control characters and maybe other troublesome characters that could cause issues in the UI or other representations. These ones I think would be beneficial to replicate.
xezon
changed the title
|
Will this see another revision or is this final? |
4 participants
This change introduces an extra validation of player names before they are allowed to join a network game.
Specifically, player names containing
,;or:are denied, as they will cause trouble with the parsing of the string representation of the GameInfo. This set of characters mirrors the set of disallowed characters in https://github.com/TheSuperHackers/GeneralsGameCode/blob/dd2bb780a9226186e1d1ffe0dc60fc8000134117/GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/LanLobbyMenu.cpp#L861C1-L869C1The player attempting to join will be presented with a "duplicate name" error, hinting that the name needs to be changed, as we can't introduce a new error type and retain retail compatibility.