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

Description seems dangerous #755

Open
simon-friedberger opened this issue Feb 9, 2024 · 5 comments
Open

Description seems dangerous #755

simon-friedberger opened this issue Feb 9, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@simon-friedberger
Copy link

From the docs, emphasis mine:

In order to add labels to pull requests, the GitHub labeler action requires write permissions on the pull-request. However, when the action runs on a pull request from a forked repository, GitHub only grants read access tokens for pull_request events, at most. If you encounter an Error: HttpError: Resource not accessible by integration, it's likely due to these permission constraints. To resolve this issue, you can modify the on: section of your workflow to use pull_request_target instead of pull_request (see example above). This change allows the action to have write access, because pull_request_target alters the context of the action and safely grants additional permissions. Refer to the GitHub token permissions documentation for more details about access levels and event contexts.

Afaict this contradicts, e.g.: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ .
Using pull_request_target is not safe at all and setting additional permissions is necessary.

@simon-friedberger simon-friedberger added bug Something isn't working needs triage labels Feb 9, 2024
@HarithaVattikuti
Copy link
Contributor

Hello @simon-friedberger
Thank you for creating this issue. We will investigate it and get back to you as soon as we have some feedback.

@WolfspiritM
Copy link

@simon-friedberger Just found this issue. According to the linked securitylab.github.com page:

  1. Workflows triggered via pull_request_target have write permission to the target repository. They also have access to target repository secrets. The same is true for workflows triggered on pull_request from a branch in the same repository, but not from external forks. The reasoning behind the latter is that it is safe to share the repository secrets if the user creating the PR has write permission to the target repository already.
  2. pull_request_target runs in the context of the target repository of the PR, rather than in the merge commit. This means the standard checkout action uses the target repository to prevent accidental usage of the user supplied code.

These safeguards enable granting the pull_request_target additional permissions. The reason to introduce the pull_request_target trigger was to enable workflows to label PRs (e.g. needs review) or to comment on the PR. The intent is to use the trigger for PRs that do not require dangerous processing, say building or running the content of the PR.

That means (as far as I understand) as long as you don't check out and use the PR source code within the same workflow it is secure to use pull_request_target to label PRs (it's even made exactly for that). It only gets insecure as soon as you check out the PR code and execute it. Even the defaults of the checkout action will not checkout the PR in this case but the target repo.

As pull_request_target switches the context to your repository there is no insecure code executed per default

@simon-friedberger
Copy link
Author

I mostly agree. However, the difference between checking out the base source and the PR source can be very non-obvious

      - uses: actions/checkout@v4
        with:
          ref: ${{ github.event.pull_request.base.ref }}
          path: local
      - uses: actions/checkout@v4
        with:
          ref: ${{ github.event.pull_request.head.sha }}
          path: pr

and I expect many people will intentionally check it out. After all, they are trying to label the pull request based on something that is inside the pull request.

In other words, I agree with you on the technical points I just think the "as long as you don't" part will not be sufficiently clear to people.

@simon-friedberger
Copy link
Author

To make a proposal:
Stop calling it safe and instead state something like:

This will grant your action write access to your repository so you must make sure not to execute untrusted code and you should use minimal privileges by setting permissions like in the example above). For more information see Keeping your GitHub Actions and workflows secure: Preventing pwn requests].

@ST-DDT
Copy link

ST-DDT commented Jun 20, 2024

⚠️ There is actually a code example in the README using/encouraging the bad usage: ⚠️

  • label-the-PR
  • run-frontend-tests
  • run-backend-tests

labeler/README.md

Lines 231 to 257 in 506e1a0

```yml
name: "Pull Request Labeler"
on:
- pull_request_target
jobs:
labeler:
permissions:
contents: read
pull-requests: write
runs-on: ubuntu-latest
steps:
- id: label-the-PR
uses: actions/labeler@v5
- id: run-frontend-tests
if: contains(steps.label-the-PR.outputs.all-labels, 'frontend')
run: |
echo "Running frontend tests..."
# Put your commands for running frontend tests here
- id: run-backend-tests
if: contains(steps.label-the-PR.outputs.all-labels, 'backend')
run: |
echo "Running backend tests..."
# Put your commands for running backend tests here
```

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

No branches or pull requests

4 participants