exec.exec leaks action input parameters to subprocess

[jhenstridge]

Describe the bug
As input parameters are passed to the action as environment variables, and exec() reuses the action's environment by default, this means the input parameters will probably be visible to the subprocess. If any secrets are being passed via parameters, they will be visible too.

To Reproduce
Steps to reproduce the behavior:

1. Write a JS action that takes a parameter named foo
2. Have the action make a call like await exec.exec('sh', ['-c', 'echo foo==$INPUT_FOO'])
3. Use action in a workflow that sets a value for the input parameter

Expected behavior
I would expect exec.exec() to strip input parameters from the environment used to run the subprocess by default. If there are cases where it is desirable to pass input parameters to the subprocess, this feels like it should be opt-in.
Add any other context about the problem here.

Additional context
While writing an action that took a login token for an online service as an input parameter, I was trying to be careful not to leak the secret: some commands I need to run have the secret passed via command line, and others don't need it. In retrospect it seems obvious that the secrets would be visible to all subprocesses through the environment variables, but it wasn't obvious from the toolkit documentation.

[bryanmacfarlane]

Toolkit exec for subprocesses has an optional arg to specify what the env dictionary looks like.

https://github.com/actions/toolkit/blob/master/packages/exec/src/interfaces.ts#L10

However, it is an interesting question whether we should strip INPUT_ vars. Right now, it would have to be a blanket INPUT_ strip unless we loaded up the action yml

[jhenstridge]

I've used the env option when I wanted to pass additional environment variables to the subprocess. This was more about whether the API should be safe by default.

Stripping all INPUT_* environment variables seems like it should be fine. If there are actions that spawn subprocessed that care about an INPUT_* environment variable, chances are that it is a real input parameter.

An alternative would be for @actions/core to remove the variables from process.env. That is the approach taken by various systemd libraries that deal with configuration information passed through environment variables that should not be passed on to subprocesses. This seems more likely to cause compatibility problems though, if an action mixes calls to core.getInput with direct access to the environment.

[rhysd]

I think INPUT_* environment variables should be filtered out. Various famous GitHub Actions receive GitHub API token as input:

• actions/github
• actions/labeler
• actions/stale
• github-pages-deploy-action
• ...

it is a common case to use input for passing sensitive values like GitHub token. When using actions/exec for those actions, the sensitive values will be leaked to subprocesses.

If subprocess really requires some INPUT_* environment variables, it would be a good design to add INPUT_* values by env option. A user can explicitly set the necessary INPUT_* variables instead of passing all of them implicitly

// Assume some_command requires INPUT_FOO. Even in the case, the `env` value can set `INPUT_FOO` value.
await exec.exec('some_command', ['args'], {
  env: {
    INPUT_FOO: '...'
  }
});

[ericsciple]

Closing since this issue is stale (limited initial activity, and none for a over a year). The current solution is to use ExecOptions.env

If we do this change, we need to major version the toolkit since this would be a breaking change. Some folks likely rely on this behavior today - i.e. their logic is written in a different scripting language or even compiled binary (have seen this in the past).

Note, inheriting the env dictionary is also the default of Node.js and common scripting languages (bash, sh, ps, cmd). OS default too

[ericsciple]

Might be good to consider for the next major version

[ericsciple]

Chatted w/ @luketomlinson and @thboop offline. Let's track for next major version. Reopening for now, feel free to close again after this is tracked for next version (or leave open depending on however you decide to track)

[jhenstridge]

Maybe add a boolean flag to ExecOptions that will cause the INPUT_* variables to be stripped from the environment if set? And the next major version could switch to the safe option by default.

[luketomlinson]

@jhenstridge I think for the time being passing in an explicit env would accomplish the same purpose as a flag. You could then explicitly pass in any env vars you might need in the subprocess without passing in INPUT_* vars

[rhysd]

I still feel changing the default behavior is safer since I cannot come up with the case where a subprocess utilizes INPUT_* environment variables of the parent process directly.

In addition to changing the default behavior, I wander it might be nice to add an option to disable filtering out the variables from env object for some possible edge cases (meant filtering out can be opt-out).