Opened 9 years ago
Closed 7 years ago
#3551 closed defect (fixed)
[PATCH] Prohibit developer overlay cheats in non-cheated games
Reported by: | elexis | Owned by: | elexis |
---|---|---|---|
Priority: | Release Blocker | Milestone: | Alpha 22 |
Component: | UI & Simulation | Keywords: | patch, beta |
Cc: | Patch: |
Description (last modified by )
Problem: There are some bugs which can be abused to enable the developer overlay in rated games (#3547, #3550 and likely others). As long as we only use a client-side check to disable the developer overlay, users can just remove that and abuse the feature. Should be a release blocker as it has been abused way too often by people using proxies and making new accounts after being banned.
What needs to be done:
- We have to prohibit the worst effects of the dev overlay in the simulation. These are:
- Control all units (cheat)
- Reveal map (cheat)
- Promote units (cheat)
- Sending commands for other players (change perspective, to be done in #3552)
I.e. we should not execute developer overlay cheats in the simulation code if the game is rated.
Furthermore unadulterated clients should display a warning message stating that this user attempted to cheat.(Not implementing the message as itself might be used to troll and blame innocent users as a malicious server.)
Why prohibiting cheats can be done securely: Those three developer overlay cheats should not be executed if ratings are enabled. If a malicious player (no matter if host or client) will remove that check, the game will become out-of-sync instantaneously and only the malicious user will execute cheat. Thus the malicious user will not be able to participate anymore in that game.
Why we can't prohibit the actual overlays: I don't see a way how to securely remove the prohibiting of the actual overlays (like the pathfinder overlay). This is local code, thus it can always be replicated/reverted after an attempted fix. The damage of those overlays is limited to revealing the map, which will always be possible for malicious clients.
How to implement: (Probably about 20 lines)
- Add the check: The commands available in the simulation, including those three cheats are coded in
Commands.js
and reside in thecommands
variable:"reveal-map": function(player, cmd, data) { // Reveal the map for all players, not just the current player, // primarily to make it obvious to everyone that the player is cheating var cmpRangeManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager); cmpRangeManager.SetLosRevealAll(-1, cmd.enable); }, "promote": function(player, cmd, data) { // No need to do checks here since this is a cheat anyway var cmpGuiInterface = Engine.QueryInterface(SYSTEM_ENTITY, IID_GuiInterface); cmpGuiInterface.PushNotification({"type": "chat", "players": [player], "message": "(Cheat - promoted units)"}); for each (var ent in cmd.entities) { var cmpPromotion = Engine.QueryInterface(ent, IID_Promotion); if (cmpPromotion) cmpPromotion.IncreaseXp(cmpPromotion.GetRequiredXp() - cmpPromotion.GetCurrentXp()); } }, "control-all": function(player, cmd, data) { data.cmpPlayer.SetControlAllUnits(cmd.flag); },
Notice the regular cheats are executed in
Cheats.js
and there we prohibit the cheat securely by checkingif (!cmpPlayer.GetCheatsEnabled())
. For those three developer overlay cheats we need to check if ratings are enabled.
2. Show the cheat notification: If ratings are disabled and the cheat was attempted to be executed, we should display the notification. It must be sent similar to the chat simulation command:
"chat": function(player, cmd, data) { var cmpGuiInterface = Engine.QueryInterface(SYSTEM_ENTITY, IID_GuiInterface); cmpGuiInterface.PushNotification({"type": cmd.type, "players": [player], "message": cmd.message}); },
A new notification type has to be added tog_NotificationsTypes
inmessages.js
. It should display a message box. Find examples in the code by searching formessageBox
. (Please test if having two message boxes simultaneously causes trouble, as we might get a second one due to the resulting out-of-sync in that case).
Attachments (4)
Change History (24)
comment:1 by , 9 years ago
Description: | modified (diff) |
---|
comment:2 by , 9 years ago
Description: | modified (diff) |
---|---|
Keywords: | patch review added; simple removed |
Milestone: | Backlog → Alpha 19 |
Summary: | Prohibit developer overlay cheats in rated games → [PATCH] Prohibit developer overlay cheats in rated games |
by , 9 years ago
Attachment: | t3551_prohibit_all_cheats_for_rated_games_v1.patch added |
---|
comment:5 by , 8 years ago
Keywords: | review added |
---|
Replying to Josh:
See my comment on #3552 for a temporary solution.
See attachment:t3551_prohibit_all_cheats_for_rated_games_v1.patch and attachment:t3552_prevent_commands_for_others_in_rated_games_v1.patch:ticket:3552 for a permanent solution. [We could be a bit more restrictive and prohibit it only when having cheats. Would be better imho (see #3155), but disabling it for rated games is the least]
Replying to Josh:
Are you sure that you want to make it a property of the player? It would make more sense to be a property of the game. You'll never have the case that some players are ranked and others aren't.
I know thats ugly, but I couldn't find a better place in the simulation to store the value. The same should be true for cheats only being accessible to either all or no players, that's why I stored it there. If you know a better place to store it in the simulation, I can update this. The PlayerManager would be one way, still ugly if not more ugly separating it from the cheats-setting.
by , 8 years ago
Attachment: | t3551_prohibit_all_cheats_for_rated_games_v2.patch added |
---|
No more ugliness as we save the rated-setting in the GuiInterface
instead of the Player
component.
by , 8 years ago
Attachment: | t3551_prohibit_all_cheats_when_cheats_are_disabled_v2.patch added |
---|
comment:7 by , 8 years ago
One of those two patches must be committed. If you are not sure, go for the rating one, as it doesn't change the currently intended way of allowing dev-overlay cheats, but only enforces the current restriction in the simulation instead of the client.
Remember it is not really acceptable to have people control all units, at least in rated games.
comment:8 by , 8 years ago
Owner: | set to |
---|
comment:9 by , 8 years ago
Milestone: | Alpha 19 → Alpha 20 |
---|---|
Priority: | Release Blocker → Should Have |
Note that at least one of the patches (didn't we have that discussion about posting N different patches already?) should use the passed in component (yes Commands.js is a bit special in that regard, but doing pointless calls is pointless anyway).
by , 8 years ago
Attachment: | t3551_prohibit_all_cheats_when_cheats_are_disabled_v3.patch added |
---|
No more pointless calls and also updates the GUI in case we go with this of the two variants (disable only for rated games vs. disable in all non-cheated games).
comment:11 by , 8 years ago
reverted in r17288 as we could no more open the developper overlay in games started with autostart
comment:12 by , 8 years ago
Milestone: | Alpha 20 → Alpha 21 |
---|
comment:13 by , 8 years ago
Keywords: | review removed |
---|---|
Milestone: | Alpha 21 → Backlog |
I shouldn't have just pushed it since the proposed patch was once committed and is broken. We need a better solution.
comment:14 by , 8 years ago
- (Not broken, as it does what it says.) The official explanation for the revert was that mimo couldn't open the dev overlay anymore in autostarted singleplayergames. That would be easily patchable if it were the only thing.
- But we also failed to find a consensus on what developer-overlay features should be allowed in multiplayergames, rated games (#3547), whether it should be linked to the cheats-setting, or to a new gamesetup option, or none of it all.
The same missing consensus is expressed in #3155 closed as won't fix by historic_bruno saying everyone should be able to debug #2676 was fixed by a patch of historic_bruno, committed by Josh, ticket reopened by leper #3579 a dupe ticket by josh arguing it should be linked to the cheats setting (as in the reverted commit of this ticket)
- New task statement: Add a gamesetup option to allow developer overlays ?
Adding a new gamesetup option as historic_bruno proposed in comment:3:ticket:2676 seems redundant and therefore wrong to me (as both settings disallow cheats, just a different set of cheats).
But as there hasn't been any feedback from leper to my concerns in #2676, I can just assume he abides by the reopening message and insists on that gamesetup option:
As it is now this does not help in debugging since it is linked to cheats being enabled. Thus using it for debugging is not possible.
(Also the gamesetup dynamic resizing issue mentioned by historic_bruno in comment:4:ticket:2676 was rewritten in #3234 and is now trivial to extend.)
comment:15 by , 8 years ago
Owner: | removed |
---|
comment:16 by , 7 years ago
Keywords: | beta added |
---|
comment:18 by , 7 years ago
Description: | modified (diff) |
---|---|
Milestone: | Backlog → Alpha 22 |
Priority: | Should Have → Release Blocker |
Summary: | [PATCH] Prohibit developer overlay cheats in rated games → [PATCH] Prohibit developer overlay cheats in non-cheated games |
Since Itms and Imarok agreed in todays staff meeting to restrict the three cheats from Commands.js
to the cheat setting, the Commands.js
change in r17282 (which wasn't a temporary fix to begin with) and reverted without any inquiry in r17288 should be reintroduced.
(r17288 could have just noticed that Autostarted games don't start with cheats enabled, contrary to gamesetup.js SP games and add the one or two lines enabling those instead of reintroducing a comment that doesn't make any sense)
comment:19 by , 7 years ago
Revert of the revert: https://code.wildfiregames.com/D455
Enable cheats for autostarted games: https://code.wildfiregames.com/D453
Looks good. I would recommend a few more validation checks in Player.js. One would be to make sure that the lobby is running before enabling ranking. If the lobby is not running, a warning or error on attempt to enable ranking seems appropriate.Edit: Are you sure that you want to make it a property of the player? It would make more sense to be a property of the game. You'll never have the case that some players are ranked and others aren't.