Harden ExecuteTask with per-arg escapeshellarg and allow-list

docs/sections/tasks/execute.md

@@ -35,12 +35,13 @@ The `params` array will be joined with spaces and appended to the command.
 
 ### escape (default: true)
 
-By default, the command and parameters are escaped using `escapeshellcmd()` for security:
+By default, the command and each entry in `params` are escaped per token using `escapeshellarg()` so that arguments cannot break out into additional shell tokens (argument injection):
 
 ```php
 $data = [
-    'command' => 'bin/cake importer run',
-    'escape' => true, // Default - commands are escaped
+    'command' => 'bin/cake',
+    'params' => ['importer', 'run'],
+    'escape' => true, // Default - command and params escaped per token
 ];
 ```
 
@@ -53,6 +54,24 @@ $data = [
 ];
 ```
 
+`escape => false` is hard-rejected unless `debug` is `true`.
+
+### Queue.executeAllowedCommands (production allow-list)
+
+When `debug` is disabled, the `command` value MUST appear verbatim in the `Queue.executeAllowedCommands` allow-list, otherwise the task throws before invoking `exec()`:
+
+```php
+// config/app.php (or app_local.php)
+'Queue' => [
+    'executeAllowedCommands' => [
+        'bin/cake',
+        '/usr/bin/php',
+    ],
+],
+```
+
+If the allow-list is unset or empty in production, every Execute job is rejected. This protects against an attacker who can write to `queued_jobs` (DB-level compromise, or upstream code that pipes user input into `createJob('Queue.Execute', ...)`) from steering the exec call.
+
 ### redirect (default: true)
 
 By default, stderr output is redirected to stdout (using `2>&1`). Disable if you need separate error handling:

src/Queue/Task/ExecuteTask.php

@@ -90,16 +90,27 @@ public function run(array $data, int $jobId): void {
 			throw new QueueException('Command escaping must be enabled when debug mode is off for security reasons');
 		}
 
-		$command = $data['command'];
+		$rawCommand = (string)$data['command'];
+		if (!Configure::read('debug')) {
+			$allowed = (array)Configure::read('Queue.executeAllowedCommands', []);
+			if (!$allowed || !in_array($rawCommand, $allowed, true)) {
+				throw new QueueException(
+					'Command `' . $rawCommand . '` is not in Queue.executeAllowedCommands allow-list',
+				);
+			}
+		}
+
 		if ($data['escape']) {
-			$command = escapeshellcmd($data['command']);
+			$command = escapeshellarg($rawCommand);
+		} else {
+			$command = $rawCommand;
 		}
 
 		if ($data['params']) {
 			$params = $data['params'];
 			if ($data['escape']) {
 				foreach ($params as $key => $value) {
-					$params[$key] = escapeshellcmd($value);
+					$params[$key] = escapeshellarg((string)$value);
 				}
 			}
 			$command .= ' ' . implode(' ', $params);

tests/TestCase/Queue/Task/ExecuteTaskTest.php

@@ -4,9 +4,11 @@
 namespace Queue\Test\TestCase\Queue\Task;
 
 use Cake\Console\ConsoleIo;
+use Cake\Core\Configure;
 use Cake\TestSuite\TestCase;
 use Exception;
 use Queue\Console\Io;
+use Queue\Model\QueueException;
 use Queue\Queue\Task\ExecuteTask;
 use RuntimeException;
 use Shim\TestSuite\ConsoleOutput;
@@ -57,7 +59,7 @@ public function setUp(): void {
 	 * @return void
 	 */
 	public function testRun() {
-		$this->Task->run(['command' => 'php -v'], 0);
+		$this->Task->run(['command' => 'php', 'params' => ['-v']], 0);
 
 		$this->assertTextContains('PHP ', $this->out->output());
 	}
@@ -68,13 +70,13 @@ public function testRun() {
 	public function testRunFailureWithRedirect() {
 		$exception = null;
 		try {
-			$this->Task->run(['command' => 'fooooobbbaraar -eeee'], 0);
+			$this->Task->run(['command' => 'fooooobbbaraar', 'params' => ['-eeee']], 0);
 		} catch (Exception $e) {
 			$exception = $e;
 		}
 
 		$this->assertInstanceOf(Exception::class, $exception);
-		$this->assertSame('Failed with error code 127: `fooooobbbaraar -eeee 2>&1`', $exception->getMessage());
+		$this->assertSame("Failed with error code 127: `'fooooobbbaraar' '-eeee' 2>&1`", $exception->getMessage());
 
 		$this->assertTextContains('Error (code 127)', $this->err->output());
 		$this->assertTextContains('fooooobbbaraar: not found', $this->out->output());
@@ -84,7 +86,7 @@ public function testRunFailureWithRedirect() {
 	 * @return void
 	 */
 	public function testRunFailureWithRedirectAndIgnoreCode() {
-		$this->Task->run(['command' => 'fooooobbbaraar -eeee', 'accepted' => []], 0);
+		$this->Task->run(['command' => 'fooooobbbaraar', 'params' => ['-eeee'], 'accepted' => []], 0);
 
 		$this->assertTextContains('Success (code 127)', $this->out->output());
 		$this->assertTextContains('fooooobbbaraar: not found', $this->out->output());
@@ -97,9 +99,93 @@ public function testRunFailureWithoutRedirect() {
 		$this->skipIf((bool)getenv('TRAVIS'), 'Not redirecting stderr to stdout prints noise to the CLI output in between test runs.');
 
 		$this->expectException(RuntimeException::class);
-		$this->expectExceptionMessage('Failed with error code 127: `fooooobbbaraar -eeee`');
+		$this->expectExceptionMessage("Failed with error code 127: `'fooooobbbaraar' '-eeee'`");
 
-		$this->Task->run(['command' => 'fooooobbbaraar -eeee', 'redirect' => false], 0);
+		$this->Task->run(['command' => 'fooooobbbaraar', 'params' => ['-eeee'], 'redirect' => false], 0);
+	}
+
+	/**
+	 * Per-token escapeshellarg neutralizes argument-injection payloads instead of
+	 * letting them re-tokenize against the shell. The rogue param has to be quoted as
+	 * a single argument and must not be interpreted as additional flags.
+	 *
+	 * @return void
+	 */
+	public function testRunEscapesArgumentInjectionPayloadAsSingleToken() {
+		$exception = null;
+		try {
+			$this->Task->run([
+				'command' => 'fooooobbbaraar',
+				'params' => ['-r --some-flag /etc/passwd'],
+			], 0);
+		} catch (Exception $e) {
+			$exception = $e;
+		}
+
+		$this->assertInstanceOf(Exception::class, $exception);
+		$this->assertStringContainsString(
+			"'fooooobbbaraar' '-r --some-flag /etc/passwd'",
+			(string)$exception->getMessage(),
+		);
+	}
+
+	/**
+	 * In production (debug=false) the command MUST be in the allow-list.
+	 * Without it, the task throws before any exec() happens.
+	 *
+	 * @return void
+	 */
+	public function testRunRejectsCommandNotInAllowListWhenDebugDisabled() {
+		Configure::write('debug', false);
+		Configure::write('Queue.executeAllowedCommands', ['php']);
+
+		$this->expectException(QueueException::class);
+		$this->expectExceptionMessage('Command `rm` is not in Queue.executeAllowedCommands allow-list');
+
+		try {
+			$this->Task->run(['command' => 'rm', 'params' => ['-rf', '/tmp/x']], 0);
+		} finally {
+			Configure::write('debug', true);
+			Configure::delete('Queue.executeAllowedCommands');
+		}
+	}
+
+	/**
+	 * In production (debug=false) the empty/missing allow-list MUST reject every command.
+	 *
+	 * @return void
+	 */
+	public function testRunRejectsAnyCommandWhenAllowListEmptyAndDebugDisabled() {
+		Configure::write('debug', false);
+		Configure::delete('Queue.executeAllowedCommands');
+
+		$this->expectException(QueueException::class);
+		$this->expectExceptionMessage('Command `php` is not in Queue.executeAllowedCommands allow-list');
+
+		try {
+			$this->Task->run(['command' => 'php', 'params' => ['-v']], 0);
+		} finally {
+			Configure::write('debug', true);
+		}
+	}
+
+	/**
+	 * In production with the command listed, the task runs as normal.
+	 *
+	 * @return void
+	 */
+	public function testRunPassesWhenCommandInAllowListAndDebugDisabled() {
+		Configure::write('debug', false);
+		Configure::write('Queue.executeAllowedCommands', ['php']);
+
+		try {
+			$this->Task->run(['command' => 'php', 'params' => ['-v']], 0);
+		} finally {
+			Configure::write('debug', true);
+			Configure::delete('Queue.executeAllowedCommands');
+		}
+
+		$this->assertTextContains('PHP ', $this->out->output());
 	}
 
 }