Use powershell scripts instead of batch scripts for state exec.
#3228
Conversation
Passing arguments through batch is not reliable, especially arguments with special characters like '<' in them.
mitchell-as
force-pushed
the
mitchell/dx-2673
branch
from
f690f5f
to
b5ea2a0
Compare
mitchell-as
force-pushed
the
mitchell/dx-2673
branch
12 times, most recently
from
d5cf84c
to
a222a0a
Compare
mitchell-as
force-pushed
the
mitchell/dx-2673
branch
from
a222a0a
to
fd239f4
Compare
Passing arguments with special characters is limited when it comes to batch scripting.
This reverts commit b2cfedd.
mitchell-as
force-pushed
the
mitchell/dx-2673
branch
from
fd239f4
to
d1aaaee
Compare
| // The '<' needs to be escaped with '^', and I don't know why. There is no way around it. | ||
| // The other exec and shell integration tests that test arg passing do not need this escape. | ||
| // Only this batch test does. | ||
| arg = "^<3" |
There was a problem hiding this comment.
I feel like this is okay because if you are explicitly writing a batch script, then you ought to know the ins and outs of how batch handles incoming arguments, so if you're doing state run <name> <args> with funky <args>, then you should to know what needs escaping and what does not.
mitchell-as
force-pushed
the
mitchell/dx-2673
branch
from
d1aaaee
to
53bc073
Compare
|
The lone test failure is on macOS and is unrelated to this PR. |
| @@ -127,7 +127,7 @@ func runWithCmd(env []string, name string, args ...string) error { | |||
| case ".bat": | |||
| // No action required | |||
| case ".ps1": | |||
| args = append([]string{"-file", name}, args...) | |||
| args = append([]string{"-executionpolicy", "bypass", "-file", name}, args...) | |||
There was a problem hiding this comment.
I wonder if we've worked through the potential implications of this... some customers might have locked down environments and passing this flag could cause execution to fail. I remember we had a discussion about this during stand but I can't recall all of the details.
There was a problem hiding this comment.
We spoke during stand that we'd like to have Mark test this in a brand-new, unprivileged environment. If that fails, we can undo all of this. In my testing, it seems to work, but I haven't tried all configurations.
There was a problem hiding this comment.
I do wonder if that will catch potential issues with more complex locked-down environment, but I guess we will see if people complain.
Passing arguments through batch is not reliable, especially arguments with special characters like '<' in them.
On Windows, Go -> cmd -> executor/exe argument passthrough via
%*is problematic because the batch script doesn't know how to do parameter escaping if a parameter contains special characters. Instead, use Go -> pwsh -> executor/exe so that powershell correctly forwards arguments.This PR adds the following new integration tests:
state execuses powershell under the hood so that something likestate exec perl testargs.pl "<3"works as expected (the original bug report) without the user needing to know anything about escaping.state run foo "^<3"wherefoois a batch script explicitly written by the user works, but requires the '^' escape (presumably the user knows this, as I mention in a stand-alone comment). Note that State Tool invokes the user-defined batch script; it doesn't create its own script to facilitate running this.state shell,foo "<3"wherefoois a powershell script explicitly written by the user works as expected. Note that State Tool invokes the user-defined script (whether it be powershell or batch); it doesn't create its own script to facilitate running this one.