WIP: Add experimental option for quoting cmdline #13483

Artoria2e5 · 2020-08-19T22:32:26Z

PR Summary

This pull request adds a new experimetal feature PSEscapeForNativeExecutables to better handle quotes, backslashes, and whitespace in command arguments.

Many cases of existing code already compensate for the lack of quoting by manually adding backslashes before quotes. These code will need to be fixed. An example of what these stuff look like can be found in the Pester changes.

PR Context

See #1995. This is a more cleaned-up version of #13099, now fully aware of the backward compatibility things. Code duplication is kept to a minimum where I can. (Sorry for effing up with the stale bot. But eh, at least this is a chance of squashing commits.)

I am still inclined to think that not engaging with the broken NeedsQuote stuff is a better way of fixing things, unlike #13482's patchy approach.

PR Checklist

* do what PowerShell#13089 says. consolidate and fix cmdline escaping. * an empty string check. native commands can accept empty arguments! * attempt wildcard by spaces. now we do it by bareword.

Empty arguments are seen as significant now, so we have to be explicitly passing an empty list in the position. Pester/Start-Process: don't rely on bad argumentList behavior D'oh. Pester/ConsoleHost: Empty command is valid What isn't valid is a missing argv entry after -c. Pester/NativeStreams: remove redundant escape Pester/Get-PSHostProcessInfo: argumentList is for lists Pester/XMLCommand: Args is supposed to be a list.

Artoria2e5 · 2020-08-19T22:42:41Z

src/System.Management.Automation/utils/PasteArguments.cs

+                    // If the stuff is unquoted, we need to double the trailing backslashes since we
+                    // are putting a quote here.
+                    int numBackSlash = 0;
+                    for (int i = stringBuilder.Length - 1; i > 0 && stringBuilder[i] == Backslash; i--)


StringBuilder really just holds a stack of chunks. So like appends, this is quite efficient given we are doing stuff on the tail, which is on top.

(I was worrying about ending up on some sort of "accidentally O(n log n)" page.)

daxian-dbw · 2020-08-19T22:58:50Z

/cc @JamesWTruher and @SteveL-MSFT

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

iSazonov · 2020-08-20T07:46:03Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

+        /// Whether to escpae the argumentList.
+        /// </summary>
+        [Parameter]
+        public bool? EscapeArgs { get; set; }


Why not SwitchParameter ?

I want people to be able to explicitly turn it off.

Wait, you can do that with a SwitchParameter? The API on the C# side makes it really non-obvious, since it gives off a 2-state instead of a 3-state with bool?.

It's not obvious, but you can explicitly "turn off" a switch parameter using: "verb-noun -switch:$false" syntax

Yeah, but then I don't think I can tell the difference between not present and set to false. I still think there's some value in having the default follow the experimental option.

There is IsPresent property.

But IsPresent cannot differentiate between a given false and not present. If you check the definition of that struct, you will see it really just holds a bool for two possibilities.

If the parameter needs to have more than two states (on, off, and not specified) it should be an enum. bool parameters do not have good UX in PowerShell.

use MyInvocation.BoundParameters.ContainsKey(nameof(EscapeArgs)) if you really need to test for whether the user provided it explicitly.

But in general if you want a bool, use a switch. And if you have a switch, name the parameter & code the function such that the default is that not provided == off.

Bool parameters and requiring arcane nonsense like -Switch:$false are both very annoying anti-patterns that are invariably confusing to users.

Opt for simplicity when designing parameters, otherwise PS users will be complaining about this for the next decade.

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

src/System.Management.Automation/utils/PasteArguments.cs

test/tools/TestExe/TestExe.cs

iSazonov · 2020-08-20T08:00:25Z

src/System.Management.Automation/engine/NativeCommandParameterBinder.cs


-                if (!string.IsNullOrEmpty(arg))
+                if (!ExperimentalFeature.IsEnabled("PSNativePSPathResolution") &&
+                    arg.Length == 0)


Do we need a null check for arg?

From a type perspective, I don't see how it can ever be null. I think it either works or throws.

On a second thought, I should use the one that has null just to be sure. Will get it changed when I get back to a physical keyboard.

that's a pretty good point since there can be so many extra things to tweak

I have no idea how laws work

iSazonov · 2020-08-21T05:50:39Z

@Artoria2e5 Please resolve the merge conflict.

ghost · 2020-08-30T02:00:13Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

I will get to the args and the tests, I promise...

JamesWTruher · 2020-10-15T22:53:15Z

based on feedback and the fact that enough has changed underneath this change that I'm going to close this and implement something a little different

JamesWTruher · 2020-10-15T22:54:03Z

whoops - sorry @Artoria2e5 - i thought this was mine

SteveL-MSFT

My main concern is backwards compatibility. I see a number of tests were modified and it's not clear if they were modified because the previous workaround aren't needed or they were modified because the previous workarounds no longer work. Also, it would be great to have a bunch of new tests specific to cover quoting test cases.

SteveL-MSFT · 2020-10-15T23:20:21Z

src/System.Management.Automation/engine/NativeCommandParameterBinder.cs

                }

-                if (!string.IsNullOrEmpty(arg))
+                if (!ExperimentalFeature.IsEnabled("PSNativePSPathResolution") &&


Is this the right experimental feature to check? If so, can you explain the intent of this?

SteveL-MSFT · 2020-10-15T23:22:53Z

test/powershell/Host/ConsoleHost.Tests.ps1

        }

-        It "Empty command should fail" {
-            & $powershell -noprofile -c ''


What does empty command do now? If it does nothing (basically emitting an empty string), should add a test for that.

Agreed. I think I should just flip the expectation of failure.

I didn't actually change the behavior of an empty command -- it used to fail because it was essentially calling 'powershell','-noprofile','-c', and it is an error to not follow -c with an argument.

Looks like I did rename it to "missing command", so I guess I'm good there?

I really need to get the pure-ps escaper in though.

SteveL-MSFT · 2020-10-15T23:23:45Z

test/powershell/Host/Startup.Tests.ps1

            Remove-Item $profileDataFile -Force
        }

-        $loadedAssemblies = & "$PSHOME/pwsh" -noprofile -command '([System.AppDomain]::CurrentDomain.GetAssemblies()).manifestmodule | Where-Object { $_.Name -notlike ""<*>"" } | ForEach-Object { $_.Name }'


For scripts that currently use double-double-quotes, will they now start failing with this PR?

This is a case of over-escaping, and it occurs as a native-command call. So yes, it will fail, since it is literally now passing the wrong command

([System.AppDomain]::CurrentDomain.GetAssemblies()).manifestmodule | Where-Object { $_.Name -notlike ""<*>"" } | ForEach-Object { $_.Name }

I understand that, but the problem is that it's very likely existing production scripts are relying on that behavior. So "fixing" it will break them.

SteveL-MSFT · 2020-10-15T23:25:12Z

test/powershell/Language/Scripting/NativeExecution/NativeStreams.Tests.ps1

-            '[Console]::Error.Write(\"foo`n`nbar`n`nbaz\"); ',
-            '[Console]::Error.Write(\"middle\"); ',
-            '[Console]::Error.Write(\"foo`n`nbar`n`nbaz\")'
+            '[Console]::Error.Write("foo`n`nbar`n`nbaz"); ',


Is the backslash escape not needed or will it now break existing code that used this pattern?

It will break such over-esacpes. Hence the experimental switch.

SteveL-MSFT · 2020-10-15T23:25:49Z

test/powershell/Modules/Microsoft.PowerShell.Core/Get-PSHostProcessInfo.Tests.ps1

        $psFileName = $IsWindows ? 'pwsh.exe' : 'pwsh'
        $psPath = Join-Path -Path $PSHOME -ChildPath $psFileName
-        $psProc = Start-Process -FilePath $psPath -ArgumentList "-File $testfilePath -LiveFilePath $aliveFile" -PassThru
+        $psProc = Start-Process -FilePath $psPath -ArgumentList '-File',$testfilePath,'-LiveFilePath',$aliveFile -PassThru


Shouldn't the old value still work?

It should since 99e8f57. Ah well... I should revert these Start-Process test changes.

Artoria2e5 · 2020-10-17T09:35:39Z

I should really get some strong coffee and just finish up the tests. As for testing the diverging & behavior, I think I could just add a utility ps function to "over-escape" the input when the feature is off.

ghost · 2020-10-28T14:00:08Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

SteveL-MSFT · 2020-12-16T23:54:23Z

@PowerShell/powershell-committee reviewed this, we would like to see the native command parameter binder leverage the new .NET API that passes an array of arguments to processes so that there isn't incorrect handling of args. We would also like to see this change early in 7.2 to gauge compatibility impact of such a change.

ghost · 2021-01-14T02:00:12Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

Artoria2e5 · 2021-05-11T02:36:41Z

Now that the new native facility has been added, I do not see any possibility for this PR to be accepted. Closing it myself.

Artoria2e5 added 2 commits August 20, 2020 06:22

Cmdline changes set for 7.2 (draft)

05dda64

* do what PowerShell#13089 says. consolidate and fix cmdline escaping. * an empty string check. native commands can accept empty arguments! * attempt wildcard by spaces. now we do it by bareword.

Artoria2e5 requested review from TravisEz13, adityapatwardhan, anmenaga and daxian-dbw as code owners August 19, 2020 22:32

ghost assigned anmenaga Aug 19, 2020

Artoria2e5 commented Aug 19, 2020

View reviewed changes

iSazonov reviewed Aug 20, 2020

View reviewed changes

Artoria2e5 added 6 commits August 21, 2020 00:02

Update NativeCommandArguments test

830d56f

Process (-EscapeArgs): correct typo, add remarks

4ed239f

Name the forceQuote parameter

1341f47

that's a pretty good point since there can be so many extra things to tweak

PasteArguments: suggested license change

42b9833

I have no idea how laws work

Add appeasement

0456433

fixup! should be two pieces

66075d0

Merge branch 'master' into cmdlinev2

0248f5a

ghost added the Review - Needed The PR is being reviewed label Aug 30, 2020

Artoria2e5 and others added 4 commits August 31, 2020 12:48

Merge branch 'master' into cmdlinev2

49fc5aa

ncpb: null-or-empty

e71e21a

I will get to the args and the tests, I promise...

Start-Process: use a separate parameter for escaped

99e8f57

NCPB: only use colon on option-looking stuff

59c9b96

JamesWTruher closed this Oct 15, 2020

JamesWTruher reopened this Oct 15, 2020

SteveL-MSFT requested changes Oct 15, 2020

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 15, 2020

ghost removed Review - Needed The PR is being reviewed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Oct 15, 2020

ghost added the Review - Needed The PR is being reviewed label Oct 28, 2020

Artoria2e5 added 5 commits October 29, 2020 16:28

revert some tests, update some other

49f80d8

appease the linter

1db9dbb

appease the language

2f1df44

APPEASE THE LANGUAGE LORD

0ab08f3

appease...

3dbb3d4

SteveL-MSFT added Backport-7.2.x-Consider Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Dec 15, 2020

ghost removed the Review - Needed The PR is being reviewed label Dec 16, 2020

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jan 6, 2021

ghost added the Review - Needed The PR is being reviewed label Jan 14, 2021

JamesWTruher mentioned this pull request Feb 9, 2021

Use ArgumentList when invoking native executables #14747

Closed

Artoria2e5 closed this May 11, 2021

adityapatwardhan removed the Backport-7.2.x-Consider label Sep 15, 2021

ghost removed the Review - Needed The PR is being reviewed label Sep 15, 2021

WIP: Add experimental option for quoting cmdline #13483

WIP: Add experimental option for quoting cmdline #13483

Uh oh!

Conversation

Artoria2e5 commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Artoria2e5 Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Aug 19, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Artoria2e5 Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vexx32 Aug 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Artoria2e5 Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Aug 21, 2020

Uh oh!

ghost commented Aug 30, 2020

Uh oh!

JamesWTruher commented Oct 15, 2020

Uh oh!

JamesWTruher commented Oct 15, 2020

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Artoria2e5 Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Artoria2e5 Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Artoria2e5 commented Aug 19, 2020 •

edited

Loading

Artoria2e5 Aug 19, 2020 •

edited

Loading

Artoria2e5 Aug 20, 2020 •

edited

Loading

vexx32 Aug 22, 2020 •

edited

Loading

Artoria2e5 Aug 20, 2020 •

edited

Loading

Artoria2e5 Oct 17, 2020 •

edited

Loading

Artoria2e5 Oct 17, 2020 •

edited

Loading

Artoria2e5 Oct 17, 2020 •

edited

Loading

Artoria2e5 commented Oct 17, 2020 •

edited

Loading