From 8f974fe7413fb0302c11d119d29bafad14533434 Mon Sep 17 00:00:00 2001 From: John Stevenson Date: Fri, 8 Oct 2021 19:55:35 +0100 Subject: [PATCH] Improve Windows escaping --- src/Composer/Util/ProcessExecutor.php | 47 +++++++--- .../Test/Downloader/GitDownloaderTest.php | 2 +- .../Test/Util/ProcessExecutorTest.php | 86 +++++++++++++++++++ tests/Composer/Test/Util/SvnTest.php | 2 +- 4 files changed, 121 insertions(+), 16 deletions(-) diff --git a/src/Composer/Util/ProcessExecutor.php b/src/Composer/Util/ProcessExecutor.php index 7cb83e39b..fe7f06845 100644 --- a/src/Composer/Util/ProcessExecutor.php +++ b/src/Composer/Util/ProcessExecutor.php @@ -13,6 +13,7 @@ namespace Composer\Util; use Composer\IO\IOInterface; +use Composer\Util\Platform; use Symfony\Component\Process\Process; use Symfony\Component\Process\ProcessUtils; @@ -155,7 +156,15 @@ class ProcessExecutor } /** - * Copy of Symfony's Process::escapeArgument() which is private + * Escapes a string to be used as a shell argument for Symfony Process. + * + * This method expects cmd.exe to be started with the /V:ON option, which + * enables delayed environment variable expansion using ! as the delimiter. + * If this is not the case, any escaped ^^!var^^! will be transformed to + * ^!var^! and introduce two unintended carets. + * + * Modified from https://github.com/johnstevenson/winbox-args + * MIT Licensed (c) John Stevenson * * @param string $argument * @@ -163,25 +172,35 @@ class ProcessExecutor */ private static function escapeArgument($argument) { - if ('' === $argument || null === $argument) { - return '""'; + if ('' === ($argument = (string) $argument)) { + return escapeshellarg($argument); } - if ('\\' !== \DIRECTORY_SEPARATOR) { + + if (!Platform::isWindows()) { return "'".str_replace("'", "'\\''", $argument)."'"; } - if (false !== strpos($argument, "\0")) { - $argument = str_replace("\0", '?', $argument); + + if (strpbrk($argument, "\r\n") !== false) { + $argument = preg_replace("/(\r\n|\n|\r)/", ' ', $argument); } - if (!preg_match('/[\/()%!^"<>&|\s]/', $argument)) { - return $argument; + + $quote = strpbrk($argument, " \t") !== false; + $argument = preg_replace('/(\\\\*)"/', '$1$1\\"', $argument, -1, $dquotes); + $meta = $dquotes || preg_match('/%[^%]+%|![^!]+!/', $argument); + + if (!$meta && !$quote) { + $quote = strpbrk($argument, '^&|<>()') !== false; } - $argument = preg_replace('/(\\\\+)$/', '$1$1', $argument); - return '"'.str_replace(array('"', '^', '%', '!', "\n"), array('""', '"^^"', '"^%"', '"^!"', '!LF!'), $argument).'"'; - } + if ($quote) { + $argument = '"'.preg_replace('/(\\\\*)$/', '$1$1', $argument).'"'; + } - private static function isSurroundedBy($arg, $char) - { - return 2 < strlen($arg) && $char === $arg[0] && $char === $arg[strlen($arg) - 1]; + if ($meta) { + $argument = preg_replace('/(["^&|<>()%])/', '^$1', $argument); + $argument = preg_replace('/(!)/', '^^$1', $argument); + } + + return $argument; } } diff --git a/tests/Composer/Test/Downloader/GitDownloaderTest.php b/tests/Composer/Test/Downloader/GitDownloaderTest.php index 3bc9686c5..cd446a73b 100644 --- a/tests/Composer/Test/Downloader/GitDownloaderTest.php +++ b/tests/Composer/Test/Downloader/GitDownloaderTest.php @@ -524,7 +524,7 @@ composer https://github.com/old/url (push) public function testUpdateDoesntThrowsRuntimeExceptionIfGitCommandFailsAtFirstButIsAbleToRecover() { - $expectedFirstGitUpdateCommand = $this->winCompat("(git remote set-url composer -- \"\" && git rev-parse --quiet --verify 'ref^{commit}' || (git fetch composer && git fetch --tags composer)) && git remote set-url composer -- \"\""); + $expectedFirstGitUpdateCommand = $this->winCompat("(git remote set-url composer -- '' && git rev-parse --quiet --verify 'ref^{commit}' || (git fetch composer && git fetch --tags composer)) && git remote set-url composer -- ''"); $expectedSecondGitUpdateCommand = $this->winCompat("(git remote set-url composer -- 'https://github.com/composer/composer' && git rev-parse --quiet --verify 'ref^{commit}' || (git fetch composer && git fetch --tags composer)) && git remote set-url composer -- 'https://github.com/composer/composer'"); $packageMock = $this->getMockBuilder('Composer\Package\PackageInterface')->getMock(); diff --git a/tests/Composer/Test/Util/ProcessExecutorTest.php b/tests/Composer/Test/Util/ProcessExecutorTest.php index 4ac0d570f..1ff1a778d 100644 --- a/tests/Composer/Test/Util/ProcessExecutorTest.php +++ b/tests/Composer/Test/Util/ProcessExecutorTest.php @@ -113,4 +113,90 @@ class ProcessExecutorTest extends TestCase $process->execute('php -r "echo \'foo\'.PHP_EOL;"'); $this->assertSame('foo'.PHP_EOL, $output->fetch()); } + + /** + * Test various arguments are escaped as expected + * + * @dataProvider dataEscapeArguments + */ + public function testEscapeArgument($argument, $win, $unix) + { + $expected = defined('PHP_WINDOWS_VERSION_BUILD') ? $win : $unix; + $this->assertSame($expected, ProcessExecutor::escape($argument)); + } + + /** + * Each named test is an array of: + * argument, win-expected, unix-expected + */ + public function dataEscapeArguments() + { + return array( + // empty argument - must be quoted + 'empty' => array('', '""', "''"), + + // null argument - must be quoted + 'empty null' => array(null, '""', "''"), + + // false argument - must be quoted + 'empty false' => array(false, '""', "''"), + + // unix single-quote must be escaped + 'unix-sq' => array("a'bc", "a'bc", "'a'\\''bc'"), + + // CR LF must be replaced + 'crlf' => array("a\r\nb\nc\rd", '"a b c d"', "'a\r\nb\nc\rd'"), + + // whitespace must be quoted + 'ws space' => array('a b c', '"a b c"', "'a b c'"), + + // whitespace must be quoted + 'ws tab' => array("a\tb\tc", "\"a\tb\tc\"", "'a\tb\tc'"), + + // no whitespace must not be quoted + 'no-ws' => array('abc', 'abc', "'abc'"), + + // double-quotes must be backslash-escaped + 'dq' => array('a"bc', 'a\^"bc', "'a\"bc'"), + + // double-quotes must be backslash-escaped with preceeding backslashes doubled + 'dq-bslash' => array('a\\"bc', 'a\\\\\^"bc', "'a\\\"bc'"), + + // backslashes not preceeding a double-quote are treated as literal + 'bslash' => array('ab\\\\c\\', 'ab\\\\c\\', "'ab\\\\c\\'"), + + // trailing backslashes must be doubled up when the argument is quoted + 'bslash dq' => array('a b c\\\\', '"a b c\\\\\\\\"', "'a b c\\\\'"), + + // meta: outer double-quotes must be caret-escaped as well + 'meta dq' => array('a "b" c', '^"a \^"b\^" c^"', "'a \"b\" c'"), + + // meta: percent expansion must be caret-escaped + 'meta-pc1' => array('%path%', '^%path^%', "'%path%'"), + + // meta: expansion must have two percent characters + 'meta-pc2' => array('%path', '%path', "'%path'"), + + // meta: expansion must have have two surrounding percent characters + 'meta-pc3' => array('%%path', '%%path', "'%%path'"), + + // meta: bang expansion must be double caret-escaped + 'meta-bang1' => array('!path!', '^^!path^^!', "'!path!'"), + + // meta: bang expansion must have two bang characters + 'meta-bang2' => array('!path', '!path', "'!path'"), + + // meta: bang expansion must have two surrounding ang characters + 'meta-bang3' => array('!!path', '!!path', "'!!path'"), + + // meta: caret-escaping must escape all other meta chars (triggered by double-quote) + 'meta-all-dq' => array('<>"&|()^', '^<^>\^"^&^|^(^)^^', "'<>\"&|()^'"), + + // other meta: no caret-escaping when whitespace in argument + 'other meta' => array('<> &| ()^', '"<> &| ()^"', "'<> &| ()^'"), + + // other meta: quote escape chars when no whitespace in argument + 'other-meta' => array('<>&|()^', '"<>&|()^"', "'<>&|()^'"), + ); + } } diff --git a/tests/Composer/Test/Util/SvnTest.php b/tests/Composer/Test/Util/SvnTest.php index 8e1db31ec..ada3818ae 100644 --- a/tests/Composer/Test/Util/SvnTest.php +++ b/tests/Composer/Test/Util/SvnTest.php @@ -47,7 +47,7 @@ class SvnTest extends TestCase return array( array('http://till:test@svn.example.org/', $this->getCmd(" --username 'till' --password 'test' ")), array('http://svn.apache.org/', ''), - array('svn://johndoe@example.org', $this->getCmd(" --username 'johndoe' --password \"\" ")), + array('svn://johndoe@example.org', $this->getCmd(" --username 'johndoe' --password '' ")), ); }