Opened 4 years ago

Closed 2 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 elexis)

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)

  1. Add the check: The commands available in the simulation, including those three cheats are coded in Commands.js and reside in the commands 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 checking if (!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 to g_NotificationsTypes in messages.js. It should display a message box. Find examples in the code by searching for messageBox. (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)

t3551_prohibit_all_cheats_for_rated_games_v1.patch (3.4 KB) - added by elexis 4 years ago.
t3551_prohibit_all_cheats_for_rated_games_v2.patch (3.9 KB) - added by elexis 4 years ago.
No more ugliness as we save the rated-setting in the GuiInterface instead of the Player component.
t3551_prohibit_all_cheats_when_cheats_are_disabled_v2.patch (1.6 KB) - added by elexis 4 years ago.
Alternative. Only allows developer-overlay cheats when the cheats-setting is enabled. Would be coherent with the current state of #2676 (r17170).
t3551_prohibit_all_cheats_when_cheats_are_disabled_v3.patch (2.3 KB) - added by elexis 4 years ago.
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).

Download all attachments as: .zip

Change History (24)

comment:1 Changed 4 years ago by elexis

Description: modified (diff)

comment:2 Changed 4 years ago by elexis

Description: modified (diff)
Keywords: patch review added; simple removed
Milestone: BacklogAlpha 19
Summary: Prohibit developer overlay cheats in rated games[PATCH] Prohibit developer overlay cheats in rated games

comment:3 Changed 4 years ago by Josh

Keywords: review removed

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.

Version 0, edited 4 years ago by Josh (next)

comment:4 Changed 4 years ago by Josh

See my comment on #3552 for a temporary solution.

comment:5 in reply to:  4 Changed 4 years ago by elexis

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.

comment:6 Changed 4 years ago by Josh

Blocked on #3579.

Changed 4 years ago by elexis

No more ugliness as we save the rated-setting in the GuiInterface instead of the Player component.

Changed 4 years ago by elexis

Alternative. Only allows developer-overlay cheats when the cheats-setting is enabled. Would be coherent with the current state of #2676 (r17170).

comment:7 Changed 4 years ago by elexis

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 Changed 4 years ago by stanislas69

Owner: set to elexis

comment:9 Changed 4 years ago by leper

Milestone: Alpha 19Alpha 20
Priority: Release BlockerShould 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).

Changed 4 years ago by elexis

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:10 Changed 4 years ago by wraitii

In 17282:

Temporary fix for #3551 in the interest of having a fair A19. Prohibit deverloper overlay cheats when cheats are disabled. Patch by elexis. Refs #3551

comment:11 Changed 4 years ago by mimo

reverted in r17288 as we could no more open the developper overlay in games started with autostart

comment:12 Changed 4 years ago by Itms

Milestone: Alpha 20Alpha 21

comment:13 Changed 3 years ago by Itms

Keywords: review removed
Milestone: Alpha 21Backlog

I shouldn't have just pushed it since the proposed patch was once committed and is broken. We need a better solution.

comment:14 Changed 3 years ago by elexis

  • (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 Changed 3 years ago by elexis

Owner: elexis deleted

comment:16 Changed 3 years ago by scythetwirler

Keywords: beta added

comment:17 Changed 3 years ago by elexis

In 19155:

Remove the "quit" simulation command and restrict the "set-shading-color" simulation command to AIs,
since they are only useful for AI debugging and counterproductive in multiplayer mode, refs #3551.

Instead, enable AI developers to exit the game from a new AI API command,
allowing to batch simulate matches. Refs #2755.

Differential Revision: D65
Reviewed By: leper
Consulted: mimo

comment:18 Changed 2 years ago by elexis

Description: modified (diff)
Milestone: BacklogAlpha 22
Priority: Should HaveRelease 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 Changed 2 years ago by elexis

Revert of the revert: https://code.wildfiregames.com/D455

Enable cheats for autostarted games: https://code.wildfiregames.com/D453

comment:20 Changed 2 years ago by elexis

Owner: set to elexis
Resolution: fixed
Status: newclosed

In 19558:

Prohibit cheats if cheats are disabled,
i.e. developer cheats to control enemy units, revealing the map and promoting units too.

Open the developer overlay only if cheats are enabled (which is always the case in singleplayer mode and only the case in multiplayer mode if explicitly enabled).
(This doesn't make it harder for developers to "debug", since they previously had to remember to disable the rated game setting too. Also every bug had to be reproduced with the replay either way)

Reverts rP17288 which reverted rP17282 (since as of rP19557, the autostart gamesetup enables cheats too).

Differential Revision: https://code.wildfiregames.com/D455
Fixes #3551
Reviewed By: echotangoecho
Agreed with Itms and Imarok in the last staff meeting.

Note: See TracTickets for help on using tickets.