Merge pull request #3364 from nextcloud/backport/3361/stable16

[stable16] Improve command arguments escaping

8 files changed, 88 insertions, 85 deletions

diff --git a/docs/commands.md b/docs/commands.md
index 88abf6bb0..2fcbd415c 100644
--- a/docs/commands.md
+++ b/docs/commands.md
@@ -22,7 +22,6 @@ Parameter | Description
 `{ROOM}` | The token of the room the command was used in
 `{USER}` | ID of the user that called the command
 `{ARGUMENTS}` | Everything the user write after the actual command
-`{ARGUMENTS_DOUBLEQUOTE_ESCAPED}` | … but with double quotes `"` escaped.
 
 ### Example
 
@@ -55,7 +54,7 @@ It should return a useful description, the first line is also displayed in a lis
 * `./occ` command used to add the command:
 
     ```
-    ./occ talk:command:add calculator calculator "/path/to/calc.sh \"{ARGUMENTS_DOUBLEQUOTE_ESCAPED}\" {ROOM} {USER}" 1 3
+    ./occ talk:command:add calculator calculator "/path/to/calc.sh {ARGUMENTS} {ROOM} {USER}" 1 3
     ```
     
 * User input by user `my user id` in the chat of room `index.php/call/4tf349j`:
@@ -68,7 +67,7 @@ It should return a useful description, the first line is also displayed in a lis
 * Executed shell command:
 
     ```
-    /path/to/calc.sh "1 + 2 + 3 + \"hello\"" '4tf349j' 'my user id'
+    /path/to/calc.sh '1 + 2 + 3 + "hello"' '4tf349j' 'my user id'
     ```
 
 ## Aliases
diff --git a/lib/Chat/Command/ShellExecutor.php b/lib/Chat/Command/ShellExecutor.php
index b2a735d92..12f98c1d1 100644
--- a/lib/Chat/Command/ShellExecutor.php
+++ b/lib/Chat/Command/ShellExecutor.php
@@ -28,6 +28,8 @@ class ShellExecutor {
 	public const PLACEHOLDER_ROOM = '{ROOM}';
 	public const PLACEHOLDER_USER = '{USER}';
 	public const PLACEHOLDER_ARGUMENTS = '{ARGUMENTS}';
+
+	// Legacy placeholder just returning the --help nowadays
 	public const PLACEHOLDER_ARGUMENTS_DOUBLEQUOTE_ESCAPED = '{ARGUMENTS_DOUBLEQUOTE_ESCAPED}';
 
 	/**
@@ -39,59 +41,23 @@ class ShellExecutor {
 	 * @throws \InvalidArgumentException
 	 */
 	public function execShell(string $cmd, string $arguments, string $room = '', string $user = ''): string {
+		if (strpos($cmd, self::PLACEHOLDER_ARGUMENTS_DOUBLEQUOTE_ESCAPED) !== false) {
+			throw new \InvalidArgumentException('Talk commands using the {ARGUMENTS_DOUBLEQUOTE_ESCAPED} are not supported anymore.');
+		}
+
 		$cmd = str_replace([
 			self::PLACEHOLDER_ROOM,
 			self::PLACEHOLDER_USER,
 			self::PLACEHOLDER_ARGUMENTS,
-			self::PLACEHOLDER_ARGUMENTS_DOUBLEQUOTE_ESCAPED,
 		], [
 			escapeshellarg($room),
 			escapeshellarg($user),
-			$this->escapeArguments($arguments),
-			str_replace('"', '\\"', $arguments),
+			escapeshellarg($arguments),
 		], $cmd);
 
 		return $this->wrapExec($cmd);
 	}
 
-	protected function escapeArguments(string $argumentString): string {
-		$arguments = explode(' ', $argumentString);
-
-		$result = [];
-		$buffer = [];
-		$quote = '';
-		foreach ($arguments as $argument) {
-			if ($quote === '') {
-				if (ltrim($argument, '"\'') === $argument) {
-					$result[] = escapeshellarg($argument);
-				} else {
-					$quote = $argument[0];
-					$temp = substr($argument, 1);
-					if (rtrim($temp, $quote) === $temp) {
-						$buffer[] = $temp;
-					} else {
-						$result[] = $quote . str_replace($quote, '\\'. $quote, substr($temp, 0, -1)) . $quote;
-						$quote = '';
-					}
-				}
-			} else if (rtrim($argument, $quote) === $argument) {
-				$buffer[] = $argument;
-			} else {
-				$buffer[] = substr($argument, 0, -1);
-
-				$result[] = $quote . str_replace($quote, '\\'. $quote, implode(' ', $buffer)) . $quote;
-				$quote = '';
-				$buffer = [];
-			}
-		}
-
-		if ($quote !== '') {
-			$result[] = escapeshellarg($quote . implode(' ', $buffer));
-		}
-
-		return implode(' ', $result);
-	}
-
 	/**
 	 * @param string $cmd
 	 * @return string
diff --git a/lib/Command/Command/Add.php b/lib/Command/Command/Add.php
index f57adc050..d9d2dd527 100644
--- a/lib/Command/Command/Add.php
+++ b/lib/Command/Command/Add.php
@@ -98,6 +98,9 @@ class Add extends Base {
 				case 5:
 					$output->writeln('<error>The enabled value is invalid</error>');
 					break;
+				case 6:
+					$output->writeln('<error>The placeholders {ROOM}, {USER} and {ARGUMENTS} must not be used inside quotes</error>');
+					break;
 				default:
 					$output->writeln('<error>The command could not be added</error>');
 					break;
diff --git a/lib/Command/Command/AddSamples.php b/lib/Command/Command/AddSamples.php
index 9b30b6ce4..a32cb48cb 100644
--- a/lib/Command/Command/AddSamples.php
+++ b/lib/Command/Command/AddSamples.php
@@ -67,7 +67,7 @@ class AddSamples extends Base {
 			$output,
 			'wiki',
 			'Wikipedia',
-			'php ' . $appPath . '/sample-commands/wikipedia.php "{ARGUMENTS_DOUBLEQUOTE_ESCAPED}"'
+			'php ' . $appPath . '/sample-commands/wikipedia.php {ARGUMENTS}'
 		);
 
 		$chmod = fileperms($appPath . '/sample-commands/calc.sh');
@@ -80,7 +80,7 @@ class AddSamples extends Base {
 				$output,
 				'calculator',
 				'Calculator',
-				$appPath . '/sample-commands/calc.sh "{ARGUMENTS_DOUBLEQUOTE_ESCAPED}"',
+				$appPath . '/sample-commands/calc.sh {ARGUMENTS}',
 				Command::RESPONSE_USER
 			);
 
@@ -97,7 +97,7 @@ class AddSamples extends Base {
 			$output,
 			'hackernews',
 			'Hacker News',
-			'php ' . $appPath . '/sample-commands/hackernews.php "{ARGUMENTS_DOUBLEQUOTE_ESCAPED}"'
+			'php ' . $appPath . '/sample-commands/hackernews.php {ARGUMENTS}'
 		);
 
 		if (empty($this->commands)) {
diff --git a/lib/Service/CommandService.php b/lib/Service/CommandService.php
index 78fdb2b74..2b25d9d83 100644
--- a/lib/Service/CommandService.php
+++ b/lib/Service/CommandService.php
@@ -142,6 +142,13 @@ class CommandService {
 					throw new \InvalidArgumentException('script', 3);
 				}
 			} else {
+				if (preg_match('/[`\'"]{(?:ARGUMENTS|ROOM|USER)}[`\'"]/i', $script)) {
+					throw new \InvalidArgumentException('script-parameters', 6);
+				}
+				if (strpos($script, '{ARGUMENTS_DOUBLEQUOTE_ESCAPED}') !== false) {
+					throw new \InvalidArgumentException('script-parameters', 6);
+				}
+
 				try {
 					$this->shellExecutor->execShell($script, '--help');
 				} catch (\InvalidArgumentException $e) {
diff --git a/tests/php/Chat/Command/ShellExecutorTest.php b/tests/php/Chat/Command/ShellExecutorTest.php
index 3d8b429d7..ae88dacf7 100644
--- a/tests/php/Chat/Command/ShellExecutorTest.php
+++ b/tests/php/Chat/Command/ShellExecutorTest.php
@@ -38,29 +38,57 @@ use Test\TestCase;
 
 class ShellExecutorTest extends TestCase {
 
-	/** @var EventDispatcherInterface|MockObject */
-	protected $dispatcher;
-
-	/** @var ShellExecutor|MockObject */
-	protected $shellExecutor;
-
-	/** @var CommandService|MockObject */
-	protected $commandService;
-
-	/** @var ILogger|MockObject */
-	protected $logger;
+	public function dataExecShellRun(): array {
+		return [
+			['admin', 'token', 'echo {ARGUMENTS}', '$PATH', '$PATH'],
+			['admin', 'token', 'echo {ARGUMENTS}', '$(pwd)', '$(pwd)'],
+			['admin', 'token', __DIR__ . '/echo-argument.sh {ARGUMENTS}', '$(pwd)', '$(pwd)'],
+			['admin', 'token', __DIR__ . '/echo-argument.sh {ARGUMENTS}', '$PATH', '$PATH'],
+			['admin', 'token', __DIR__ . '/echo-option.sh -a {ARGUMENTS}', '$(pwd)', '$(pwd)'],
+			['admin', 'token', __DIR__ . '/echo-option.sh -a {ARGUMENTS}', '$PATH', '$PATH'],
+
+			['admin', 'token', 'echo {ARGUMENTS}', '\\$PATH', '\\$PATH'],
+			['admin', 'token', 'echo {ARGUMENTS}', '\\$(pwd)', '\\$(pwd)'],
+			['admin', 'token', __DIR__ . '/echo-argument.sh {ARGUMENTS}', '\\$(pwd)', '\\$(pwd)'],
+			['admin', 'token', __DIR__ . '/echo-argument.sh {ARGUMENTS}', '\\$PATH', '\\$PATH'],
+			['admin', 'token', __DIR__ . '/echo-option.sh -a {ARGUMENTS}', '\\$(pwd)', '\\$(pwd)'],
+			['admin', 'token', __DIR__ . '/echo-option.sh -a {ARGUMENTS}', '\\$PATH', '\\$PATH'],
+
+			['admin', 'token', 'echo {ARGUMENTS}', '`echo $PATH`', '`echo $PATH`'],
+			['admin', 'token', 'echo {ARGUMENTS}', '`pwd`', '`pwd`'],
+			['admin', 'token', __DIR__ . '/echo-argument.sh {ARGUMENTS}', '`pwd`', '`pwd`'],
+			['admin', 'token', __DIR__ . '/echo-argument.sh {ARGUMENTS}', '`echo $PATH`', '`echo $PATH`'],
+			['admin', 'token', __DIR__ . '/echo-option.sh -a {ARGUMENTS}', '`pwd`', '`pwd`'],
+			['admin', 'token', __DIR__ . '/echo-option.sh -a {ARGUMENTS}', '`echo $PATH`', '`echo $PATH`'],
+
+			['admin', 'token', 'echo {ARGUMENTS}', '\\`echo $PATH\\`', '\\`echo $PATH\\`'],
+			['admin', 'token', 'echo {ARGUMENTS}', '\\`pwd \\`', '\\`pwd \\`'],
+			['admin', 'token', __DIR__ . '/echo-argument.sh {ARGUMENTS}', '\\`pwd \\`', '\\`pwd \\`'],
+			['admin', 'token', __DIR__ . '/echo-argument.sh {ARGUMENTS}', '\\`echo $PATH\\`', '\\`echo $PATH\\`'],
+			['admin', 'token', __DIR__ . '/echo-option.sh -a {ARGUMENTS}', '\\`pwd \\`', '\\`pwd \\`'],
+			['admin', 'token', __DIR__ . '/echo-option.sh -a {ARGUMENTS}', '\\`echo $PATH\\`', '\\`echo $PATH\\`'],
+		];
+	}
 
-	/** @var IL10N|MockObject */
-	protected $l10n;
+	/**
+	 * @dataProvider dataExecShellRun
+	 * @param string|null $actorId
+	 * @param string $roomToken
+	 * @param string $cmd
+	 * @param string $arguments
+	 * @param string $output
+	 */
+	public function testExecShellRun(?string $actorId, string $roomToken, string $cmd, string $arguments, string $output): void {
+		$executor = new ShellExecutor();
+		$this->assertSame($output, $executor->execShell($cmd, $arguments, $roomToken, $actorId));
 
-	/** @var Executor */
-	protected $executor;
+	}
 
 	public function dataExecShell(): array {
 		return [
 			['admin', 'token', '', '', '', ''],
-			['admin', 'token', '/var/www/nextcloud/script.sh {USER} {ROOM} {ARGUMENTS}', 'foo bar "hello bear"', "/var/www/nextcloud/script.sh 'admin' 'token' 'foo' 'bar' \"hello bear\"", 'output1'],
-			['admin', 'token', '/var/www/nextcloud/script.sh {USER} {ROOM} --arguments="{ARGUMENTS_DOUBLEQUOTE_ESCAPED}"', 'foo bar "hello bear"', "/var/www/nextcloud/script.sh 'admin' 'token' --arguments=\"foo bar \\\"hello bear\\\"\"", "out\nput\n2"],
+			['admin', 'token', '/var/www/nextcloud/script.sh {USER} {ROOM} {ARGUMENTS}', 'foo bar "hello bear"', "/var/www/nextcloud/script.sh 'admin' 'token' 'foo bar \"hello bear\"'", 'output1'],
+			['admin', 'token', '/var/www/nextcloud/script.sh {USER} {ROOM} --arguments {ARGUMENTS}', 'foo bar "hello bear"', "/var/www/nextcloud/script.sh 'admin' 'token' --arguments 'foo bar \"hello bear\"'", "out\nput\n2"],
 		];
 	}
 
@@ -84,29 +112,17 @@ class ShellExecutorTest extends TestCase {
 			->willReturn($output);
 
 		$this->assertSame($output, self::invokePrivate($executor, 'execShell', [$cmd, $arguments, $roomToken, $actorId]));
-
 	}
 
-	public function dataEscapeArguments(): array {
-		return [
-			['foobar',             "'foobar'"],
-			['foo bar',            "'foo' 'bar'"],
-			['"foo" bar',          "\"foo\" 'bar'"],
-			['"foo"bar',           "'\"foo\"bar'"],
-			['"foo bar"',          '"foo bar"'],
-			['"foo foo"bar bar"',  '"foo foo\\"bar bar"'],
-			['"foo foo\"bar bar"', '"foo foo\\\\"bar bar"'],
-			['" foo bar "',        '" foo bar "'],
-			['" foo bar ',         "'\" foo bar '"],
-		];
-	}
+	public function testLegacyArguments(): void {
+		$executor = $this->getMockBuilder(ShellExecutor::class)
+			->setMethods(['wrapExec'])
+			->getMock();
 
-	/**
-	 * @dataProvider dataEscapeArguments
-	 * @param string $arguments
-	 * @param string $expected
-	 */
-	public function testEscapeArguments(string $arguments, string $expected): void {
-		$this->assertSame($expected, self::invokePrivate(new ShellExecutor(), 'escapeArguments', [$arguments]));
+		$executor->expects($this->never())
+			->method('wrapExec');
+
+		$this->expectException(\InvalidArgumentException::class);
+		self::invokePrivate($executor, 'execShell', ['echo "{ARGUMENTS_DOUBLEQUOTE_ESCAPED}"', 'arguments', '', '']);
 	}
 }
diff --git a/tests/php/Chat/Command/echo-argument.sh b/tests/php/Chat/Command/echo-argument.sh
new file mode 100755
index 000000000..cf024e95c
--- /dev/null
+++ b/tests/php/Chat/Command/echo-argument.sh
@@ -0,0 +1 @@
+echo "$1"
diff --git a/tests/php/Chat/Command/echo-option.sh b/tests/php/Chat/Command/echo-option.sh
new file mode 100755
index 000000000..c263f955b
--- /dev/null
+++ b/tests/php/Chat/Command/echo-option.sh
@@ -0,0 +1,11 @@
+while getopts ":a:" o; do
+    case "${o}" in
+        a)
+            s=${OPTARG}
+            ;;
+        *)
+            ;;
+    esac
+done
+
+echo "${s}"