bug #10479 [2.3][Process] Fix escaping on Windows (romainneutron)
This PR was merged into the 2.3 branch.
Discussion
----------
[2.3][Process] Fix escaping on Windows
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| License | MIT
Windows escaping is broken since the last merges.
After digging more on Windows escaping, I realised some things:
- We forbid environment variable expansion by escaping `%APPDATA%` to `^%"APPDATA"^%`
- We explicitly ask for variable expansion at runtime (running the command line with the [`/V:ON`](https://github.com/symfony/symfony/blob/2.3/src/Symfony/Component/Process/Process.php#L235) flag). Running a command containing `!APPDATA!` will be escaped and expanded (our previous rule is easily overriden)
- On platform that are not windows, we use strong escaping that prevents any variable expansion (`$PATH` will be escaped to `'$PATH'` that is not interpreted as the current PATH)
We have three possibilities:
- Keep this behavior as this.
- Prefer a consistent API and use a strong escaping strategy everywhere, but it would result in a BC break (see #8975).
- Allow environment variable expansion and escape `%APPDATA%` to `"%APPDATA%"`
Any thoughts about this ?
Commits
-------
0f65f90
[Process] Fix escaping on Windows
This commit is contained in:
commit
1e9e8afa5d
@ -47,20 +47,18 @@ class ProcessUtils
|
|||||||
|
|
||||||
$escapedArgument = '';
|
$escapedArgument = '';
|
||||||
$quote = false;
|
$quote = false;
|
||||||
foreach (preg_split('/([%"])/i', $argument, -1, PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE) as $part) {
|
foreach (preg_split('/(")/i', $argument, -1, PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE) as $part) {
|
||||||
if ('"' === $part) {
|
if ('"' === $part) {
|
||||||
$escapedArgument .= '\\"';
|
$escapedArgument .= '\\"';
|
||||||
} elseif ('%' === $part) {
|
} elseif (self::isSurroundedBy($part, '%')) {
|
||||||
$escapedArgument .= '^%';
|
// Avoid environment variable expansion
|
||||||
|
$escapedArgument .= '^%"'.substr($part, 1, -1).'"^%';
|
||||||
} else {
|
} else {
|
||||||
|
// escape trailing backslash
|
||||||
if ('\\' === substr($part, -1)) {
|
if ('\\' === substr($part, -1)) {
|
||||||
$part .= '\\';
|
$part .= '\\';
|
||||||
}
|
}
|
||||||
$part = escapeshellarg($part);
|
$quote = true;
|
||||||
if ('"' === $part[0] && '"' === $part[strlen($part) - 1]) {
|
|
||||||
$part = substr($part, 1, -1);
|
|
||||||
$quote = true;
|
|
||||||
}
|
|
||||||
$escapedArgument .= $part;
|
$escapedArgument .= $part;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@ -73,4 +71,9 @@ class ProcessUtils
|
|||||||
|
|
||||||
return escapeshellarg($argument);
|
return escapeshellarg($argument);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private static function isSurroundedBy($arg, $char)
|
||||||
|
{
|
||||||
|
return 2 < strlen($arg) && $char === $arg[0] && $char === $arg[strlen($arg) - 1];
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
@ -142,13 +142,13 @@ class ProcessBuilderTest extends \PHPUnit_Framework_TestCase
|
|||||||
|
|
||||||
public function testShouldEscapeArguments()
|
public function testShouldEscapeArguments()
|
||||||
{
|
{
|
||||||
$pb = new ProcessBuilder(array('%path%', 'foo " bar'));
|
$pb = new ProcessBuilder(array('%path%', 'foo " bar', '%baz%baz'));
|
||||||
$proc = $pb->getProcess();
|
$proc = $pb->getProcess();
|
||||||
|
|
||||||
if (defined('PHP_WINDOWS_VERSION_BUILD')) {
|
if (defined('PHP_WINDOWS_VERSION_BUILD')) {
|
||||||
$this->assertSame('^%"path"^% "foo "\\"" bar"', $proc->getCommandLine());
|
$this->assertSame('^%"path"^% "foo \\" bar" "%baz%baz"', $proc->getCommandLine());
|
||||||
} else {
|
} else {
|
||||||
$this->assertSame("'%path%' 'foo \" bar'", $proc->getCommandLine());
|
$this->assertSame("'%path%' 'foo \" bar' '%baz%baz'", $proc->getCommandLine());
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -30,7 +30,7 @@ class ProcessUtilsTest extends \PHPUnit_Framework_TestCase
|
|||||||
array('"\"php\" \"-v\""', '"php" "-v"'),
|
array('"\"php\" \"-v\""', '"php" "-v"'),
|
||||||
array('"foo bar"', 'foo bar'),
|
array('"foo bar"', 'foo bar'),
|
||||||
array('^%"path"^%', '%path%'),
|
array('^%"path"^%', '%path%'),
|
||||||
array('"<|>"\\"" "\\""\'f"', '<|>" "\'f'),
|
array('"<|>\\" \\"\'f"', '<|>" "\'f'),
|
||||||
array('""', ''),
|
array('""', ''),
|
||||||
array('"with\trailingbs\\\\"', 'with\trailingbs\\'),
|
array('"with\trailingbs\\\\"', 'with\trailingbs\\'),
|
||||||
);
|
);
|
||||||
|
Reference in New Issue
Block a user