ci: let a trusted release dispatch update the tap, and harden the guard - #16187
teamleaderleo wants to merge 1 commit into
Conversation
Follow-up to #16140, which closed the injection. Four things it left. The gate now requires `workflow_run.event == 'push'`, but release.yml runs on both a `v*` tag push and workflow_dispatch, and the dispatch is how a release is recovered when the tag-push run fails. That happened three times in September: v0.64.23, v0.64.24 and v0.64.25 all concluded failure. Recovering by dispatching release.yml on the tag publishes the DMG but leaves workflow_run.event as workflow_dispatch, so the gate skips silently and the tap stays behind. That is the same failure this file's comment says it was recently fixed for. Accept a dispatch as well, using the contains(fromJSON(...)) idiom already in ci-compile-attribution.yml. A dispatch needs write access and still has to pass the head_repository check, so no new author is admitted, and pull_request stays out. `steps.version.outputs.version` was the last value still substituted into a script, and its safety rested entirely on the semver regex above it. Take it through env: too, so widening that regex later cannot become code execution. The guard's sweep knew only `github.event.inputs`, not the bare `inputs.` spelling of the same workflow_dispatch value. It also probed the regex with one payload containing slashes, so a regex widened to accept prerelease suffixes passed. Add the `inputs.` prefix, pin the regex, and add a digit-prefixed hostile payload. The comment explaining the security property had lost a sentence boundary, which inverted what it said. Verified. Each new assertion mutation-tested: widening the regex to `^[0-9]+\.[0-9]+\.[0-9]+.*$` gives 2 failures, interpolating `${{ inputs.version }}` into a script gives 1, reverting the gate to push-only gives 1, and restoring gives exit 0 with 8 ok. The widened regex no longer creates the marker file, which is the env: change doing its job. actionlint clean at CI severity (SHELLCHECK_OPTS="-S warning"), YAML parses, and test_release_homebrew_gate.py, test_ci_workflow_run_sources.py and test_ci_actionlint_covers_every_workflow.py all pass. Registry valid at 366 tests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 18 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Closing this in favour of #16197, which is the same change with a stronger guard. My own duplicate, not anyone else's: I opened this one earlier today, then reviewed #16140 and rebuilt the fix from the review's findings without first checking my own open PRs. Entirely my error, and the lesson is mine to keep. #16197 carries everything here plus three things this one lacks:
It also lands as two commits, guard first (failing 3 checks against the current workflow) then fix, which is the shape this repo wants for a regression test. This branch's CI was also sitting on a stale base: its — Raindrop g2 🫧 |
1 participant
Follow-up to #16140. A review subagent went over that diff after it merged, so the four findings land here instead of on it.
The substantive one: the gate locks out release recovery
#16140 added a
workflow_rungate toupdate-homebrew.ymlthat admits onlygithub.event.workflow_run.event == 'push'.release.ymlruns on av*tag push and onworkflow_dispatch, and the dispatch is how a release gets rescued when the tag-push run fails. That is not hypothetical: the tag-push runs for v0.64.23, v0.64.24 and v0.64.25 all failed, and 20 of the last 30Release macOS appruns were dispatches. Under the merged gate every one of those recoveries leaves the tap behind, which is the stuck-tap failure the file's own comment says it was recently fixed for.The gate now accepts either event:
contains(fromJSON('["push","workflow_dispatch"]'), github.event.workflow_run.event)This admits no new authors. A dispatch of
release.ymlneeds write access, and the run still has to satisfy the existingpathandhead_repository.full_namechecks, so a fork pull request's copy of a workflow with the same display name is still rejected.pull_request, the trigger a fork can actually reach, stays out.Three hardening items
Download DMG and get SHA256was the last step still interpolating${{ steps.version.outputs.version }}straight into its script. It goes throughenv:now, so the step no longer depends on the semver regex one step above staying strict.Get versionhad lost a sentence boundary and read as the opposite of what it means. Rewritten.github.event.workflow_runandgithub.event.inputs.inputs.is the other spelling of aworkflow_dispatchinput and reaches the same attacker-influenced value, so it is in the prefix tuple now.Guard changes
tests/test_ci_homebrew_untrusted_input.pygrew a second hostile payload (v1.2.3$(touch ...), whose leading digits make it look like a release ref), an assertion pinning the version regex to a bare three-part semver rather than trusting the payload list to cover every future widening, and the new gate condition.Verification
Every new assertion was mutation-tested, each mutant restored immediately after:
^[0-9]+\.[0-9]+\.[0-9]+.*$${{ inputs.version }}into a scriptThe widened-regex mutant did not create the marker file, only tripped the "is skipped" assertion. That is direct evidence the
env:change removed the script's dependence on the regex, so it is a hardening rather than a bug fix.actionlintis clean on the workflow.This is a CI fix, so it skips team review and merges on green.
— Raindrop g2 🫧
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the Homebrew tap gate so releases recovered via
workflow_dispatchre-runs ofrelease.ymlcan update the tap; the push-only gate was silently skipping those recoveries, leaving the cask stuck.The dispatch path still requires write access and the existing repository checks, so no new authors are admitted —
pull_request, the only trigger a fork can reach, stays rejected.Hardening
env:instead of interpolating it into the script.inputs.spelling of a dispatch input.Written for commit da7eb13. Summary will update on new commits.