Closed ConorMacBride closed 2 years ago
stefanzweifel
commented
2 years ago Thanks for sharing this! Sounds lovely that it now seems to be possible to use this in PRs made by forks. Will do some verification and testing in my own repos and update the README.
stefanzweifel
commented
2 years ago Just did a quick test in a playground repository, where the commits were made by a user that is not a contributor to the repository: https://github.com/stefanzweifel/git-auto-commit-action-demo-app/pull/33
repository: ${{ github.event.pull_request.head.repo.full_name }} and listen to the pull_request_target event.The run fails due to a permission error:
remote: Permission to wnxtrash/git-auto-commit-action-demo-app.git denied to github-actions[bot]. fatal: unable to access 'https://github.com/wnxtrash/git-auto-commit-action-demo-app/': The requested URL returned error: 403
… which is honestly something I expected. GitHub introduced these contraints as a security measure against bad actors. I would have been surprised if they removed them. There was also no announcement from GitHub about such a change on their official channels (blog, Twitter).
I assume your workflow run was successful, as you're a member of the sunpy Github organization? In my example the wnxtrash User has no affiliation with the test repo. And therefore doesn't have push rights.
I'm closing this issue for now.
ConorMacBride
commented
2 years ago The pull_request_target trigger only takes effect if it is present in the workflow file in the PR base branch, i.e., the workflow https://github.com/stefanzweifel/git-auto-commit-action-demo-app/blob/main/.github/workflows/format_php.yml not the PR branch. The security measure is that GitHub Actions runs the version of the workflow from stefanzweifel/git-auto-commit-action-demo-app (main) and not wnxtrash/git-auto-commit-action-demo-app (main). So you have full control of the actions that are run and it is therefore trusted with a GITHUB_TOKEN with write permissions.
Here's an example of a workflow running with write permission even though the user who triggered it doesn't have special repository or organisation permissions: https://github.com/sunpy/sunpy/runs/5952858318?check_suite_focus=true#step:1:14
stefanzweifel
commented
2 years ago The security measure is that GitHub Actions runs the version of the workflow from stefanzweifel/git-auto-commit-action-demo-app (main) and not wnxtrash/git-auto-commit-action-demo-app (main). So you have full control of the actions that are run and it is therefore trusted with a GITHUB_TOKEN with write permissions.
You're totally right. That's how this works. I've been too far away from the innerworkings of GitHub Actions in the last few months. 😅
And this actually also worked in my demo repository. https://github.com/stefanzweifel/git-auto-commit-action-demo-app/pull/34
Will update the README shortly with an example using repository: ${{ github.event.pull_request.head.repo.full_name }}.
achieveordie
commented
1 year ago @stefanzweifel @ConorMacBride Although this is incredibly useful, doing this would introduce security vulnerabilities. Quoting from the link:
TL;DR: Combining
pull_request_targetworkflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.
I think it is important to point it out in the README.md section for all the users of this action.
vincerubinetti
commented
1 year ago I'm having trouble getting this to work, and I believe I'm doing everything the same, except for that I'm calling the workflow from another workflow, but I believe it's still getting the right repo and ref and all that. Here are my workflows and logs. In this case I'm trying to do a pull request from greenelab/lwt-test:test into vincerubinetti/lab-website-template:main (I also tried main into main). I'm still getting the error:
remote: Permission to greenelab/lwt-test.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/greenelab/lwt-test/': The requested URL returned error: 403
Error: Invalid status code: 128Both repos also have Actions enabled, and the most permissive settings:
I happen to be an admin under both organizations, vincerubinetti and greenelab.
Link to run (will dissappear in the future): https://github.com/vincerubinetti/lab-website-template/actions/runs/4118566556/jobs/7111241089
vincerubinetti
commented
1 year ago In fact I tried to do it the other way, where the workflow runs in the forked repository instead, and I couldn't get that to work either.
Next time a user forks your project and enabled GitHub Actions and opened a Pull Request, the Workflow will run on the forked repository and will push commits to the same branch.
I forked the repo, enabled github actions (and all permissions), then opened a pull request, and still no workflow ran in the forked repo, only the base repo.
According to this section in the GitHub docs:
No pull request events occur on the forked repository.
This makes me think the "Workflow should run in forked repository" of your readme isn't right somehow...
GitHub is sure making this frustrating. All I want to do is make a commit to a pull request, whether it be to the base or to the fork, as long as it gets added to the PR. It also needs to work regardless of whether the PR is opened from a branch or a fork.
stefanzweifel
commented
1 year ago Thanks @vincerubinetti for this detailed feedback with logs and workflows! Verifying and testing if and how this action still works with PRs is still on my todo list. Other projects nudged it down. Will try to take a closer look this weekend.
These logs help a lot!
vincerubinetti
commented
1 year ago Thanks for looking into this. To be clear, I think it's pretty clearly a GitHub issue, not an issue with this action, but it helps to have someone more competent than myself helping. I'm not sure how @ConorMacBride and you were able to get this working in the middle of last year. Maybe something changed since then...
Fwiw, I've tried so many things. One thing I recently tried was using the checkout action to checkout the fork, do my modifications, copy the changed files to an upper folder, then do a normal checkout (with various permutations), then push with git-auto-commit. Nothing has worked.
If you're a git wizard (I'm definitely not), I might recommend trying to SSH into a running workflow using Tmate. Then you could poke around, switch branches/refs/etc, and see what GitHub allows you to push: - uses: mxschmitt/action-tmate@v3. Much more rapid and realtime. I tried to do this a bit, but I simply don't understand Git and GitHub well enough.
stefanzweifel
commented
1 year ago @vincerubinetti I also only know learned that GitHub updated the default permissions of the GITHUB_TOKEN last Tuesday to ready-only.
https://github.blog/changelog/2023-02-02-github-actions-updating-the-default-github_token-permissions-to-read-only/
If you've tested this in a new repository, you might now need to set permissions for the token or change the settings in the repository. (They've introduced permissions in 2021: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/)
permissions:
contents: write
steps:
- uses: actions/checkout@v3
- # Change repo
- uses: stefanzweifel/git-auto-commit-action@v4Still need to test and document this.
vincerubinetti
commented
1 year ago Unfortunately I couldn't get it to work with that either (setting it in both the caller and called workflows in my case).
I think I had already noticed that the default for new repos started being the second option, where you only get read access for contents. But as I mentioned in a previous comment, I've been making sure to set it to the first option to give write access to all scopes. So I would think any setting of permissions in the workflow shouldn't be needed anyway?
Here's an article that might be useful: https://joht.github.io/johtizen/build/2022/01/20/github-actions-push-into-repository.html https://github.com/EndBug/add-and-commit#working-with-prs
Need to step away from the problem a bit because I'm going crazy, but tag me if you have new info/understanding/debugging tips/whatever.
stefanzweifel
commented
1 year ago I think I almost figured it out @vincerubinetti
As GitHub changed the default permissions of the GITHUB_TOKEN for new repositories, I've created a fresh new repository to test the action.
I've added the following workflow that listens to the push and pull_request_target triggers. It appends the current time to a markdown file.
name: Update File
on:
push:
branches:
- main
pull_request_target:
workflow_dispatch:
jobs:
update-file:
runs-on: ubuntu-latest
permissions:
contents: write
steps:
- uses: actions/checkout@v3
with:
repository: ${{ github.event.pull_request.head.repo.full_name }}
ref: ${{ github.head_ref }}
- run: echo - $(date +%Y-%m-%d-%H-%M-%S) >> file-to-update.md
- uses: stefanzweifel/git-auto-commit-action@v4
with:
commit_message: Commit made through git-auto-commitThe settings in the repository have not been updated. By default the GITHUB_TOKEN has only read permissions; though I change that in the workflow by setting contents: write.
With another GitHub account I've created a fork of the repository. (I've enabled GitHub Actions in the fork, as they are disabled by default after forking)
On the fork, I made a simple change to a markdown file and opened a pull request on the original repository.
GitHub Actions then ran my workflow in the original repository only and successfully updated the markdown file and pushed it to the fork. (https://github.com/stefanzweifel/git-auto-commit-demo-app/pull/4/commits/0ac8ebe77cdcd57a41347b6f958d3ad488f1ac2b)
This is basically the same setup as you have @vincerubinetti. But you still ran into permission issues. :/ Two things came to mind that could block the write access:
I assume when the "Allow edits and access to secrets by maintainers"-checkbox is not checked, that the original repository doesn't have the permission to push something to the fork.
Protected branches can't be circumvented with a default GITHUB_TOKEN. A personal access token is always required. https://github.com/stefanzweifel/git-auto-commit-action#push-to-protected-branches
I forked the repo, enabled github actions (and all permissions), then opened a pull request, and still no workflow ran in the forked repo, only the base repo.
According to this section in the GitHub docs:
No pull request events occur on the forked repository.
This makes me think the "Workflow should run in forked repository" of your readme isn't right somehow...
I think I can confirm that. I was only able to trigger a run in the fork when I accidentally opened a pull request in the same forked repository.
Will update the docs about this soon.
vincerubinetti
commented
1 year ago @stefanzweifel Thank you for all of your hard work looking into this.
I did see your readme instructions about "Allow edits by maintainers", but while testing, I actually never saw that option appear in my GitHub UI! Now that I think about it more, I'm betting that this is the source of the issue.
Check out this issue: https://github.com/orgs/community/discussions/5634
The way I was testing out my workflow was merging in a fork from greenelab into vincerubinetti. The latter is my personal account obviously. The former is an organization... one that I have the highest permissions in (owner).
Apparently, according to that issue, GitHub doesn't allow "edits by maintainers" from org forks!!! 🙃 I tried the same test the other way around (vincerubinetti into greenelab), I had the checkbox available, I left it checked, and lo-and-behold this action was able to commit with no problems!
It's not your responsibility to do so, but maybe next to your "Allow edits by maintainers" you could put a small warning about org forks and link to that discussion.
stefanzweifel
commented
1 year ago @vincerubinetti Didn't know about that limitation of org forks. Thanks for investigating.
It's not your responsibility to do so, but maybe next to your "Allow edits by maintainers" you could put a small warning about org forks and link to that discussion.
Will do!
slorber
commented
9 months ago @stefanzweifel @ConorMacBride Although this is incredibly useful, doing this would introduce security vulnerabilities. Quoting from the link:
TL;DR: Combining
pull_request_targetworkflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.I think it is important to point it out in the
README.mdsection for all the users of this action.
Similarly I'm not sure if usage of pull_request_target is secure here.
Warning: For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission unless the permissions key is specified and the workflow can access secrets, even when it is triggered from a fork. Although the workflow runs in the context of the base of the pull request, you should make sure that you do not check out, build, or run untrusted code from the pull request with this event. Additionally, any caches share the same scope as the base branch. To help prevent cache poisoning, you should not save the cache if there is a possibility that the cache contents were altered. For more information, see "Keeping your GitHub Actions and workflows secure: Preventing pwn requests" on the GitHub Security Lab website.
Here an explicit permission is set, which seems fine? 🤔
If the workflow can access secrets then an attacker just has to update the CI commands in their PR to steal secrets?
stefanzweifel
commented
8 months ago @slorber
If the workflow can access secrets then an attacker just has to update the CI commands in their PR to steal secrets?
In my test repository I added a secret and updated the workflow to echo out that workflow. GitHub is clever enough and will never actually display the secret value in the log files. https://github.com/stefanzweifel/git-auto-commit-action-demo-app/actions/runs/7105521724/job/19342893706#step:6:28
But a bad-actor could theoretically update the workflow to consume the secret by using their own code and potentially steal the secret. So yeah, pull_request_target isn't 100% secure either.
More than happy to hear ideas for improvements for the docs in the README or even pull requests for that.
slorber
commented
8 months ago Thanks for the test.
If it's not logged in their UI, it doesn't mean that users can't steal the secret unfortunately, so I'm not sure it's super safe.
Honestly this pull_request_target is turning my brain upside down. It's hard to reason about and even be 100% sure its usage is safe. Last time I used it on Docusaurus after carefully reading the doc and trying to be secure, Meta security team audited it a few months later and found out it's possible to exploit it, and asked me to remove usage. contacted me a few months later explaining me it's possible.
Author of competing solution "autofix.ci" apparently says it's unsafe, and built a product around that limitation (https://twitter.com/maximilianhils/status/1723715268407427338)
stefanzweifel
commented
8 months ago @slorber
If it's not logged in their UI, it doesn't mean that users can't steal the secret unfortunately, so I'm not sure it's super safe.
I agree. pull_request_target isn't safe, as a bad actor can steal them your secrets, if you have any. And apparently there's no Action-only fix available.
Thanks for the link for autofix.ci.
To make things clearer for others, I will update the README with a warning, stating that pull_request_target can be used to steal secrets and will point users to autofix.ci.
I have plans to publish a repository with example workflows in the coming months. The pull_request_target-example will also have a big warning pointing out the security issues.
slorber
commented
8 months ago Thanks
I spent the day trying to create an autolint system to fix PRs of external contributors to my open-source repo, and tried various techniques without any good result.
https://github.com/slorber/lint-autofix-ci-demo https://github.com/slorber/lint-autofix-ci-demo/pull/1
I tried splitting the workflow 2 as suggested:
It felt quite inconvenient to pass data (git diff) between the 2 workflows using artifacts. Apparently you could also use job outputs, but didn't try yet: https://twitter.com/yarlob/status/1735350688052342809
I also tried to implement a workflow where the linting would happen when you comment the PR, but couldn't push to the forked branch due to permission issues: https://github.com/slorber/lint-autofix-ci-demo/blob/7f9745c1782c247cef3a313b366f52b158ff84ed/.github/workflows/lint-autofix-comment.yml
Even with the "pull_request_target" unsafe workflow, I couldn't get it to push to my own remote branch with permissions issues: https://github.com/slorber/lint-autofix-ci-demo/blob/7f9745c1782c247cef3a313b366f52b158ff84ed/.github/workflows/lint-autofix-unsecure-test.yml https://github.com/slorber/lint-autofix-ci-demo/actions/runs/7212924277/job/19651657295?pr=1
Who has been able to push to a fork branch here, even unsafely with pull_request_target?
Does it require a PAT?
Or can the default GitHub token + bot account be enough to push to contributors branches?
EDIT: I confirm a PAT seems to be required to push to fork branches
In the readme section "Advanced Uses" > "Use in forks from public repositories" it says that "GitHub Action has to be enabled on the forked repository." and "the Workflow will run on the forked repository", however this doesn't seem to be the case. The docs for
pull_request_targetand this GitHub blog post suggests that the main differencepull_request_targethas compared topull_requestis that it runs from the base of the PR instead of the PR merge commit, i.e., any changes to the workflow file in the PR will not take effect until merged.An example is, this workflow file is triggered by
pull_request_target, and when you open a PR from a fork (with GitHub Actions disabled) it runs within the repository where the PR is (not the fork).And this GitHub Action is able to commit to the fork while running in the non-fork repo. It's able to commit with the default
GITHUB_TOKENand you can checkout the PR branch at the fork repository with:So it seems like the limitations on running in PRs from forks no longer apply! 🎉