Update README regarding pull_request_target

[ylemkimon]

GitHub has introduced a new event type: pull_request_target, which allows to run workflows from base branch and pass a token with write permission.

In order to solve this, we’ve added a new pull_request_target event, which behaves in an almost identical way to the pull_request event with the same set of filters and payload. However, instead of running against the workflow and code from the merge commit, the event runs against the workflow and code from the base of the pull request. This means the workflow is running from a trusted source and is given access to a read/write token as well as secrets enabling the maintainer to safely comment on or label a pull request. This event can be used in combination with the private repository settings as well.

It could be potentially dangerous to checkout and run code from public forks, but people will still try it. I think the best place for the warning is the checkout action, itself.

Rendered

[ylemkimon]

@matfax The pull_request_target runs in the context of the base repository of the pull request, i.e., its GITHUB_TOKEN has read/write access even for pull requests from forked repos.

[matfax]

I've tested various things now. Even though GITHUB_TOKEN doesn't seem to be available all the time, the github.token from the context has indeed write permissions as you stated. actions/checkout sets this context token as repository token, so it's available anyways once checkout is used. I find it rather odd that GitHub introduced pull_request_target as some kind of safe alternative but how are you supposed to check anything in a pull request if you can't even check out its code? Imho, they should change the permissions of the token if used in pull_request_target to read only the repository + write access to the pull request (except merge). The use case of checking forked pull requests is integrated and documented insufficiently.

[MinusFour]

Is this a general warning or is this something specific to the checkout action? Github documentation isn't very clear about the token scope. It was my understanding that you'd need to pass down the token to your job. Is the real danger that this action persists the token during the whole job for git? Wouldn't this be safer if you just opt out of that behavior or are there other potential avenues to obtain the token?

I guess any other action can just as well leak the token through the filesystem.

[matfax]

@MinusFour It depends. Actions (such as actions/checkout) have implicit access to the token. That means you have to build trust in their code and what they do with the token. Since the default token expires after workflow execution, long term implications beyond the timespan of the execution are very unlikely. Executing code doesn't seem to have implicit access through environment variables,. This is what I tested and read from others in the forums as well. But don't take my word for it; maybe there's a way to access the context without an action.yml. For example, what happens if a test action defines any token reference in its action.yml just for uploading the results, but the executed tests could gain access to the same context via the test runner? Fortunately, this can be checked by a look into the action.yml files.

▶️ I suggest not to use any test runner action that defines a default token. Use separate steps for evaluation etc.
▶️ If you use automatic action updates (e.g., via dependabot), always check for changes of the action.yml.

Next, there is the issue with the checkout token. Like many actions, checkout needs a token to evade API traffic restrictions. By default, this token will be stored during the whole workflow and deleted during its cleanup. Since git credentials are stored in the file system, that means a test could edit and push to the origin if this token has write permissions. I imagine it's even possible to force-push an empty git and lose the whole repository.

▶️ Enable branch protection and prevent force-pushes to the main or master branch.
▶️ Set persist-credentials of actions/checkout to false if you don't need git access later on. This should be the default, imho.
▶️ Create your own read-only PAT (personal access token) and pass this one to the action to override the action's default token reference. You won't be able to auto fix code and push it, though. Do this in a separate job if desired.
▶️ Alternatively, use pull_request instead of pull_request_target if you don't need any write access because pull_request still has a read-only access default token. But no access to any other secrets. Such as a coverage upload secret if you require one. This is the limitation here.

What's also integrated a bit prematurely, as it seems to me, is the token trigger behavior. The default GitHub token does and can not trigger any event. This was GitHub's decision to not see any loops where actions trigger themselves on and on. If a pull_request action does an auto fix and pushes it to the pull request with the default token, action checks won't be executed, and so it might seem as if all check pass after the fix. Thus, certain actions would require a git PAT to circumvent this. These PATs have greater access to the repositories, however. They don't expire after the workflow execution and could be stored somewhere for later access. And this isn't even the only use case where the default token doesn't work. Then, there are also actions that depend on it that the token doesn't trigger a repetitive execution. This security feature isn't supposed to be projected on the choice of default vs. PAT token. It should be a cross-cutting concern that can be enabled token-specifically just like any other scope.

💡 I think it would be best if they made default tokens read-only and offer an integrated way to generate step-wise tokens with individually-scoped access if any further access is required. A more granular scoping on repository-level wouldn't be bad either, e.g. to delimit a token scope to push on master branch of this repository. This is just my personal opinion. I'm not affiliated with GitHub.

Until this happens, I'm currently working on an interactive event dispatcher action that requires maintainer approval for a check to run. This gives the maintainer the option to evaluate the pull request security-wise before any token is passed to it.

[MinusFour]

This is for a private repository (personally, I think the pull_target_request as it is makes more sense for them right now).

Executing code doesn't seem to have implicit access through environment variables,. This is what I tested and read from others in the forums as well. But don't take my word for it; maybe there's a way to access the context without an action.yml. For example, what happens if a test action defines any token reference in its action.yml just for uploading the results, but the executed tests could gain access to the same context via the test runner?

This is what worries me the most. As it is right now, a simple flow that checkout code, install dependencies and run tests could very easily reach secrets and whatnot. For example, the test suite could just run git to use the token.

Set persist-credentials of actions/checkout to false if you don't need git access later on. This should be the default, imho.

I also believe this should be the default. It's essentially upgrading permissions for every step in the workflow that might not need them.

Alternatively, use pull_request instead of pull_request_target if you don't need any write access because pull_request still has a read-only access default token. But no access to any other secrets. Such as a coverage upload secret if you require one. This is the limitation here.

Unfortunately for repos that use self hosted runners and need to secure some of the runners, I think this would be a bad idea. Anyone could just change the runs_on to use these runners. This is why I'm moving to pull_request_target. It would be nice if they would offer more granular permissions for it for sure. At the very least be able to turn off secrets/tokens.

In general I think it's a fair warning for both types of repositories, private or public.

[valentijnscholten]

I think the pull_request_target thing is a strange construct. If you use the checkout without ref, you get the code from the base branch. So you don't have access to the code from the pull request.
If you use ref: refs/pull/${{ github.event.pull_request.number }}/merge you do get the code from the pull request, but it seems the base commit is no longer accessible to create a diff.

bash:

    BASE_COMMIT=$(
        jq \
            --raw-output \
            .pull_request.base.sha \
            "$GITHUB_EVENT_PATH"
    )

    new_files_in_branch=$(
        git diff \
            --name-only \
            --diff-filter=AM \
            "$BASE_COMMIT"
    )

output: fatal: bad object 747d125eea9ee7a722297bd4fe7f91ef3305d2b0

This is a PR across forks which is a scenario the pull_request_target was specifically designed for to my understanding. If use just pull_request, everything works including the diff. But I have no access to the checks API to report the conclusion of my tests as the github token is read only.

EDIT: Note sure if this was the best place to comment, but there could be another checkout option that could help checking out "the right thing" for PRs over forked repo's: https://github.community/t/github-actions-are-severely-limited-on-prs/18179/

[ylemkimon]

@valentijnscholten Not being able to reach the base commit seems to be caused by fetch-depth: 1. You can set fetch-depth: 2 or fetch-depth: 0 (unlimited) to fetch both the base and head commit.

[valentijnscholten]

I have tried all different kinds of combinations, including fetch depth 0.
What seems to work is

        with:
          fetch-depth: 0
          ref: ${{github.event.pull_request.head.ref}}
          repository: ${{github.event.pull_request.head.repo.full_name}}

But this checks out the original branch of the PR author. So this doesn't include the code that was added to the base branch later. When you try to checkout the merge commit to get the actual merge of the HEAD of the base branch and the HEAD of the PR, the result is that just the code from the base branch is checkout and none of the PR code is checked out.

        with:
          fetch-depth: 0
          ref: ${{github.event.pull_request.merge_commit}}

So same results as

        with:
          fetch-depth: 0

[ylemkimon]

@valentijnscholten github.event.pull_request.merge_commit is not available for pull_request_target event. You should use refs/pull/${{ github.event.pull_request.number }}/merge.

[valentijnscholten]

Thanks, will try. Just also found out that the below still gives errors because it can't find the base commit:

        with:
          fetch-depth: 0
          ref: ${{github.event.pull_request.head.ref}}
          repository: ${{github.event.pull_request.head.repo.full_name}}