From b27021d30c1a1badff8f6a9e2c34b95de33cc647 Mon Sep 17 00:00:00 2001 From: Justin Grote Date: Fri, 29 Oct 2021 17:36:22 -0700 Subject: [PATCH 1/3] PowershellArgumentNeedsEscaping does not account for a single quoted string Fixes #1608 Apply suggested Rosyln Fix --- src/PowerShellEditorServices/Utility/StringEscaping.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/PowerShellEditorServices/Utility/StringEscaping.cs b/src/PowerShellEditorServices/Utility/StringEscaping.cs index 5736a9aad..35cbcba8c 100644 --- a/src/PowerShellEditorServices/Utility/StringEscaping.cs +++ b/src/PowerShellEditorServices/Utility/StringEscaping.cs @@ -9,9 +9,9 @@ internal static class StringEscaping public static StringBuilder SingleQuoteAndEscape(string s) { return new StringBuilder(s.Length) - .Append("'") + .Append('\'') .Append(s.Replace("'", "''")) - .Append("'"); + .Append('\''); } public static bool PowerShellArgumentNeedsEscaping(string argument) From 6cad36869453d8363301c93d96a3107e81c189e1 Mon Sep 17 00:00:00 2001 From: Justin Grote Date: Fri, 29 Oct 2021 22:56:39 -0700 Subject: [PATCH 2/3] Define new function for handling PS Argument Escaping --- .../Handlers/ConfigurationDoneHandler.cs | 13 ++--- .../Utility/StringEscaping.cs | 21 ++++++++ .../Utility/ArgumentEscapingTests.cs | 52 +++++++++++++++++++ 3 files changed, 76 insertions(+), 10 deletions(-) create mode 100644 test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs diff --git a/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs b/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs index 7750585c0..1ded392ff 100644 --- a/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs +++ b/src/PowerShellEditorServices/Services/DebugAdapter/Handlers/ConfigurationDoneHandler.cs @@ -163,16 +163,9 @@ private static PSCommand BuildPSCommandFromArguments(string command, IReadOnlyLi foreach (string arg in arguments) { - sb.Append(' '); - - if (StringEscaping.PowerShellArgumentNeedsEscaping(arg)) - { - sb.Append(StringEscaping.SingleQuoteAndEscape(arg)); - } - else - { - sb.Append(arg); - } + sb + .Append(' ') + .Append(StringEscaping.EscapePowershellArgument(arg)); } return new PSCommand().AddScript(sb.ToString()); diff --git a/src/PowerShellEditorServices/Utility/StringEscaping.cs b/src/PowerShellEditorServices/Utility/StringEscaping.cs index 35cbcba8c..ab1c9c521 100644 --- a/src/PowerShellEditorServices/Utility/StringEscaping.cs +++ b/src/PowerShellEditorServices/Utility/StringEscaping.cs @@ -16,6 +16,14 @@ public static StringBuilder SingleQuoteAndEscape(string s) public static bool PowerShellArgumentNeedsEscaping(string argument) { + //Already quoted arguments dont require escaping unless there is a quote inside as well + if (argument.StartsWith("'") && argument.EndsWith("'")) + { + var dequotedString = argument.TrimStart('\'').TrimEnd('\''); + // need to escape if there is a quote between single quotes + return dequotedString.Contains("'"); + } + foreach (char c in argument) { switch (c) @@ -33,5 +41,18 @@ public static bool PowerShellArgumentNeedsEscaping(string argument) return false; } + + public static string EscapePowershellArgument(string argument) + { + if (PowerShellArgumentNeedsEscaping(argument)) + { + return SingleQuoteAndEscape(argument).ToString(); + } + else + { + return argument; + } + } + } } diff --git a/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs b/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs new file mode 100644 index 000000000..f4dd819a5 --- /dev/null +++ b/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs @@ -0,0 +1,52 @@ +using Xunit; +using Microsoft.PowerShell.EditorServices.Utility; +using System.Management.Automation; +using System.Linq; + +namespace Microsoft.PowerShell.EditorServices.Test.Session +{ + public class ArgumentEscapingTests + { + [Trait("Category", "ArgumentEscaping")] + [Theory] + [InlineData("/path/to/file", "/path/to/file")] + [InlineData("'/path/to/file'", "'/path/to/file'")] + [InlineData("not|allowed|pipeline", "'not|allowed|pipeline'")] + [InlineData("doublequote\"inmiddle", "'doublequote\"inmiddle'")] + [InlineData("am&persand", "'am&persand'")] + [InlineData("semicolon;", "'semicolon;'")] + [InlineData(":colon", "':colon'")] + [InlineData(" has space s", "' has space s'")] + [InlineData("[brackets]areOK", "[brackets]areOK")] + [InlineData("$(expressionsAreOK)", "$(expressionsAreOK)")] + [InlineData("{scriptBlocksAreOK}", "{scriptBlocksAreOK}")] + public void CorrectlyEscapesPowerShellArguments(string Arg, string expectedArg) + { + string quotedArg = StringEscaping.EscapePowershellArgument(Arg); + Assert.Equal(expectedArg, quotedArg); + } + + [Trait("Category", "ArgumentEscaping")] + [Theory] + [InlineData("/path/to/file", "/path/to/file")] + [InlineData("'/path/to/file'", "/path/to/file")] + [InlineData("not|allowed|pipeline", "not|allowed|pipeline")] + [InlineData("doublequote\"inmiddle", "doublequote\"inmiddle")] + [InlineData("am&persand", "am&persand")] + [InlineData("semicolon;", "semicolon;")] + [InlineData(":colon", ":colon")] + [InlineData(" has space s", " has space s")] + [InlineData("[brackets]areOK", "[brackets]areOK")] + // [InlineData("$(echo 'expressionsAreOK')", "expressionsAreOK")] + // [InlineData("{scriptBlocksAreOK}", "{scriptBlocksAreOK}")] + public void CanEvaluateArgumentsSafely(string Arg, string expectedOutput) + { + var escapedArg = StringEscaping.EscapePowershellArgument(Arg); + var psCommand = new PSCommand().AddScript($"& Write-Output {escapedArg}"); + using var pwsh = System.Management.Automation.PowerShell.Create(); + pwsh.Commands = psCommand; + var scriptOutput = pwsh.Invoke().SingleOrDefault(); + Assert.Equal(expectedOutput, scriptOutput); + } + } +} From 2f29721b2e3130d721dc3c17c82d7139ba5e4725 Mon Sep 17 00:00:00 2001 From: Justin Grote Date: Mon, 1 Nov 2021 09:31:31 -0700 Subject: [PATCH 3/3] Handle additional cases --- src/PowerShellEditorServices/Utility/StringEscaping.cs | 6 ++++-- .../Session/PathEscapingTests.cs | 4 +++- .../Utility/ArgumentEscapingTests.cs | 6 ++++-- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/PowerShellEditorServices/Utility/StringEscaping.cs b/src/PowerShellEditorServices/Utility/StringEscaping.cs index ab1c9c521..2113d20d4 100644 --- a/src/PowerShellEditorServices/Utility/StringEscaping.cs +++ b/src/PowerShellEditorServices/Utility/StringEscaping.cs @@ -8,9 +8,11 @@ internal static class StringEscaping { public static StringBuilder SingleQuoteAndEscape(string s) { + var dequotedString = s.TrimStart('\'').TrimEnd('\''); + var psEscapedInnerQuotes = dequotedString.Replace("'", "`'"); return new StringBuilder(s.Length) .Append('\'') - .Append(s.Replace("'", "''")) + .Append(psEscapedInnerQuotes) .Append('\''); } @@ -20,7 +22,7 @@ public static bool PowerShellArgumentNeedsEscaping(string argument) if (argument.StartsWith("'") && argument.EndsWith("'")) { var dequotedString = argument.TrimStart('\'').TrimEnd('\''); - // need to escape if there is a quote between single quotes + // need to escape if there is a single quote between single quotes return dequotedString.Contains("'"); } diff --git a/test/PowerShellEditorServices.Test/Session/PathEscapingTests.cs b/test/PowerShellEditorServices.Test/Session/PathEscapingTests.cs index 495166e09..34c8a4951 100644 --- a/test/PowerShellEditorServices.Test/Session/PathEscapingTests.cs +++ b/test/PowerShellEditorServices.Test/Session/PathEscapingTests.cs @@ -65,11 +65,13 @@ public void CorrectlyWildcardEscapesPaths_Spaces(string unescapedPath, string es [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("/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'")] + [InlineData("../../Quote'InPathTest.ps1", "'../../Quote`'InPathTest.ps1'")] + public void CorrectlyQuoteEscapesPaths(string unquotedPath, string expectedQuotedPath) { string extensionQuotedPath = StringEscaping.SingleQuoteAndEscape(unquotedPath).ToString(); diff --git a/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs b/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs index f4dd819a5..002d3819f 100644 --- a/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs +++ b/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs @@ -20,6 +20,8 @@ public class ArgumentEscapingTests [InlineData("[brackets]areOK", "[brackets]areOK")] [InlineData("$(expressionsAreOK)", "$(expressionsAreOK)")] [InlineData("{scriptBlocksAreOK}", "{scriptBlocksAreOK}")] + [InlineData("'quote ' in middle of argument'", "'quote `' in middle of argument'")] + public void CorrectlyEscapesPowerShellArguments(string Arg, string expectedArg) { string quotedArg = StringEscaping.EscapePowershellArgument(Arg); @@ -37,7 +39,7 @@ public void CorrectlyEscapesPowerShellArguments(string Arg, string expectedArg) [InlineData(":colon", ":colon")] [InlineData(" has space s", " has space s")] [InlineData("[brackets]areOK", "[brackets]areOK")] - // [InlineData("$(echo 'expressionsAreOK')", "expressionsAreOK")] + [InlineData("$(echo 'expressionsAreOK')", "expressionsAreOK")] // [InlineData("{scriptBlocksAreOK}", "{scriptBlocksAreOK}")] public void CanEvaluateArgumentsSafely(string Arg, string expectedOutput) { @@ -45,7 +47,7 @@ public void CanEvaluateArgumentsSafely(string Arg, string expectedOutput) var psCommand = new PSCommand().AddScript($"& Write-Output {escapedArg}"); using var pwsh = System.Management.Automation.PowerShell.Create(); pwsh.Commands = psCommand; - var scriptOutput = pwsh.Invoke().SingleOrDefault(); + var scriptOutput = pwsh.Invoke().First(); Assert.Equal(expectedOutput, scriptOutput); } }