Improve PowerShell command and argument escaping #1611

JustinGrote · 2021-11-01T18:22:09Z

@rjmholt please ensure that changing '' to backtick-quote is what was intended, it looked like a bug to me that was causing the triple-quotes.

@SeeminglyScience @andschwa Per my last PR, maybe we are being "overly helpful" with the escaping of the arguments and should instead throw errors to encourage editors to fix their problems at the source?

For instance in a launch.json, specifying a parameter like this is a clear mistake, but this tool will accept it, escape it, and maybe produce a difficult-to-troubleshoot result:

args: "-Path","I'putaquoteinthewrongplaceaccidentally"

andyleejordan · 2021-11-01T18:42:57Z

Really appreciating the tests 🥳

JustinGrote · 2021-11-01T18:49:50Z

I'm gonna rewrite this to be less aggressive on the argument escaping.

For instance, if someone has a launch.json that is "","arg1","arg2", we should allow expressions to be used.

e.g. "Path/to/my/script.ps1" "-Path" "$ENV:MyCustomEnvPath" will not work with the current escaping I've set up

I think very minimal escaping should be done for these parameters, it doesn't provide any defense really because I can just code inject into the "command" .ps1 script
If you need the character explicitly you can still backtick it out e.g. "-Path" "I need a `' literal quote here"

…string Fixes #1608 Apply suggested Rosyln Fix

JustinGrote · 2021-11-02T04:28:09Z

@andschwa the first parameter in the launch config is now always quoted, which will work for both path and command arguments. It passes all tests and all vscode extension tests currently (I removed the pathing escape tests which are no longer relevant).

SeeminglyScience

Lookin' good :) couple of questions and suggestions

SeeminglyScience · 2021-11-02T12:30:42Z

src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs

-                {
-                    sb.Append(arg);
-                }
+                sb


Nit: consider,

sb.Append(' ').Append(ArgumentEscaping.Escape(arg));

Personally I like using text as the variable for a string builder to enable this:

text.Append(' ') .Append(ArgumentEscaping.Escape(arg));

Very minor.

I was matching style of other things in PSES, we can certainly do this if @andschwa doesn't mind.

Totally fine. I'm not super aggressive about style, I think anytime a style question is in play it should be addressed by tooling rather than back and forth on PRs. Do what you think is readable 😄

src/PowerShellEditorServices/Utility/ArgumentUtils.cs

SeeminglyScience · 2021-11-02T12:36:27Z

src/PowerShellEditorServices/Utility/PathUtils.cs

-                    case '`':
-                        sb.Append('`').Append(curr);
-                        break;
+            var wildcardEscapedPath = WildcardPattern.Escape(path);


May need to manually handle ` still due to PowerShell/PowerShell#16306. There is a good chance that this seemingly over engineered logic was here due to issue with that API. Unsure when it was added though.

Probably do need to escape it to handle a situation where it exists in a file name at least at the command level, will probably leave it alone at an argument level

#123 is the relevant reasoning, I don't think we need to escape every path that comes in, this should be more handled on the output side if required as part of a cmdlet argument, if it's thought the cmdlet will interpret handling characters, because in some cases that may be exactly what the caller wants.

I'll try to add a test for SetLineBreakpoint which is the only other place this is referenced.

Probably do need to escape it to handle a situation where it exists in a file name at least at the command level, will probably leave it alone at an argument level

Yeah it gets tricky with no real way to please everyone here. On one hand you want to enable VSCode variables like ${workspaceFolder} in which case you can't really escape manually, and on the other hand some folks may want to use expressions.

Like you said though for the command path always escape is a clear winner for sure.

@SeeminglyScience the compromise for my implementation is that:

Command is always wrapped in double quotes because it is used by the call operator, so ${} expressions will still work if you want to pass a file and not a command, the only file that won't work in this case is if the filename contains $(), I think that's reasonable enough to just say "dont name your files this way", should probably have an explicit exception check for this.

Argument is only wrapped in quotes if you specify a space. If you really need to be literal or use workspacefolder in an arg and have it be safe, you should wrap it in quotes, a scriptblock, or $(). I think that's a good compromise

@SeeminglyScience looks like all the debugserver tests were commented out by @rjmholt after his commit and probably need to be rewritten. @andschwa can you come up with a way to get a debugservice instantiated to a point I can run tests against it? Been trying to figure it out and it's too much for my non-C# brain at the moment.

Yeah it gets tricky with no real way to please everyone here. On one hand you want to enable VSCode variables like ${workspaceFolder} in which case you can't really escape manually, and on the other hand some folks may want to use expressions.

Wouldn't this be interpolated before it gets to the LSP server? I could be wrong, I just assumed that the Code client would interpolate their own variable expressions, and then send the full string over.

@JustinGrote

looks like all the debugserver tests were commented out

I will prioritize this. I really want them up. Thanks for working on this issue!

Yeah it gets tricky with no real way to please everyone here. On one hand you want to enable VSCode variables like ${workspaceFolder} in which case you can't really escape manually, and on the other hand some folks may want to use expressions.

Wouldn't this be interpolated before it gets to the LSP server? I could be wrong, I just assumed that the Code client would interpolate their own variable expressions, and then send the full string over.

They do, the problem is that you don't know that they are a path or an interpolation once they arrive to the LSP. I think this implemention is a good compromise.

src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs

JustinGrote · 2021-11-03T18:22:10Z

I built a .vsix of this and it is working for pester test extension, native pester codelens, and all my debug launch.json configurations for the last day or so without issue. It is good as is unless we want to adjust for @SeeminglyScience's nit, and cover the debugservice tests in a separate PR since looks like they all need to be redone anyways.

andyleejordan · 2021-11-03T20:18:06Z

cover the debugservice tests in a separate PR since looks like they all need to be redone anyways.

💯 Can merge!

src/PowerShellEditorServices/Utility/ArgumentUtils.cs

andyleejordan · 2021-11-03T20:22:34Z

src/PowerShellEditorServices/Utility/ArgumentUtils.cs

+        public static string Escape(string Arg)
+        {
+            // if argument is a scriptblock return as-is
+            if (Arg.StartsWith("{") && Arg.EndsWith("}"))


@JustinGrote Should we strip whitespace first? In case it's { ... } or some other variation with (pointless) spaces before/after.

@JustinGrote Should we strip whitespace first? In case it's { ... } or some other variation with (pointless) spaces before/after.

I think this is another case of "overly helpful", if you specified your debug config and left a space at the beginning or the end before your scriptblock, I have to think you may have had an intention there.

test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs

andyleejordan · 2022-02-11T18:10:26Z

Coming back to to this @JustinGrote and deciding we need to not quote escape args with spaces in them.

andyleejordan self-requested a review November 1, 2021 18:43

JustinGrote added 2 commits November 1, 2021 20:56

PowershellArgumentNeedsEscaping does not account for a single quoted …

2bf6b66

…string Fixes #1608 Apply suggested Rosyln Fix

Simplify logic by using quotes up front

1168018

JustinGrote marked this pull request as ready for review November 2, 2021 04:26

SeeminglyScience reviewed Nov 2, 2021

View reviewed changes

JustinGrote added 2 commits November 2, 2021 09:07

Address @SeeminglyScience feedback

ad6fde2

Remove unnecessary space in call operator

13e86d4

JustinGrote changed the title ~~PowershellArgumentNeedsEscaping does not account for a single quoted string~~ PowershellArgumentNeedsEscaping does not account for a single quoted string - MOVED TO 1611 Nov 2, 2021

JustinGrote changed the title ~~PowershellArgumentNeedsEscaping does not account for a single quoted string - MOVED TO 1611~~ Better Powershell Command and Argument Escape Handling Nov 2, 2021

JustinGrote mentioned this pull request Nov 2, 2021

Fix: Add special EscapePowershellArgument handling - MOVED to 1611 #1609

Closed

andyleejordan approved these changes Nov 3, 2021

View reviewed changes

Apply suggestions from code review

d078874

andyleejordan changed the title ~~Better Powershell Command and Argument Escape Handling~~ Improve PowerShell command and argument escaping Nov 3, 2021

andyleejordan added Area-Debugging Issue-Bug A bug to squash. labels Nov 3, 2021

andyleejordan enabled auto-merge (squash) November 3, 2021 21:15

andyleejordan merged commit 7cc7569 into PowerShell:master Nov 3, 2021

JustinGrote deleted the 1608 branch November 3, 2021 21:55

andyleejordan mentioned this pull request Feb 15, 2022

Fix running untitled scripts with arguments (but break line breakpoints) #1702

Merged

andyleejordan mentioned this pull request Nov 17, 2022

F5 with script containing single quotes results in 'not recognized as a ....' error PowerShell/vscode-powershell#4269

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve PowerShell command and argument escaping #1611

Improve PowerShell command and argument escaping #1611

JustinGrote commented Nov 1, 2021 •

edited

Loading

andyleejordan commented Nov 1, 2021 •

edited

Loading

JustinGrote commented Nov 1, 2021 •

edited

Loading

JustinGrote commented Nov 2, 2021

SeeminglyScience left a comment

SeeminglyScience Nov 2, 2021

JustinGrote Nov 2, 2021

andyleejordan Nov 3, 2021

SeeminglyScience Nov 2, 2021

JustinGrote Nov 2, 2021

JustinGrote Nov 2, 2021

JustinGrote Nov 2, 2021

SeeminglyScience Nov 2, 2021

JustinGrote Nov 2, 2021

JustinGrote Nov 2, 2021

andyleejordan Nov 3, 2021

andyleejordan Nov 3, 2021

JustinGrote Nov 3, 2021

JustinGrote commented Nov 3, 2021 •

edited

Loading

andyleejordan commented Nov 3, 2021

andyleejordan Nov 3, 2021

JustinGrote Nov 3, 2021

andyleejordan commented Feb 11, 2022

Improve PowerShell command and argument escaping #1611

Improve PowerShell command and argument escaping #1611

Conversation

JustinGrote commented Nov 1, 2021 • edited Loading

andyleejordan commented Nov 1, 2021 • edited Loading

JustinGrote commented Nov 1, 2021 • edited Loading

JustinGrote commented Nov 2, 2021

SeeminglyScience left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinGrote commented Nov 3, 2021 • edited Loading

andyleejordan commented Nov 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleejordan commented Feb 11, 2022

JustinGrote commented Nov 1, 2021 •

edited

Loading

andyleejordan commented Nov 1, 2021 •

edited

Loading

JustinGrote commented Nov 1, 2021 •

edited

Loading

JustinGrote commented Nov 3, 2021 •

edited

Loading