Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Do not default the player name to "Player" #9080

Merged
merged 6 commits into from Apr 24, 2021

Conversation

rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Apr 22, 2021

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.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

src/console_cmds.cpp Outdated Show resolved Hide resolved
@LordAro
Copy link
Member

LordAro commented Apr 22, 2021

I'd fully expect the game to trim leading/trailing spaces for me, no need to error for that usecase

@rubidium42
Copy link
Contributor Author

I'd fully expect the game to trim leading/trailing spaces for me, no need to error for that usecase

Just implemented that, with a new function for trimming strings.

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think auto-trimming is an awesome move :D That is really good!

src/network/network_client.cpp Outdated Show resolved Hide resolved
src/network/network_server.cpp Outdated Show resolved Hide resolved
src/string.cpp Outdated Show resolved Hide resolved
src/network/network_client.cpp Outdated Show resolved Hide resolved
@rubidium42 rubidium42 force-pushed the validate_client_name branch 2 times, most recently from 7db3747 to 8e54695 Compare April 23, 2021 13:18
TrueBrain
TrueBrain previously approved these changes Apr 23, 2021
@TrueBrain TrueBrain dismissed their stale review April 23, 2021 13:51

I have been told I have missed a bug :P

@rubidium42 rubidium42 changed the title Validate client name Feature: Do not default the player name to "Player" Apr 23, 2021
…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.
Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming you find no more bugs yourself

@LordAro
Copy link
Member

LordAro commented Apr 23, 2021

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

@rubidium42
Copy link
Contributor Author

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 *c == ' ' with isspace(*c), where anything but ' ' would already be removed by the string validation of OpenTTD.

@rubidium42 rubidium42 merged commit 2999d30 into OpenTTD:master Apr 24, 2021
@rubidium42 rubidium42 deleted the validate_client_name branch April 24, 2021 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants