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.

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

merged 2 commits into from
Jul 19, 2023

Conversation

EmanueleCoppola
Copy link
Contributor

I'm encountering a similar escaping issue on Windows as described in #681.

However, in the current version, the Windows command appears to be completely broken.
This PR addresses the problem and fixes the execution on Windows.

@freekmurze freekmurze merged commit 1e526e2 into spatie:main Jul 19, 2023
@freekmurze
Copy link
Member

Thanks!

@BARNZ
Copy link

BARNZ commented Jul 26, 2023

@freekmurze @EmanueleCoppola Unfortunately this commit breaks the headerHtml and footerHtml functions as they are no longer properly escaped when the command is sent through to puppeteer. All our reports are failing to generate today on windows after updating this package - a bit of triage shows this commit to be the culprit.

I've had a look at #681 but its not clear to me how specifically this commit solves that issue? We don't experience the issue listed in #681 although we do set the NODE_PATH env var and the setNodeBinary to our global puppeteer v20 install.

Here's a simple example comparing the command output that gets sent to puppeteer:

Current release - broken:

"C:\laragon\bin\nodejs\node-v18\node.exe" "C:\laragon\www\adt\vendor\spatie\browsershot\src/../bin/browser.cjs" "{\"url\":\"file:\/\/C:\\Users\\mattb\\AppData\\Local\\Temp\\611586084-0060668001690334044\\index.html\",\"action\":\"pdf\",\"options\":{\"path\":\"C:\\laragon\\www\\adt\\storage\\temp\/reports\/5d0d033d-fead-4c8d-808e-0761f4f45205\/generated.pdf\",\"args\":[],\"viewport\":{\"width\":1920,\"height\":1080},\"displayHeaderFooter\":true,\"ignoreHttpsErrors\":true,\"margin\":{\"top\":\"15mm\",\"right\":\"10mm\",\"bottom\":\"40mm\",\"left\":\"10mm\"},\"format\":\"A4\",\"headerTemplate\":\"<h1>test<\/h1>\"}}"

Prior release - working:

C:^\laragon^\bin^\nodejs^\node-v18^\node.exe ^"C:^\laragon^\www^\adt^\vendor^\spatie^\browsershot^\src/../bin/browser.cjs^" ^"^{^\^"url^\^":^\^"file:^\/^\/C:^\^\Users^\^\mattb^\^\AppData^\^\Local^\^\Temp^\^\1872440106-0693158001690334210^\^\index.html^\^",^\^"action^\^":^\^"pdf^\^",^\^"options^\^":^{^\^"path^\^":^\^"C:^\^\laragon^\^\www^\^\adt^\^\storage^\^\temp^\/reports^\/84c6cf48-1efc-4493-88af-1fd32ca07e41^\/generated.pdf^\^",^\^"args^\^":^[^],^\^"viewport^\^":^{^\^"width^\^":1920,^\^"height^\^":1080^},^\^"displayHeaderFooter^\^":true,^\^"ignoreHttpsErrors^\^":true,^\^"margin^\^":^{^\^"top^\^":^\^"15mm^\^",^\^"right^\^":^\^"10mm^\^",^\^"bottom^\^":^\^"40mm^\^",^\^"left^\^":^\^"10mm^\^"^},^\^"format^\^":^\^"A4^\^",^\^"headerTemplate^\^":^\^"^<h1^>test^<^\/h1^>^\^"^}^}^"

The difference being how the headerTemplate parameter is escaped at the end. In the broken one I get an error The specified path is invalid. I'm guessing because the < and > charaters are no longer escaped.

I think this commit should be reverted until a better solution for #681 is found.


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

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!

@EmanueleCoppola
Copy link
Contributor Author

@BARNZ I opened a pull request that should definitely fix the command escape on Windows that should also support spaces in the path like C:\Program Files\nodejs\node.exe.

Could you please take a look at it #757 and see if you have any problem with your specific use case?

P.S. You're right #681 it's totally unrelated, I probably pasted the wrong discussion id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants