Page MenuHomeWildfire Games

Remove unused exploitable JS SubmitUserReport function
ClosedPublic

Authored by elexis on Jul 22 2018, 5:22 PM.

Details

Summary

As mentioned in https://wildfiregames.com/forum/index.php?/topic/24325-gdpr/ there is this UserReport function which is

  • not used
  • not mentioned in the disclaimer userreport.txt but would have to be for GDPR compliance (EDIT: it says Performance details: a snapshot of timing data a few seconds after you start a match or change graphics settings.)
  • allows mods to upload arbitrary data (such as the lobby password or ingame chatlogs) to arbitrary servers without modifying any C++ code
  • incomplete if it were used, because the players civs, nomad setting, difficulty setting, AIs for example are also determining the mapgeneration and performance, not only seed and size
  • could be implemented more safely in C++ rather than exposing this to JS
  • inherently too much data, as the code comment says:

this information isn't currently useful for anything much and it generates a massive amount of data to transmit and store

Removal was discussed with Philip and Imarok on 2016-06-26-QuakeNet-#0ad-dev.log:

16:00 < elexis> rP16213 I guess we can nuke that
16:01 < elexis> introduced in rP8939
16:02 < elexis> even if someone would look at that data, its not really useful
16:02 < elexis> measures the performance only at second 5 and 60, that doesnt say much
16:05 < Philip> elexis: It was mainly for checking graphics performance, which is most consistently measured near the start of the game
16:06 < elexis> Philip: want to keep that?
16:08 < elexis> Philip: depends on what you want to measure. compare individual clients? players have different amounts of models in the initial view. want to measure the performance of one client? then do more measurements
16:11 < Philip> elexis: I wanted to measure stuff like http://zaynar.co.uk/0ad-pub/performance-20110317.png (which in that case showed we were doing something that gave terrible performance on R600)
16:12 < Philip> i.e. a very rough comparison of lots of GPUs

So something similar to that could be useful, but the problems above should be solved prior to committing that code.

Test Plan

Make sure it compiles, all related unused functions are removed too.

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 22 2018, 5:22 PM
Vulcan added a subscriber: Vulcan.Jul 22 2018, 5:32 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...

source/ps/scripting/JSInterface_Debug.cpp
|  45| »   return·*(volatile·int*)0;
|    | [MAJOR] CPPCheckBear (nullPointer):
|    | Null pointer dereference
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 397| 397| 				// Players see colors depending on diplomacy
| 398| 398| 				g_DisplayedPlayerColors[i] =
| 399| 399| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 400|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
|    | 400|+						g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 401| 401| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 402| 402| 					getDiplomacyColor("enemy");
| 403| 403| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 398| 398| 				g_DisplayedPlayerColors[i] =
| 399| 399| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 400| 400| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 401|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
|    | 401|+							g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 402| 402| 					getDiplomacyColor("enemy");
| 403| 403| 
| 404| 404| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 399| 399| 					g_ViewedPlayer == i ? getDiplomacyColor("self") :
| 400| 400| 					g_Players[g_ViewedPlayer].isAlly[i] ? getDiplomacyColor("ally") :
| 401| 401| 					g_Players[g_ViewedPlayer].isNeutral[i] ? getDiplomacyColor("neutral") :
| 402|    |-					getDiplomacyColor("enemy");
|    | 402|+								getDiplomacyColor("enemy");
| 403| 403| 
| 404| 404| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 405| 405| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
| 655| 655| 					"civ": setStringTags(g_CivData[g_Players[g_ViewedPlayer].civ].Name, { "font": "sans-bold-stroke-14" }),
| 656| 656| 					"hotkey_civinfo": colorizeHotkey("%(hotkey)s", "civinfo"),
| 657| 657| 					"hotkey_structree": colorizeHotkey("%(hotkey)s", "structree")
| 658|    |-			});
|    | 658|+				});
| 659| 659| 	}
| 660| 660| 
| 661| 661| 	// Following gaia can be interesting on scripted maps
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1239|1239| 
|1240|1240| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1242|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|1243| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|1244| 	});
|1245|1245| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1240|1240| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|1242| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1243|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|1244| 	});
|1245|1245| 
|1246|1246| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|1242| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|1243| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|    |-	});
|    |1244|+		});
|1245|1245| 
|1246|1246| 	let resCodes = g_ResourceData.GetCodes();
|1247|1247| 	for (let r = 0; r < resCodes.length; ++r)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/session/session.js
|1739|1739| 	for (let rct of resourcesCounterTypes)
|1740|1740| 		for (let rt of resourcesTypes)
|1741|1741| 			reportObject[rt + rct.substr(9)] = playerStatistics[rct][rt];
|1742|    |-			// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|    |1742|+	// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|1743|1743| 
|1744|1744| 	reportObject.vegetarianFoodGathered = playerStatistics.resourcesGathered.vegetarianFood;
|1745|1745| 	for (let type of unitsClasses)

binaries/data/mods/public/gui/session/session.js
|1067| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1142| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1143| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1144| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

Link to build: https://jenkins.wildfiregames.com/job/differential/684/display/redirect

elexis retitled this revision from Remove unused undocumented exploitable JS SubmitUserReport function to Remove unused exploitable JS SubmitUserReport function.Jul 29 2018, 9:03 PM
elexis edited the summary of this revision. (Show Details)
vladislavbelov accepted this revision.Jul 29 2018, 9:04 PM
This revision is now accepted and ready to land.Jul 29 2018, 9:04 PM
elexis requested review of this revision.Aug 5 2018, 5:16 PM

19:12 < Vladislav> elexis: I checked that there wasn't any other occurrence of the deleted methods.

Then the review was as careful as the upload of the first version, as JSI_Debug::GetProfilerState and CProfileViewer::SaveToJS were not noticed.

binaries/data/mods/public/gui/pregame/mainmenu.js
88 ↗(On Diff #6813)

The label said
If you want to send a message to the developers, you can enter one here:

It's questionable if it should be reimplemented ever.
If so, it would pose this security risk that JS mods could upload arbitrary data to arbitrary places (unless reading GUI input from C++).

In rP12971 the input element was removed, but not this function, so it was a bug in that commit.

It was removed because devs couldnt follow these comments and wanted that on bugreports, so maybe its a good reason to not implement it again and let the performance numbers speak for itself next time.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 5 2018, 5:39 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Aug 5 2018, 5:39 PM