-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Escape shell arguments correctly #3603
Conversation
Lets use quote function from my other project: https://github.com/google/zx/blob/d1cd1aa02e3fab7ca4f048d9571b77b6ce25d559/src/util.ts#L31-L48 |
But that code would create the following command, always:
This would again produce a wrong |
@antonmedv I have adjusted the PR now to the new escaping method, under the assumption that deployments onto a Windows server are not really supported (and not common place) anyway. |
This is true. Only Linux. |
Let's add some test for this function. |
I added some basic tests. |
at a first glance it won't work with single quotes in string, will it? i mean, the escaped string will be OK (not making problems when running the shell command), but it won't produce valid JSON. taking Erik's name as an example
this will produce oh, and to clarify, the TARGET OS is not an issue, the escapeshellarg problem is on the machine that DEPLOYS the code. just read the reasoning in the previous PR: #3569 one thing is to escape correctly when deploying from windows. but the other thing is to handle single quotes in the string, like the example with Erik's name - that is a problem when deploying from any OS. |
It will. Single quotes will be replaced by
What makes you say that? Single quotes must be escaped by |
well, AFAIK there is no escaping of single quotes in JSON. this is valid JSON:
this is invalid JSON:
not a pro here, just used random JSON online validator in combination with https://www.json.org/json-en.html |
While it's not technically necessary, it's good practise to do so. PHP's |
produces:
you sure? am i doing something wrong or missing something? |
Sorry, you are right. And escaping with Instead, the command needs to look like this:
which will result in
in the And this is exactly what I have updated the PR accordingly and boiled it down to the strictly necessary pieces (assuming a Linux server target). @s4muel can you please test these changes again? |
@antonmedv this should be the correct solution. Any chance of getting this merged and released? |
This might seem a trivial bug, but it's having great impact on users deploying from Windows, because old releases are not deleted which might create issues (I've just hit another server that had no free inodes due to old releases not being cleaned). Is there anything we can do to have this merged and released? |
I've been hit by this bug. I've reviewed this PR and it looks good to me! |
This is my take on fixing #3478. The current usage of
escapeshellarg
is wrong, as it will executed on the local system and thus not escape the shell argument appropriately - since the argument is used for a command that is run on the remote system.This PR basically creates an
escape_shell_argument
that can be made aware of the remote host's operating system. It is a copy of Symfony\Component\Process\Process::escapeArgument and allows to override the directory separator.So this solution first resolves the directory separator on the remote system and then escapes the shell argument based on that detection.