Improve PowerShell command and argument escaping (#1611)

Co-authored-by: Andy Schwartzmeyer <andrew@schwartzmeyer.com>

Author: JustinGrote

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

@@ -156,23 +156,19 @@ private static PSCommand BuildPSCommandFromArguments(string command, IReadOnlyLi
                 return new PSCommand().AddCommand(command);
             }
 
-            // We are forced to use a hack here so that we can reuse PowerShell's parameter binding
+            // HACK: We use AddScript instead of AddArgument/AddParameter to reuse Powershell parameter binding logic.
+            // We quote the command parameter so that expressions can still be used in the arguments.
             var sb = new StringBuilder()
-                .Append("& ")
-                .Append(StringEscaping.SingleQuoteAndEscape(command));
+                .Append('&')
+                .Append('"')
+                .Append(command)
+                .Append('"');
 
             foreach (string arg in arguments)
             {
-                sb.Append(' ');
-
-                if (StringEscaping.PowerShellArgumentNeedsEscaping(arg))
-                {
-                    sb.Append(StringEscaping.SingleQuoteAndEscape(arg));
-                }
-                else
-                {
-                    sb.Append(arg);
-                }
+                sb
+                .Append(' ')
+                .Append(ArgumentEscaping.Escape(arg));
             }
 
             return new PSCommand().AddScript(sb.ToString());

src/PowerShellEditorServices/Utility/ArgumentUtils.cs

@@ -0,0 +1,40 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+using System.Text;
+using System.Management.Automation.Language;
+
+namespace Microsoft.PowerShell.EditorServices.Utility
+{
+    internal static class ArgumentEscaping
+    {
+        /// <summary>
+        /// Escape a PowerShell argument while still making it able to be evaluated in AddScript.
+        ///
+        /// NOTE: This does not "sanitize" parameters, e.g., a pipe in one argument might affect another argument.
+        /// This is intentional to give flexibility to specifying arguments.
+        /// It also does not try to fix invalid PowerShell syntax, e.g., a single quote in a string literal.
+        /// </summary>
+        public static string Escape(string Arg)
+        {
+            // if argument is a scriptblock return as-is
+            if (Arg.StartsWith("{") && Arg.EndsWith("}"))
+            {
+                return Arg;
+            }
+
+            // If argument has a space enclose it in quotes unless it is already quoted
+            if (Arg.Contains(" "))
+            {
+                if (Arg.StartsWith("\"") && Arg.EndsWith("\"") || Arg.StartsWith("'") && Arg.EndsWith("'"))
+                {
+                    return Arg;
+                }
+
+                return "\"" + Arg + "\"";
+            }
+
+            return Arg;
+        }
+    }
+}

src/PowerShellEditorServices/Utility/PathUtils.cs

@@ -38,48 +38,22 @@ public static string NormalizePathSeparators(string path)
             return string.IsNullOrWhiteSpace(path) ? path : path.Replace(AlternatePathSeparator, DefaultPathSeparator);
         }
 
-        public static string WildcardEscape(string path)
-        {
-            return WildcardPattern.Escape(path);
-        }
-
         /// <summary>
         /// Return the given path with all PowerShell globbing characters escaped,
         /// plus optionally the whitespace.
         /// </summary>
         /// <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>
+        /// <returns>The path with *, ?, [, and ] escaped, including spaces if required</returns>
         internal static string WildcardEscapePath(string path, bool escapeSpaces = false)
         {
-            var sb = new StringBuilder();
-            for (int i = 0; i < path.Length; i++)
-            {
-                char curr = path[i];
-                switch (curr)
-                {
-                    // Escape '[', ']', '?' and '*' with '`'
-                    case '[':
-                    case ']':
-                    case '*':
-                    case '?':
-                    case '`':
-                        sb.Append('`').Append(curr);
-                        break;
+            var wildcardEscapedPath = WildcardPattern.Escape(path);
 
-                    default:
-                        // Escape whitespace if required
-                        if (escapeSpaces && char.IsWhiteSpace(curr))
-                        {
-                            sb.Append('`').Append(curr);
-                            break;
-                        }
-                        sb.Append(curr);
-                        break;
-                }
+            if (escapeSpaces)
+            {
+                wildcardEscapedPath = wildcardEscapedPath.Replace(" ", "` ");
             }
-
-            return sb.ToString();
+            return wildcardEscapedPath;
         }
     }
 }

src/PowerShellEditorServices/Utility/StringEscaping.cs

@@ -2,16 +2,12 @@
 // Licensed under the MIT License.
 
 using Xunit;
-using System.IO;
-using Microsoft.PowerShell.EditorServices.Services;
 using Microsoft.PowerShell.EditorServices.Utility;
 
 namespace Microsoft.PowerShell.EditorServices.Test.Session
 {
     public class PathEscapingTests
     {
-        private const string ScriptAssetPath = @"..\..\..\..\PowerShellEditorServices.Test.Shared\scriptassets";
-
         [Trait("Category", "PathEscaping")]
         [Theory]
         [InlineData("DebugTest.ps1", "DebugTest.ps1")]
@@ -53,68 +49,5 @@ public void CorrectlyWildcardEscapesPaths_Spaces(string unescapedPath, string es
             string extensionEscapedPath = PathUtils.WildcardEscapePath(unescapedPath, escapeSpaces: true);
             Assert.Equal(escapedPath, extensionEscapedPath);
         }
-
-        [Trait("Category", "PathEscaping")]
-        [Theory]
-        [InlineData("DebugTest.ps1", "'DebugTest.ps1'")]
-        [InlineData("../../DebugTest.ps1", "'../../DebugTest.ps1'")]
-        [InlineData("C:\\Users\\me\\Documents\\DebugTest.ps1", "'C:\\Users\\me\\Documents\\DebugTest.ps1'")]
-        [InlineData("/home/me/Documents/weird&folder/script.ps1", "'/home/me/Documents/weird&folder/script.ps1'")]
-        [InlineData("./path/with some/spaces", "'./path/with some/spaces'")]
-        [InlineData("C:\\path\\with[some]brackets\\file.ps1", "'C:\\path\\with[some]brackets\\file.ps1'")]
-        [InlineData("C:\\look\\an*\\here.ps1", "'C:\\look\\an*\\here.ps1'")]
-        [InlineData("/Users/me/Documents/?here.ps1", "'/Users/me/Documents/?here.ps1'")]
-        [InlineData("/Brackets [and s]paces/path.ps1", "'/Brackets [and s]paces/path.ps1'")]
-        [InlineData("/file path/that isn't/normal/", "'/file path/that isn''t/normal/'")]
-        [InlineData("/CJK.chars/脚本/hello.ps1", "'/CJK.chars/脚本/hello.ps1'")]
-        [InlineData("/CJK chars/脚本/[hello].ps1", "'/CJK chars/脚本/[hello].ps1'")]
-        [InlineData("C:\\Animal s\\утка\\quack.ps1", "'C:\\Animal s\\утка\\quack.ps1'")]
-        [InlineData("C:\\&nimals\\утка\\qu*ck?.ps1", "'C:\\&nimals\\утка\\qu*ck?.ps1'")]
-        public void CorrectlyQuoteEscapesPaths(string unquotedPath, string expectedQuotedPath)
-        {
-            string extensionQuotedPath = StringEscaping.SingleQuoteAndEscape(unquotedPath).ToString();
-            Assert.Equal(expectedQuotedPath, extensionQuotedPath);
-        }
-
-        [Trait("Category", "PathEscaping")]
-        [Theory]
-        [InlineData("DebugTest.ps1", "'DebugTest.ps1'")]
-        [InlineData("../../DebugTest.ps1", "'../../DebugTest.ps1'")]
-        [InlineData("C:\\Users\\me\\Documents\\DebugTest.ps1", "'C:\\Users\\me\\Documents\\DebugTest.ps1'")]
-        [InlineData("/home/me/Documents/weird&folder/script.ps1", "'/home/me/Documents/weird&folder/script.ps1'")]
-        [InlineData("./path/with some/spaces", "'./path/with some/spaces'")]
-        [InlineData("C:\\path\\with[some]brackets\\file.ps1", "'C:\\path\\with`[some`]brackets\\file.ps1'")]
-        [InlineData("C:\\look\\an*\\here.ps1", "'C:\\look\\an`*\\here.ps1'")]
-        [InlineData("/Users/me/Documents/?here.ps1", "'/Users/me/Documents/`?here.ps1'")]
-        [InlineData("/Brackets [and s]paces/path.ps1", "'/Brackets `[and s`]paces/path.ps1'")]
-        [InlineData("/file path/that isn't/normal/", "'/file path/that isn''t/normal/'")]
-        [InlineData("/CJK.chars/脚本/hello.ps1", "'/CJK.chars/脚本/hello.ps1'")]
-        [InlineData("/CJK chars/脚本/[hello].ps1", "'/CJK chars/脚本/`[hello`].ps1'")]
-        [InlineData("C:\\Animal s\\утка\\quack.ps1", "'C:\\Animal s\\утка\\quack.ps1'")]
-        [InlineData("C:\\&nimals\\утка\\qu*ck?.ps1", "'C:\\&nimals\\утка\\qu`*ck`?.ps1'")]
-        public void CorrectlyFullyEscapesPaths(string unescapedPath, string escapedPath)
-        {
-            string extensionEscapedPath = StringEscaping.SingleQuoteAndEscape(PathUtils.WildcardEscapePath(unescapedPath)).ToString();
-            Assert.Equal(escapedPath, extensionEscapedPath);
-        }
-
-        [Trait("Category", "PathEscaping")]
-        [Theory]
-        [InlineData("NormalScript.ps1")]
-        [InlineData("Bad&name4script.ps1")]
-        [InlineData("[Truly] b&d Name_4_script.ps1")]
-        public void CanDotSourcePath(string rawFileName)
-        {
-            string fullPath = Path.Combine(ScriptAssetPath, rawFileName);
-            string quotedPath = StringEscaping.SingleQuoteAndEscape(fullPath).ToString();
-
-            var psCommand = new System.Management.Automation.PSCommand().AddScript($". {quotedPath}");
-
-            using (var pwsh = System.Management.Automation.PowerShell.Create())
-            {
-                pwsh.Commands = psCommand;
-                pwsh.Invoke();
-            }
-        }
     }
 }

test/PowerShellEditorServices.Test.Shared/scriptassets/[Truly] b&d Name_4_script.ps1 renamed to test/PowerShellEditorServices.Test.Shared/scriptassets/[Truly] b&d `N$(){}me_4_script.ps1

@@ -0,0 +1,69 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+using Xunit;
+using Microsoft.PowerShell.EditorServices.Utility;
+using System.IO;
+using System.Management.Automation;
+using System.Linq;
+
+namespace Microsoft.PowerShell.EditorServices.Test.Session
+{
+    public class ArgumentEscapingTests
+    {
+        [Trait("Category", "ArgumentEscaping")]
+        [Theory]
+        [InlineData(" has spaces", "\" has spaces\"")]
+        [InlineData("-Parameter", "-Parameter")]
+        [InlineData("' single quote left alone'", "' single quote left alone'")]
+        [InlineData("\"double quote left alone\"", "\"double quote left alone\"")]
+        [InlineData("/path/to/fi le", "\"/path/to/fi le\"")]
+        [InlineData("'/path/to/fi le'", "'/path/to/fi le'")]
+        [InlineData("|pipeline", "|pipeline")]
+        [InlineData("am&pe rsand", "\"am&pe rsand\"")]
+        [InlineData("semicolon ;", "\"semicolon ;\"")]
+        [InlineData(": colon", "\": colon\"")]
+        [InlineData("$(expressions should be quoted)", "\"$(expressions should be quoted)\"")]
+        [InlineData("{scriptBlocks should not have escaped-spaces}", "{scriptBlocks should not have escaped-spaces}")]
+        [InlineData("-Parameter test", "\"-Parameter test\"")] //This is invalid, but should be obvious enough looking at the PSIC invocation
+        public void CorrectlyEscapesPowerShellArguments(string Arg, string expectedArg)
+        {
+            string quotedArg = ArgumentEscaping.Escape(Arg);
+            Assert.Equal(expectedArg, quotedArg);
+        }
+
+        [Trait("Category", "ArgumentEscaping")]
+        [Theory]
+        [InlineData("/path/t o/file", "/path/t o/file")]
+        [InlineData("/path/with/$(echo 'expression')inline", "/path/with/expressioninline")]
+        [InlineData("/path/with/$(echo 'expression') inline", "/path/with/expression inline")]
+        [InlineData("am&per sand", "am&per sand")]
+        [InlineData("'inner\"\"quotes'", "inner\"\"quotes")]
+        public void CanEvaluateArguments(string Arg, string expectedOutput)
+        {
+            var escapedArg = ArgumentEscaping.Escape(Arg);
+            var psCommand = new PSCommand().AddScript($"& Write-Output {escapedArg}");
+            using var pwsh = System.Management.Automation.PowerShell.Create();
+            pwsh.Commands = psCommand;
+            var scriptOutput = pwsh.Invoke<string>().First();
+            Assert.Equal(expectedOutput, scriptOutput);
+        }
+
+        [Trait("Category", "ArgumentEscaping")]
+        [Theory]
+        [InlineData("NormalScript.ps1")]
+        [InlineData("Bad&name4script.ps1")]
+        [InlineData("[Truly] b&d `Name_4_script.ps1")]
+        public void CanDotSourcePath(string rawFileName)
+        {
+            var ScriptAssetPath = @"..\..\..\..\PowerShellEditorServices.Test.Shared\scriptassets";
+            var fullPath = Path.Combine(ScriptAssetPath, rawFileName);
+            var escapedPath = PathUtils.WildcardEscapePath(fullPath).ToString();
+            var psCommand = new PSCommand().AddScript($"& \"{escapedPath}\"");
+
+            using var pwsh = System.Management.Automation.PowerShell.Create();
+            pwsh.Commands = psCommand;
+            pwsh.Invoke();
+        }
+    }
+}