Closed drigz closed 4 months ago
@mering PTAL - I also updated the description. I decided against git branch --contains
because it seemed to duplicate the existing checks - and might be annoying if we start maintaining release branches.
EDIT: Markus told me that there's a "PTAL" button in GitHub! 🎉
@mering PTAL - I also updated the description. I decided against
git branch --contains
because it seemed to duplicate the existing checks - and might be annoying if we start maintaining release branches.
I agree that the branch=main
query filter together with head_repository.id
check would be at least equivalent to the git branch --contains
(probably even superior).
Without this parameter, the API will also return commits from PRs, which may not have been reviewed and could be malicious.
This has the downside that if you're trying to fix the release workflow you need to change the schedules or be limited to one attempted fix per day, but the alternatives (see below) are also not ideal.
For defense in depth, we also check against the head_repository id, which I tested by temporarily changing
event=schedule
toactor=totoro642
, in which case the action fails with:Alternatives considered:
push
and then having the release workflow filter withevent=push
instead. This would make it easier to manually trigger a release from a recently submitted change, but doesn't address the (more common) issue that the Postsubmit fails due to some unrelated cloud-side breakage, and must be retriggered after intervening on the GKE cluster to get a green run.git clone
pattern that makes the attack possible, but at the cost of allowing actions to trigger other actions, which feels like it could open another class of attacks (such as triggering a release from older, insecure code).http://b/348316770