Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Windows command escaping #756

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Fix Windows command escaping #756

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c276cc3
Fix escapeshellcmd on windows
EmanueleCoppola Jul 17, 2023
a9816d2
Use same escapeshellarg function on UNIX for consistency
EmanueleCoppola Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions src/Browsershot.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,15 @@ protected function callBrowser(array $command): string
throw new ProcessFailedException($process);
}

protected function escapeshellarg(string $arg)
{
if ($this->isWindows()) {
return '"' . str_replace('"', '\\"', $arg) . '"';
}

return escapeshellarg($arg);
}

protected function getFullCommand(array $command)
{
$nodeBinary = $this->nodeBinary ?: 'node';
Expand All @@ -940,13 +949,11 @@ protected function getFullCommand(array $command)

if ($this->isWindows()) {
$fullCommand =
$nodeBinary.' '
.escapeshellarg($binPath).' '
.'"'
.$optionsCommand
.'"';
$this->escapeshellarg($nodeBinary).' '
.$this->escapeshellarg($binPath).' '
.$optionsCommand;

return escapeshellcmd($fullCommand);
return $fullCommand;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically its the change to this line here - escapeshellcmd ensures that html tags such as < and > are properly escaped. If I revert just this line here then everything continues to work same as before. What was the reason for removing this @EmanueleCoppola ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon further testing it looks like escapeshellcmd can make a mess of the command particularly for more complex page headers and can cause json parsing to break inside browser.cjs.

I have side-stepped my issue altogether by simply using writeOptionsToFile for the time being.

Copy link
Contributor Author

@EmanueleCoppola EmanueleCoppola Jul 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking into it more, I realized that there are several problems when dealing with Windows escaping. One of the issues is that it doesn't work well with spaces in the paths of executable files, like "C:\Program Files\nodejs\node.exe".

To solve this problem, the best approach could be to utilize the actual Symfony Process command escaping and let it handle the escaping of paths and commands.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep agreed!

}

$setIncludePathCommand = "PATH={$this->includePath}";
Expand All @@ -957,7 +964,7 @@ protected function getFullCommand(array $command)
$setIncludePathCommand.' '
.$setNodePathCommand.' '
.$nodeBinary.' '
.escapeshellarg($binPath).' '
.$this->escapeshellarg($binPath).' '
.$optionsCommand;
}

Expand All @@ -980,11 +987,7 @@ protected function getOptionsCommand(string $command): string
$command = "-f {$temporaryOptionsFile}";
}

if ($this->isWindows()) {
return str_replace('"', '\"', $command);
}

return escapeshellarg($command);
return $this->escapeshellarg($command);
}

protected function arraySet(array &$array, string $key, $value): array
Expand Down