Parametrise PR values #6
Conversation
|
The Actions runner does not verify that inputs marked as 'required' are actually given: actions/runner#1070
When operating from any event other than pull_request et al., the commenter Action does not know which issue to comment on.
|
@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? |
|
@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. |
|
@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 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! |
|
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 :-) |
Since it's not working for PRs coming from forks. See: rossjrw/pr-preview-action#6
Since it's not working for PRs coming from forks. See: rossjrw/pr-preview-action#6
|
I've manually set this up for egui based on pull_request_target and it works great! 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. |
|
Hey @rossjrw, Can we do something like this to run the workflow only if it's approved by the maintainer? 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? |
|
@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. |
|
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. |
|
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. |
|
I'm looking forward to this :) our (TokTok org) projects only support fork PRs (except from dependabot). We don't do development on the main repo. |
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:
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:
workflow_run, which fixes Resolve permissions error when preview is initiated from fork #3pull_requestevent, values that the action needs that previously came from the pull request directly will be parametrised