Page MenuHomeWildfire Games

Forbid lobby account registration with parentheses and other characters not thought of
ClosedPublic

Authored by elexis on Jul 6 2017, 6:32 PM.

Details

Summary

Some vampire in the lobby found it funny to register names of the pattern "some existing playername (rating of that player)".
Joining lobby games where the mocked player joined results in being trolled and confused.

While it is still quite easy to detect which the fake account is and
while trolling can't be prevented unless removing mankinds ability to express itself completely,
screw those parentheses that are used for ratings in particular.

The related lobby account character check code is

  • messy (multiple regex when one would be sufficient)
  • dead (unused options and defined in gui/common/ scope but most certainly never going to be used globally).
  • incomplete (because it uses a blacklist of forbidden symbols instead of a whitelist of well-known characters).
  • covers up a bug halfway (preventing [ and ] in the nickname instead of using escapeText. /nicking [font="foo"] will colorize each new lobby message and spam an error each time. -> separate fix)
Test Plan

Notice that we are in string freeze.
Notice that the user can still register arbitrary names when not using 0AD, so this patch only affects the vast majority of trolls.
Notice that (without changing the code) the username variable has already been checked, so we don't need those other two calls to sanitizePlayerName.

Check the leaderboard and see that those are almost all characters used by people.
Notice there are some people who want to use [XX] country tags in their name. If they already didn't fail trying to do so, now they certainly should.
Visit https://regex101.com/ and copy over that regex then test that it works as advertized and doesn't pass spaces, unicode or parentheses f.e..
Try to register an account.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jul 6 2017, 6:32 PM
Sandarac added inline comments.
binaries/data/mods/public/gui/lobby/prelobby.js
95 ↗(On Diff #2827)

Get rid of that semicolon.

Imarok added a subscriber: Imarok.Jul 6 2017, 7:02 PM
Imarok added inline comments.
binaries/data/mods/public/gui/lobby/prelobby.js
204 ↗(On Diff #2827)

Wanna remove that braces while at it?

Vulcan added a subscriber: Vulcan.Jul 6 2017, 7:55 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1701/ for more details.

Vulcan added a comment.Jul 6 2017, 7:56 PM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/functions_utility.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/functions_utility.js
|  43|  43| 		return -1;
|  44|  44| 	else if (lowerX > lowerY)
|  45|  45| 		return 1;
|  46|    |-	else
|  47|    |-		return 0;
|    |  46|+	return 0;
|  48|  47| }
|  49|  48| 
|  50|  49| /**

binaries/data/mods/public/gui/common/functions_utility.js
| 194| »   }·catch·(e)·{
|    | [NORMAL] ESLintBear (no-empty):
|    | Empty block statement.

binaries/data/mods/public/gui/common/functions_utility.js
| 158| »   »   if·(word.toLowerCase().indexOf(lastWord.toLowerCase())·!=·0)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/prelobby.js
| 101| »   else·if·(!password)
|    | [MAJOR] ESLintBear:
|    | Parsing error: Unexpected token else

binaries/data/mods/public/gui/lobby/prelobby.js
| 101| »   else·if·(!password)
|    | [MAJOR] JSHintBear:
|    | Expected an identifier and instead saw 'else'.

binaries/data/mods/public/gui/lobby/prelobby.js
| 101| »   else·if·(!password)
|    | [NORMAL] JSHintBear:
|    | Expected an assignment or function call and instead saw an expression.

binaries/data/mods/public/gui/lobby/prelobby.js
| 101| »   else·if·(!password)
|    | [MAJOR] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/gui/lobby/prelobby.js
| 153| »   while·((message·=·Engine.LobbyGuiPollMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/290/ for more details.

elexis updated this revision to Diff 2839.Jul 7 2017, 1:41 PM

Remove that semicolon.
Cleanup should be in a separate commit, since there is a lot to do (refs rP17581 rP17584).

elexis added inline comments.Jul 7 2017, 1:55 PM
binaries/data/mods/public/gui/common/functions_utility.js
144 ↗(On Diff #2839)

This one allowed everything between 32 and 127 (decimal) and I don't think we should allow most of these characters
http://www.asciitable.com/index/asciifull.gif

binaries/data/mods/public/gui/lobby/prelobby.js
95 ↗(On Diff #2827)

Thanks. (In particular I recall 4 people including the teacher hectically not finding such a semicolon for 20 minutes after an if statement in a seminar and we just found it before the end of it)

Vulcan added a comment.Jul 7 2017, 2:28 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1709/ for more details.

Vulcan added a comment.Jul 7 2017, 2:30 PM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/functions_utility.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/functions_utility.js
|  43|  43| 		return -1;
|  44|  44| 	else if (lowerX > lowerY)
|  45|  45| 		return 1;
|  46|    |-	else
|  47|    |-		return 0;
|    |  46|+	return 0;
|  48|  47| }
|  49|  48| 
|  50|  49| /**

binaries/data/mods/public/gui/common/functions_utility.js
| 194| »   }·catch·(e)·{
|    | [NORMAL] ESLintBear (no-empty):
|    | Empty block statement.

binaries/data/mods/public/gui/common/functions_utility.js
| 158| »   »   if·(word.toLowerCase().indexOf(lastWord.toLowerCase())·!=·0)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/prelobby.js
| 161| »   »   switch(message.level)·{
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 221| »   switch(page)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 153| »   while·((message·=·Engine.LobbyGuiPollMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/295/ for more details.

elexis updated this revision to Diff 2847.Jul 7 2017, 2:35 PM

Allow logins with deprecated nicknames.
Also clear the feedback caption when clicking on cancel.

Imarok accepted this revision.Jul 7 2017, 4:24 PM

Assuming you will fix the string after the release.
Tested and proofread it.

This revision is now accepted and ready to land.Jul 7 2017, 4:24 PM
Vulcan added a comment.Jul 7 2017, 4:51 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1712/ for more details.

Vulcan added a comment.Jul 7 2017, 4:53 PM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/lobby/prelobby.js
| 162| »   »   switch(message.level)·{
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 222| »   switch(page)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/lobby/prelobby.js
| 154| »   while·((message·=·Engine.LobbyGuiPollMessage())·!=·undefined)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with 'undefined'.
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/functions_utility.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/common/functions_utility.js
|  43|  43| 		return -1;
|  44|  44| 	else if (lowerX > lowerY)
|  45|  45| 		return 1;
|  46|    |-	else
|  47|    |-		return 0;
|    |  46|+	return 0;
|  48|  47| }
|  49|  48| 
|  50|  49| /**

binaries/data/mods/public/gui/common/functions_utility.js
| 194| »   }·catch·(e)·{
|    | [NORMAL] ESLintBear (no-empty):
|    | Empty block statement.

binaries/data/mods/public/gui/common/functions_utility.js
| 158| »   »   if·(word.toLowerCase().indexOf(lastWord.toLowerCase())·!=·0)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/298/ for more details.

elexis added a comment.EditedJul 7 2017, 5:36 PM

(The string isn't wrong, just not complete, so it's not that much of an issue actually if we don't change it for the sake of being complete. It should probably only say that the string contains illegal characters)

Also thanks Imarok for the links to do the same change on the server level:
https://github.com/processone/ejabberd/issues/388

Perhaps its better to remove the GUI check after all so that wfg hosted lobbies can prevent all besides these allowed characters and privately hosted lobbies could allow all characters.
In that case we need a new string (or a string somehow transported from ejabberd which will likely fail at the gloox/glooxwrapper stage).

We decided there is no use-case for nickchange, but we can't find a way to disable it (except for visitors (allow_visitor_nickchange)).

Config & String removal -> #4671

This revision was automatically updated to reflect the committed changes.