Can I pass untrusted data to @actions/core commands?

[wagoid]

Hi there! I've got an action that uses the toolkit, called commitlint-github-action. BTW thanks for the amazing work! ❤️

This action used to call 2 features of this package: core.setFailed and core.setOutput. We logged commit messages information on these 2 commands.

After this announcement regarding CVE-2020-15228, I needed to disable command execution because I log untrusted data to the STDOUT, as they advised:

If you need to log untrusted information such as issue titles, bodies, or commit messages to STDOUT we recommend that you disable command processing prior to doing that.

I see that support for set-env and add-path through commands has been removed from runners, so I was tempted to update my action to issue commands again. But then I noticed this feature was not completely removed from runners as people can set the ACTIONS_ALLOW_UNSECURE_COMMANDS env var at the job level. So if someone enables the commitlint action on forked PRs, there's still a vulnerability on my action since people can add the env var on forked PRs.

All this bases on the assumption that the toolkit can't guarantee that the commands done through @actions/core are safe. But is that assumption correct? I see that there is some logic to escape data, would that be enough to call core.setFailed and core.setOutput without worrying that I'm passing untrusted information to it?

Or if you confirm that I can't use commands even through @actions/core functions, do you think there's an alternative solution that doesn't involve disabling workflow commands? 🙌

[thboop]

Hey @wagoid, thanks for reaching out.

This CVE has been resolved. Unless an workflow author explicitly opts into ACTIONS_ALLOW_UNSECURE_COMMANDS logging unsanitized data out to stdout is safe. We strongly recommend users do not opt into ACTIONS_ALLOW_UNSECURE_COMMANDS it was just done for a slight period of back-compat for production critical workflows.

core.setFailed it is safe to pass un-sanitized input.
core.setOutput, is a special case. Passing information should be fine but where you consume it you may want to be careful. We've seen lots of examples of using it in unique ways, so depending on your use case you may want to validate input. For example, if you set an output that was sql commands and in the next step you executed that sql against a production instance with no validation, you would be opening a security vulnerability.

That being said, I prefer to validate input from users whenever possible!

I'm going to close this out, but if you have any other follow up questions, feel free to reach out and we can reopen this!

[wagoid]

Hey @thboop, thanks a lot for the detailed explanation! Your answer was very helpful 🌟