update actions in response to pull_request_target concerns

[bmw]

this pr is in response to https://words.filippo.io/compromise-survey/. ohemorange and i read this late on a friday to (speaking for myself at least) much panic as it has some very strong words to say about the github actions trigger pull_request_target which we use. looking into the issue more, i also found that the popular static analysis tool zizmor flags any github actions workflow that uses the pull_request_target trigger with the message:

error[dangerous-triggers]: use of fundamentally insecure workflow trigger
pull_request_target is almost always used insecurely

this only added to my concern

the general problem with pull_request_target is that it runs with additional privileges (e.g. potential write access, access to secrets) in an environment containing values that can be set by an attacker. these values include things such as references to the arbitrary code contained in the triggering pr and pr titles which have been used to perform shell injection attacks. not carefully treating these values like the untrusted data it is while executing code in the privileged environment given to pull_request_target has resulted in many supply chain attacks

that's not to say that pull_request_target CAN'T be used securely. zizmor even has an issue brainstorming how to not warn about all uses of the trigger as some are clearly fine and the only way to accomplish what the user wants. i'm going to argue that our uses of the trigger are ok

looking through the links provided by filippo's blog and zizmor's docs, i think we can break down attacks used against pull_request_target into roughly 2 categories:

1. shell injection: "Nx S1ingularity" and "Ultralytics" from filippo's blog
2. checking out and running a PR's code: "Kong Ingress Controller" and "Rspack" from filippo's blog and https://ptrpa.ws/nixpkgs-actions-abuse from zizmor docs

i think none of our pull_request_target workflows have these problems. none of them use a shell (the zizmor issue i linked earlier suggests that any pull_request_target workflow that uses a run block should always be flagged as insecure). instead, our workflows just call action-mattermost-notify which can be pretty easily audited (as all the other files in the repo are boilerplate). passing possible attacker controlled values directly to an action written in another language is one of the approaches for mitigating script injection recommended by github. our workflows also do not check out the triggering pr's code

despite all that, i took this opportunity to cleanup and harden things a bit. i reduced the permissions for each workflow and confirmed they each still work on my fork. i also pinned the mattermost action to an exact version and added some inline documentation

with these changes, our github workflows trigger few to no warnings/errors when checked with zizmor, octoscan, and openssf scorecard

if this pr is approved, i'll make similar changes to our josepy repo

[ohemorange]

Do we need to update all of our branches to contain this? Or will github only use this version, even if actions are taken against older branches? I see there's a branches filter, do we need to use that?

I also find this paragraph confusing:

When a workflow is triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission, even when it is triggered from a public fork. For more information, see Events that trigger workflows.

Does that mean those are the default permissions granted, and can be overridden with permissions: {}? Or does it mean that it ignores existing permissions and grants those instead?

[bmw]

Do we need to update all of our branches to contain this? Or will github only use this version, even if actions are taken against older branches? I see there's a branches filter, do we need to use that?

i don't think we do because of this upcoming change!

When a workflow is triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission, even when it is triggered from a public fork. For more information, see Events that trigger workflows.

Does that mean those are the default permissions granted, and can be overridden with permissions: {}? Or does it mean that it ignores existing permissions and grants those instead?

i'm pretty sure it means the former

it's a little extra confusing because i also did the thing their permission docs (which i believe is what you're referring to there) say the sentence before that

Owners of an organization can restrict write access for the GITHUB_TOKEN at the repository level. For more information, see Disabling or limiting GitHub Actions for your organization.

because of that, our default token already has reduced, read only permissions, but you can see them being reduced even more thru this pr by comparing logs from our review requested workflow on main to my fork

[ohemorange]

i don't think we do because of this upcoming change!

this is fantastic!

looks like despite the uncertain documentation, the test you ran is confirming it is in fact reducing permissions. great!

[ohemorange]

thanks for looking into and testing all of this!