Fix Pester CodeLens run/debug by not quoting params/already quoted args

[rkeithhill]

Requires a corresponding PR in vscode-powershell.

We should look this one over good as it has potential to break folks.

I think the odds are low. Essentially we added the QuoteEscapString
method because we get raw paths from VSCode when debugging
the current file. Those paths are quoted because they may contain
spaces and because we use single quotes, existing single quotes
have to be doubled e.g C:\Don'tUseQuotesInPaths to
'C:\Don''tUseQuotesInPaths'.

This unfortunately was always being invoked on args. Now we
check if the arg is a parameter (starts with '-') or is already quoted. The thinking is if the users is specifying a quoted arg, it's on them
to handle any necessary escaping.

BTW is there another char that can act as the start of a parameter -
like the longer dash - whatever that is called.

Longer term, we need a better solution for how to handle paths
from VSCode. Maybe we handle them there and eliminate the
escaping logic in PSES?

[rkeithhill]

Ran across another case where the path quote escaping it biting us. I tried to pass these two parameters to debug launch -PesterOption and @{IncludeVSCodeMarker=$true} and we wound up with this in the script args string -PesterOption '@{IncludeVSCodeMarker=$true}'. Sigh, guess we could special case @ and $ and whatever but now I'm really thinking we punt on this whole path escaping thing and check the paths on the VSCode before we send them. Thoughts?

[TylerLeonhardt]

check the paths on the VSCode before we send them.

Can you expand on this?

[rkeithhill]

For some history, check out:

#190
microsoft/vscode#4057

Related issues:
PowerShell/vscode-powershell#1526

OK, I think this is the PR that broke the Pester CodeLens provider:
https://github.com/PowerShell/PowerShellEditorServices/pull/765/files

The PR above changed to quote and escape every argument. I don't think we can do that. If I pass an argument of @{Recurse = $true} and that is converted to '@{Recurse = $true}' it changes the arg from a hashtable to string literal.

I'm not sure what the right answer is but I think we should avoid messing with the debug script args in PSES. That said, we have to see if we can avoid regression here:

PowerShell/vscode-powershell#1400
PowerShell/vscode-powershell#1526

[rkeithhill]

Hmm, we might need to rethink a few things on the #765 - sorry I didn't catcht these during the review:

On line 772 (RHS), we are automatically escaping the scriptToLaunch. Yeah, we can't do that. We renamed this field in the debug config from program to script to allow folks to enter arbitrary script - not just a filename. For instance, here is a debug config I use from time-to-time:

        {
            "type": "PowerShell",
            "request": "launch",
            "name": "PowerShell Pester Tests",
            "script": "Invoke-Pester -PesterOption @{IncludeVSCodeMarker=$true}",
            "args": [],
            "cwd": "${workspaceFolder}"
        },

So not only can you enter a command instead of a script, you can even supply parameters. Unless we want to change what is supported by the script debug field, we need to test the contents of this field to see if tt corresponds to an existing file before we modify the path. If the user provides both a path and args within the script field then it is up to them to proper quote/escape the path.

I think the original code failed with & in the path because the old EscapePath only handled [] and <space> and not other special chars like & and (). I think the new escaping methods are much better.

So should we try to fix them in this PR - to revise a bit the changes in #765. I "think" what I'm proposing is to not quote args passed via args - this means that users will have to double qoute & escape in VSCode - like so: "args: [ "'C:\Program Files (x86)'" ]. We could atttempt to process the args in the resolveDebugConfiguration in the extension but I worry that getting too cute will bite us again.

[rjmholt]

Yes, I see I was overzealous in #765.

[rjmholt]

I think to make this work we're going to have to throw away whatever our existing contract was with people -- some people are going to have to be broken so that all things can work in some way.

It sounds like we need to leave quoting/escaping up to people themselves.

Maybe we can have a check method that fires a warning if it thinks an argument should be quoted but isn't.

So:

• No string quoting/escaping on input, users pass complicated arguments at their own risk
• Possibly have a method on the VSCode side to log a warning if it thinks an input might be badly formed
• Document our best practices and what not to do. I get the impression that because we've just left it up to people to work out debug configurations, they've all developed different ways of doing things. We should instead write down what we know works and make sure we can point to it in markdown in the repo.

[rjmholt]

I vote we replace all this logic with arguments = string.Join(launchParams.Args, " ")

[rkeithhill]

Sounds good to me.

[rjmholt]

As you say @rkeithhill, there are several characters PowerShell accepts to begin a parameter.

[rkeithhill]

• No string quoting/escaping on input, users pass complicated arguments at their own risk

Agreed and this applys to debug launch config args field and the pop up you get when you launch Debug Current File w/Script Args.

It could be I'm one of the few folks (involved with the original design) what the script field can specify arbitrary script. In fact, the schema description for this field says: Optional: Absolute path to the PowerShell script to launch under the debugger. Now we do ship with one debug configuration with this script settting Invoke-Pester but we don't ship any that look like this "script": "Invoke-Pester -SomParam somArg". With this in mind, maybe we only guarantee to support the case of "script": <script-path> | <command-name>. This would simplify the logic in the ExecuteScriptWithArgs. If you want to specify the args to a <command-name> then you have to use the "args": field. I like this idea.

We've gotten by for a long time with that code in ExecuteScriptWithArgs. The old code (pre #765) failed because it didn't escape all the troublesome characters like & and (. PR #765 has a better approach of just single quote the filepath (if it is an existing file) and double up any interior single quotes. I think this is all we need to make this work pretty. I'll know more after some testing.

[rjmholt]

If you want to specify the args to a then you have to use the "args": field. I like this idea.

100% agree. It makes args not redundant and <command> | <script-path> seems very consistent with PowerShell's own invocation concepts.

So this means:

• script/program: Unescaped path or command name - we will escape it (I'm happy if we ask people to escape it too).
  • (Ideally we could change the name to command, and deprecate old ones, but that's a good way to make life harder I guess)
• args: User-escaped list of parameters/arguments to the script. Ideally we could validate/sanitise this string (probably on the VSCode side) to prevent weird bugs from unpaired quotes or things.

?

Any other things we need to nail down?

[rkeithhill]

So this work - funky path in script with args:

    Launch script is: . 'c:\Users\Keith\.vscode\extensions\ms-vscode.powershell-1.9.0\examples\Foo & (x86) [bar]\foo.ps1' 50 10 blah

[rjmholt]

Ok so I've been discussing this with @TylerLeonhardt all morning because we were trying to work out what the right design is here. I'll try to summarise here:

• We should honour the LSP's paths-are-URIs principle everywhere in messages where feasible
• For the script/program field then, Editor Services should expect a file URI pointing to the file
• Users won't want to enter a URI in the configuration, so vscode-PowerShell should have that field set like a LiteralPath and then we process it in the front-end into a URI
• args can be anything, so I don't think it makes sense to convert paths in that to URIs; they should be passed verbatim to PowerShell in any circumstance
• The back end should then:
  • Perform escaping for script/program by converting it from a URI and then PowerShell escaping it when necessary
  • Not touch args

[rjmholt]

Do you think that will work @rkeithhill? Want to discuss everything properly with you so we've got all the bases covered

[rkeithhill]

Agreed on all the "not touching args" comments.

RE the script field, I think we are applying LSP rules to the VSCode debug protocol that don't apply. Here is the spec on the launch request:

https://github.com/Microsoft/vscode-debugadapter-node/blob/9bcba8996b37e4916849d061325b8529e55be9d7/debugProtocol.json#L673

Note that command is a string with no mention of uri. Also note this comment in the description:

Since launching is debugger/runtime specific, the arguments for this request are not part of this specification.

Also note how the VSCode sample/mock debugger handles this field (program is the equivalent of our script field):

https://github.com/Microsoft/vscode-mock-debug/blob/042d19a27a8e3a08f27a24110506b53fbecc75ce/src/extension.ts#L50

It simply uses ${file} and doesn't convert to a uri.

I believe we should keep the script processing as-is on the VSCode side. On the PSES side, all we need to do is single quote the value (escaping embedded single quotes) if the value is determined to be an existing file. This is exactly what PS does for you when you tab-complete a filename and that handles all the other special chars.

cc @weinand Do you have any input on whether an extension needs to convert launch config file paths (program) to a uri before sending it to its corresponding debug server?

[rjmholt]

Yeah, that was my original inclination as well, to also pass the script path verbatim and rely on users to escape that path properly themselves.

But I can also imagine sending it through as a URI mainly to simplify our API requirements.

I don't feel strongly either way. The first seems more consistent but the second seems less likely to annoy people.

In the first case, I'm wondering:

• Should we do any escaping at all (including put quotes around the script)?
• If we do, should we also regex escape?

While I can see arguments for escaping, it's sort of what's gotten us into trouble so far. I feel like it would ideally behave like LiteralPath, but (1) the logic we pass it to has no LiteralPath functionality and (2) args are passed verbatim; users must escape those themselves. So I think we should just pass script verbatim as well. Then in the default config scenario, the extension should escape the path it sends down.

[rkeithhill]

rely on users to escape that path properly themselves.

They shouldn't need to do that. The change that you made that had the biggest impact was to switch from doing this (literally what .AddScript was executing`):

. C:\Program` Files` (x86)\`[foo`]'bar arg1 arg2
^ note the unescaped ( and if &|$@{ would have been in that path, those would have been unescaped as well.

to this:

. 'C:\Program Files (x86)\[foo']''bar' arg1 arg2

This fixes a lot of funky path issues by simply single quoting the path instead of trying to escape all the various chars which we weren't doing before your PR. IIRC we only escaped [] and <space>. That is why folks were running into issues with & and (x86).

The above covers the AddScript case we use when there are args. When there are no args, we use AddCommand and I've verified that you can pass a string with all those funky chars in the path and PowerShell handles it correctly. This is why folks were saying these paths worked fine until they started adding arguments to the launch config. :-) So in the "no args" case - no escaping/quoting is needed anywhere and it works just fine.

While I can see arguments for escaping, it's sort of what's gotten us into trouble so far.

BTW agreed 100% That is why this PR (in it's current state) does only one thing and only to the script path and then only when it is a file path and that is to single quote it. OK once you single quote a path you have to escape embedded single quoutes but that's it. Like I said, this is exactly what PowerShell does when tab-completing paths. It was the "escaping chars only" approach that got us into trouble before.

[rjmholt]

Ah, so having us do the escaping is helpful because it depends on which PSCommand builder method we use as to whether we want to escape or not?

As usual, I'm in favour of taking as much of the magic out as possible, but if the above is the case, then it makes sense to promise to escape the path if necessary.

Regex escaping still troubles me a bit, but I suppose we can cross that bridge if we need to later.

[rkeithhill]

so having us do the escaping is helpful

I want to make sure we are not talking past one another. :-) I don't think we need to do any "escaping" per se WRT to the script debug config string. We just need to "single quote" the script but only when its value corresponds to an existing file. So no escaping except for the ever-so-slight ' escaping that your new method QuoteEscapeString() does. But I'm only saying this WRT that one debug config field. In other places, like for breakpoint paths that VSCode sends us, the current mechanism (WildcardEscapePath) seems to be working fine.

This is a really "low touch" approach and seems less dangerous - to me at least. The other thing to keep in mind is that is scenario only comes into play when we use PSCommand.AddScript which we provide in this form:

. <<single-quoted-path-name>|<script-value-not-touched>> [arg1 [arg2 […]]]

This means we don't do any regex escaping in this scenario.

And this is very similar to what you get at the command line right? You type C:\program<tab> and you wind up with a path like:

& 'C:\Program Files\Git\bin\git.exe' log

In our case, we choose to dot source rather than use & but more or less it's the same way of executing a script file. BTW I'd like to consider making the use of & or . a debug option of some sort. Need to think about that. There are times I'd prefer not to dot source what I'm trying to execute. We used to not do that but as we pushed the debugger to be more similar to ISE debugging this got added. I've missed the old approach. Not saying it should be the default but as an option, I think it makes sense.

[rjmholt]

That all makes sense to me.

The only part I want to really verify is:

This means we don't do any regex escaping in this scenario.

If I have a script like C:\Users\me\Documents\My Scripts\[script].ps1 or even /home/me/Documents/My Scripts/scr*pt.ps1, will that work along all the code paths we use?

When I was experimenting with different pathological script names for #765, I found that there wasn't a great mechanism for dot-sourcing such a script and that single quotes didn't do anything. PSCommand.AddCommand I think works with these script names, PSCommand.AddScript I can't remember, and straight string templating seemed to be the least safe.

I ended up opening PowerShell/PowerShell#7999 and PowerShell/PowerShell#7995 as a result.

[rkeithhill]

According to the Windows FileExplorer a path cannot have a * in it:

But I'm updating my test path to be more abusive - including $ @{} - and the tab-completed path still works:

11-20 09:13:50 22ms 56> . '.\Foo & (x86) [bar] isn''t @{good} dir& $name\foo.ps1' arg1 arg2
In foo.ps1 with args: arg1, arg2
C:\Users\Keith

Just to re-iterate I'm not suggesting this particular change for anywhere else - just in this case where we are dot sourcing the user's script. That WildcardEscapePath method you wrote is working good for the breakpoint paths AFAICT. :-)

[TylerLeonhardt]

I think on Unix a * is valid. Not sure if that changes anything.

[rjmholt]

Yeah * isn't valid on Windows, but it is on *nix (hence /home/me/...). Ok cool, I just wanted to check that. I'm pretty happy then that we've settled on the right behaviour. Thanks for indulging me @rkeithhill :)

[TylerLeonhardt]

LGTM @rkeithhill! Thank you for addressing this.

[rjmholt]

LGTM!