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

ci: flash_analysis: work with PRs from forks #24395

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dakejahl
Copy link
Contributor

Fixes an issue with the flash_analysis workflow which would fail on the post_pr_comment step on PRs from forks. Currently all PRs from forks fail CI due to this workflow.

See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

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.

It seems like a security issue though.. Can someone who has more experience with github workflows help me out with this?

@haumarco
Copy link
Contributor

isnt the issue with this change that it would run the workflow in the context of the base branch, not the PR branch → This means CI won’t actually check the new code in the PR. I would suggest the simple fix by adding contents: read here. What do you think?

@alexcekay
Copy link
Member

alexcekay commented Feb 24, 2025

Hi @dakejahl, @haumarco,
thanks for looking into this. I had this topic on my plate, but got distracted by other stuff.
The whole problem is caused by security settings of Github and is described in the README of the comment action we use (https://github.com/peter-evans/create-or-update-comment):

Note: In public repositories this action does not work in pull_request workflows when triggered by forks. Any attempt will be met with the error, Resource not accessible by integration. This is due to token restrictions put in place by GitHub Actions. Private repositories can be configured to enable workflows from forks to run without restriction. See here for further explanation. Alternatively, use the pull_request_target event to comment on pull requests.

So based on that description it seems like pull_request_target is a possible solution. @haumarco already described the problem with this. The normal pull_request target performs a merge commit and so compares:

  • The base
  • The changes of the PR merged into the base

As far as I know pull_request_target does not create a merge commit, so we would measure different things in the case of PRs from forks and PRs from origin. We should also have a look at: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ to prevent security issues due to granting forks access to resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants