wearerequired / lint-action

✨ GitHub Action for detecting and auto-fixing lint errors
MIT License
575 stars 139 forks source link

⚠️ Not working with forks #13

Open samuelmeuli opened 4 years ago

samuelmeuli commented 4 years ago

Creating annotations and auto-fixes works as expected when the code is on a branch in the same repository.

Unfortunately, it currently doesn't seem work with pull requests from forks: The action has no permission to push auto-fix changes or create annotations.

samuelmeuli commented 4 years ago

It seems like this is impossible with GitHub's current token scopes.

The action needs permissions for two operations:

If the action is triggered on changes to the main repository, it will work because the GITHUB_TOKEN has permissions to modify that repository. On forks, however, this becomes tricky to impossible:

push event pull_request event
commit to main repo [Expected behavior] Annotations and auto-fixing work [Expected behavior] Annotations and auto-fixing work
commit to fork [Expected behavior] User needs to manually activate GitHub Actions on the fork. Even then, annotations will not display on PRs from that fork. [Unexpected behavior] No permission to push auto-fix changes to the fork. No permission to create annotations.

Note that the pull_request event always runs on the main repository, even for pushes to the fork. The token scopes are defined accordingly.

If anybody is aware of a workaround, I'd be very happy to hear it :)

Related: https://github.com/actions/checkout/issues/124#issuecomment-580018424

samuelmeuli commented 4 years ago

All I can do for now is document these limitations in the README and provide more helpful error messages.

If you'd also like to see these permissions changed, please submit feedback to GitHub.

MarvinT commented 4 years ago

Would it be possible to catch the 403 error when the PR is coming from a fork so it doesn't mark the action as having failed?

ocean90 commented 4 years ago

GitHub introduced a new pull_request_target event which might help here, see https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/#improvements-for-public-repository-forks

1212gmartinez commented 4 years ago

@samuelmeuli Thanks for making this easy to use library!

Have you had any chance to consider supporting github's pull_request_target. It would be great to have the fork support available.

ocean90 commented 4 years ago

@1212gmartinez There's a WIP feature branch for this which you can try: https://github.com/wearerequired/lint-action/tree/feature/pull_request_target

You should be able to use it with wearerequired/lint-action@feature/pull_request_target.

1212gmartinez commented 4 years ago

Awesome, thanks!

moodysalem commented 4 years ago

Seems not to be working for us

fatal: Cannot force update the current branch.
Error: Command failed: git branch --force master --track fork/master
fatal: Cannot force update the current branch.

https://github.com/Uniswap/uniswap-interface/pull/1121/checks?check_run_id=1164841104

moodysalem commented 4 years ago

possibly related? https://stackoverflow.com/questions/29559053/force-update-the-current-branch-how

issue is with git branch --force master --track fork/master

is it because the fork branches are named master?

in another case the linter passes but the check status is never reported

https://github.com/Uniswap/uniswap-interface/pull/1060 image

yuekui commented 4 years ago

Instead of using pull_request_target, we could check repo's owner to skip lint action:

    - name: Run Lint
      uses: wearerequired/lint-action@v1
      if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.owner.login == github.repository_owner }}
      with:
        github_token: ${{ secrets.github_token }}
        black: true
        flake8: true
        auto_fix: true

So that lint action would be run only on push or pull_request events within the same repository.

jnm2 commented 3 years ago

If you write workflow commands to stdout instead of making web API calls which depend on the token, the authentication issue goes away and the performance may even be better.

Example from docs page (https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-a-warning-message):

echo "::warning file=app.js,line=1,col=5::Missing semicolon"

Both warning and error annotations can be created this way. A GitHub action using this approach: https://github.com/cschleiden/jest-github-actions-reporter

alexbbt commented 3 years ago

pull_request_target worked very well for me. See my PR here for an example: https://github.com/join-monster/join-monster/pull/449. Read this before using this new event though, as it has security implications: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

This does not work when your fork branch name is master. See this comment: https://github.com/wearerequired/lint-action/issues/13#issuecomment-708611002

github-actions[bot] commented 3 years ago

A stale label has been added to this issue because it has been open 15 days with no activity. To keep this issue open, add a comment within 5 days.

thatkookooguy commented 3 years ago

Hey! Was this solved? I see that the pull_request_target branch has been merge and pull_request_target is being referenced in code.

But still when I try to use pull_request_target with this aciton and a fork opens a PR, I get the 404 error.

see this dependabot PR for example

will it work if I change github_token: ${{ secrets.BOT_TOKEN }}to github_token: ${{ secrets.GITHUB_TOKEN }}? is that related to the different things you can do on a fork compared to repo PRs?

ocean90 commented 3 years ago

@Thatkookooguy Pull requests by Dependabot don't have access to secrets anymore, see https://github.com/dependabot/dependabot-core/issues/3253#issuecomment-852541544. You should be able to use the the default GITHUB_TOKEN secret in combination with pull_request_target. Consider also limiting the default permissions. For a working example see test-action.yml.

jnm2 commented 3 years ago

Please consider using workflow commands (stdout) so that tokens are not needed and pull_request_target is not needed, as mentioned above (https://github.com/wearerequired/lint-action/issues/13#issuecomment-811518586).

thatkookooguy commented 3 years ago

@ocean90 thanks for the tip! will definitely check it out.

jnm2 commented 3 years ago

@ocean90 I'm confused. Why did you downvote?

moodysalem commented 2 years ago

Please consider using workflow commands (stdout) so that tokens are not needed and pull_request_target is not needed, as mentioned above (#13 (comment)).

This particular change seems like it should not be blocked, but won't help with auto_fix: true

@samuelmeuli Will you take a PR? Seems like this piece of code needs changing: https://github.com/wearerequired/lint-action/blob/a25b25ac232deed2e3d2f802b111da885f8aa617/src/github/api.js#L21