Page MenuHomeWildfire Games

Don't process commands sent by observers.
ClosedPublic

Authored by Freagarach on Jun 12 2020, 8:54 AM.

Details

Reviewers
bb
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP24063: Do not process commands sent by observers.
Trac Tickets
#5140
Summary

Observers should not be able to affect the simulation state, therefore commands sent by observers should be ignored. A better way is probably not even trying to send the command in the network code.

Note that the reveal map cheat does not work anymore for observers after this. Is that bad? If so, all commands should be checked individually, I guess.

Test Plan

Reproduce behaviour in the ticket, apply patch, verify that one cannot resign as an observer anymore.

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

Freagarach created this revision.Jun 12 2020, 8:54 AM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '&&' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
| 531| 531| 			if (cmpGarrisonHolder)
| 532| 532| 			{
| 533| 533| 				// Only the owner of the garrisonHolder may unload entities from any owners
| 534|    |-				if (!IsOwnedByPlayer(player, garrisonHolder) && !data.controlAllUnits
| 535|    |-				    && player != +cmd.owner)
|    | 534|+				if (!IsOwnedByPlayer(player, garrisonHolder) && !data.controlAllUnits &&
|    | 535|+				    player != +cmd.owner)
| 536| 536| 						continue;
| 537| 537| 
| 538| 538| 				if (!cmpGarrisonHolder.UnloadTemplate(cmd.template, cmd.owner, cmd.all))
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
| 533| 533| 				// Only the owner of the garrisonHolder may unload entities from any owners
| 534| 534| 				if (!IsOwnedByPlayer(player, garrisonHolder) && !data.controlAllUnits
| 535| 535| 				    && player != +cmd.owner)
| 536|    |-						continue;
|    | 536|+					continue;
| 537| 537| 
| 538| 538| 				if (!cmpGarrisonHolder.UnloadTemplate(cmd.template, cmd.owner, cmd.all))
| 539| 539| 					notifyUnloadFailure(player, garrisonHolder);
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'metadata'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1146|1146| 
|1147|1147| 	// send Metadata info if any
|1148|1148| 	if (cmd.metadata)
|1149|    |-		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner" : player } );
|    |1149|+		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata": cmd.metadata, "owner" : player } );
|1150|1150| 
|1151|1151| 	// Tell the units to start building this new entity
|1152|1152| 	if (cmd.autorepair)
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'owner'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1146|1146| 
|1147|1147| 	// send Metadata info if any
|1148|1148| 	if (cmd.metadata)
|1149|    |-		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner" : player } );
|    |1149|+		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner": player } );
|1150|1150| 
|1151|1151| 	// Tell the units to start building this new entity
|1152|1152| 	if (cmd.autorepair)
|    | [NORMAL] ESLintBear (space-in-parens):
|    | There should be no spaces inside this paren.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1146|1146| 
|1147|1147| 	// send Metadata info if any
|1148|1148| 	if (cmd.metadata)
|1149|    |-		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner" : player } );
|    |1149|+		Engine.PostMessage(ent, MT_AIMetadata, { "id": ent, "metadata" : cmd.metadata, "owner" : player });
|1150|1150| 
|1151|1151| 	// Tell the units to start building this new entity
|1152|1152| 	if (cmd.autorepair)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1254|1254| 		}
|1255|1255| 
|1256|1256| 		lastTowerControlGroup = cmpSnappedStartObstruction.GetControlGroup();
|1257|    |-		//warn("setting lastTowerControlGroup to control group of start snapped entity " + cmd.startSnappedEntity + ": " + lastTowerControlGroup);
|    |1257|+		// warn("setting lastTowerControlGroup to control group of start snapped entity " + cmd.startSnappedEntity + ": " + lastTowerControlGroup);
|1258|1258| 	}
|1259|1259| 
|1260|1260| 	var i = 0;
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1318|1318| 
|1319|1319| 				if (i > 0)
|1320|1320| 				{
|1321|    |-					//warn("   updating previous wall piece's secondary control group to " + newTowerControlGroup);
|    |1321|+					// warn("   updating previous wall piece's secondary control group to " + newTowerControlGroup);
|1322|1322| 					var cmpPreviousObstruction = Engine.QueryInterface(pieces[i-1].ent, IID_Obstruction);
|1323|1323| 					// TODO: ensure that cmpPreviousObstruction exists
|1324|1324| 					// TODO: ensure that the previous obstruction does not yet have a secondary control group set
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '&&' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1481|1481| 		// Check that all its members are selected
|1482|1482| 		var fid = formationIds[0];
|1483|1483| 		var cmpFormation = Engine.QueryInterface(+fid, IID_Formation);
|1484|    |-		if (cmpFormation && cmpFormation.GetMemberCount() == formation.members[fid].length
|1485|    |-			&& cmpFormation.GetMemberCount() == formation.entities.length)
|    |1484|+		if (cmpFormation && cmpFormation.GetMemberCount() == formation.members[fid].length &&
|    |1485|+			cmpFormation.GetMemberCount() == formation.entities.length)
|1486|1486| 		{
|1487|1487| 			cmpFormation.DeleteTwinFormations();
|1488|1488| 			// The whole formation was selected, so reuse its controller for this command
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'closeClusters' to undefined.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1584|1584| 	while (clusters.length > 1)
|1585|1585| 	{
|1586|1586| 		// search two clusters that are closer than the required distance
|1587|    |-		let closeClusters = undefined;
|    |1587|+		let closeClusters;
|1588|1588| 
|1589|1589| 		for (let i = matrix.length - 1; i >= 0 && !closeClusters; --i)
|1590|1590| 			for (let j = i - 1; j >= 0 && !closeClusters; --j)
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1589|1589| 		for (let i = matrix.length - 1; i >= 0 && !closeClusters; --i)
|1590|1590| 			for (let j = i - 1; j >= 0 && !closeClusters; --j)
|1591|1591| 				if (matrix[i][j] < distSq)
|1592|    |-					closeClusters = [i,j];
|    |1592|+					closeClusters = [i, j];
|1593|1593| 
|1594|1594| 		// if no more close clusters found, just return all found clusters so far
|1595|1595| 		if (!closeClusters)
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1613|1613| 		}
|1614|1614| 		// remove the rows and columns in the matrix for the merged clusters,
|1615|1615| 		// and the clusters themselves from the cluster list
|1616|    |-		clusters.splice(closeClusters[0],1);
|    |1616|+		clusters.splice(closeClusters[0], 1);
|1617|1617| 		clusters.splice(closeClusters[1],1);
|1618|1618| 		matrix.splice(closeClusters[0],1);
|1619|1619| 		matrix.splice(closeClusters[1],1);
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1614|1614| 		// remove the rows and columns in the matrix for the merged clusters,
|1615|1615| 		// and the clusters themselves from the cluster list
|1616|1616| 		clusters.splice(closeClusters[0],1);
|1617|    |-		clusters.splice(closeClusters[1],1);
|    |1617|+		clusters.splice(closeClusters[1], 1);
|1618|1618| 		matrix.splice(closeClusters[0],1);
|1619|1619| 		matrix.splice(closeClusters[1],1);
|1620|1620| 		for (let i = 0; i < matrix.length; ++i)
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1615|1615| 		// and the clusters themselves from the cluster list
|1616|1616| 		clusters.splice(closeClusters[0],1);
|1617|1617| 		clusters.splice(closeClusters[1],1);
|1618|    |-		matrix.splice(closeClusters[0],1);
|    |1618|+		matrix.splice(closeClusters[0], 1);
|1619|1619| 		matrix.splice(closeClusters[1],1);
|1620|1620| 		for (let i = 0; i < matrix.length; ++i)
|1621|1621| 		{
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1616|1616| 		clusters.splice(closeClusters[0],1);
|1617|1617| 		clusters.splice(closeClusters[1],1);
|1618|1618| 		matrix.splice(closeClusters[0],1);
|1619|    |-		matrix.splice(closeClusters[1],1);
|    |1619|+		matrix.splice(closeClusters[1], 1);
|1620|1620| 		for (let i = 0; i < matrix.length; ++i)
|1621|1621| 		{
|1622|1622| 			if (matrix[i].length > closeClusters[0])
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1620|1620| 		for (let i = 0; i < matrix.length; ++i)
|1621|1621| 		{
|1622|1622| 			if (matrix[i].length > closeClusters[0])
|1623|    |-				matrix[i].splice(closeClusters[0],1);
|    |1623|+				matrix[i].splice(closeClusters[0], 1);
|1624|1624| 			if (matrix[i].length > closeClusters[1])
|1625|1625| 				matrix[i].splice(closeClusters[1],1);
|1626|1626| 		}
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/helpers/Commands.js
|1622|1622| 			if (matrix[i].length > closeClusters[0])
|1623|1623| 				matrix[i].splice(closeClusters[0],1);
|1624|1624| 			if (matrix[i].length > closeClusters[1])
|1625|    |-				matrix[i].splice(closeClusters[1],1);
|    |1625|+				matrix[i].splice(closeClusters[1], 1);
|1626|1626| 		}
|1627|1627| 		// add a new row of distances to the matrix and the new cluster
|1628|1628| 		clusters.push(newCluster);

binaries/data/mods/public/simulation/helpers/Commands.js
|  47| »   if·(g_Commands[cmd.type])
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'g_Commands' was used before it was defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|  51| »   »   g_Commands[cmd.type](player,·cmd,·data);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'g_Commands' was used before it was defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 792| »   »   let·ent·=·pickRandom(cmpRangeManager.GetEntitiesByPlayer(cmd.player).filter(ent·=>·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ent' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Commands.js
|1277| ····»   »   »   error("[TryConstructWall]·Expected·last·tower·control·group·to·be·available,·none·found·(1st·pass,·iteration·"·+·i·+·")");
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/helpers/Commands.js
|1278| ····»   »   »   break;
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/helpers/Commands.js
|1508| »   »   »   »   var·lastFormationTemplate·=·undefined;
|    | [NORMAL] ESLintBear (no-undef-init):
|    | It's not necessary to initialize 'lastFormationTemplate' to undefined.

binaries/data/mods/public/simulation/helpers/Commands.js
|  57| var·g_Commands·=·{
|    | [NORMAL] JSHintBear:
|    | 'g_Commands' was used before it was defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 535| »   »   »   »   ····&&·player·!=·+cmd.owner)
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/helpers/Commands.js
| 730| »   »   »   »   var·cmpGUIInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface);
|    | [NORMAL] JSHintBear:
|    | 'cmpGUIInterface' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 951| »   »   for·(var·i·=·0;·i·<·length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
| 964| »   »   var·count·=·0;
|    | [NORMAL] JSHintBear:
|    | 'count' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1111| »   »   var·cmpGuiInterface·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_GuiInterface);
|    | [NORMAL] JSHintBear:
|    | 'cmpGuiInterface' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1363| »   »   var·piece·=·pieces[j];
|    | [NORMAL] JSHintBear:
|    | 'piece' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1446| »   »   var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1485| »   »   »   &&·cmpFormation.GetMemberCount()·==·formation.entities.length)
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '&&'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/helpers/Commands.js
|1511| »   »   »   »   »   var·cmpUnitAI·=·Engine.QueryInterface(ent,·IID_UnitAI);
|    | [NORMAL] JSHintBear:
|    | 'cmpUnitAI' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1544| »   »   »   var·cmpFormation·=·Engine.QueryInterface(formationEnt,·IID_Formation);
|    | [NORMAL] JSHintBear:
|    | 'cmpFormation' is already defined.

binaries/data/mods/public/simulation/helpers/Commands.js
|1587| »   »   let·closeClusters·=·undefined;
|    | [NORMAL] JSHintBear:
|    | It's not necessary to initialize 'closeClusters' to 'undefined'.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2401/display/redirect

bb added a subscriber: bb.Jun 12 2020, 11:50 AM

I don't think this is the correct place to kill these commands, they should probably never be send: i.e. kill them in ccmpCommandQueue's PostNetworkCommand. However leaving this here is good two, since an ill-willed player could revert the code from ccmpCommandQueue and still crash everyone.

Stan added a subscriber: Stan.Jun 12 2020, 11:51 AM

He could revert it here too ^^

bb added a comment.Jun 12 2020, 11:59 AM

That is not true, since all players run the code in commands.js

In D2810#119790, @bb wrote:

I don't think this is the correct place to kill these commands, they should probably never be send: i.e. kill them in ccmpCommandQueue's PostNetworkCommand. However leaving this here is good two, since an ill-willed player could revert the code from ccmpCommandQueue and still crash everyone.

Arguably this is the correct place for the check, and as an optimisation they should never be sent.

Silier added a subscriber: Silier.Jun 12 2020, 12:37 PM
Silier added inline comments.
binaries/data/mods/public/simulation/helpers/Commands.js
11 ↗(On Diff #12255)

real question is, should not this return null for -1?

Freagarach added inline comments.Jun 12 2020, 12:42 PM
binaries/data/mods/public/simulation/helpers/Commands.js
11 ↗(On Diff #12255)

// All players at or below ID 0 get gaia-level data. (Observers for example) in PlayerManager.js.

wraitii added inline comments.Jun 12 2020, 1:47 PM
binaries/data/mods/public/simulation/helpers/Commands.js
11 ↗(On Diff #12255)

Introduced in rP14849.

This reads like a broken hack tbh, -1 is INVALID_PLAYER and returning the GAIA player interface won't work in a number of cases, including here for example.

GAIA also may or may not exist in general, so we should perhaps try to review the things that make this necessary and fix them.

Freagarach planned changes to this revision.Jun 16 2020, 7:19 PM

Underlying problem ought to be fixed.

Freagarach updated this revision to Diff 12397.Jun 20 2020, 1:13 PM

Possibly fix the underlying cause.

bb accepted this revision.Sep 21 2020, 9:16 PM

Note that the reveal map cheat does not work anymore for observers after this. Is that bad?

Observers can already see the whole map, slightly bad that observers don't have the retroMe cheat anymore, but well, meh.

binaries/data/mods/public/simulation/components/GuiInterface.js
656 ↗(On Diff #12397)

maybe safeguard too

656 ↗(On Diff #12397)
734–736 ↗(On Diff #12397)

This is correct since we can't trust the gui for asking for a valid player

binaries/data/mods/public/simulation/components/PlayerManager.js
90 ↗(On Diff #12397)

This has been there since the introduction of observer mode: rP14849, looks like a hack to me

This revision is now accepted and ready to land.Sep 21 2020, 9:16 PM
bb added inline comments.Sep 21 2020, 10:17 PM
binaries/data/mods/public/simulation/components/PlayerManager.js
91 ↗(On Diff #12397)
Stan added a comment.Sep 21 2020, 10:29 PM
In D2810#132054, @bb wrote:

Observers can already see the whole map, slightly bad that observers don't have the retroMe cheat anymore, but well, meh.

D1442 for the record

Freagarach updated this revision to Diff 13518.Sep 22 2020, 8:14 PM
Freagarach marked 3 inline comments as done.

Inlines.

Thanks for the review @bb!

This revision was automatically updated to reflect the committed changes.