Fix PowerShell path escaping #765

rjmholt · 2018-10-10T02:36:47Z

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

Replaces the strange regex logic we use with some glob-escaping string mangling.

Also single-quotes the place we dot-source a debug path.

Obsoletes some static public methods that have no business being public.

Adds a bunch of tests. Changes an existing one to check the ampersand case that we used to crash on.

rjmholt · 2018-10-10T06:28:11Z

I should also include a test for unusual characters, in aid of PowerShell/vscode-powershell#1392

SeeminglyScience

LGTM! One major concern, the rest are nits or comments :)

src/PowerShellEditorServices/Session/PowerShellContext.cs

src/PowerShellEditorServices/Properties/AssemblyInfo.cs

rjmholt · 2018-10-10T20:32:58Z

Ok, I've added testing for international characters and for dot-sourcing files with unusual characters in them (and checked the latter fail if I don't quote). So I think this is ready now :)

TylerLeonhardt

LGTM! Love the addition of all those tests

rkeithhill · 2018-10-11T02:13:58Z

test/PowerShellEditorServices.Test/Session/PathEscapingTests.cs

+
+namespace Microsoft.PowerShell.EditorServices.Test.Session
+{
+    public class PathEscapingTests


rkeithhill · 2018-10-11T02:17:02Z

test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs

@@ -105,7 +105,7 @@ public async Task DebuggerAcceptsScriptArgs(string[] args)
            // it should not escape already escaped chars.
            ScriptFile debugWithParamsFile =
                this.workspace.GetFile(
-                    @"..\..\..\..\PowerShellEditorServices.Test.Shared\Debugging\Debug` With Params `[Test].ps1");
+                    @"..\..\..\..\PowerShellEditorServices.Test.Shared\Debugging\Debug` W&ith Params `[Test].ps1");


What was the deal with the & char? It isn't a wildcard char. Ah, wait a tic. I see. On WinPS it's a reserved char and on PS Core it is used to create a (background) job.

Yeah, it's a "token separator", so it will break a bareword string

rkeithhill · 2018-10-11T02:28:20Z

src/PowerShellEditorServices/Session/PowerShellContext.cs

@@ -796,7 +797,7 @@ public async Task ExecuteScriptWithArgs(string script, string arguments = null,
                if (File.Exists(script) || File.Exists(scriptAbsPath))
                {
                    // Dot-source the launched script path
-                    script = ". " + EscapePath(script, escapeSpaces: true);
+                    script = ". " + QuoteEscapeString(script);


Will this continue to work if someone has a debug launch config like this:

"script": "${workspaceFolder}/foo`[bar`].ps1"

I "think" if we single quote a path, we have to evaluate backticks as well and perhaps strip some out. For instance, with the above path the user tried to be helpful and escape the wildcard chars for us (even though unnecessary, this works today). If the backticks are left in, the path will fail with this change - I think. However take a file named foobar.ps1` and it requires the backtick as a literal char.

I'll give it a test. I don't think we can do both though. Even if it means a breaking change, personally my preference is to enable all paths to work rather than having some paths work two ways and others not work at all.

It feels like we should treat the config setting as a LiteralPath -- it's possible to call a file foo`[bar`].ps1 on the filesystem (I experimented with that while making the fix). In fact on Windows it's not possible to make *.ps1 or ?.ps1 but on *nix it is, and have `?.ps1 right next to them in the directory.

Worth noting here too that Set-PSBreakpoint does not have a -LiteralPath parameter -- something which there is a desire to fix I think.

@rkeithhill I just tested with two files in the same directory ([script].ps1 and `[script`].ps1) and in 1.9.0, backticks or not both hit the first file.

Unfortunately the same happens in 1.9.1, so looks like I still need to do some work in this PR.

So I think I'm hitting a powershell issue here. I'll look into a workaround and open an issue there

I've found something interesting. PowerShell behaves strangely with backtick escaping: PowerShell/PowerShell#7999.

It looks like we've always been receiving the message in the first place with the backtick escaped (or maybe the [ escaped) but not other places:

2018-10-11 11:31:44.987 [DIAGNOSTIC] C:\Users\roholt\Documents\Dev\PowerShellEditorServices\src\PowerShellEditorServices.Protocol\MessageProtocol\MessageReader.cs: In method 'ReadMessage', line 114: READ MESSAGE: { "command": "launch", "arguments": { "type": "PowerShell", "request": "launch", "name": "Launch Script", "script": "C:\\Users\\roholt\\Documents\\Dev\\sandbox/``[script``].ps1", "args": [], "cwd": "C:\\Users\\roholt\\Documents\\Dev\\sandbox", "createTemporaryIntegratedConsole": false, "internalConsoleOptions": "neverOpen", "__sessionId": "ecbc3316-e3f6-48ea-a3cf-dc6730758edc" }, "type": "request", "seq": 2 }

This miraculously worked because PowerShell escapes ``[ the same as it escapes `[, which I think is a bug.

So I'm currently trying to track down why the message we get doubles the backticks.

rkeithhill · 2018-10-11T02:30:52Z

src/PowerShellEditorServices/Session/PowerShellContext.cs

+        /// <param name="path">The path to process.</param>
+        /// <param name="escapeSpaces">Specify True to escape spaces in the path, otherwise False.</param>
+        /// <returns>The path with [ and ] escaped.</returns>
+        internal static string GlobEscapePath(string path, bool escapeSpaces = false)


Minor nit but does PowerShell really do globbing? Maybe this should be EscapeWildcardsInPath() or something like that?

Ah good point. Discussing with @JamesWTruher we were calling it "globbing" but the help topic is about_Wildcards. I will rename.

rjmholt · 2018-10-11T22:52:38Z

Ok, I've finally worked out where the problem lay on our end. I'm going to do a bit more work to make this performant, but this is now working properly.

rjmholt · 2018-10-11T23:40:07Z

Ok, thought I could jimmy some more StringBuilder usage in there, but I rely on string searching/iteration too much for it to be a good idea.

Gonna ask for a second review here (@tylerl0706, @SeeminglyScience, @rkeithhill), since I've changed some things now.

TylerLeonhardt

Just a small comment on the string concatenation

TylerLeonhardt · 2018-10-12T00:17:13Z

src/PowerShellEditorServices/Session/PowerShellContext.cs

-                launchedScript = script + " " + arguments;
-                command.AddScript(launchedScript, false);
+                reportedScript += " " + arguments;
+                escapedScriptPath += " " + arguments;


Can you use a StringBuilder or string interpolation here instead?

Same for line 804, 801

rjmholt · 2018-10-12T18:45:25Z

So it turns out that PowerShell (back to v3 at least) has a System.Management.Automation.WildcardPattern.Escape() method.

But frankly it's broken:

> [WildcardPattern]::Escape('`[test`]')
``[test``]

And it's also subject to a PR or two currently. See PowerShell/PowerShell#7407.

Fix PowerShell path escaping

b737a89

rjmholt requested review from TylerLeonhardt, rkeithhill and SeeminglyScience October 10, 2018 02:36

SeeminglyScience requested changes Oct 10, 2018

View reviewed changes

rjmholt and others added 5 commits October 10, 2018 09:17

Only use quote escaping in dot source template

4785078

Add obsolete attributes

3123832

Remove hidden attribute

4dbf2f8

Add international character escaping tests

08c8ae3

Add dot source tests for strange paths

e07b5cd

rjmholt mentioned this pull request Oct 10, 2018

Add InternalsVisibleTo attributes with assembly strong names to AssemblyInfo.cs files #766

Open

TylerLeonhardt approved these changes Oct 10, 2018

View reviewed changes

SeeminglyScience approved these changes Oct 10, 2018

View reviewed changes

rkeithhill reviewed Oct 11, 2018

View reviewed changes

Change use of glob to wildcard

2d6a37c

rjmholt mentioned this pull request Oct 11, 2018

Add -LiteralPath parameter to Set-PSBreakpoint PowerShell/PowerShell#7995

Closed

Fix remaining uses of glob to wildcard

ce20e65

rjmholt mentioned this pull request Oct 11, 2018

Backtick escaping inconsistent PowerShell/PowerShell#7999

Open

Correctly escape path to script

063b782

rjmholt changed the title ~~Fix PowerShell path escaping~~ WIP: Fix PowerShell path escaping Oct 11, 2018

rjmholt changed the title ~~WIP: Fix PowerShell path escaping~~ Fix PowerShell path escaping Oct 11, 2018

TylerLeonhardt reviewed Oct 12, 2018

View reviewed changes

Fix git idiocy

7fab6d8

rjmholt force-pushed the fix-path-escaping-debug branch from 73d7b6d to 7fab6d8 Compare October 12, 2018 16:27

rjmholt merged commit 78d96b8 into PowerShell:master Oct 15, 2018

rkeithhill mentioned this pull request Nov 19, 2018

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

Merged

rjmholt deleted the fix-path-escaping-debug branch December 12, 2018 05:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PowerShell path escaping #765

Fix PowerShell path escaping #765

rjmholt commented Oct 10, 2018 •

edited

Loading

rjmholt commented Oct 10, 2018

SeeminglyScience left a comment

rjmholt commented Oct 10, 2018

TylerLeonhardt left a comment

rkeithhill Oct 11, 2018

rkeithhill Oct 11, 2018

rjmholt Oct 11, 2018

rkeithhill Oct 11, 2018

rjmholt Oct 11, 2018

rjmholt Oct 11, 2018

rjmholt Oct 11, 2018

rjmholt Oct 11, 2018

rjmholt Oct 11, 2018 •

edited

Loading

rkeithhill Oct 11, 2018

rjmholt Oct 11, 2018

rjmholt commented Oct 11, 2018

rjmholt commented Oct 11, 2018

TylerLeonhardt left a comment

TylerLeonhardt Oct 12, 2018

rjmholt commented Oct 12, 2018

Fix PowerShell path escaping #765

Fix PowerShell path escaping #765

Conversation

rjmholt commented Oct 10, 2018 • edited Loading

rjmholt commented Oct 10, 2018

SeeminglyScience left a comment

Choose a reason for hiding this comment

rjmholt commented Oct 10, 2018

TylerLeonhardt 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

rjmholt Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Oct 11, 2018

rjmholt commented Oct 11, 2018

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt commented Oct 12, 2018

rjmholt commented Oct 10, 2018 •

edited

Loading

rjmholt Oct 11, 2018 •

edited

Loading