Feature: Do not default the player name to "Player" #9080
Conversation
|
I'd fully expect the game to trim leading/trailing spaces for me, no need to error for that usecase |
rubidium42
force-pushed
the
validate_client_name
branch
from
7f2fd52 to
75d8b95
Compare
Just implemented that, with a new function for trimming strings. |
rubidium42
force-pushed
the
validate_client_name
branch
2 times, most recently
from
7db3747 to
8e54695
Compare
…er when changing it using the console/settings
…th invalid names can actually join
rubidium42
force-pushed
the
validate_client_name
branch
from
8e54695 to
6cb26f2
Compare
rubidium42
changed the title
…erver This so names from other clients are known valid in the client as well, instead allowing some compromised/bad server to potentially crash clients upon certain expectations.
|
There's more than one sort of whitespace, and though it'd be very difficult to get a tab, newline, carriage return or vertical tab character into that edit window, I'd expect a basic "trim" function to filter out all whitespace characters, not just spaces |
That could indeed be an expectation, but filtering out all whitespace becomes complex really fast. First, you need to decode the UTF-8 to a wide char, then you need to check for a white space character using iswspace. That function is locale dependent so it behaves differently for different people. The only thing that is certain is that it returns true when isspace returns true. Then there's also that the string validation routines filter out all the special characters you mentioned. Since those characters cannot be entered, why add logic for detecting them while trimming? So practically this would make the trimming significantly more complex with the UTF8 decode and locale dependent determination whether something is a space. Or I would be replacing the |
Motivation / Problem
Reviewing #9067 I noticed a "validation" of the client name getting added twice (after figuring out the UI breaks when no player name is set), looking at it the same validation also happens in other places. However, the rules for a valid client name differ between the different implementations, and there are many paths where the validation is not done.
So effectively this should solve the problem of players joining with either no name (which becomes "Player" when joining a server, but a self-hosted server will then have a client without name) or a "invalid" name. A name is invalid when it is empty of starts with spaces (that breaks tab completion).
In the places where the user interacts with OpenTTD the spaces will automatically be trimmed, however when receiving a client name over the network that is not done. After all, the name should be valid by that point so it is a 'broken' client.
Description
Add a function (of authority) to validate a client name and use that everywhere where the client name is checked.
Add a (helper) function to check whether a client name is right, and if not show an error message. Every location where you would try to "join" a server (includes starting your own server) and where you change your name this check is performed.
The server will now reject people joining with invalid names and will kick people changing their name to an invalid name. With this rejection it changes the network protocol, but this validation is performed after the version check so it is inconsequential (unless clients/servers fudge their version).
Add trimming of the spaces at the begin and end of the client name; includes a few new functions in string.cpp.
Limitations
I would be nicer for the user to get an UI where they can fix their user name in. However, that is quite complicated to accomplish as you would need to create a new window for that with the appropriate infrastructure to continue at the locations where you bailed out. The requires some significant rework of the code for relatively little gain, as for the end user the warning is usually shown in the Multiplayer window where you already have the edit box to enter it. The error message even tells the user where that edit box is.
Other cases where this could be triggered are when joining from the command line. Most likely you already set your nick before, so it is unlikely to happen there. Finally is changing the setting. You do that on the console and immediately get an error message (also on the console) that the name was wrong, so you can fix it there.
It does not solve the current "Player" issue, as that name is still allowed. However, for new installs that name will not be prefilled anymore so it should slowly die out.
The edit box in the Multiplayer UI does not perform direct validation as that is quite a nuisance and clearing the filled in value would then already show an error message, making it quite a hassle. Especially as the chance that they do not fill anything in that edit box when they change it is likely quite small, whereas players joining MP for the first time are far more likely to just click join without thinking about the player name.
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.