GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||
GeezerVison is showing github.com — Fix passing escaped double quoted spaces to native executables by douglaswth · Pull Request #2182 · PowerShell/PowerShell Conversation
Contributor
This fixes a regression that seems to have been introduced in PowerShell 5 where spaces in segments of a string with an even number of quotation marks even though some may be escaped are ignored for quoting when executing native programs. I discovered this issue when trying to run a utility that needs to have double quotes to delimit strings in its query language: rsc json --x1 "object:has(.name:val(\`"rs low space on C: drive\`"))" With PowerShell versions before 5, this worked correctly and the query argument was accepted, but with PowerShell 5 the argument passed was getting through to the utility broken up: Out of curiosity, I tried it out with the latest PowerShell 6 alpha and the same issue still exists, even on Linux! So I have come up with this fix which is to not count backslash escaped quotes in the quote count for the
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
|
There was an error while loading. Please reload this page.
|
It's a very simple command. Feel free to use this - if you want: https://github.com/Pscx/Pscx/blob/master/Src/EchoArgs/EchoArgs.cs Should compile on .NET Core. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
sergei (@vors) I have added tests that use Keith Hill (@rkeithhill) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Is it needed? What does create this file?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but it was showing up at least on my Linux box.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, we should do a wild-card, like *.nuget.props
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Wow, that was fast! Great job! Let's make it vigorous. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorry for hustle, I know that I suggested this name run yesterday. I didn't find a good one at the moment, but now I think we should use bin instead of run, because it would match dotnet output.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🕝 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Should Contain usage here makes it's harder to troubleshoot the error. When it accured.
Here, I'm trying to execute the test on the inbox powershell v5.1
Describing Native Command Arguments
[+] Should handle quoted spaces correctly 619ms
[-] Should handle spaces between escaped quotes 127ms
Expected: file TestDrive:\test.txt to contain {Arg 0 is <a"b c"d>}
19: $testPath | Should Contain 'Arg 0 is <a"b c"d>'
at <ScriptBlock>, F:\dev\PowerShell\test\powershell\Scripting\NativeExecution\NativeCommandArguments.Tests.ps1: line 19
As you see, I don't have information about the produced outpuit immediately in my disposal.
Just to find what we are currently produce I would need to run commands by hands.
I suggest to replace $testPath | Should Contain 'Arg 0 is <ab cd>' by
$lines = Get-Contetxt $testPath ($lines | measure).Count | Should Be 3 # this line will help get a better error message in case there is only one line in the output $lines[0] | Should Be 'Arg 0 is <ab cd>'
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why bother writing the file? You can just capture the output in a variable with:
$lines = & $echoArgs $a 'a"b c"d' a"b c"d
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I was trying to do what I thought would be reasonable, but it didn't seem to work with Pester:
& $echoArgs $a 'a"b c"d' a"b c"d | Should Match @" Arg 0 is <ab cd> Arg 1 is <ab cd> Arg 2 is <ab cd> "@
It seems like Pester only tries to match the first line of output when using Should Match. 😕
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It's because powershell take output line by line.
You can add | Out-String | to get a single string.
Mulit-line probably indeed appropriate here, but I'd like to bring up couple things:
Be, not Match, because Match takes regex.I suggest use what I described above, with array of 3 strings. It avoids problems with line-ends and produce a reasonable good error message.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'd like to understand this better.
"a\`"b c\`"d"
There is a mix of two things:
backticks as powershell string escape mechanics and \ as command line args escape mechanics
This is an equivalent of 'a\"b c\"d'
When I use ehcoargs from cmd:
EchoArgs.exe a\"b c\"d
I'm getting
Arg 0 is <a"b>
Arg 1 is <c"d>
So the behavior that is currently described with Arg 1 doesn't seem right.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
That's because it should be sent to EchoArgs as EchoArgs.exe "a\"b c\"d" with the outer quotes.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I see. I'd like to dig this a little bit more on all platforms, just to make sure we are doing the right thing.
At the meantime, can I ask you to add comments in the tests, so it's easier to maintain them?
Particularly the line
"a\`"b c\`"d"
already requires knowledge about both PS and native args passing escaping.
Will come back to this today after some experiments.
Thank you, again, for such thoughtful approach to the PR!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I verified this behavior is aligned with bash and cmd. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
* Move EchoArgs from test/EchoArgs to test/tools/EchoArgs * Use "dotnet publish" for building EchoArgs in build.psm1 so the test can call it directly
* Rename Publish-EchoArgs to Publish-PSTestTools so it can be used for other tools as well in the future * Publish EchoArgs to the bin directory instead of run to match convention * Add source URL to EchoArgs header comment * Use wildcard of "*.nuget.props" to match "test/csharp/csharp.nuget.props" in .gitignore
* Use a variable (which can be indexed by line) instead of a file to test the output of arguments passed to echoargs * Add comments explaining the tests
|
Thank you so much for the great contribution! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I just realised where this |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ah, looks like the CLI team is making some serious progress! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
We still need to add a backslash before a backtick-escaped double quote? This does not solve the double-escaping problem. (That is, we have to escape a double quote for both PowerShell and CommandLineToArgvW.) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…d-double-quoted-spaces-to-native.exe Fix passing escaped double quoted spaces to native executables
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.