Fix coverage workflow

[jsoref]

This fixes:

1. https://github.com/google/temporian/actions/runs/8083954295
   comment
   Unhandled error: HttpError: Not Found
2. Anyone using this workflow in a repository with paranoid default permissions (which my organizations do) see https://docs.github.com/en/organizations/managing-organization-settings/disabling-or-limiting-github-actions-for-your-organization#configuring-the-default-github_token-permissions -- note: you should apply the restriction to your repositories too (probably at the org level, but at least at the repository level) -- but, obviously that's a bigger change.
3. Pull Requests will now get comments (this fixes Disable coverage action in external PRs #381)

[ianspektor]

Does this fix #381? If so that's extremely helpful :)

[jsoref]

It would, yes :)

[jsoref]

Or, rather. kinda.

That thing is broken in fun ways.

It depends what you're trying to really achieve.

Personally I'd get rid of writing a comment on the PR and only do step summary.

My workflow/action (check-spelling) supports both, but the effort required to maintain commenting is scary to most people.

To actually get comments on PRs from forks requires changing from pull_request to pull_request_target and changing the checkout logic to carefully checkout the attacker's branch.

Actually, let me revise this so it's less scary.

[ianspektor]

Personally I'd get rid of writing a comment on the PR and only do step summary.

Didn't know about step summaries (ref for anyone else reading this).

Is there a way to make them visible in the PR itself instead of in the Action summary? Having to go check the summary pretty much makes it useless, what we want is an obviously visible warning if your PR is decreasing coverage.

[jsoref]

I'm hoping github will eventually improve access to step summaries -- at the very least, i'm hoping github will replace details links with links to step summaries. -- I'm also hoping that larger entities (hi!) will lean on github to make that change.

Sadly, no, step summaries don't have great visibility, but the permissions required for commenting are generally sufficiently scary that most of the time projects I work with decide to use step summaries instead of allowing my workflow to comment.

Granting pull-requests: write allows a job to try to do all of the things listed here:
https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps?apiVersion=2022-11-28#repository-permissions-for-pull-requests (some of them are guarded by requiring secondary permissions, but there's still enough damage that it really isn't advisable). (Minus the caveat of whether github actually gives out the write -- as you've seen, it doesn't for PRs from a fork, but it would for pull_request_target -- see below.)

An alternative is having a bot or some purpose built (and carefully audited/maintained) code that has a token (with pull-requests: write) and have it be responsible for consuming/posting.

Anyway, if you trust workflows and that people can review/audit them, then, you can spin a wheel.

The other thing you can do is include some try blocks to make sure that when you don't have write permissions, the step doesn't actually fail.

Note that changing from push / pull_request to pull_request_target also changes how testing / reviewing workflows works. If a change is made to a workflow that uses pull_request_target, that workflow change isn't tested as part of the pull request, the new changes would only take effect after merging (otherwise an attacker could put whatever they wanted in their PR, which would obviously be awful).

Basically this stuff is a bit of a headache -- sorry :)

[jsoref]

@ianspektor: anyway, if you tell me what you'd like, I'm happy to adjust the workflow to do it (within reason)

[ianspektor]

Hey 👋🏼 Don't have time to dive into this atm but will try to get to it early next week - thanks so much for the very detailed explanations!

[ianspektor]

Alright, here's what I propose:

• We need the comment. We can additionally do the step summary too, if the comment on PRs from a fork isn't possible. But having the step summary only beats the purpose of this action, which is alerting us when a PR decreases coverage.
• Changing to pull_request_target sounds like it makes sense. I don't mind the change in behavior (workflow changes not affecting their own PR). I don't completely understand what you mean by "changing the checkout logic to carefully checkout the attacker's branch" here - please do ellaborate if necessary.
• If changing to pull_request_target isn't the way to go, the try blocks as a simple solution for not failing on forks sound OK too.

Thoughts?

[jsoref]

Ok, here's what I have, this is a same repo PR with comment:
check-spelling-sandbox#2 (comment)

Here's a PR from a different repo with comment:
check-spelling-sandbox#3 (comment)

[jsoref]

"changing the checkout logic to carefully checkout the attacker's branch" here - please do ellaborate if necessary.

So, this requires essays, and there are some, e.g.: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/.

TL;DR:

If someone is triggering an event in their own repository, they already had the ability to write to the repository, and thus it isn't interesting if what they push can make additional changes to their repository. -- This is conceptually, e.g. me pushing to my own repository and a workflow in my repository being given permissions: { contents: "write" }.

If someone is triggering an event in someone else's repository, then it'd be bad if they could cause a change to the repository to be made without consent of the owners of that repository. This is the case you have here. I'm making a significant change to a workflow. -- If my change were directly run before you merged it, that'd mean my PR could trigger additional commits to be added and that's really bad. GitHub has a number of protections for this: first by default workflows running from PRs across forks are limited to permissions: *: read -- even if a workflow says it wants permissions: _something_: write. This is the standard pull_request event -- and actions/checkout will, by default, check out the merge of the pull request head and the pull request target branch.

Sometimes, you need to be able to allow a PR to do things with write permissions (the comment here is the classic example), so github has a pull_request_target event -- but it runs in the context of the target. So, to prevent repository owners and workflow authors from making their repositories too easily take over, the default for actions/checkout is to check out the pull request target -- where the code is code that the target repository has already accepted.

In the proposed workflow, for a future PR w/ this enabled, there are three jobs and they'll look like this:

coverage (pr)

GITHUB_TOKEN Permissions
  Contents: read
  Metadata: read

Checkout

Run actions/checkout@v3
  with:
    ref: refs/pull/3/merge

-- this part is where things could be dangerous (as the code being run is from a fork and is thus untrustworthy) -- if not for the careful permissions established by the workflow changes here --

coverage (main)

GITHUB_TOKEN Permissions
  Contents: read
  Metadata: read

Checkout

Run actions/checkout@v3
  with:
    ref: main

comment

GITHUB_TOKEN Permissions
  Metadata: read
  PullRequests: write

no checkout

If there was a checkout, it'd default to using the pull request target's branch. Here the code (and in fact, all of the workflow steps themselves) came from the pull request target.

---

In general, changes to workflows that have pull_request_target events are not seen until after they're merged, so testing them involves setting up a repository where the commits are already present (I'm not certain about how pull request approvals interacts with this restriction, but my guess is that it doesn't change anything).

[ianspektor]

Thanks again for the extremely careful explanations.

changes to workflows that have pull_request_target events are not seen until after they're merged

We could also just change it to pull_request temporarily while testing, if it's us modifying it.

[jsoref]

We could also just change it to pull_request temporarily while testing, if it's us modifying it.

Yeah. I'd personally just create a PR into a branch that has the changes I'm testing (as I did here to show things). But, that's probably because I spend 99% of my time dealing with my workflows that use pull_request_target.

[ianspektor]

One more thing: could you add some very concrete (~1-line) explanations in the tricky/not-so-standard parts?

• Permissions
• why pull_request_target and not pull_request
• checkout ref logic

Add links if needed

[jsoref]

Sadly the various doc/blog links are kinda lousy, here's the "basic" text:

Restricting permissions for tokens
To help mitigate the risk of an exposed token, consider restricting the assigned permissions. For more information, see "Automatic token authentication."

It isn't wrong, but it isn't remotely helpful.

Modifying the permissions for the GITHUB_TOKEN

You can modify the permissions for the GITHUB_TOKEN in individual workflow files. If the default permissions for the GITHUB_TOKEN are restrictive, you may have to elevate the permissions to allow some actions and commands to run successfully. If the default permissions are permissive, you can edit the workflow file to remove some permissions from the GITHUB_TOKEN. As a good security practice, you should grant the GITHUB_TOKEN the least required access.

Again, the information is kinda here, but it's more or less buried in the last sentence of the paragraph.

There's a three part series: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ but none of them suggest chaining jobs to handle untrusted actions even though that's really the way to go 🙍‍♂️.

[jsoref]

Fwiw, once this is merged, someone should go in and bump actions/checkout to @v4 and actions/github-script to @v7 to get rid of the node16 warnings.

I'm leaving that for a separate PR as I don't really want to extend this PR -- it's already sufficiently complicated.

[ianspektor]

Fwiw, once this is merged, someone should go in and bump actions/checkout to @v4 and actions/github-script to @v7 to get rid of the node16 warnings.

#388

[ianspektor]

Thanks a ton for both the contribution and the extensive info @jsoref 💪🏼 lmk if you'd like to take #388 too