Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for passing arguments to Invoke-Expression #23836

Open
MatejKafka opened this issue May 23, 2024 · 20 comments
Open

Add support for passing arguments to Invoke-Expression #23836

MatejKafka opened this issue May 23, 2024 · 20 comments
Labels
Issue-Enhancement the issue is more of a feature request than a bug Needs-Triage The issue is new and needs to be triaged by a work group.

Comments

@MatejKafka
Copy link

MatejKafka commented May 23, 2024

Summary of the new feature / enhancement

This is a follow-up on one specific aspect of #8816.

The irm ... | iex pattern is well-known and widely used for installing software, similarly to curl ... | sh on POSIX-like systems. We may not be happy about it, but it is widely used, and projects will use continue to use it in the future unless Invoke-Expression is removed alltogether.

#8816 floated some alternative options, converging on extending Invoke-Command. However, compatibility across all commonly used versions of PowerShell is vital for an installer script; a script that only works on some versions of PowerShell is not very useful as a one-liner, since now you have to give multiple one-liners and make the user choose based on the version of PowerShell they're using. This gives me a strong reason to believe that software vendors (including me) will NOT switch to any new backwards-imcompatible option to run downloaded scripts in one command.

One of the limitations of irm ... | iex mentioned in #8816 was that there's no intuitive way to pass optional arguments to the installer script. Unlike some of the issues, I believe that this issue may be resolved in a backwards-compatible way by allowing Invoke-Expression to receive additional arguments and passing them to the invoked script.

Users on older versions of PowerShell will still be able to use the iex "& {$(irm ...)} arg" hack shown in the original issue, while users on new versions of PowerShell can pass arguments the way they would intuitively expect, the same way as with curl ... | sh.

Proposed technical implementation details (optional)

Add a new -ArgumentList parameter to Invoke-Expression, with Parameter(ValueFromRemainingArguments = true). As a result, users should be able to do the following:

irm ... | iex arg -Flag1 -Flag2

If the script provides a param() block, the arguments should be bound as if the script was invoked as a scriptblock with &.

@MatejKafka MatejKafka added Issue-Enhancement the issue is more of a feature request than a bug Needs-Triage The issue is new and needs to be triaged by a work group. labels May 23, 2024
@mklement0
Copy link
Contributor

mklement0 commented May 23, 2024

I like the idea in principle, but, in my mind, to make it feasible, more changes are required:

  • Invoke-Expression runs code in the current scope, which effectively dot-sources an installation script('s code), polluting the caller's scope.

  • Similarly, an exit call is interpreted as exiting the caller's scope, meaning instant termination of an interactive session for calls from the interactive prompt and possibly premature exit from calls in scripts.

    • Yet, well-designed installer scripts should use exit, because that is how exit codes are communicated to the caller.

  • It follows that, with the current irm ... | iex approach:

    • There is currently no way to run a downloaded script that terminates with an exit statement without risking premature exit of the interactive session, the current script.

    • Conversely, for a script that deliberately avoids exit calls, the only - hacky - way to report an exit code is by setting $global:LASTEXITCODE directly.

    • Similarly, the current scope-pollution problem requires a non-obvious workaround: iex "& { $(irm ...) }"


Therefore, to me, for an enhancement to Invoke-Expression to be worthwhile, the following behaviors would need to be available at least on an opt-in basis:

  • Making exit in the code string terminate only the Invoke-Expression call, not the interactive session / the running script.
  • In that case, reflecting a (numeric) exit argument in $LASTEXITCODEafterwards, as in regular script execution.
  • Allowing execution in a child scope, to prevent pollution / inadvertent modification of the caller's scope (which, as an opt-in, would be the inverse of Invoke-Command's -NoNewScope switch, which is one of the reasons I suggested enhancing Invoke-Command instead in the linked issue).

If all of these have to be opt-ins - lest backward compatibility be broken - I fear that the additional switches needed, let alone the need to remember to use them, makes this enhancement less appealing.

That said, perhaps a single switch - say -AsScriptFile, aliased to -s, for symmetry with POSIX-compatible shells (which only need -s if pass-through arguments are also specified) - could serve as a catch-all opt-in to all these behaviors:

irm ... | iex -s [arg ...]

@rhubarb-geek-nz
Copy link

A couple of observations..

curl ... | sh is a really bad pattern because curl could fail and only download half the script, and sh would have no idea it was truncated.

curl ... | sh is more equivalent to Invoke-RestMMethod | pwsh.exe, effectively as if the user had just typed the scriipt, and in a completely new process.

@mklement0
Copy link
Contributor

mklement0 commented May 23, 2024

curl ... | sh is a really bad pattern

It is not only a very common pattern, but also a perfectly reasonably one, assuming the context of a pre-vetted (installer) script.
That the curl transfer could fail partway through does not negate the utility of this pattern; such a failure is a hypothetical that is not a real-world concern; dealing with it would substantially complicate the code.

Invoke-RestMMethod | pwsh.exe

You would think that that's the equivalent, but it - unfortunately - isn't:

And, yes, both ... | sh and ... | pwsh run the code in a child process, which is both a plus and a minus:

  • It is a plus in that it provides process isolation around the effects of the code being executed.
  • It is a minus in that:
    • it prevents the code from modifying the caller's process-level environment variables, such as $env:PATH
    • with PowerShell, specifically, a CLI call is expensive.

An in-process solution, such as via the enhancements to Invoke-Expression discussed here, addresses the minuses.

@rhubarb-geek-nz
Copy link

rhubarb-geek-nz commented May 24, 2024

such a failure is a hypothetical that is not a real-world concern; dealing with it would substantially complicate the code.

Tutorials often include the disclaimer that error handling is omitted for clarity, however the real world does include partial failure scenarios, and error handling is part of software engineering. Running half a downloaded script at elevated privileges is not a scenario I would like to be in.

In the PowerShell world the intention was for signed scripts to ensure the integrity of the script, that can only be done by downloading the entire script first in order to validate the signature.

While it may be common to pipe unseen content downloaded from the internet into a shell running as root, I would never recommend it as a practice.

@mklement0
Copy link
Contributor

The merits of the most defensive programming techniques available vs. pragmatic, real-world solutions in relation to trusted sources strike me as an entirely separate discussion, and, as such, an unhelpful distraction from what this issue is about.

@rhubarb-geek-nz
Copy link

rhubarb-geek-nz commented May 24, 2024

as such, an unhelpful distraction from what this issue is about.

Not when is is used as a justification for the requirement of the feature, and the existing supported PowerShell pattern of downloading the script first then running solves both the problem of script integrity and passing of arguments.

Compare with docker

curl -fsSL https://get.docker.com -o get-docker.sh
sudo sh get-docker.sh
Executing docker install script, commit: 7cae5f8b0decc17d6571f9f52eb840fbc13b2737
<...>

and dotnet

Invoke-WebRequest 'https://dot.net/v1/dotnet-install.ps1' -Proxy $env:HTTP_PROXY -ProxyUseDefaultCredentials -OutFile 'dotnet-install.ps1';
./dotnet-install.ps1 -InstallDir '~/.dotnet' -Version '6.0.2' -Runtime 'dotnet' -ProxyAddress $env:HTTP_PROXY -ProxyUseDefaultCredentials;

Neither suggest piping the script.

@mklement0

This comment was marked as resolved.

@rhubarb-geek-nz
Copy link

rhubarb-geek-nz commented May 24, 2024

Looking at what the definition of an expression is 7. Expressions in the PowerShell language, I don't see that expressions have param blocks.

In PowerShell functions, cmdlets, scripts and script blocks all support param blocks.

It looks to me that what you want to use is Invoke-Command but the reason you can't is that you can't pass in the script block as pipeline input.

Sounds like what you are wanting is the equivalent of

'param($foo); Write-Output "foo is $foo"' | ForEach-Object { Invoke-Command -ScriptBlock ([ScriptBlock]::Create($_)) -ArgumentList 'bar' }

Perhaps an argument/flag on Invoke-Command to read the ScriptBlock as ValueFromPipeline would be better, ideally treating multiple lines of text as part of a single script, and waits for the entire script to be read before invoking.

@MatejKafka
Copy link
Author

MatejKafka commented May 24, 2024

@mklement0

I like the idea in principle, but, in my mind, to make it feasible, more changes are required:

That's #8816. :) The reason why I opened this issue is to explicitly focus on passing arguments, which I think can be tackled as a separate feature without a dependency on the other features you mention.

Invoke-Expression runs code in the current scope, which effectively dot-sources an installation script('s code), polluting the caller's scope.

Yes, which is unfortunate for this use case. However, the issue can currently be solved by the author of the installer by wrapping everything in & { ... }. I'm not sure it warrants explicit support from Invoke-Expression, especially due to missing compatibility with older PowerShell versions, where the installer would still need to use & { ... }.

Yet, well-designed installer scripts should use exit, because that is how exit codes are communicated to the caller

Since the installer is typically invoked from a pre-existing PowerShell session, I'll disagree here. It's a PowerShell script, it should communicate failure using the standard PowerShell way – by throwing errors. Errors also work when the installer is invoked as pwsh -Command 'irm ... | iex', since pwsh will exit with non-zero exit code on error.

In practice, I don't remember ever coming across a PowerShell installer script that would call exit, but your experience may very well be different.

@MatejKafka
Copy link
Author

MatejKafka commented May 24, 2024

@rhubarb-geek-nz

Not when is is used as a justification for the requirement of the feature, and the existing supported PowerShell pattern of downloading the script first then running solves both the problem of script integrity and passing of arguments.

Do note that the issue with partial download does NOT apply to PowerShell. irm will download the whole script before passing it to iex as a single string. As @mklement0 already said, this discussion is irrelevant for this PowerShell-specific issue.

@mklement0
Copy link
Contributor

mklement0 commented May 24, 2024

@MatejKafka:

It's a alternative proposal to #8816, focused on Invoke-Expression (whereas the former landed on Invoke-Command).

Note that we'd have backward compatibility either way, given that no one wants to change the existing (default) Invoke-Expression behavior.

Enhancing Invoke-Expression rather than Invoke-Command would be a more familiar way forward, given the well-established irm ... | iex pattern, as you say, but the added functionality would by definition not be backward-compatible.

For that reason, focusing on adding argument support alone is not enough, in my estimation: if new functionality is added, I prefer it to be one that fully addresses the use case.

You're right that the exit issue is less pressing than I thought:

  • As long as the code being piped to Invoke-Expression always uses script-terminating (runspace-terminating) error (throw / $ErrorAction = 'Stop'; -ErrorAction Stop) to signal failure (https://aka.ms/install-powershell.ps1 is an example of that), life is good: execution via Invoke-Expression only aborts the code being evaluated, and a CLI call results in exit code 1 (though invariably so).

  • That said, I think a solution that treats exit as it would inside a script file is still preferable, given that users may choose to run scripts via direct download that are not (primarily) designed for it.

Having a single switch that opts into script file-like behavior could provide all desired behaviors: argument support, no caller-scope pollution, no exit surprises.

Even if we leave the exit issue aside, with respect to the new (and therefore backward-incompatible) functionality, the choice is between:

# Current proposal: still requires workaround for scope pollution
iex "& { $(irm ...) } @args" [arg ...]

vs.

# With -AsScriptFile / -s opt-in:
irm ... | iex -s [arg ...]

@MatejKafka
Copy link
Author

MatejKafka commented May 24, 2024

@mklement0

Note that we'd have backward compatibility either way

My bad, I wasn't clear. What I meant is that for an installer script, compatibility with older versions of PowerShell is typically paramount. Therefore, authors of installers will not alter their installation scripts to make use of newly added features if it would make the script incompatible with older versions of PowerShell. This leads me to believe that preventing scope pollution and supporting exit is not something that would be utilized in the scripts, since neither of the features works in older versions.

Even if we leave the exit issue aside, with respect to the new (and therefore backward-incompatible) functionality, the choice is between:

Not exactly. The following installer script currently works both on PowerShell 5 and the latest version, when invoked as irm ... | iex and does not pollute the parent scope:

param($Param1 = "default", $Param2)

& {
  $NewVar = "hello"
  echo "Using $Param1..."
  # ...
}

My proposal is only to simplify the code needed to pass arguments to such script from:

iex "& {$(irm ...)} arg -Param2 arg2"

to:

irm ... | iex arg -Param2 arg2"

@mklement0
Copy link
Contributor

So you're saying that there are installation scripts out there that explicitly compensate for the scope-pollution issue that irm ... | iex entails by enclosing their script body in & { ... }?

And your proposal is focused solely on making argument-passing to such specially-crafted scripts easier?

Note that the scope-pollution issue is implicitly avoided by the current invocation pattern you cite as necessary for passing arguments,
iex "& {$(irm ...)} arg -Param2 arg2" - whether or not the original script is specially crafted to avoid scope pollution, due to use of & (execution in a child scope).

If you're proposing that the mere fact of passing arguments should implicitly transition from same-scope execution to child-scope execution: I think that amounts to ill-advised, obscure behavior, as there is then no justification for
irm ... | iex - without arguments - continuing to perform same-scope execution.

A more consistent approach would be to make irm ... | iex arg -Param2 arg2 the new, backward-incompatible syntax for
iex ". {$(irm ...)} arg -Param2 arg2 - but, again, that doesn't seem worth it to me, without also at least (consistently) solving the scope-pollution issue for not specially crafted scripts, such as https://aka.ms/install-powershell.ps1.

@MatejKafka
Copy link
Author

MatejKafka commented May 24, 2024

So you're saying that there are installation scripts out there that explicitly compensate for the scope-pollution issue that irm ... | iex entails by enclosing their script body in & { ... }?

Not sure if there are scripts doing this other than mine; my point is that if they care about scope pollution, they can already handle it today. While I do agree that it would be better to have iex do it automatically, I'm not sure that it's valuable enough to warrant adding another switch parameter.

Adding the parameter would give the authors of installer scripts three options:

  1. Ignore scope pollution and tell users to run irm ... | iex.
  2. Design the script to not pollute scope by using the & {...} trick and tell users to run irm ... | iex.
  3. Design the script to assume that it's running in a new scope, and explain to users that if they're on PowerShell vX.Y.Z or higher, they should run irm ... | iex -AsScriptFile and if they're on a lower version, they should run irm ... | iex and have the script pollute their scope.

My guess is that if the author doesn't care (or doesn't know) about the issue, they'll go with 1), and if they do care, they'll go with 2), since it provides the best experience for all users.

There's another case where the parameter may be useful, which is when a script is designed using 1), but a savvy user wants to prevent it from polluting the parent scope. However, the user has the choice of just running & {irm ... | iex}, which is 1) shorter then the name of the parameter, 2) more intuitive to other users who don't know about the parameter.

If you're proposing that the mere fact of passing arguments should implicitly transition from same-scope execution to child-scope execution

Definitely not.


A more consistent approach would be to make irm ... | iex arg -Param2 arg2 the new, backward-incompatible syntax

The value that I see in my proposal is that it does not require cooperation from the script author. The author is free to say "The installer script takes parameters. To pass them, use the following invocation: iex "& {$(irm ...)} arg". My proposal gives an option to the savvy user to use a simpler syntax to pass arguments.

An additional point is that the default installation one-liner will be used by many users, and if the defaults are chosen well, most of the users will never need to learn how to pass parameters. The users who are picky enough to want to change the defaults are also more likely to know about the various ways of passing arguments through iex, or at least understand that there are multiple options depending on which version of PowerShell they're using.

@mklement0
Copy link
Contributor

@MatejKafka, I appreciate the explanation, but to me it comes down to this:

Introducing a new feature solely for the benefit of providing syntactic sugar for a well-established, but fundamentally flawed idiom is ill-advised - even if scripts that compensate for the flaws exist.

The scope-pollution issue cannot be solved without an opt-in, if backward-compatibility is to be preserved.

The alternative to 3. is:

  • Tell users targeting older versions to use iex "& {$(irm ...)}" to avoid scope pollution and, in order to also pass arguments, to use iex "& {$(irm ...)} arg".

  • Tell users on newer versions to use irm ... | iex -s to avoid scope pollution and, in order to also pass arguments, to use irm ... | iex -s arg

@MatejKafka
Copy link
Author

@rhubarb-geek-nz This issue is about extending Invoke-Expression, due to reasons that I've already extensively described above. If you want to extend Invoke-Command or create a new cmdlet, feel free to open another issue, but your example is off-topic for this issue and does not add to the discussion (it's pretty obvious how such a cmdlet would be implemented).

@rhubarb-geek-nz
Copy link

Invoke-Expression treats each record in the input pipeline as its own string expression

 '1+2','2+3','3+4' | Invoke-Expression
3
5
7

If an argument list was provided it would only make sense for a few scenarios for the same argument list to be given to multiple different expressions.

If you were providing a single argument list as described by the given scenarios then it would be reasonable to expect the entire input to be treated as a single script, so all lines would be joined with newline separators before converting to a single script.

If all unbound arguments were converted into an argument list it would change the behaviour for errors where the script has mistakenly missed the quotations.

PS> Invoke-Expression 1 2
Invoke-Expression: A positional parameter cannot be found that accepts argument '2'.
PS> Invoke-Expression '"foo"'  '"bar"'
Invoke-Expression: A positional parameter cannot be found that accepts argument '"bar"'.
PS> Invoke-Expression 'foo' $bar
Invoke-Expression: A positional parameter cannot be found that accepts argument '$null'.

@mklement0
Copy link
Contributor

All these behaviors (I originally didn't consider the multi-string pipeline-input case, because it doesn't apply to Invoke-RestMethod) can be tied to opt-in via the proposed -s / -AsScriptFile (perhaps -AsScript is enough?); to summarize:

If and only if -s is specified (otherwise, the current behavior remains in effect):

  • Collect all pipeline input up front - whether provided as a single, multiline string or via individual lines to be joined.
  • Allow arbitrary pass-through parameters (via ValueFromRemainingArguments)
  • Treat the collected code as if it were the content of a script file being invoked, which means:
    • Execute the code in a child scope and pass it any pass-through arguments.
    • Make exit only exit the code being evaluated, and set $LASTEXITCODE accordingly.

In the use case with pass-through arguments, the -s option then parallels Bash (e.g., 'echo [$@]' | bash -s one two); without pass-through arguments, Bash still accepts -s, but it is not necessary - whereas in our case it would be.

This solution is fully backward-compatible and doesn't even change error messages for the broken invocations you mention.

@rhubarb-geek-nz
Copy link

rhubarb-geek-nz commented Jun 3, 2024

Assuming discussions about the difference between could and should with comparisons to UNIX shell are permitted.

I see Invoke-Expression as being a very light-weight construct similar to POSIX shell "eval". All eval does is invoke an expression within the current process. I see very close parallels with Invoke-Expression running in the current scope and executing the given expression provided as a string.

To be honest, I am very surprised the original PowerShell did not create an alias for eval resolving to Invoke-Expression.

The equivalent wanted for curl|sh is much more consistent with using Invoke-WebRequest | Invoke-Commmand, where currently the issue is it can't read a script from the pipeline.

Based on the principle of "do one thing and do it well", I think Invoke-Expression already does that. I think the proposal is to make it do something else as well.

So on the topic of extending Invoke-Expression,, while it could be done, I don't think it should.

@mklement0
Copy link
Contributor

mklement0 commented Jun 4, 2024

  • Yes, Invoke-Expression is like eval in POSIX-compatible shells, but it blurs the line between eval and ... | bash, for instance, due to supporting pipeline input and, generally, PowerShell scripts executing in-process, making ... | iex the counterpart of ... | bash

  • I too originally advocated for extending Invoke-Command, as extensively discussed in Make it easier to download and execute scripts as a single operation, by enhancing Invoke-Command #8816, but on further reflection there are problematic aspects:

    • Invoke-Command is virtually exclusively used for remote code execution (in local executions, it is a mostly inferior analog to & { ... } [arg ...]; it only has value if you want to capture output streams via -*Variable common parameters, but that is rarely seen in practice). As such, adding a local use case may be confusing.
    • Invoke-Command already has a plethora of parameters and parameter sets, making discovery of the relevant parameters for yet another use case challenging.
    • Invoke-Command only supports positional argument-passing, via -ArgumentList (i.e. not also named ones), though I suppose that could be fixed for all use cases.
  • Make it easier to download and execute scripts as a single operation, by enhancing Invoke-Command #8816 suggested making Invoke-Command accept Invoke-WebRequest output directly as pipeline input, which was actually originally proposed by @lzybkr for Invoke-Expression in Add Direct Evaluation of Web Content as PS Code #5909 (comment)

This proposal, applied to Invoke-Expression, would provide the most seamless extension of the current functionality:

  • All the execution-as-if-it-were-a-script-file behaviors listed above could implicitly be applied in this case.
  • Pass-through arguments could be supported without an extra switch, as their support can the be tied to this uses case (Microsoft.PowerShell.Commands.WebResponseObject as pipeline input)

Note that this implies that iwr (Invoke-WebRequest) rather than irm (Invoke-RestMethod) must then be used.

That said, the -s / -AsScript switch could still be implemented to also enable irm input (or text from any source); it would then be implied with iwr input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement the issue is more of a feature request than a bug Needs-Triage The issue is new and needs to be triaged by a work group.
Projects
None yet
Development

No branches or pull requests

4 participants
@mklement0 @MatejKafka @rhubarb-geek-nz and others