From 506c6db295ee2c95601ac84b0051cee5912423c4 Mon Sep 17 00:00:00 2001 From: Andrew Schwartzmeyer Date: Tue, 15 Feb 2022 13:11:38 -0800 Subject: [PATCH 1/4] Remove `ArgumentEscaping.Escape` for launch script arguments Attempting to be "helpful" by escaping arguments proved to cause more issues than it solved. We can instead massively simplify our script launching logic (and fix yet another test that had a "maybe bug") by just launching what the user gave us. This should also be easier for the user to debug. --- .../Utility/ArgumentUtils.cs | 40 ----------- .../Utility/PSCommandExtensions.cs | 20 +----- .../Debugging/DebugServiceTests.cs | 15 ++-- .../Utility/ArgumentEscapingTests.cs | 69 ------------------- 4 files changed, 13 insertions(+), 131 deletions(-) delete mode 100644 src/PowerShellEditorServices/Utility/ArgumentUtils.cs delete mode 100644 test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs diff --git a/src/PowerShellEditorServices/Utility/ArgumentUtils.cs b/src/PowerShellEditorServices/Utility/ArgumentUtils.cs deleted file mode 100644 index dd73f4d52..000000000 --- a/src/PowerShellEditorServices/Utility/ArgumentUtils.cs +++ /dev/null @@ -1,40 +0,0 @@ -// 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 - { - /// - /// 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. - /// - 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; - } - } -} diff --git a/src/PowerShellEditorServices/Utility/PSCommandExtensions.cs b/src/PowerShellEditorServices/Utility/PSCommandExtensions.cs index bc5a53d05..1d5186184 100644 --- a/src/PowerShellEditorServices/Utility/PSCommandExtensions.cs +++ b/src/PowerShellEditorServices/Utility/PSCommandExtensions.cs @@ -129,25 +129,11 @@ private static StringBuilder AddCommandText(this StringBuilder sb, Command comma return sb; } - public static PSCommand BuildCommandFromArguments(string command, IReadOnlyList arguments) + public static PSCommand BuildCommandFromArguments(string command, IEnumerable arguments) { // 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(' ') - .Append('"') - .Append(command) - .Append('"'); - - foreach (string arg in arguments ?? System.Linq.Enumerable.Empty()) - { - sb - .Append(' ') - .Append(ArgumentEscaping.Escape(arg)); - } - - return new PSCommand().AddScript(sb.ToString()); + string script = string.Concat(". ", command, " ", string.Join(" ", arguments ?? Array.Empty())); + return new PSCommand().AddScript(script); } } } diff --git a/test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs b/test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs index 00996db96..5013e33db 100644 --- a/test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs +++ b/test/PowerShellEditorServices.Test/Debugging/DebugServiceTests.cs @@ -176,8 +176,16 @@ await debugService.SetCommandBreakpointsAsync( Assert.Equal("[ArrayList: 0]", var.ValueString); } - [Fact] - public async Task DebuggerAcceptsScriptArgs() + // See https://www.thomasbogholm.net/2021/06/01/convenient-member-data-sources-with-xunit/ + public static IEnumerable DebuggerAcceptsScriptArgsTestData => new List() + { + new object[] { new object[] { "Foo -Param2 @('Bar','Baz') -Force Extra1" } }, + new object[] { new object[] { "Foo", "-Param2", "@('Bar','Baz')", "-Force", "Extra1" } } + }; + + [Theory] + [MemberData(nameof(DebuggerAcceptsScriptArgsTestData))] + public async Task DebuggerAcceptsScriptArgs(string[] args) { // The path is intentionally odd (some escaped chars but not all) because we are testing // the internal path escaping mechanism - it should escape certains chars ([, ] and space) but @@ -197,9 +205,6 @@ public async Task DebuggerAcceptsScriptArgs() Assert.True(breakpoint.Verified); }); - // TODO: This test used to also pass the args as a single string, but that doesn't seem - // to work any more. Perhaps that's a bug? - var args = new[] { "Foo", "-Param2", "@('Bar','Baz')", "-Force", "Extra1" }; Task _ = ExecutePowerShellCommand(debugWithParamsFile.FilePath, args); AssertDebuggerStopped(debugWithParamsFile.FilePath, 3); diff --git a/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs b/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs deleted file mode 100644 index d6c155211..000000000 --- a/test/PowerShellEditorServices.Test/Utility/ArgumentEscapingTests.cs +++ /dev/null @@ -1,69 +0,0 @@ -// 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().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(); - } - } -} From 7e22c883788633118c7c2ae4fbf3c7bff5ac2318 Mon Sep 17 00:00:00 2001 From: Andrew Schwartzmeyer Date: Wed, 9 Feb 2022 13:25:03 -0800 Subject: [PATCH 2/4] Fix running untitled scripts with arguments (but break line breakpoints) The extant hack that enabled line breakpoints in untitled scripts is untenable. It shifted the user's args by one since it ran the untitled script as the first arg to an inline script `. $args[0]` with `$args[0]` being the script contents, instead of the expected `. {