From a0c3943d3a7442641498452a013546abab0e2db7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 13 Feb 2021 09:01:40 +0100 Subject: [PATCH] Windows: fix escaping of external commands The `escapeshellcmd()` function apparently does not escape spaces within a path on Windows which can result in broken functionality. While the sniffs and report affected by this are apparently not used that much based on the lack of bug reports, fixing it still seemed like the _right thing to do_. Noticed while running the unit tests on a fresh install on Windows 10. At some point over the past years, Node has apparently changed their default install directory on Windows and the order in which they register their paths to the Windows system `PATH`. This means that `where csslint` may result in a `$cmd` path like `C:\Program Files\nodejs\csslint.cmd`, which would be escaped to `C:^\Program Files^\nodejs^\csslint.cmd` on Windows, which in turn results in a `'C:\Program' is not recognized as an internal or external command, operable program or batch file.` error. I could have changed the install path for NVM on my machine, but that would just have hidden the underlying issue. It does appear to be a known issue with the function based on the last two comments in this upstream bug report: https://bugs.php.net/bug.php?id=43261, however as that issue is closed, I don't expect this to be fixed in PHP itself, though it might be worth it to open a new issue upstream about it (as those two comments were left on a closed issue years after the close). Fixed now by checking an escaped path for unescaped spaces when on Windows and if necessary, escaping them. The escaping is done in such a way that, even if PHP itself would start escaping these spaces, the `Common::escapeshellcmd()` function will still handle this correctly. --- src/Reports/Notifysend.php | 3 ++- .../Generic/Sniffs/Debug/CSSLintSniff.php | 3 ++- .../Sniffs/Debug/ClosureLinterSniff.php | 3 ++- .../Generic/Sniffs/Debug/ESLintSniff.php | 3 ++- .../Generic/Sniffs/Debug/JSHintSniff.php | 5 +++-- .../Generic/Sniffs/PHP/SyntaxSniff.php | 3 ++- .../Squiz/Sniffs/Debug/JSLintSniff.php | 5 +++-- .../Sniffs/Debug/JavaScriptLintSniff.php | 3 ++- .../Zend/Sniffs/Debug/CodeAnalyzerSniff.php | 3 ++- src/Util/Common.php | 22 +++++++++++++++++++ 10 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/Reports/Notifysend.php b/src/Reports/Notifysend.php index 5ffc5baf5d..0841662234 100644 --- a/src/Reports/Notifysend.php +++ b/src/Reports/Notifysend.php @@ -18,6 +18,7 @@ use PHP_CodeSniffer\Config; use PHP_CodeSniffer\Files\File; +use PHP_CodeSniffer\Util\Common; class Notifysend implements Report { @@ -58,7 +59,7 @@ public function __construct() { $path = Config::getExecutablePath('notifysend'); if ($path !== null) { - $this->path = escapeshellcmd($path); + $this->path = Common::escapeshellcmd($path); } $timeout = Config::getConfigData('notifysend_timeout'); diff --git a/src/Standards/Generic/Sniffs/Debug/CSSLintSniff.php b/src/Standards/Generic/Sniffs/Debug/CSSLintSniff.php index d299dc845c..81284787a2 100644 --- a/src/Standards/Generic/Sniffs/Debug/CSSLintSniff.php +++ b/src/Standards/Generic/Sniffs/Debug/CSSLintSniff.php @@ -12,6 +12,7 @@ use PHP_CodeSniffer\Config; use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Common; class CSSLintSniff implements Sniff { @@ -54,7 +55,7 @@ public function process(File $phpcsFile, $stackPtr) $fileName = $phpcsFile->getFilename(); - $cmd = escapeshellcmd($csslintPath).' '.escapeshellarg($fileName).' 2>&1'; + $cmd = Common::escapeshellcmd($csslintPath).' '.escapeshellarg($fileName).' 2>&1'; exec($cmd, $output, $retval); if (is_array($output) === false) { diff --git a/src/Standards/Generic/Sniffs/Debug/ClosureLinterSniff.php b/src/Standards/Generic/Sniffs/Debug/ClosureLinterSniff.php index f6b6b729bc..19204718b0 100644 --- a/src/Standards/Generic/Sniffs/Debug/ClosureLinterSniff.php +++ b/src/Standards/Generic/Sniffs/Debug/ClosureLinterSniff.php @@ -12,6 +12,7 @@ use PHP_CodeSniffer\Config; use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Common; class ClosureLinterSniff implements Sniff { @@ -71,7 +72,7 @@ public function process(File $phpcsFile, $stackPtr) $fileName = $phpcsFile->getFilename(); - $lintPath = escapeshellcmd($lintPath); + $lintPath = Common::escapeshellcmd($lintPath); $cmd = $lintPath.' --nosummary --notime --unix_mode '.escapeshellarg($fileName); exec($cmd, $output, $retval); diff --git a/src/Standards/Generic/Sniffs/Debug/ESLintSniff.php b/src/Standards/Generic/Sniffs/Debug/ESLintSniff.php index 081c3c1ebf..d11d347050 100644 --- a/src/Standards/Generic/Sniffs/Debug/ESLintSniff.php +++ b/src/Standards/Generic/Sniffs/Debug/ESLintSniff.php @@ -12,6 +12,7 @@ use PHP_CodeSniffer\Config; use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Common; class ESLintSniff implements Sniff { @@ -76,7 +77,7 @@ public function process(File $phpcsFile, $stackPtr) $eslintOptions[] = '--config '.escapeshellarg($configFile); } - $cmd = escapeshellcmd(escapeshellarg($eslintPath).' '.implode(' ', $eslintOptions).' '.escapeshellarg($filename)); + $cmd = Common::escapeshellcmd(escapeshellarg($eslintPath).' '.implode(' ', $eslintOptions).' '.escapeshellarg($filename)); // Execute! exec($cmd, $stdout, $code); diff --git a/src/Standards/Generic/Sniffs/Debug/JSHintSniff.php b/src/Standards/Generic/Sniffs/Debug/JSHintSniff.php index 8e1f18bd0d..06de35940a 100644 --- a/src/Standards/Generic/Sniffs/Debug/JSHintSniff.php +++ b/src/Standards/Generic/Sniffs/Debug/JSHintSniff.php @@ -13,6 +13,7 @@ use PHP_CodeSniffer\Config; use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Common; class JSHintSniff implements Sniff { @@ -56,10 +57,10 @@ public function process(File $phpcsFile, $stackPtr) } $fileName = $phpcsFile->getFilename(); - $jshintPath = escapeshellcmd($jshintPath); + $jshintPath = Common::escapeshellcmd($jshintPath); if ($rhinoPath !== null) { - $rhinoPath = escapeshellcmd($rhinoPath); + $rhinoPath = Common::escapeshellcmd($rhinoPath); $cmd = "$rhinoPath \"$jshintPath\" ".escapeshellarg($fileName); exec($cmd, $output, $retval); diff --git a/src/Standards/Generic/Sniffs/PHP/SyntaxSniff.php b/src/Standards/Generic/Sniffs/PHP/SyntaxSniff.php index 9ef8c2ea66..1519aa1376 100644 --- a/src/Standards/Generic/Sniffs/PHP/SyntaxSniff.php +++ b/src/Standards/Generic/Sniffs/PHP/SyntaxSniff.php @@ -13,6 +13,7 @@ use PHP_CodeSniffer\Config; use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Common; class SyntaxSniff implements Sniff { @@ -53,7 +54,7 @@ public function process(File $phpcsFile, $stackPtr) } $fileName = escapeshellarg($phpcsFile->getFilename()); - $cmd = escapeshellcmd($this->phpPath)." -l -d display_errors=1 -d error_prepend_string='' $fileName 2>&1"; + $cmd = Common::escapeshellcmd($this->phpPath)." -l -d display_errors=1 -d error_prepend_string='' $fileName 2>&1"; $output = shell_exec($cmd); $matches = []; if (preg_match('/^.*error:(.*) in .* on line ([0-9]+)/m', trim($output), $matches) === 1) { diff --git a/src/Standards/Squiz/Sniffs/Debug/JSLintSniff.php b/src/Standards/Squiz/Sniffs/Debug/JSLintSniff.php index c50e764e48..e4b49b520a 100644 --- a/src/Standards/Squiz/Sniffs/Debug/JSLintSniff.php +++ b/src/Standards/Squiz/Sniffs/Debug/JSLintSniff.php @@ -12,6 +12,7 @@ use PHP_CodeSniffer\Config; use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Common; class JSLintSniff implements Sniff { @@ -56,8 +57,8 @@ public function process(File $phpcsFile, $stackPtr) $fileName = $phpcsFile->getFilename(); - $rhinoPath = escapeshellcmd($rhinoPath); - $jslintPath = escapeshellcmd($jslintPath); + $rhinoPath = Common::escapeshellcmd($rhinoPath); + $jslintPath = Common::escapeshellcmd($jslintPath); $cmd = "$rhinoPath \"$jslintPath\" ".escapeshellarg($fileName); exec($cmd, $output, $retval); diff --git a/src/Standards/Squiz/Sniffs/Debug/JavaScriptLintSniff.php b/src/Standards/Squiz/Sniffs/Debug/JavaScriptLintSniff.php index 0c0f35566e..30dca6722d 100644 --- a/src/Standards/Squiz/Sniffs/Debug/JavaScriptLintSniff.php +++ b/src/Standards/Squiz/Sniffs/Debug/JavaScriptLintSniff.php @@ -13,6 +13,7 @@ use PHP_CodeSniffer\Exceptions\RuntimeException; use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Common; class JavaScriptLintSniff implements Sniff { @@ -56,7 +57,7 @@ public function process(File $phpcsFile, $stackPtr) $fileName = $phpcsFile->getFilename(); - $cmd = '"'.escapeshellcmd($jslPath).'" -nologo -nofilelisting -nocontext -nosummary -output-format __LINE__:__ERROR__ -process '.escapeshellarg($fileName); + $cmd = '"'.Common::escapeshellcmd($jslPath).'" -nologo -nofilelisting -nocontext -nosummary -output-format __LINE__:__ERROR__ -process '.escapeshellarg($fileName); $msg = exec($cmd, $output, $retval); // Variable $exitCode is the last line of $output if no error occurs, on diff --git a/src/Standards/Zend/Sniffs/Debug/CodeAnalyzerSniff.php b/src/Standards/Zend/Sniffs/Debug/CodeAnalyzerSniff.php index d18c947294..5df4b0fc2c 100644 --- a/src/Standards/Zend/Sniffs/Debug/CodeAnalyzerSniff.php +++ b/src/Standards/Zend/Sniffs/Debug/CodeAnalyzerSniff.php @@ -14,6 +14,7 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Config; use PHP_CodeSniffer\Exceptions\RuntimeException; +use PHP_CodeSniffer\Util\Common; class CodeAnalyzerSniff implements Sniff { @@ -53,7 +54,7 @@ public function process(File $phpcsFile, $stackPtr) // In the command, 2>&1 is important because the code analyzer sends its // findings to stderr. $output normally contains only stdout, so using 2>&1 // will pipe even stderr to stdout. - $cmd = escapeshellcmd($analyzerPath).' '.escapeshellarg($fileName).' 2>&1'; + $cmd = Common::escapeshellcmd($analyzerPath).' '.escapeshellarg($fileName).' 2>&1'; // There is the possibility to pass "--ide" as an option to the analyzer. // This would result in an output format which would be easier to parse. diff --git a/src/Util/Common.php b/src/Util/Common.php index 49fbe23d00..e60eec1df2 100644 --- a/src/Util/Common.php +++ b/src/Util/Common.php @@ -239,6 +239,28 @@ public static function isStdinATTY() }//end isStdinATTY() + /** + * Escape a path to a system command. + * + * @param string $cmd The path to the system command. + * + * @return string + */ + public static function escapeshellcmd($cmd) + { + $cmd = escapeshellcmd($cmd); + + if (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') { + // Spaces are not escaped by escapeshellcmd on Windows, but need to be + // for the command to be able to execute. + $cmd = preg_replace('`(?