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

FIX Use pull_request_target so we have access to secrets #65

Merged

Conversation

GuySartorelli
Copy link
Member

As per https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflows-in-forked-repositories

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

This explains why I saw it working in my own account (PR from my account to my account) but it failed in the org (where the PR came from a fork).

Swapping to pull_request_target does elevate the default permissions for the built-in token as per https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

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.

We do explicitly specify the permissions key and intentionally keep it empty, so we should be able to access secrets while having a read-only github token.

Other things to note:

you should make sure that you do not check out, build, or run untrusted code from the pull request with this event

We're not doing any of those

This event runs in the context of the base of the pull request, rather than in the context of the merge commit, as the pull_request event does. This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow.

So this should be robust against changes to the action within the PR that triggers it, unlike when using the pull_request event. It's actually keeping our secret safer than if the pull_request event allowed access to secrets.

Note

Two commits here - one to do what I mentioned above and another to update the name of the action and job per comments on the issue.

Issue

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Sorry, my original feedback didn't capitilise

@@ -1,10 +1,10 @@
<?php

$content = <<<'EOT'
name: Add new pull requests to a github project
name: Add new prs to github project
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
name: Add new prs to github project
name: Add new PRs to github project

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@GuySartorelli GuySartorelli force-pushed the pulls/main/use-pr-target branch from 1c10a37 to 8550aa1 Compare June 18, 2024 23:00
@emteknetnz emteknetnz merged commit 3580256 into silverstripe:main Jun 19, 2024
1 check passed
@emteknetnz emteknetnz deleted the pulls/main/use-pr-target branch June 19, 2024 05:14
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.

2 participants