Parametrise PR values

[rossjrw]

Why doesn't PR Preview Action support preview from forks?

If you came here from the README this is probably the question you're asking.

The answer is that there are big security implications of allowing people you don't know and shouldn't trust (forkers) from being able to add code directly to your repository without any oversight. Even if your previews are isolated to a single branch (e.g. gh-pages), letting anyone insert whatever code they like there is not a good idea.

Consider what a malicious PR with write access to your repository is able to do:

1. Worst case scenario the malicious user may be able to get access to your main branch by writing code executed by e.g. your build process. They could e.g. edit your Actions workflow files to export all of your repo secrets.
2. If you can limit write to just that one branch, that user has write access to your entire website and can put whatever they want there.
3. If you can limit write to just that one branch and somehow just to that PR's given preview directory, that user can still create something you don't want on your website i.e. [you].github.io/pr-preview/pr-N/[something horrible] - and then anyone who sees it may assume that you endorse it

If you need previews and you need them from forks, don't use a tool that deploys forked code right back into your repository. Use a tool that hosts previews elsewhere, like Vercel/Netlify or some equivalent.

I'd like to add support for forks to this action eventually, but it'll require careful consideration of all the ramifications - along with thoroughly documenting how to navigate them.

Further reading: https://nathandavison.com/blog/github-actions-and-the-threat-of-malicious-pull-requests

---

This PR will:

• Remove the 'auto' action
  • This is a breaking change, so will need to be included in the v2 release
• Recommend using two separate workflows to deploy and remove previews respectively
• Add a recommendation for forks using workflow_run, which fixes Resolve permissions error when preview is initiated from fork #3
• To be able to call the action from a non-pull_request event, values that the action needs that previously came from the pull request directly will be parametrised
• Create a bunch of examples for different use cases in dedicated dirs, including the aforementioned
  • Instead of just adding md readmes, make a simple Jekyll site and move examples there (possibly in another PR)
• Make the source-dir input no longer required, and explicitly fail if it is given when action=remove

[github-actions]

PR Preview Action v1.1.1
🚀 Deployed preview to https://rossjrw.github.io/pr-preview-action/pr-preview/pr-6/
on branch gh-pages at 2022-04-14 18:22 UTC

[ryoqun]

@rossjrw hey, just found this action a perfect-match to our needs if pr previews can be enabled from forks. so, wondering if this pr can be merged soon?

[rossjrw]

@ryoqun Not soon, no - before I'm willing to release this kind of change I need to be sure that I can appropriately document the horrible security risks of allowing unmoderated forks from deploying code to your repository, and how to work around it.

I took a glance at your profile and it looks like you're using a different repo entirely to contain preview deployments, which seems very sensible. If you need this functionality right now, you could look at extracting the key steps from https://github.com/rossjrw/pr-preview-action/blob/022361539c71c58a7141d4fe8c3e0e4a1c34f9c5/action.yml and putting them in a GitHub Actions workflow directly in your main repo, with the necessary parameters. Alternatively, it looks like ACCESS-NRI#1 had some luck using the commits from this as-yet-unmerged PR to parametrise the PR values.

As always my top recommendation is to use a tool that's actually designed to do this reliably and is maintained by people who are paid to maintain it well, e.g. Vercel, Netlify.

[stoobie]

@rossjrw I'm wondering if you considered using the pull_request_target event. The issue I've encountered is with a PR from a fork trying to push to the gh-pages branch on the origin repo. The pull_request_target enables this, but in order to deal with the security issues you point out, it's necessary to isolate the workflows and even set it so that previews from forks require manual approval.

[zerothi]

Is there a reason why forked repos PR's couldn't be hosted on the forks gh-pages? That would bypass the security risk, no?

[rossjrw]

Is there a reason why forked repos PR's couldn't be hosted on the forks gh-pages? That would bypass the security risk, no?

@zerothi That's an interesting proposal. I'd like to see a proof-of-concept to explore in more detail, but just off the top of my head / with cursory research:

1. Forking a repo doesn't automatically deploy a Pages site - the forker would still need to go into the forked repo's settings and turn it on themselves. That's probably feasible with clear enough instructions - the inital workflow run could leave a comment instructing the forker to do so, but it wouldn't then be able to automatically update when/if the forker fixes it
2. The coupling between 'branch in forked repo' and 'pull request in original repo' is one-way - the PR knows about the fork but the fork doesn't know about the PR. It'd be difficult (impossible?) to run the preview workflow in the forked repo with the right parameters without doing some sort of lookup on the original repo's PR, assuming that information is available (it's possible that the forked repo's github token doesn't even have read access to the original repo). (To be fair, off the top of my head the only information that's needed is the PR number to create the URL/directory - and that could be replaced with the branch name)
3. I don't know if it would be possible to have the workflow in the original repo (that leaves a comment) know anything about the workflow in the forked repo (that creates the preview). It would be good to have the comment only be posted if the deployment is successful, but it might not be possible to determine that. (re point number 1, it might not be possible to determine if the fork has Pages enabled)

[zerothi]

1. True, I guess such a single shot thing would be ok to leverage for the committer, but of course, automating this later would be ideal.
2. I see, that is of course an issue, hmm... That may be a deal breaker here. Access to non-PR branches greatly limits its functionality.
3. Hmm, I see. I can also understand that this may be problematic for other security reasons, say if the PR requires some tokens for running.

[aspiers]

I definitely appreciate and applaud the caution with regard to security here; however aren't these risks precisely what https://docs.github.com/en/actions/managing-workflow-runs/reviewing-deployments is designed to solve? Personally I'd be fine with allowing workflows from forkers if I have to approve them before they can run.

Maybe I'm missing something but it doesn't feel to me like you need to carry the whole responsibility of making this safe. Ultimately if you issue clear caveats and document the safety considerations, then it's up to users of the action to act responsibly based on that. And if their repo breaks, they get to keep the pieces ;-)

[aspiers]

I also see that #38 was merged, so the pattern of deploying to a separate repo is an option, with the security isolation that provides. Nice!

[rossjrw]

@aspiers You're right - but it is my responsibility to avoid implying that things are guaranteed to be safe and secure. I don't know of any instances of malicious actors abusing this preview action and I'm very keen for it to stay that way.

This action is really very basic and most of the upcoming work is about documenting how to use it. You can do almost all of the things you describe right now using different configuration options around the action - e.g. the pull_request_target event gives a forked workflow permission to access the base repo, you can add deployment reviews if you like, as you pointed out you can even deploy to a different repo. There are a few things you can't do because of hardcoded PR information but that's what this PR #6 is for, to make the action even more generic.

I'm sure these options are valid approaches for an experienced user, but I feel that packaging an Action carries the implication that anyone can drop it into a project and use it regardless of skill level. I don't want to present these nuanced use cases to a user who might not understand the caveats that come with them, so in the meantime, stating that forks aren't supported is easiest on my end. But I certainly can't stop you from finding clever workarounds that work for your use case!

[aspiers]

Thanks a lot for the reply an extra info - I think we're probably in violent agreement :-) I wrote "Ultimately if you issue clear caveats and document the safety considerations" so I certainly wasn't advocating for you implying any misplaced guarantees around safety, or facilitating a more risky approach without the appropriate warnings. Perhaps where we may slightly differ in opinion is that I generally prefer for advanced users not to be limited through aiming to protect users who completely disregard their responsibility for their own security. But I can see arguments for both sides :-)

[lucasmerlin]

I've manually set this up for egui based on pull_request_target and it works great!
Here is the PR in case someone is interested: emilk/egui#5131

I've created a separate github org to host the preview deployments, so they are deployed on a separate domain not directly associated with the project.

[Spiral-Memory]

Hey @rossjrw,

Can we do something like this to run the workflow only if it's approved by the maintainer?

on:
  pull_request_review:
    types: [submitted]

jobs:
  approved_job:
    if: >
      github.event.review.state == 'approved' &&
      (
        contains(github.event.review.author_association, 'COLLABORATOR') ||
        contains(github.event.review.author_association, 'OWNER')
      )
    runs-on: ubuntu-latest

    steps:
     # Your steps here

This will ensure the workflow runs only if it's approved by a collaborator or the owner of the repository.

Will it still have security issues?

[rossjrw]

@Spiral-Memory It will have the same security issues. Best case scenario is that your contributors thoroughly review every revision of every pull request, ensuring no malicious attacks get through. Worst case scenario is that your contributors don't review thoroughly enough each and every single time, or, if you allow previews for new commits in PRs that have at some point been approved, the malicious actor simply adds the malicious content post-review.

The security hole is still there, even if there's a manual step in the way. It's up to you where to decide the right balance between security and convenience for your project. Your solution is possible, as far as I can see, but as the developer of this Action I can't recommend it.

[Spiral-Memory]

Thank you so much for your response, @rossjrw .

Yes, I understand the security risks involved. However, I plan to ensure that workflow will run only if the PR is approved by the maintainer or owner of the repo, and it won’t run after approval by any other contributor, only by maintainers.

Also, I’m planning to separate the build workflow (which will be in the pull_request trigger) and deployment workflow, which will only run on the pull_request_target trigger, so that malicious actors can't execute any scripts while the project is building.

Though I’m not sure, and would appreciate confirmation, I think if someone adds a new commit post-approval, GitHub removes the approved tag. I haven’t tested it yet.

So, in my opinion, the security risk is the same as merging the PR. As you pointed out, it’s about balancing convenience and risk, and this approach feels suitable to me.

I would love to hear your thoughts on any other security risks I should consider while implementing this.

Also, i would love to know the other approaches that exist for deploying the PR-preview.

[samsrabin]

Just wanted to echo what others have said about the utility of allowing runs on PRs from forks, although the security implications are of course important to consider. Requiring maintainer approval seems like a good idea, but yes, that trusts that maintainers always thoroughly review everything. Something I would personally add for my repo would be "don't allow this workflow to run at all on fork PRs if anything other than the text source files [RestructuredText, .rst] are touched." We have other ways to test our builds that we would fall back to in such cases.