From f06df01e8df0392a51c32b56884f9241803117f1 Mon Sep 17 00:00:00 2001 From: Muller Sebastien Date: Tue, 6 Jul 2021 14:00:15 +0200 Subject: [PATCH 1/5] Changed taskExec to use symfony process placeholders --- src/Common/CommandArguments.php | 63 ++++++++------ src/Common/ProcessUtils.php | 85 ++++++++----------- src/Task/Base/Exec.php | 6 +- src/Task/Remote/Rsync.php | 6 ++ tests/integration/ExecTest.php | 4 + tests/phpunit/Common/CommandArgumentsTest.php | 41 ++------- tests/phpunit/Task/RsyncTest.php | 20 ++--- .../Traits/Common/CommandArgumentsHost.php | 3 +- 8 files changed, 110 insertions(+), 118 deletions(-) diff --git a/src/Common/CommandArguments.php b/src/Common/CommandArguments.php index 276b82b08..9c232aff3 100644 --- a/src/Common/CommandArguments.php +++ b/src/Common/CommandArguments.php @@ -15,7 +15,35 @@ trait CommandArguments protected $arguments = ''; /** - * Pass argument to executable. Its value will be automatically escaped. + * @var array + */ + protected $argumentsEnv = []; + + + /** + * Adds an argument and its placeholder value, to be executed + * + * @param string $argument + * @param string|null $key + * @param string|null $prefix + * @param string|null $separator + * @return void + */ + public function addArgument($argument, $key = null, $prefix = null, $separator = ' ') + { + if (is_null($key)) { + $key = "arg" . md5($argument); + } + + $this->arguments .= null == $prefix ? '' : $separator . $prefix; + if (!is_null($argument)) { + $this->arguments .= ' "${:' . $key . '}"'; + $this->argumentsEnv[$key] = $argument; + } + } + + /** + * Pass argument to executable. It will be changed to a placeholder. * * @param string $arg * @@ -27,8 +55,8 @@ public function arg($arg) } /** - * Pass methods parameters as arguments to executable. Argument values - * are automatically escaped. + * Pass methods parameters as arguments to executable. Argument are + * automatically passed as placeholders * * @param string|string[] $args * @@ -40,7 +68,10 @@ public function args($args) if (!is_array($args)) { $args = $func_args; } - $this->arguments .= ' ' . implode(' ', array_map('static::escape', $args)); + + foreach ($args as $arg) { + $this->addArgument($arg); + } return $this; } @@ -58,25 +89,9 @@ public function rawArg($arg) return $this; } - /** - * Escape the provided value, unless it contains only alphanumeric - * plus a few other basic characters. - * - * @param string $value - * - * @return string - */ - public static function escape($value) - { - if (preg_match('/^[a-zA-Z0-9\/\.@~_-]+$/', $value)) { - return $value; - } - return ProcessUtils::escapeArgument($value); - } - /** * Pass option to executable. Options are prefixed with `--` , value can be provided in second parameter. - * Option values are automatically escaped. + * Option values are automatically passed as placeholders. * * @param string $option * @param string $value @@ -89,15 +104,15 @@ public function option($option, $value = null, $separator = ' ') if ($option !== null and strpos($option, '-') !== 0) { $option = "--$option"; } - $this->arguments .= null == $option ? '' : " " . $option; - $this->arguments .= null == $value ? '' : $separator . static::escape($value); + + $this->addArgument($value, null, $option, $separator); return $this; } /** * Pass multiple options to executable. The associative array contains * the key:value pairs that become `--key value`, for each item in the array. - * Values are automatically escaped. + * Values are passed as placeholders. * * @param array $options * @param string $separator diff --git a/src/Common/ProcessUtils.php b/src/Common/ProcessUtils.php index 1d8725e35..ac4cb0220 100644 --- a/src/Common/ProcessUtils.php +++ b/src/Common/ProcessUtils.php @@ -10,9 +10,9 @@ use Symfony\Component\Process\Exception\InvalidArgumentException; /** - * ProcessUtils is a bunch of utility methods. We want to allow Robo 1.x - * to work with Symfony 4.x while remaining backwards compatibility. This - * requires us to replace some deprecated functionality removed in Symfony. + * ProcessUtils is a bunch of utility methods. + * These methods are usually private, and are needed to execute and escape + * some functions for display purposes */ class ProcessUtils { @@ -24,58 +24,47 @@ private function __construct() } /** - * Escapes a string to be used as a shell argument. + * Symfony Process has a private method to replace Placeholders in command lines, + * which we use to rebuild a Command Description. * - * This method is a copy of a method that was deprecated by Symfony 3.3 and - * removed in Symfony 4; it will be removed once there is an actual - * replacement for escapeArgument. - * - * @param string $argument - * The argument that will be escaped. - * - * @return string - * The escaped argument. + * @param string $commandline + * @param array $env + * @return void */ - public static function escapeArgument($argument) + public static function replacePlaceholders(string $commandline, array $env) { - //Fix for PHP bug #43784 escapeshellarg removes % from given string - //Fix for PHP bug #49446 escapeshellarg doesn't work on Windows - //@see https://bugs.php.net/bug.php?id=43784 - //@see https://bugs.php.net/bug.php?id=49446 - if ('\\' === DIRECTORY_SEPARATOR) { - if ('' === $argument) { - return escapeshellarg($argument); + return preg_replace_callback('/"\$\{:([_a-zA-Z]++[_a-zA-Z0-9]*+)\}"/', function ($matches) use ($commandline, $env) { + if (!isset($env[$matches[1]]) || false === $env[$matches[1]]) { + throw new InvalidArgumentException(sprintf('Command line is missing a value for parameter "%s": ', $matches[1]).$commandline); } - $escapedArgument = ''; - $quote = false; - foreach (preg_split('/(")/', $argument, -1, PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE) as $part) { - if ('"' === $part) { - $escapedArgument .= '\\"'; - } elseif (self::isSurroundedBy($part, '%')) { - // Avoid environment variable expansion - $escapedArgument .= '^%"' . substr($part, 1, -1) . '"^%'; - } else { - // escape trailing backslash - if ('\\' === substr($part, -1)) { - $part .= '\\'; - } - $quote = true; - $escapedArgument .= $part; - } - } - if ($quote) { - $escapedArgument = '"' . $escapedArgument . '"'; - } - - return $escapedArgument; - } - - return "'" . str_replace("'", "'\\''", $argument) . "'"; + return self::escapeArgument($env[$matches[1]]); + }, $commandline); } - private static function isSurroundedBy($arg, $char) + /** + * Used by Symfony Process to form the final command line, with escaped parameters. + * Robo uses placeholders to handle the process arguments without escaping the whole string. + * + * @param string|null $argument + * @return string + */ + public static function escapeArgument(?string $argument): string { - return 2 < strlen($arg) && $char === $arg[0] && $char === $arg[strlen($arg) - 1]; + if ('' === $argument || null === $argument) { + return '""'; + } + if ('\\' !== \DIRECTORY_SEPARATOR) { + return "'".str_replace("'", "'\\''", $argument)."'"; + } + if (false !== strpos($argument, "\0")) { + $argument = str_replace("\0", '?', $argument); + } + if (!preg_match('/[\/()%!^"<>&|\s]/', $argument)) { + return $argument; + } + $argument = preg_replace('/(\\\\+)$/', '$1$1', $argument); + + return '"'.str_replace(['"', '^', '%', '!', "\n"], ['""', '"^^"', '"^%"', '"^!"', '!LF!'], $argument).'"'; } } diff --git a/src/Task/Base/Exec.php b/src/Task/Base/Exec.php index 5d08e3063..df94f940f 100644 --- a/src/Task/Base/Exec.php +++ b/src/Task/Base/Exec.php @@ -11,6 +11,7 @@ use Robo\Result; use Robo\Common\CommandReceiver; use Robo\Common\ExecOneCommand; +use Robo\Common\ProcessUtils; /** * Executes shell script. Closes it when running in background mode. @@ -98,8 +99,9 @@ public function background($arg = true) */ protected function getCommandDescription() { - return $this->getCommand(); + return ProcessUtils::replacePlaceholders($this->getCommand(), $this->argumentsEnv); } + /** * {@inheritdoc} */ @@ -132,7 +134,7 @@ public function run() { $this->hideProgressIndicator(); // TODO: Symfony 4 requires that we supply the working directory. - $result_data = $this->execute(Process::fromShellCommandline($this->getCommand(), getcwd())); + $result_data = $this->execute(Process::fromShellCommandline($this->getCommand(), getcwd(), $this->argumentsEnv)); $result = new Result( $this, $result_data->getExitCode(), diff --git a/src/Task/Remote/Rsync.php b/src/Task/Remote/Rsync.php index eb463be8b..daca8d880 100644 --- a/src/Task/Remote/Rsync.php +++ b/src/Task/Remote/Rsync.php @@ -6,6 +6,7 @@ use Robo\Task\BaseTask; use Robo\Exception\TaskException; use Robo\Common\ExecOneCommand; +use Robo\Common\ProcessUtils; /** * Executes rsync in a flexible manner. @@ -432,6 +433,11 @@ public function run() return $this->executeCommand($command); } + public function getCommandDescription() + { + return ProcessUtils::replacePlaceholders($this->getCommand(), $this->argumentsEnv); + } + /** * Returns command that can be executed. * This method is used to pass generated command from one task to another. diff --git a/tests/integration/ExecTest.php b/tests/integration/ExecTest.php index ab2791a63..e5baa1746 100644 --- a/tests/integration/ExecTest.php +++ b/tests/integration/ExecTest.php @@ -29,6 +29,10 @@ public function testExecLsCommand() public function testMultipleEnvVars() { + if (strncasecmp(PHP_OS, 'WIN', 3) == 0) { + $this->markTestSkipped('Environment variables is not supported on Windows.'); + } + $task = $this->taskExec('env')->interactive(false); $task->env('FOO', 'BAR'); $task->env('BAR', 'BAZ'); diff --git a/tests/phpunit/Common/CommandArgumentsTest.php b/tests/phpunit/Common/CommandArgumentsTest.php index 0a5c189ea..b0a73f54e 100644 --- a/tests/phpunit/Common/CommandArgumentsTest.php +++ b/tests/phpunit/Common/CommandArgumentsTest.php @@ -13,58 +13,33 @@ class CommandArgumentsTest extends TestCase public function casesArgs() { return [ 'no arguments' => [ - ' ', - ' ', + '', + '', [], ], 'empty string' => [ - " ''", ' ""', - [''], + ' ""', + [""], ], 'space' => [ " ' '", ' " "', [' '], ], - 'no escape - a' => [ - " a", - " a", - ['a'], - ], - 'no escape - A' => [ - " A", - " A", - ['A'], - ], - 'no escape - 0' => [ - " 0", - " 0", - ['0'], - ], - 'no escape - --' => [ - " --", - " --", - ['--'], - ], - 'no escape - @_~.' => [ - " @_~.", - " @_~.", - ['@_~.'], - ], '$' => [ " 'a\$b'", - ' "a$b"', + ' a$b', ['a$b'], ], '*' => [ " 'a*b'", - ' "a*b"', + ' a*b', ['a*b'], ], 'multi' => [ - " '' a '\$PATH'", - ' "" a "$PATH"', + ' "" \'a\' \'$PATH\'', + ' "" a $PATH', ['', 'a', '$PATH'], ], ]; diff --git a/tests/phpunit/Task/RsyncTest.php b/tests/phpunit/Task/RsyncTest.php index 48db2b9d3..8d07e1e33 100644 --- a/tests/phpunit/Task/RsyncTest.php +++ b/tests/phpunit/Task/RsyncTest.php @@ -7,9 +7,9 @@ class RsyncTest extends TestCase // tests public function testRsync() { - $linuxCmd = 'rsync --recursive --exclude .git --exclude .svn --exclude .hg --checksum --whole-file --verbose --progress --human-readable --stats src/ \'dev@localhost:/var/www/html/app/\''; + $linuxCmd = "rsync --recursive --exclude '.git' --exclude '.svn' --exclude '.hg' --checksum --whole-file --verbose --progress --human-readable --stats 'src/' 'dev@localhost:/var/www/html/app/'"; - $winCmd = 'rsync --recursive --exclude .git --exclude .svn --exclude .hg --checksum --whole-file --verbose --progress --human-readable --stats src/ "dev@localhost:/var/www/html/app/"'; + $winCmd = 'rsync --recursive --exclude .git --exclude .svn --exclude .hg --checksum --whole-file --verbose --progress --human-readable --stats "src/" "dev@localhost:/var/www/html/app/"'; $cmd = stripos(PHP_OS, 'WIN') === 0 ? $winCmd : $linuxCmd; @@ -28,7 +28,7 @@ public function testRsync() ->progress() ->humanReadable() ->stats() - ->getCommand() + ->getCommandDescription() ); $linuxCmd = 'rsync \'src/foo bar/baz\' \'dev@localhost:/var/path/with/a space\''; @@ -45,12 +45,12 @@ public function testRsync() ->toHost('localhost') ->toUser('dev') ->toPath('/var/path/with/a space') - ->getCommand() + ->getCommandDescription() ); - $linuxCmd = 'rsync src/foo src/bar \'dev@localhost:/var/path/with/a space\''; + $linuxCmd = "rsync 'src/foo' 'src/bar' 'dev@localhost:/var/path/with/a space'"; - $winCmd = 'rsync src/foo src/bar "dev@localhost:/var/path/with/a space"'; + $winCmd = 'rsync "src/foo" "src/bar" "dev@localhost:/var/path/with/a space"'; $cmd = stripos(PHP_OS, 'WIN') === 0 ? $winCmd : $linuxCmd; @@ -62,12 +62,12 @@ public function testRsync() ->toHost('localhost') ->toUser('dev') ->toPath('/var/path/with/a space') - ->getCommand() + ->getCommandDescription() ); - $linuxCmd = 'rsync --rsh \'ssh -i ~/.ssh/id_rsa\' src/foo \'dev@localhost:/var/path\''; + $linuxCmd = "rsync --rsh 'ssh -i ~/.ssh/id_rsa' 'src/foo' 'dev@localhost:/var/path'"; - $winCmd = 'rsync --rsh "ssh -i ~/.ssh/id_rsa" src/foo "dev@localhost:/var/path"'; + $winCmd = 'rsync --rsh "ssh -i ~/.ssh/id_rsa" "src/foo" "dev@localhost:/var/path"'; $cmd = stripos(PHP_OS, 'WIN') === 0 ? $winCmd : $linuxCmd; @@ -80,7 +80,7 @@ public function testRsync() ->toUser('dev') ->toPath('/var/path') ->remoteShell('ssh -i ~/.ssh/id_rsa') - ->getCommand() + ->getCommandDescription() ); } } diff --git a/tests/src/Traits/Common/CommandArgumentsHost.php b/tests/src/Traits/Common/CommandArgumentsHost.php index 363764dcd..9d1b7ef90 100644 --- a/tests/src/Traits/Common/CommandArgumentsHost.php +++ b/tests/src/Traits/Common/CommandArgumentsHost.php @@ -3,6 +3,7 @@ namespace Robo\Traits\Common; use Robo\Common\CommandArguments; +use Robo\Common\ProcessUtils; class CommandArgumentsHost { @@ -13,6 +14,6 @@ class CommandArgumentsHost */ public function getArguments() { - return $this->arguments; + return ProcessUtils::replacePlaceholders($this->arguments, $this->argumentsEnv); } } From 0da2288e9caed72bcad263b3ff67716a9a60bc95 Mon Sep 17 00:00:00 2001 From: Sebastien Muller Date: Tue, 6 Jul 2021 16:06:29 +0200 Subject: [PATCH 2/5] Fix addArgument access modifier --- src/Common/CommandArguments.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Common/CommandArguments.php b/src/Common/CommandArguments.php index 9c232aff3..c6fb3507d 100644 --- a/src/Common/CommandArguments.php +++ b/src/Common/CommandArguments.php @@ -29,7 +29,7 @@ trait CommandArguments * @param string|null $separator * @return void */ - public function addArgument($argument, $key = null, $prefix = null, $separator = ' ') + protected function addArgument($argument, $key = null, $prefix = null, $separator = ' ') { if (is_null($key)) { $key = "arg" . md5($argument); From 4fbd183801c767b5e8b939632f6e5c828a1fa7dc Mon Sep 17 00:00:00 2001 From: Sebastien Muller Date: Wed, 7 Jul 2021 09:49:49 +0200 Subject: [PATCH 3/5] Restore CommandArguments escape() method with a deprecated doc --- src/Common/CommandArguments.php | 13 +++++++++++++ src/Common/ProcessUtils.php | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/Common/CommandArguments.php b/src/Common/CommandArguments.php index c6fb3507d..287260596 100644 --- a/src/Common/CommandArguments.php +++ b/src/Common/CommandArguments.php @@ -42,6 +42,19 @@ protected function addArgument($argument, $key = null, $prefix = null, $separato } } + /** + * Escape a command line argument. + * + * @param string $argument + * @return string + * + * @deprecated Use \Robo\Common\ProcessUtils::escapeArgument() instead. + */ + public function escape($argument) + { + return ProcessUtils::escapeArgument($argument); + } + /** * Pass argument to executable. It will be changed to a placeholder. * diff --git a/src/Common/ProcessUtils.php b/src/Common/ProcessUtils.php index ac4cb0220..07b55d1ad 100644 --- a/src/Common/ProcessUtils.php +++ b/src/Common/ProcessUtils.php @@ -29,7 +29,7 @@ private function __construct() * * @param string $commandline * @param array $env - * @return void + * @return string */ public static function replacePlaceholders(string $commandline, array $env) { From 19fe04322193645798fe54b5e1c34d0771671bd4 Mon Sep 17 00:00:00 2001 From: Sebastien Muller Date: Wed, 7 Jul 2021 17:54:49 +0200 Subject: [PATCH 4/5] Fix static method --- src/Common/CommandArguments.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Common/CommandArguments.php b/src/Common/CommandArguments.php index 287260596..97b349ca0 100644 --- a/src/Common/CommandArguments.php +++ b/src/Common/CommandArguments.php @@ -50,7 +50,7 @@ protected function addArgument($argument, $key = null, $prefix = null, $separato * * @deprecated Use \Robo\Common\ProcessUtils::escapeArgument() instead. */ - public function escape($argument) + public static function escape($argument) { return ProcessUtils::escapeArgument($argument); } From 3a87e4a826e506d80faf4f34f1065b159410dc1a Mon Sep 17 00:00:00 2001 From: Sebastien Muller Date: Thu, 8 Jul 2021 14:33:56 +0200 Subject: [PATCH 5/5] Restored escape() previous behaviour --- src/Common/CommandArguments.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Common/CommandArguments.php b/src/Common/CommandArguments.php index 97b349ca0..5de5853a9 100644 --- a/src/Common/CommandArguments.php +++ b/src/Common/CommandArguments.php @@ -52,6 +52,9 @@ protected function addArgument($argument, $key = null, $prefix = null, $separato */ public static function escape($argument) { + if (preg_match('/^[a-zA-Z0-9\/\.@~_-]+$/', $argument)) { + return $argument; + } return ProcessUtils::escapeArgument($argument); }