-
Notifications
You must be signed in to change notification settings - Fork 13
Solution for #20: Windows process argument quoting #178
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
Conversation
This comment has been minimized.
This comment has been minimized.
Also, this PR seems to come with drafts for:
Please edit this comment if there's anything else. It's good to know how the implementation is going to change. |
Implementation in Rakudo | ||
------------------------ | ||
|
||
The serialization of the arguments happens in Rakudo. Backends are expected to not touch the single arument string they receive from Rakudo on Windows in any way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably good for backend consistency. I'm an iota concerned about reproducing a quoting algorithm, which has security implications, but relieved that there's tests covering this.
|
||
[Node.js](https://github.com/nodejs/node/blob/382e859afc7e66600dccfadd4125088444e063c3/lib/child_process.js#L486) exposes a `shell` flag which wraps the call in `cmd.exe /d /s /c "<command>"` by hand and enables `UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS`. | ||
|
||
[Java](https://codewhitesec.blogspot.com/2016/02/java-and-command-line-injections-in-windows.html)s `ProcessBuilder` has a very sophisticated and complex processing of arguments. e.g. it detects whether the user called a `.bat` file or `.cmd` file and acts on that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad we didn't go this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. Approved!
@patrickbkr so is this PR ready now? |
@AlexDaniel: I think this PR is ready. But IIUC it still needs to go through the review period. |
@AlexDaniel: 13 days have passed. I think the two week review period is meant for people to look at this and give feedback. I think barely anyone looked at this. What shall we do? Edit: It seems someone needs to add all the reviewers. I don't have permission to add reviewers. AlexDaniel, can you? |
I think (I hope I'm wrong) that @AlexDaniel is taking a break from Raku. As an admin I will merge this tomorrow: I don't see a point in adding reviewer at this point in time, as that would only delay the process. |
I'm fine with a review period. But someone needs to add the reviewers... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought I'd put an approve on this long ago; it's fine.
I was adding reviewers simply to generate email spam so that the reviewers are notified. You can also do that by simply tagging their names (by copying the list from https://github.com/Raku/problem-solving#reviewers). I also hope that reviewers will check the PRs at least once a month, or ideally at least once every 14 days.
That is correct. |
No description provided.