Auto-preview to nextstrain.org fails on PRs from forks and dependabot

[victorlin]

I've been testing out using forks to create PRs (#1456; #1412 fork necessary from before joining the team).

For PRs from forks, the Github Action workflow used to create PRs to nextstrain.org for preview (src) fails with a generic error message (example):

  fatal: could not read Username for 'https://github.com/': No such device or address
  Error: The process '/usr/bin/git' failed with exit code 128

This is due to, for PRs from forks, workflows triggered by the pull_request event having read-only permissions and thus no access to secrets.NEXTSTRAIN_BOT_PAT which results in this error. The dependabot issue is discussed here: peter-evans/create-pull-request#871

In order for a workflow to have write access, it should use pull_request_target which comes with some security caveats. I'm having a hard time wrapping my brain around if this could be a security issue for the existing usage (PR from auspice fork -> PR to nextstrain.org with forked auspice code -> preview app).

It would be ideal if we could use pull_request_target then require manual approval for first-time contributors like this, but unfortunately this doesn't apply with pull_request_target.

Another thing to note is that PRs from forks require manual creation of Heroku Review Apps (doc):

For security and billing reasons, Heroku does not automatically create review apps for pull requests to public repos that are sent from forks. You can still create review apps for these pull requests manually.

Overall, this is not something that needs to be fixed in the grand scheme of things, since we can still test with manual creation of Heroku Review Apps. Just wanted to document a bit of research.

[tsibley]

I'm having a hard time wrapping my brain around if this could be a security issue for the existing usage (PR from auspice fork -> PR to nextstrain.org with forked auspice code -> preview app).

There are a lot of security concerns here. Some things that come to mind.

The Auspice PR → nextstrain.org PR transition involves a privilege escalation, as the nextstrain.org PR is created as an internal team member (@nextstrain-bot) instead of as the author of the Auspice PR. This means that the nextstrain.org PR has full access to workflow secrets, which could be read/exfiltrated by the external author of the Auspice PR.

Similarly, the nextstrain.org PR → Heroku review app transition is another escalation, as it provides access to the secrets we provide for review apps. This includes very sensitive information.

While initial manual approval seems like a way out, it would be pretty easy to submit an innocuous and plausible PR, wait for someone to approve an auto PR/review app for it, and then push additional malicious code (in the same PR or another that's auto-approved now). There are also variations on this where a new package that an innocuous-looking PR adds is actually malicious and controlled by the PR author.

It would be pretty complicated to secure this chain of workflows, and I don't think it's worth it.

[jameshadfield]

I'd prefer to err on the side of caution here. I initially implemented the nextstrain.org PRs to get a review-app and thus automate what I had been doing locally for a long time - in some situations it's useful but it has also resulted in a lot of nextstrain.org PRs which need to be periodically tidied up.

Closing this issue, but @victorlin feel free to reopen if you want to persue this.