Alex GH Action is failing on forks

[robdodson]

Describe the bug
This PR is failing when Alex tries to run. The error says: ##[error]Resource not accessible by integration.

From a bit of Googling it seems like maybe we need to change the workflow to use the pull_request_target event instead of pull_request so things can have the proper permissions.
https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/#improvements-for-public-repository-forks

I'm still a bit unclear on how this works but we can land a PR and see if it fixes it 🤷‍♂️

[robdodson]

Reading the instructions I think I understand a bit better.

I think it's essentially saying that if a fork sends in a pull request that modifies an existing workflow, or adds a new workflow, it won't actually execute the workflow. Instead, for a workflow to execute on a PR coming from a fork, the workflow must already be defined in the base repo/branch. I think this avoids a situation where a fork maliciously sends in a change to an existing workflow that tries to steal the ${{ secrets.GITHUB_TOKEN }}.

So if a workflow already exists in the base branch, then it is allowed to have elevated permissions when it runs against a PR coming in from a fork.

[robdodson]

Some final thoughts:

Normally when a fork sends in a PR, the GITHUB_TOKEN is set to read-only permissions. This has been working fine so far because all our linters just log to the Actions console.

But I'm guessing that the alex-recommends Action (the one that failed in the link above) needs elevated permissions because it tries to post a comment to the PR thread with the results of the linter pass. Since it only has a read-only token, it can't post, and blows up.

Changing this to use pull_request_target makes it safe for alex-recommends to have elevated permissions because the workflow already exists in the base branch and we've vetted it.

[robdodson]

This is now failing because the dorny/paths-filter action doesn't work with the new pull_request_target feature.

• I've PR'd that action to add support (Add support for pull_request_target dorny/paths-filter#29)
• I'll update our workflow to remove the path filtering for now

[harryherbig]

@robdodson we are currently evaluating the new pull_request_target trigger and stumbled upon the lint workflow of this repo that you changed 5 days ago.
We experimented with this new trigger to enable workflows for pull requests from forks and had to add an explicit workflow step to checkout the PRs ref to be able to test / lint it.
Do you agree, that in the current state your workflow is only linting the origin/master and not the changed code of the pull request, or are we missing something?

[robdodson]

This is our workflow now: https://github.com/GoogleChrome/web.dev/blob/master/.github/workflows/lint-and-test-workflow.yml

I don't think we needed to do anything to get it to lint just the files in the PR, I think the alex-recommends action does that by default. It seems to be working because when I see a PR that touches a file, we start seeing linter errors just for that file. Here's an example: #3725

[bobaaaaa]

The alex-recommends action might work as expected but I also guess the npm run lint:x does always check the master. You can look at the log output of action/checkout. All ref's pointing to the last master commit.

We had a long debug session about this today.

[robdodson]

Yeah I think our npm scripts will always lint everything in the project but for alex we specifically rely on the GH Action since it only runs against changed files.

[robdodson]

ah yeah now I realized we are hitting the issue @harryurban brought up. Our test command is no longer testing the changes introduced in the PR, it's testing whatever is currently on master.