From 8f974fe7413fb0302c11d119d29bafad14533434 Mon Sep 17 00:00:00 2001 From: John Stevenson Date: Fri, 8 Oct 2021 19:55:35 +0100 Subject: [PATCH 1/3] Improve Windows escaping --- src/Composer/Util/ProcessExecutor.php | 51 +++++++---- .../Test/Downloader/GitDownloaderTest.php | 2 +- .../Test/Util/ProcessExecutorTest.php | 86 +++++++++++++++++++ tests/Composer/Test/Util/SvnTest.php | 2 +- 4 files changed, 123 insertions(+), 18 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 (!preg_match('/[\/()%!^"<>&|\s]/', $argument)) { - return $argument; - } - $argument = preg_replace('/(\\\\+)$/', '$1$1', $argument); - return '"'.str_replace(array('"', '^', '%', '!', "\n"), array('""', '"^^"', '"^%"', '"^!"', '!LF!'), $argument).'"'; - } + if (strpbrk($argument, "\r\n") !== false) { + $argument = preg_replace("/(\r\n|\n|\r)/", ' ', $argument); + } - private static function isSurroundedBy($arg, $char) - { - return 2 < strlen($arg) && $char === $arg[0] && $char === $arg[strlen($arg) - 1]; + $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; + } + + if ($quote) { + $argument = '"'.preg_replace('/(\\\\*)$/', '$1$1', $argument).'"'; + } + + 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 '' ")), ); } From 0783b043d2f392b4b21e77cf0abed8bedcd04cd6 Mon Sep 17 00:00:00 2001 From: John Stevenson Date: Fri, 8 Oct 2021 21:46:07 +0100 Subject: [PATCH 2/3] Fix Windows escaping in tests --- .../Test/Downloader/FossilDownloaderTest.php | 6 ------ .../Test/Downloader/GitDownloaderTest.php | 2 +- .../Test/Downloader/HgDownloaderTest.php | 6 ------ .../EventDispatcher/EventDispatcherTest.php | 2 +- tests/Composer/Test/TestCase.php | 21 +++++++++++++++++++ tests/Composer/Test/Util/SvnTest.php | 6 ------ 6 files changed, 23 insertions(+), 20 deletions(-) diff --git a/tests/Composer/Test/Downloader/FossilDownloaderTest.php b/tests/Composer/Test/Downloader/FossilDownloaderTest.php index ab8e0ab71..dedbc89fb 100644 --- a/tests/Composer/Test/Downloader/FossilDownloaderTest.php +++ b/tests/Composer/Test/Downloader/FossilDownloaderTest.php @@ -15,7 +15,6 @@ namespace Composer\Test\Downloader; use Composer\Downloader\FossilDownloader; use Composer\Test\TestCase; use Composer\Util\Filesystem; -use Composer\Util\Platform; class FossilDownloaderTest extends TestCase { @@ -168,9 +167,4 @@ class FossilDownloaderTest extends TestCase $this->assertEquals('source', $downloader->getInstallationSource()); } - - private function getCmd($cmd) - { - return Platform::isWindows() ? strtr($cmd, "'", '"') : $cmd; - } } diff --git a/tests/Composer/Test/Downloader/GitDownloaderTest.php b/tests/Composer/Test/Downloader/GitDownloaderTest.php index cd446a73b..71342f478 100644 --- a/tests/Composer/Test/Downloader/GitDownloaderTest.php +++ b/tests/Composer/Test/Downloader/GitDownloaderTest.php @@ -701,7 +701,7 @@ composer https://github.com/old/url (push) $cmd = str_replace('cd ', 'cd /D ', $cmd); $cmd = str_replace('composerPath', getcwd().'/composerPath', $cmd); - return strtr($cmd, "'", '"'); + return $this->getCmd($cmd); } return $cmd; diff --git a/tests/Composer/Test/Downloader/HgDownloaderTest.php b/tests/Composer/Test/Downloader/HgDownloaderTest.php index 34363d29a..f14502055 100644 --- a/tests/Composer/Test/Downloader/HgDownloaderTest.php +++ b/tests/Composer/Test/Downloader/HgDownloaderTest.php @@ -15,7 +15,6 @@ namespace Composer\Test\Downloader; use Composer\Downloader\HgDownloader; use Composer\Test\TestCase; use Composer\Util\Filesystem; -use Composer\Util\Platform; class HgDownloaderTest extends TestCase { @@ -157,9 +156,4 @@ class HgDownloaderTest extends TestCase $this->assertEquals('source', $downloader->getInstallationSource()); } - - private function getCmd($cmd) - { - return Platform::isWindows() ? strtr($cmd, "'", '"') : $cmd; - } } diff --git a/tests/Composer/Test/EventDispatcher/EventDispatcherTest.php b/tests/Composer/Test/EventDispatcher/EventDispatcherTest.php index f1a7bb1db..f16874948 100644 --- a/tests/Composer/Test/EventDispatcher/EventDispatcherTest.php +++ b/tests/Composer/Test/EventDispatcher/EventDispatcherTest.php @@ -401,7 +401,7 @@ class EventDispatcherTest extends TestCase $dispatcher->dispatch('helloWorld', new CommandEvent('helloWorld', $composer, $io)); $expected = "> helloWorld: @hello World".PHP_EOL. - "> hello: echo Hello " .escapeshellarg('World').PHP_EOL; + "> hello: echo Hello " .$this->getCmd("'World'").PHP_EOL; $this->assertEquals($expected, $io->getOutput()); } diff --git a/tests/Composer/Test/TestCase.php b/tests/Composer/Test/TestCase.php index d43d3d911..6e9477b14 100644 --- a/tests/Composer/Test/TestCase.php +++ b/tests/Composer/Test/TestCase.php @@ -16,6 +16,7 @@ use Composer\Semver\VersionParser; use Composer\Package\AliasPackage; use Composer\Semver\Constraint\Constraint; use Composer\Util\Filesystem; +use Composer\Util\Platform; use Composer\Util\Silencer; use PHPUnit\Framework\TestCase as BaseTestCase; use Symfony\Component\Process\ExecutableFinder; @@ -125,4 +126,24 @@ abstract class TestCase extends BaseTestCase parent::setExpectedException($exception, $message, $code); } } + + /** + * Transforms an escaped non-Windows command to match Windows escaping. + * + * @param string $cmd + * + * @return string The transformed command + */ + protected function getCmd($cmd) + { + if (Platform::isWindows()) { + $cmd = preg_replace_callback("/('[^']*')/", function ($m) { + // Double-quotes are used only when needed + $char = (strpbrk($m[1], " \t^&|<>()") !== false || $m[1] === "''") ? '"' : ''; + return str_replace("'", $char, $m[1]); + }, $cmd); + } + + return $cmd; + } } diff --git a/tests/Composer/Test/Util/SvnTest.php b/tests/Composer/Test/Util/SvnTest.php index ada3818ae..8d38c567b 100644 --- a/tests/Composer/Test/Util/SvnTest.php +++ b/tests/Composer/Test/Util/SvnTest.php @@ -14,7 +14,6 @@ namespace Composer\Test\Util; use Composer\Config; use Composer\IO\NullIO; -use Composer\Util\Platform; use Composer\Util\Svn; use Composer\Test\TestCase; @@ -130,9 +129,4 @@ class SvnTest extends TestCase $this->assertEquals($this->getCmd(" --no-auth-cache --username 'foo' --password 'bar' "), $reflMethod->invoke($svn)); } - - private function getCmd($cmd) - { - return Platform::isWindows() ? strtr($cmd, "'", '"') : $cmd; - } } From 906442117c68d138bc882a54ea961534e8c7814a Mon Sep 17 00:00:00 2001 From: John Stevenson Date: Wed, 13 Oct 2021 14:49:18 +0100 Subject: [PATCH 3/3] Carriage returns are ignored by cmd --- src/Composer/Util/ProcessExecutor.php | 5 ++--- tests/Composer/Test/Util/ProcessExecutorTest.php | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Composer/Util/ProcessExecutor.php b/src/Composer/Util/ProcessExecutor.php index fe7f06845..82a8cfaa7 100644 --- a/src/Composer/Util/ProcessExecutor.php +++ b/src/Composer/Util/ProcessExecutor.php @@ -180,9 +180,8 @@ class ProcessExecutor return "'".str_replace("'", "'\\''", $argument)."'"; } - if (strpbrk($argument, "\r\n") !== false) { - $argument = preg_replace("/(\r\n|\n|\r)/", ' ', $argument); - } + // New lines break cmd.exe command parsing + $argument = strtr($argument, "\n", ' '); $quote = strpbrk($argument, " \t") !== false; $argument = preg_replace('/(\\\\*)"/', '$1$1\\"', $argument, -1, $dquotes); diff --git a/tests/Composer/Test/Util/ProcessExecutorTest.php b/tests/Composer/Test/Util/ProcessExecutorTest.php index 1ff1a778d..40e098b1a 100644 --- a/tests/Composer/Test/Util/ProcessExecutorTest.php +++ b/tests/Composer/Test/Util/ProcessExecutorTest.php @@ -144,8 +144,8 @@ class ProcessExecutorTest extends TestCase // 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'"), + // new lines must be replaced + 'new lines' => array("a\nb\nc", '"a b c"', "'a\nb\nc'"), // whitespace must be quoted 'ws space' => array('a b c', '"a b c"', "'a b c'"),