ci: Don't comment from forks

[lorenzleutgeb]

See #133

[github-actions]

No difference in nix flake show.

[alejandrosame]

Seems simple enough. However I wonder how other automations like stalebot do it then.

[fricklerhandwerk]

Wouldn't that mean that PRs from forks won't get CI checks any more? @lorenzleutgeb the documentation you linked in the other thread says one could instead trigger on pull_request_target events, which "will execute the action from the target branch and not from the origin PR". What does that even mean? The GitHub documentation about it seems not helpful.

[lorenzleutgeb]

Wouldn't that mean that PRs from forks won't get CI checks any more?

That depends on what you mean by "CI checks". If you mean "getting checks" in the sense of "nix flake check is executed", then the answer is no. If you mean "getting checks" in the sense of "there'll be a comment about differences in nix flake show in the PR", then the answer is yes.

The change I am proposing here just disables posting the result of the step with id "flake-show-diff", if the PR comes from a fork. The reason is that this step will always fail for forks, because, for security reasons, PRs built in the context of forks have limited permissions and are not allowed to post comments to the PR.

All other steps, like nix flake check will still run. The change I am proposing here quite precisely removes the problematic step.

@lorenzleutgeb the documentation you linked in the other thread says one could instead trigger on pull_request_target events, which "will execute the action from the target branch and not from the origin PR". What does that even mean? The GitHub documentation about it seems not helpful.

  ─── B ─── M
      │
      └ A

Let's say B (base) and M (main) are commits in ngi-nix/ngipkgs/main and A (attack) is commits in eve/ngipkgs/attack. Eve opens a PR and wants to merge evil/ngipkgs/attack into ngi-nix/ngipkgs/main. The changes in A are an attack, say that code would steal the secret that gives write access to the ngi cache at Cachix.

GitHub calls eve/ngipkgs/attack the "head branch", and ngi-nix/ngipkgs/main the "base branch".

  ─── B ─── M ─── M'
      │           │
      └ F ────────┘

Assume that there is no merge conflict. M' is the (hypothetical) merge commit. Normally, GitHub Actions for the pull_request event would run on M' (so that you can for example generate a meaningful diff of nix flake show), but since the head branch is in another repo, only with limited permissions. For example, in that context you can't comment on the PR. But you also won't be able to steal secrets.

Now, the pull_request_target event will be emitted in (almost?) the same cases as pull_request, but the difference is that the corresponding GitHub action will be executed in the context of the base branch and with full permissions. So it runs at M, but it knows that there's a PR about merging F into M. For example, $GITHUB_HEAD_REF will still be F.

The GitHub docs state:

This prevents execution of unsafe code from the head of the pull request that could alter your repository or steal any secrets you use in your workflow. This event allows your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request.

[lorenzleutgeb]

Closing in favor of #176