if it's a pain to run PyPy tests under coverage, I think it would be fine to do the pypy tests as a separate CI job that aren't run under coverage. None of our code is currently PyPy-specific (we had a workaround for a PyPy bug for a while but it was a tiny branch of code).

Could you fix the pre-commit failures?

According to https://docs.github.com/actions/how-tos/security-for-github-actions/security-guides/automatic-token-authentication, a PR from a fork can never have write access (expect the repo itself).

A relevant bit:

Finally, if the workflow was triggered by a pull request from a forked repository, and the Send write tokens to workflows from pull requests setting is not selected, the permissions are adjusted to change any write permissions to read only.

Maintainers note from the action on the error: https://github.com/marocchino/sticky-pull-request-comment/tree/v2/#error-resource-not-accessible-by-integration

check your Settings > Actions > General > Workflow permissions, and make sure to enable read and write permissions.

The alternative is to provide a GITHUB_TOKEN (defaults to ${{ github.token }}) currently.

@JelleZijlstra I think we need your decision / help here to finish this PR.
If we want to have a coverage report message the action needs write permission on the PR, see links above for the options.
We could also use codecov. I thought it also required a secret token, on a second look it might also without (on public repos). It comes with its own pros and cons.

Read and write permissions for workflows are already enabled, so this might work once we merge it into main?

I've changed the code to a working variant. However, it should be reviewed carefully as we would need to add an exception:

zizmor's warning is correct that workflow_run can be unsafe: See https://docs.zizmor.sh/audits/#dangerous-triggers

Here is a good article about it that I followed to create the new variant: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ to avoid insecure pitfalls.

The most important bit, coverage_report.yml does not use execute user code, i.e. code checkout and code run.
The new workflow (and any modifications of it) will only be active after a merge. Any future PR modifying it should be reviewed carefully.

The new workflow consists of github actions and one variable write:

• download a file containing the PR number and store it in the env (more below)
• download the coverage report formatted as markdown
• post the markdown in the respective PR number

zizmor does not report a potentially dangerous GITHUB_ENV write https://docs.zizmor.sh/audits/#github-env

However a user could store what they want in pr_number.txt. Potentially a guard checking that it only contains a small number (so it has to be a PR number) should be added.

As a conclusion to feel save would be to ask someone who is more knowledgeable about the risk to have a look and make a review

Following the guide one could rewrite the workflow to only use github scripts to extract the PR number and write the comment. This would save the call to modify GITHUB_ENV

As a conclusion to feel save would be to ask someone who is more knowledgeable about the risk to have a look and make a review

@woodruffw -- don't suppose you could give this PR a once over from a security perspective, could you? typing_extensions is a top-10 PyPI package, so it's probably better for us to be safe than sorry here!

Thanks for the ping @AlexWaygood! Yeah, I'll be able to do a review of this in a moment.

Okay, I did a quick pass over this. I think @Daraan is right that this can't execute arbitrary code from a third-party fork with unintentional privileges.

However, this is susceptible to another kind of confusion/delegation issue that's typical with workflow_run:

• The "called" workflow (coverage_report.yml) uses "Test and lint" as its caller;
• However, any PR can name any workflow like that, which means that effectively any workflow (and not just the "trusted" caller) can invoke coverage_report.yml.

In practice, what that means is that someone can submit a PR containing foo.yml with name: Test and lint, and foo.yml will fully control a dispatch into coverage_report.yml. In this particular case that means that foo.yml will control both pr_number.txt and the generated Markdown fully (via its artifact), meaning that they can insert comments on PRs other than the one that they opened.

As far as I can tell, there's unfortunately no really good way to workaround that with workflow_run itself -- the ideal situation is that the workflow_run is only restricted to "trusted" triggers, but in this case the goal is explicitly untrusted ones (like pull_request). The other workaround would be to somehow plug a trusted input into it (like a label action, since only maintainers can do those), but there's no real way to forward those into a workflow_run.

TL;DR: A malicious PR creator can use this to change coverage results on other PRs, or change the markdown to anything they please (probably mostly as a griefing vector that's hard to moderate, since it's a non-user commenter). My suggestion would be to keep the coverage check, but to either limit the comment flow to first-party (i.e. non-fork) PRs or remove the comment flow entirely (since admittedly it isn't very useful if it's only on non-fork PRs).

Thank you very much for the review. That is kind of the scenario I see as well.

Some new thoughts that I had in the meantime:

• GITHUB_OUTPUT can like be used instead of ENV
• pip install coverage and merging the coverage files in this workflow seems save. Iirc correctly coverage needs access to the source. Adding the source files as artifact would work as well. Then an attacker has no longer reign over the markdown file.
• I did not look into pull_request_target yet as more can go wrong there, but as it access to the PR number the upload of the PR is also taken from the PR creator.

Is there any difference to adding a new foo.yml with the same workflow name compared to just rewriting the existing ci file? Both are in the hand of the PR creator.

• pip install coverage and merging the coverage files in this workflow seems save. Iirc correctly coverage needs access to the source. Adding the source files as artifact would work as well. Then an attacker has no longer reign over the markdown file.

Making sure I understand: you're proposing merging the coverage files in the workflow_run instead? I think that would eliminate the immediate griefing issue, but it doesn't prevent the underlying one (where the PR number itself is entirely attacker controlled). I think it's also a bit risky, since I'm not sure coverage combine is guaranteed to be safe to run on untrusted inputs (which the coverage inputs would then become).

I did not look into pull_request_target yet as more can go wrong there, but as it access to the PR number the upload of the PR is also taken from the PR creator.

Yeah, unfortunately I think pull_request_target is equally risky -- it doesn't have exactly the same footgun as workflow_run around the PR number, but it requires executing the test suite in a privileged context and that seems pretty risky.

Is there any difference to adding a new foo.yml with the same workflow name compared to just rewriting the existing ci file? Both are in the hand of the PR creator.

Yeah, those two are 100% equivalent, I just said foo.yml to be generic 🙂 -- the underlying problem is fundamentally that the "input" to a workflow_run is 100% untrusted unless you constrain its triggers, which in this case can't easily be done.

Thanks for taking a look! I think the immediate vulnerability (attackers can post comments on random PRs) is fairly harmless, but it does feel like there's a risk attackers could do other sketchy things that we haven't thought of, or that this vulnerability could compound with permissions we grant in other GitHub Actions.

As an alternative, could we forego the comment workflow and just have the coverage check print out the result in its action output, and fail if coverage is below some threshold?

As an alternative, could we forego the comment workflow and just have the coverage check print out the result in its action output, and fail if coverage is below some threshold?

this also has the advantage of probably simplifying the workflow a fair bit :-)

Yeah, I think having the output stick to the run console is probably the way to go in terms of access control! I wish there was a way to grant a "let this untrusted job post exactly one comment to this exact PR" token in GHA, but alas 😅

I've reworked the crucial parts again:

• PR number is now acquired by an API call that queries in which PR (of the repository) the head commit that triggered the workflow is in. In case of a PR duplicate (multiple results) it errors out.
  Sidenote: I also tried via REST https://docs.github.com/de/rest/commits/commits?apiVersion=2022-11-28#list-pull-requests-associated-with-a-commit. But the result was always empty.
• The PR workflow has to upload the coverage.xml artifact, i.e. coverage combine run on fork. The xml is processed by the https://github.com/irongut/CodeCoverageSummary/ action get the markdown comment
• The markdown file of the action is posted as the comment.

Do you want to give it another look?

As an alternative, could we forego the comment workflow and just have the coverage check print out the result in its action output, and fail if coverage is below some threshold?

Easily possible :)

it's also not clear to me how this works when PRs contain some shared history or have identical commits due to cherry picking, so this feels pretty risky.)

I do not see a problem in cherry picks as the sha is important which will not be equal after picking.

A shared history and two PRs ABC and AB could exist. If B is the last commit in a PR. We could not disambiguate which is the triggering PR (ABC or AB). In that case abort.

I think at this point I'd really value taking an incremental approach here:

• Let's first land a simple workflow that just prints the coverage report to the terminal and fails CI if coverage drops under a certain percentage
• After we land that, we can open a followup issue/PR to investigate if there's a secure way for us to have a comment workflow (which would certainly be nice, but doesn't seem essential)

I think at this point I'd really value taking an incremental approach here:

* Let's first land a simple workflow that just prints the coverage report to the terminal and fails CI if coverage drops under a certain percentage

* After we land that, we can open a followup issue/PR to investigate if there's a secure way for us to have a comment workflow (which would certainly be nice, but doesn't seem _essential_)

Lets do that for now.

I removed the second workflow_run file. I've kept the comment support in for now, it will only apply to 1st party PRs, i.e. branches that are on the repo itself - so if you do a PR on your own fork you will receive the comment.
Ofc I can remove the action fully if you prefer.

EDIT:

I set the thresholds to 90 / 95. Below 90% it would fail.

Currently we are 97%, despite some branches missing in the 3.9 and 3.10 range that are not run in the current tests.

Tested it out in #651 and #652 and it seems to be working well -- thanks @Daraan!!