Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve permissions error when preview is initiated from fork #3

Open
rossjrw opened this issue Apr 6, 2022 · 5 comments · May be fixed by #6
Open

Resolve permissions error when preview is initiated from fork #3

rossjrw opened this issue Apr 6, 2022 · 5 comments · May be fixed by #6
Labels
bug Something isn't working
Milestone

Comments

@rossjrw
Copy link
Owner

rossjrw commented Apr 6, 2022

https://github.com/scpwiki/interwiki/runs/5834681840?check_suite_focus=true

This workflow run failed because it did not have permission to push to the upstream repository.

I believe this happened because the workflow was executed from the fork. The pull_request event is executed in the context of the merge commit, whereas the pull_request_target event is executed in the base repository. For internal PRs, these are identical; for forks, they are not. In particular this means that the action will not have permission to push to the repository when using the pull_request event type, as its GITHUB_TOKEN lacks that permission.

Switching to the pull_request_target event might resolve this, but there are security considerations to take into account: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Changes shouldn't be needed to this action, but I will need to amend documentation to cover this use case. The README is getting pretty bloated - I should take this chance to move some of the code samples into the repository proper.

@rossjrw rossjrw added the bug Something isn't working label Apr 6, 2022
@rossjrw
Copy link
Owner Author

rossjrw commented Apr 12, 2022

As recommended by the linked article, the flow I'm going to go with is a pull_request event that builds the deployment and saves it to an artifact, followed by a workflow_run that downloads the artifact and deploys it.

workflow_run is unique in that it executes with permissions inherited from the base repository, not the fork, so it will be able to deploy code. It's important to ensure that any code execution (i.e. the build script) is run in a non-privileged environment, in this case the pull_request event.

Downside to workflow_run is that it runs totally isolated from the calling context, and won't appear as e.g. a failing check in the PR. This is in contrast with e.g. workflow_call which is tightly integrated with the PR system and will appear as its own check; however, workflow_call does not execute with elevated permissions.

Also, the action currently strongly assumes that it's going to be run from a pull_request event, going so far as to assume github.event refers to a pull request. I need to make it also work for workflow_run, and in so doing I might as well remove its dependency on any specific kind of event.

That'll mean that I need to be able to pass it all the values that it would normally take directly from the pull request, via parameter instead. I can probably just set the defaults to the pull request values.

@rossjrw rossjrw linked a pull request Apr 14, 2022 that will close this issue
6 tasks
@rossjrw rossjrw added this to the v2 milestone Apr 14, 2022
@electrofelix
Copy link
Contributor

Would an option here be to have an example that uses pull_request_target for closed & labeled where it requires that a specific label has been applied indicating a review has taken place. The action can then remove the label at the end of the run to ensure that any updates require a fresh review. The closed event can always skip performing a build which prevents the risk of executing code under control of the fork.

I'm planning to set up a local action this way to support preview until workflow_run support is available at least, but I might still keep with the labeled approach to give things a once over before letting it publish either way.

@rossjrw
Copy link
Owner Author

rossjrw commented Jun 26, 2022

Good idea, if perhaps a little complex to set up and then have your team remember. Depending on the scope of your project, at that point it may be a better idea to outsource that work to somewhere you know it's been done correctly e.g. Netlify, Vercel.

robmoss added a commit to robmoss/git-is-my-lab-book that referenced this issue Nov 2, 2022
We can only publish PR previews from the original repository. The action
fails when the PR originates from a forked repository. For details, see
rossjrw/pr-preview-action#3.
GregSutcliffe added a commit to ansible-community/community-website that referenced this issue May 17, 2023
Forks cannot initiate previews using default GH action token. See
rossjrw/pr-preview-action#3 for details
@Fir121
Copy link

Fir121 commented Apr 20, 2024

Not knowledgeable about this workflow, but I've been reading the docs and a few issues and I was wondering to get past this security problem is there no way to prompt the repository owners to allow deployment of a preview for the created PR. Ie: Require the owner to click a button or put a comment and only preview the PR after that?

@rossjrw
Copy link
Owner Author

rossjrw commented Apr 21, 2024

@Fir121 Yes, the button you describe is possible - have a look at Deployment Reviews. I learned about this just the other day from @aspiers in #6 (comment)

There's any number of things you could put between a forked PR and that PR's preview running your repo with elevated permissions, and you could do any of those things right now, but I'm still wary at this time. That being said, if you happen to take the time to put together something functional that uses this or something like this, I'd be happy to have a look and see if I feel it's worth recommended in this documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants