Lintly icon indicating copy to clipboard operation
Lintly copied to clipboard

Better support / documentation of `pull_request_target` github action events

Open David-Baddeley opened this issue 3 years ago • 3 comments

This might be something to address in the ci module instead, but it took a bit of work getting this to run on PRs from a fork (which is pretty common for an open source workflow). Raising this here in case it saves someone time.

Steps I needed to take:

  1. use the pull_request_target trigger rather than the pull_request trigger (so that GITHUB_TOKEN has write access to the PR comments)
  2. specify the --pr option explicitly as --pr=${{ github.event.pull_request.number }}

The second would seem to be because the ci module doesn't correctly infer the PR number when you use pull_request_target.

I also needed to some gymnastics with repository checkout, although this is largely documented elsewhere. Full workflow below for completeness:

name: Lint

on: [pull_request_target]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
      with: 
        # need to specify reference here - otherwise you just get the target master
        ref: refs/pull/${{ github.event.pull_request.number }}/head
    - name: Set up Python
      uses: actions/setup-python@v1
      with:
        python-version: 3.6
    - name: Install dependencies
      run: pip install flake8 lintly
    
    - name: Lint with flake8
      run: |
        # checking out as above gives you a merge commit with the master as HEAD 
        # need to fetch origin/master under an alias to be able to take the diff
        git fetch origin master:om
        git diff ${{ github.event.pull_request.head.sha }} om | flake8 --diff | lintly --pr=${{ github.event.pull_request.number }} --commit-sha=${{ github.event.pull_request.head.sha }}
      env:
        LINTLY_API_KEY: ${{ secrets.GITHUB_TOKEN }}

David-Baddeley avatar Oct 13 '20 03:10 David-Baddeley

Thanks for putting so much work into this @David-Baddeley! I'm pretty sure I've used Lintly with forked repos in the past, so I'm surprised that isn't working. 🤔

ci.py grabs the PR number via the GITHUB_REF env variable. That env var must not be available for pull_request_target triggers. As far as I know, GitHub Actions doesn't have a direct env var with the PR number (or at least they didn't when I made ci.py).

It sounds like perhaps the issue is that the GitHub Actions token has limited rights if a PR is created across forks. I believe that's because they don't want folks changing your GitHub actions YAML in a PR and using the token to do something malicious.

Does that sound like the issue you're running across?

grantmcconnaughey avatar Oct 17 '20 18:10 grantmcconnaughey

I also ran into this issue and I adjusted @David-Baddeley's workaround to work for arbitrary base branches: https://github.com/pymor/pymor/pull/1237/commits/5a8abe4ebef2ea169fe5eda5b70d563bc065408f

renefritze avatar Jan 19 '21 10:01 renefritze

After very kindly being informed by a community member that relying solely on the pull_request_target workflow opens one up for arbitrary code injection attacks, I switched to a two step implementation as suggested in https://securitylab.github.com/research/github-actions-preventing-pwn-requests

The relevant files are https://github.com/pymor/pymor/blob/main/.github/workflows/linter.yml and https://github.com/pymor/pymor/blob/main/.github/workflows/process_linter_results.yml

renefritze avatar Feb 03 '21 13:02 renefritze