Missing escapeText in the lobby, replay menu and remove 256 character limit
AbandonedPublic
Actions

Authored by elexis on Jul 7 2017, 7:54 PM.

Details

Reviewers: None
Commits: rP21853: Remove 256 character limit from escapeText.

Summary

As reported in D715, if a lobby user adds a [ character to the nickname, the GUI will break completely when trying to parse that as a tag.
User input in tthe lobby should always be escaped.

The 256 character limitation from rP8494 is not used anywhere and bad because many strings (pathnames) can be longer legitimately.
If there was a reason to limit, then the caller should decide.

Test Plan

Exactly every user generated string used in g_NetMessageTypes and in the functions used therein should be escaped.
Add this replay to your replay directory to test formatPlayerInfo which is also used in the lobby.
Notice CNetServerWorker::SanitisePlayerName removes GUI tags and allows every other character, even weird ones like zero-width whitespace and non-supported unicode characters:

playername = ""+ "?❄❄?" + playername;

For good measure, playernames should also be escaped in the sesion, gamesetup, summary screen and so forth, which will be a reasonable addition in #3307.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 2548
Build 4312: Vulcan Build (Windows)	Jenkins
Build 4311: Vulcan Build	Jenkins
Build 4310: arc lint + arc unit

Event Timeline

elexis created this revision.Jul 7 2017, 7:54 PM

Remove those silly early returns too in un/escapeText.

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
| 190| »   }·catch·(e)·{
|    | [NORMAL] ESLintBear (no-empty):
|    | Empty block statement.

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

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 100| »   »   if·(g_MapNames.indexOf(replay.attribs.settings.Name)·==·-1·&&·replay.attribs.settings.Name·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 177| »   if·(attribs.settings.PlayerData.length·&&·attribs.settings.PlayerData[0]·==·null)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  41|  41|  * The playerlist will be assembled using these values.
|  42|  42|  */
|  43|  43| const g_PlayerStatuses = {
|  44|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  44|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  42|  42|  */
|  43|  43| const g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  45|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  43|  43| const g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  46|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  47|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|  50|  50| 

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

binaries/data/mods/public/gui/lobby/lobby.js
|1132| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

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

binaries/data/mods/public/gui/lobby/lobby.js
| 452| »   if·(mapSizeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 456| »   if·(playersNumberFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 460| »   if·(mapTypeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 592| »   »   case·'name':
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/gui/lobby/lobby.js
|1152| »   »   if·(msg.type·==·"chat"·&&·Engine.LobbyGetMucMessageCount()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

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

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/1717/ for more details.

Harbormaster completed remote builds in B2546: Diff 2853.Jul 7 2017, 9:02 PM

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 100| »   »   if·(g_MapNames.indexOf(replay.attribs.settings.Name)·==·-1·&&·replay.attribs.settings.Name·!=·"")
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with ''.

binaries/data/mods/public/gui/replaymenu/replay_menu.js
| 177| »   if·(attribs.settings.PlayerData.length·&&·attribs.settings.PlayerData[0]·==·null)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with 'null'.
|    | [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
| 185| »   }·catch·(e)·{
|    | [NORMAL] ESLintBear (no-empty):
|    | Empty block statement.

binaries/data/mods/public/gui/common/functions_utility.js
| 149| »   »   if·(word.toLowerCase().indexOf(lastWord.toLowerCase())·!=·0)
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  41|  41|  * The playerlist will be assembled using these values.
|  42|  42|  */
|  43|  43| const g_PlayerStatuses = {
|  44|    |-	"available": { "color": "0 219 0",     "status": translate("Online") },
|    |  44|+	"available": { "color": "0 219 0", "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  42|  42|  */
|  43|  43| const g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|    |-	"away":      { "color": "229 76 13",   "status": translate("Away") },
|    |  45|+	"away":      { "color": "229 76 13", "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  43|  43| const g_PlayerStatuses = {
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|    |-	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|    |  46|+	"playing":   { "color": "200 0 0", "status": translate("Busy") },
|  47|  47| 	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '"status"'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/lobby/lobby.js
|  44|  44| 	"available": { "color": "0 219 0",     "status": translate("Online") },
|  45|  45| 	"away":      { "color": "229 76 13",   "status": translate("Away") },
|  46|  46| 	"playing":   { "color": "200 0 0",     "status": translate("Busy") },
|  47|    |-	"offline":   { "color": "0 0 0",       "status": translate("Offline") },
|    |  47|+	"offline":   { "color": "0 0 0", "status": translate("Offline") },
|  48|  48| 	"unknown":   { "color": "178 178 178", "status": translateWithContext("lobby presence", "Unknown") }
|  49|  49| };
|  50|  50| 

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

binaries/data/mods/public/gui/lobby/lobby.js
|1132| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

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

binaries/data/mods/public/gui/lobby/lobby.js
| 452| »   if·(mapSizeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 456| »   if·(playersNumberFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 460| »   if·(mapTypeFilter.selected·!=·0·&&
|    | [NORMAL] JSHintBear:
|    | Use '!==' to compare with '0'.

binaries/data/mods/public/gui/lobby/lobby.js
| 592| »   »   case·'name':
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.

binaries/data/mods/public/gui/lobby/lobby.js
|1152| »   »   if·(msg.type·==·"chat"·&&·Engine.LobbyGetMucMessageCount()·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

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

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/1718/ for more details.

Harbormaster completed remote builds in B2548: Diff 2855.Jul 7 2017, 10:38 PM

save.js, gamesetup.js and messages.js might need some escaping too or not?
Maybe even modselection too (ppl can be stupid in mod names aswell)

binaries/data/mods/public/gui/lobby/lobby.js
812	why not here
839	and here
945	(in theory ppl could play on custom maps)
binaries/data/mods/public/gui/replaymenu/replay_menu.js
113	(rebase)

As reported by DeineLakeien, typing /\/\ triggers the translate("The command '%(cmd)s' is not supported."), { string and every chat text has the purple color by default then.

elexis added a commit: rP21853: Remove 256 character limit from escapeText..Jul 21 2018, 12:33 PM

elexis mentioned this in rP14098: Multiplayer lobby based on the XmPP protocol. Special thanks to Philip….Jul 21 2018, 1:05 PM

save.js, gamesetup.js and messages.js might need some escaping too or not?
Maybe even modselection too (ppl can be stupid in mod names aswell)

Correct that the other files must be considered too (as mentioned in the testplan too).
The lobby is a bit more important because it affects every online user, sometimes up to 120 simultaneously and it's already 1500 lines of code to review.
It's also more relevant again because people started messing with this in the lobby.

About the 256 character limit, consider the following message from play0ad.com:

“0 A.D.” is a time period that never actually existed: In the usual calendar, one goes from 1 B.C. to 1 A.D. and skips zero. This reflects the historical fiction in the game: Who would have won if all the factions were pitted against each other when each of them was at its prime?

Those are 281 characters, so it impacts people who legitimately want to write 3 sentences in a chat message and have half their content cutoff when they possibly wanted to make some point.
If someone is spamming, he can get kicked or possibly muted in the future instead.

Here a discontinued continuation of the patch, the nick highlighting problem is not possible to be solved cleanly.

patch27 KBDownload

patch31 KBDownload

binaries/data/mods/public/gui/lobby/lobby.js
227	These are too early for my taste, would rather have them in the return value of ircFormat, i.e. in the last call before the displayed string is constructed, so we work with the original data as long as possible (and could for example decide to not colorize under some conditions, such as moderators and combine the `escapeText` with the gui tags in the same function, making it easier to read and maintain. Also it's easier to memorize if everything works with the original value except the return value.) But the nickname highlighting conflicts with the implementation and highlighting doesn't work with nicknames that have characters that become escaped.
812	correct
839	incorrect, that list uses the nick as an identifier, not to be displayed
945	First I thought you meant that translate could fail and mods should broadcast translations of the mapname. But you meant that missing escapeText of course, correct.
948	here too then

nani reported that typing /|\ breaks colors of the local client.

elexis retitled this revision from Missing escapeText in the lobby and remove 256 character limit to Missing escapeText in the lobby, replay menu and remove 256 character limit.Nov 15 2019, 4:58 PM

The diff in D2412 will contain all escapeText calls that were identified to be necessary for the player to protect from GUI tag attackers, making this diff obsolete except for the replaymenu and maybe gamesetup/session/summary/... (IIRC NetServer prohibited those names, but in singleplayer it still breaks)

binaries/data/mods/public/gui/lobby/lobby.js
527	More likely that moderators will be unaware of color parsing and then use [foo] and thus not conveying the kick reason to the subject, so better escapeText the reason too.
839	I was incorrect, It's still of CGUIString type and thus will still complain about this not being a proper tag even if not displayed.
945	and not only a map author making a mistake but a malicious player inserting an wicked game with broken tags. translations are assumed to not be broken by wfg or theyd have to be escaped everywhere. (I guess in no case case one would want to provide translators the freedom to apply tags but then not specify the tag values (color, font, ..) in the code, i.e. escapeTexting every translate call that doesnt explicitly provide tags in english strings should be possible to enforce, but greater scope than this diff)
948	These values are actually under control of the player, so should escapeText. Also sprintf for that /