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

Why is GITHUB_TOKEN necessary? #218

Open
ajschmidt8 opened this issue Nov 22, 2022 · 9 comments
Open

Why is GITHUB_TOKEN necessary? #218

ajschmidt8 opened this issue Nov 22, 2022 · 9 comments

Comments

@ajschmidt8
Copy link

I'm wondering why a GITHUB_TOKEN is necessary for this action to work correctly.

Both pull_request and pull_request_target events deliver a payload that contains the current pull request title (src). The full payload is available via the github context under the event property (src).

@ajschmidt8
Copy link
Author

ajschmidt8 commented Nov 22, 2022

I see the code and comment below, which indicates that The pull request info on the context isn't up to date.

// The pull request info on the context isn't up to date. When
// the user updates the title and re-runs the workflow, it would
// be outdated. Therefore fetch the pull request via the REST API
// to ensure we use the current title.
const {data: pullRequest} = await client.rest.pulls.get({
owner,
repo,
pull_number: contextPullRequest.number
});

This leads me to believe that the authenticated client.rest.pulls.get call was implemented to account for potential race conditions when multiple events are fired in quick succession. Is that correct? If so, it seems like a more appropriate solution could be to use GitHub Actions' native concurrency settings below to ensure that only the latest event is ever used.

https://docs.github.com/en/actions/using-jobs/using-concurrency

@ajschmidt8
Copy link
Author

I traced that comment back to the PR below. It doesn't seem that there's any additional context in the PR description.

@amannn
Copy link
Owner

amannn commented Nov 23, 2022

Hey @ajschmidt8 and thank you for your comment!

The reason why I added the necessity for the token is described in the comment from the code snippet you've linked to above. If you know how to avoid this while still being able to support the same feature set, I'd be more than happy to review a PR!

@lucasgonze
Copy link

If so, it seems like a more appropriate solution could be to use GitHub Actions' native concurrency settings below to ensure that only the latest event is ever used.

@ajschmidt8 Can you say more about why this is a race condition?

Let's discuss what the concurrency fix is.

@ajschmidt8
Copy link
Author

ajschmidt8 commented Apr 25, 2023

@ajschmidt8 Can you say more about why this is a race condition?

Let's discuss what the concurrency fix is.

If a PR title is updated multiple times in quick succession, then it could trigger the workflow to run multiple times in parallel.

This could cause problems depending on the details of the title changes on each trigger.

To prevent this workflow from running multiple times in parallel, you could set up concurrency limits like this:

name: "Lint PR"

on:
  pull_request_target:
    types:
      - opened
      - edited
      - synchronize

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

jobs:
  main:
    name: Validate PR title
    runs-on: ubuntu-latest
    steps:
      - uses: amannn/action-semantic-pull-request@v5

This would ensure that any pending workflow runs are canceled before a new one begins.

I don't think that's related to the GITHUB_TOKEN issue here though.

Upon further inspection of the source code, it's the following code block that requires users to use the pull_request_target event (which has write permissions):

await client.request('POST /repos/:owner/:repo/statuses/:sha', {
owner,
repo,
sha: pullRequest.head.sha,
state: newStatus,
target_url: 'https://github.com/amannn/action-semantic-pull-request',
description: isWip
? 'This PR is marked with "[WIP]".'
: validationError
? 'PR title validation failed'
: 'Ready for review & merge.',
context: 'action-semantic-pull-request'
});
}

This command writes a custom status back to the PR depending on the results of the function.

The default GITHUB_TOKEN provided to pull requests doesn't have write permissions, only read permissions (see docs here).

Composite actions aren't typically supposed to write additional statuses to PRs. They're simply supposed to run some code and exit with a non-zero code if there are problems.

Writing additional statuses is what requires a GITHUB_TOKEN with write permissions and adds weird complexity to this action.

@lucasgonze
Copy link

If a PR title is updated multiple times in quick succession, then it could trigger the workflow to run multiple times in parallel.

This could cause problems depending on the details of the title changes on each trigger.

I think that's true. Also, the workaround of calling the Github API wouldn't fix it systematically, just reduce the frequency by adding a little latency. Also, I think you're right that this isn't why the pull_request_target trigger is necessary.

I'm not sure that pull_request_target changes the privilege level of the token, though.

The architecture documented in the securitylab.github.com blog might be the fix:

Together with the pull_request_target, a new trigger workflow_run was introduced to enable scenarios that require building the untrusted code and also need write permissions to update the PR with e.g. code coverage results or other test results. To do this in a secure manner, the untrusted code must be handled via the pull_request trigger so that it is isolated in an unprivileged environment. The workflow processing the PR should then store any results like code coverage or failed/passed tests in artifacts and exit. The following workflow then starts on workflow_run where it is granted write permission to the target repository and access to repository secrets, so that it can download the artifacts and make any necessary modifications to the repository or interact with third party services that require repository secrets (e.g. API tokens).

@ajschmidt8
Copy link
Author

Agreed, the API hits to read pull request data shouldn't be necessary. Those details are included in the payload provided by GitHub.

pull_request_target does change the permission though.

Check the details here: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

Specifically the red warning (emphasis mine):

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...

@actuarysailor
Copy link

actuarysailor commented May 20, 2023

I have the secrets.GITHUB_TOKEN defined, but still getting error.
image
image

Same issue if I updated to the most recent version too.

image

@actuarysailor
Copy link

I have the secrets.GITHUB_TOKEN defined, but still getting error. image image

Same issue if I updated to the most recent version too.

image

Note the error seems to be because I am using this as a shared workflow; e.g. workflow_call. If I run it directly within my repo, it works fine

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

No branches or pull requests

4 participants