Page MenuHomeWildfire Games

Missing escapeText in the lobby and remove 256 character limit
Needs ReviewPublic

Authored by elexis on Jul 7 2017, 7:54 PM.
This revision needs review, but there are no reviewers specified.

Details

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.

Event Timeline

elexis created this revision.Jul 7 2017, 7:54 PM
elexis updated this revision to Diff 2855.Jul 7 2017, 8:02 PM

Remove those silly early returns too in un/escapeText.

Vulcan added a subscriber: Vulcan.Jul 7 2017, 8:16 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
| 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.

Vulcan added a comment.Jul 7 2017, 9:02 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/1717/ for more details.

Vulcan added a comment.Jul 7 2017, 9:04 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.

bb added a subscriber: bb.EditedDec 26 2017, 3:18 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)

elexis added a comment.Jan 5 2018, 6:40 PM

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.

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.


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