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

meta: add external PRs to project board #102

Merged
merged 2 commits into from
May 20, 2022
Merged

Conversation

thomasheartman
Copy link
Collaborator

The pull_request hook runs in the context of the proposed changes. That means that for forks, this action won't have access to the required secrets for it to complete. As such, PRs from outside contributors won't work correctly.

The pull_request_target hook, however, runs in the context of the target branch, and thus has all the permissions it needs. The github docs for this hook also indicate that this is the way to go:

This event allows your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request.

Interesting note for PRs

It seems that when this change is proposed as a PR, the expected action (add new item to project board) does not run. However, this does not affect other new PRs. After merging, the pipeline works and triggers as expected, for both external and internal contributors.

Isn't this a potentially dangerous?

Good question! As far as I understand: no, it's not. The long answer is in Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests, but I'll try and summarize it here.

Some choice cuts from the article:

TL;DR: Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.

However, we do not check the PR out!

Due to the dangers inherent to automatic processing of PRs, GitHub’s standard pull_request workflow trigger by default prevents write permissions and secrets access to the target repository. However, in some scenarios such access is needed to properly process the PR. To this end the pull_request_target workflow trigger was introduced.

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 is: when using pull_request_target, the action will not check out the PR branch and thus, the PR cannot inject any code changes into the action. On the other hand pull_request uses the code that's in the PR to run actions, which is why it requires explicit authorization before being run.

pull_request_target runs in the context of the target branch (most commonly main) and only runs code that already exists in the target branch. No code from the PR gets used.

The `pull_request` hook runs in the context of the proposed changes. That means that for forks, this action won't have access to the required secrets for it to complete. As such, PRs from outside contributors won't work correctly.

The `pull_request_target` hook, however, runs in the context of the target branch, and thus has all the permissions it needs. The [github docs for this hook](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target) also indicate that this is the way to go:

> This event allows your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request.

## Interesting note for PRs

It seems that when this change is proposed as a PR, the expected action (_add new item to project board_) does not run. However, this does not affect other new PRs. After merging, the pipeline works and triggers as expected, for both external and internal contributors.

## Isn't this a potentially dangerous? 

Good question! As far as I understand: no, it's not. The long answer is in [Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests](https://securitylab.github.com/research/github-actions-preventing-pwn-requests/), but I'll try and summarize it here.

Some choice cuts from the article: 

> TL;DR: Combining pull_request_target workflow trigger with an **explicit checkout of an untrusted PR** is a dangerous practice that may lead to repository compromise.

However, we do not check the PR out! 

> Due to the dangers inherent to automatic processing of PRs, GitHub’s standard pull_request workflow trigger by default prevents write permissions and secrets access to the target repository. However, in some scenarios such access is needed to properly process the PR. To this end the pull_request_target workflow trigger was introduced.

> 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 is: when using `pull_request_target`, the action will not check out the PR branch and thus, the PR cannot inject any code changes into the action. On the other hand `pull_request` uses the code that's in the PR to run actions, which is why it requires explicit authorization before being run. 

`pull_request_target` runs in the context of the target branch (most commonly `main`) and only runs code that already exists in the target branch. No code from the PR gets used.
Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@coveralls
Copy link

coveralls commented May 20, 2022

Pull Request Test Coverage Report for Build 2360666911

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+92.8%) to 92.764%

Totals Coverage Status
Change from base Build 2358712117: 92.8%
Covered Lines: 1859
Relevant Lines: 2004

💛 - Coveralls

@rarruda rarruda merged commit 1f9f77f into main May 20, 2022
@rarruda rarruda deleted the thomasheartman-patch-1 branch May 20, 2022 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants