Re-enable Coverage

[techman83]

I have enabled coverage in a few other repos, and the contents: read permission, combined with the pull_request: write appears to do the trick.

Extra

• Bumped actions/setup-python to 5
• Added pip cacheing

[HebaruSan]

Looks like it still doesn't work.

[techman83]

@HebaruSan yup 😑 - I honestly have no idea why, works fine on a couple of other repos with the exact same workflow!

[techman83]

This needed a lot more than permissions! I have pointed it at my own fork of the action for now, but the report will only run once the workflow is in the default branch 🙂

I'll fix the commits up before merging. Make it clearer about what was changed.

[techman83]

@HebaruSan - I've been using this process a bunch now. Seems to work well. I'll be keeping my fork active until the upstream decides to merge or I find an alternative 🙂

[HebaruSan]

So it's two separate jobs now, but only one runs for pull requests, so we won't actually be able to tell whether a PR increases or decreases the number? Am I missing something?

[techman83]

There's no functional difference, except the report is triggered 'workflow_run', which gets the PR context included in its event payload. It won't run until it's in the default branch of the repo (which is why it hasn't run yet), due to the security implications it is solving.

If we want to enforce that it is successful, we can use branch protection rules.

[HebaruSan]

Can you summarize those security implications? It seems really weird to me to have a tool like this that doesn't run until after merging to the main branch. I thought the whole point was to make sure that proposed changes are properly tested BEFORE merging.

[techman83]

I thought the whole point was to make sure that proposed changes are properly tested BEFORE merging.

I'll give you the TL;DR first, as I don't think I was clear on the point that this only applies to this specific pr. There are currently no workflows in our default branch that listen to the workflow_run trigger. On the next PR, this will run as if it was triggered by the PR, we can enforce the requirement if we desire by utilising a branch protection rule.

Further reading

The overview is covered here, and initially I had similar feelings on the matter. However when I dug further into the why, it became clearer, which is why submitted the changes upstream.

The crux of the matter is allowing untrusted code (a fork) to have access to the Github Token, which is required by this action to write the PR comment. GitHub have made now two triggers for a pull request. pull_request and pull_request_target. Pull requests opened from a branch in the same repo run with permissions availble, pull requests opened by a fork gets a reduced permission set unless you set the trigger to be pull_request_target, which has the following note from the docs

Warning: For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission unless the permissions key is specified and the workflow can access secrets, even when it is triggered from a fork. Although the workflow runs in the context of the base of the pull request, you should make sure that you do not check out, build, or run untrusted code from the pull request with this event. Additionally, any caches share the same scope as the base branch. To help prevent cache poisoning, you should not save the cache if there is a possibility that the cache contents were altered. For more information, see "Keeping your GitHub Actions and workflows secure: Preventing pwn requests" on the GitHub Security Lab website.

Building the coverage report in the pull_request triggered workflow, and saving it, allows us to trigger on the completion of that workflow, and as it runs from our default branch, it doesn't have the same security implications of running code that is potentially un-trusted.

[HebaruSan]

I'll give you the TL;DR first, as I don't think I was clear on the point that this only applies to this specific pr.

Ahh, indeed that helps a lot, thanks!
Let's give it a go. 👍

[techman83]

Awesome! Thanks for reviewing @HebaruSan