Security concern with using `pull_request_target`

[HonkingGoose]

I see this action uses pull_request_target to work on public forks. Have you seen this warning in the GitHub Docs, 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 might be good to mention this warning in the README.md as well?

[amannn]

As far as I understand the situation this shouldn't be a concern for action-semantic-pull-request. When using pull_request_target we're executing the latest configuration that is available in the main branch. E.g. if a fork changes the configuration to output the GITHUB_TOKEN, this change won't have any effect until the pull request is merged into master. I.e. you're not in a situation where the token can be exposed by someone opening a pull request.

[HonkingGoose]

Response to maintainer

As far as I understand the situation this shouldn't be a concern for action-semantic-pull-request. When using pull_request_target we're executing the latest configuration that is available in the main branch. E.g. if a fork changes the configuration to output the GITHUB_TOKEN, this change won't have any effect until the pull request is merged into master. I.e. you're not in a situation where the token can be exposed by someone opening a pull request.

Okay, so the code should be safe when people use it as intended, if I'm understanding you correctly.

What about dangerous patterns?

From the GitHub Docs:

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.

I think this means that you should never use actions/checkout in the same file as the action-semantic-pull-request.

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.

And I think you shouldn't use actions/setup-MANAGER-NAME with a cache step in the actions-semantic-pull-request file as well.

Possible warnings for the docs

If I'm correct about the above, maybe it's a good idea to warn the users, like this:

Important warning

Warning: Never checkout the fork's repository or use caching in the same .yml file where you run amannn/action-semantic-pull-request. Read the GitHub Docs about pull_request_target to learn why this is a bad idea.

Avoid doing this:

name: "Lint PR"

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

jobs:
  main:
    steps:
      - name: Checkout repository <--- Never do this
        uses: actions/checkout@v3.0.2

      - name: Set up Node.js 16 <--- Don't do this either
        uses: actions/setup-node@v3.1.1
        with:
          node-version: 16
          cache: npm

      - name: Install dependencies
        run: npm ci

      - name: Jest tests
        run: npm run jest

      - name: Validate PR title
        runs-on: ubuntu-latest
        steps:
          - uses: amannn/action-semantic-pull-request@v4
            env:
              GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

[amannn]

I think the checkout action checks out the base branch in this case, so we're not executing potentially malicious code from an opened pull request. If you feel like validating this it'd be great to hear your feedback!

[HonkingGoose]

Hmm, I don't know enough code to validate the behavior of actions/checkout. I'll let somebody else do it for us. 😄

[JamesHenry]

FYI I decided to lock down the permissions to just reading PRs for this reason, and everything seems to be working well:

angular-eslint/angular-eslint@f27ce8c

[HonkingGoose]

@amannn Do you want me to create a PR to put @JamesHenry's config into an example in the README.md?

[amannn]

I'm struggling to understand why that would be necessary. By passing the GITHUB_TOKEN to the action, you grant it read/write access to the repository. You can of course limit the permissions to only read, but that's a measure you can take if you don't trust this action. You can of course also look through the source code to validate that this action doesn't do anything harmful to your repository.

Am I missing something?

[HonkingGoose]

By passing the GITHUB_TOKEN to the action, you grant it read/write access to the repository.

Correct.

You can of course limit the permissions to only read, but that's a measure you can take if you don't trust this action.

I think its a good idea in general, regardless of trust, to limit the scope of each action. This helps prevent some bad stuff in case the repository goes rogue for whatever reason.

It turns out that this action works with just read access. I think we should highlight that in the README.md. This way people will be a little bit safer, compared to running the action with the full permissions.

You can of course also look through the source code to validate that this action doesn't do anything harmful to your repository.

You're right, people should check the action code before running it. 😉

Am I missing something?

Limiting the scope to read only is a measure that makes end-users a little more secure. On it's own it might not do much, but combined with other "defense in depth" measures it might prevent bad stuff.

People should still:

• Review the action code before running it on their repositories
• Limit the scope of the action permissions
• Pin the action to a Git SHA
• Use a bot to update the action
• Check the release notes, and the code diff for each release

In summary: I'm trying to explain that even small changes add up when combined with other best practices. You as author have the final say over what goes in your README.md, and you can run the project as you want. 😉

[amannn]

You're definitely raising some good points and I agree that there are some actions a consumer should take before using 3rd party code. For me putting these things in the README conflicts a bit with the ease-of-use of the action – I wouldn't want to tell a consumer to carefully review the code of the action either.

Btw. it could be that the wip feature of the action needs write access since we change the status of the PR in this case:

action-semantic-pull-request/src/index.js

Lines 145 to 161 in 348e2e6

	// When setting the status to "pending", the checks don't
	// complete. This can be used for WIP PRs in repositories
	// which don't support draft pull requests.
	// https://developer.github.com/v3/repos/statuses/#create-a-status
	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'
	});

So I think for the time being I wouldn't change the README in regards to this.