fix(ci): update-homebrew keeps run metadata out of scripts - #16140
Conversation
…ripts update-homebrew.yml substitutes github.event.workflow_run.head_branch and the dispatch input into its `run:` script as text. The test renders the version step the way the runner does, with a hostile branch name, and checks nothing executes; it also requires the gate to accept only the real release workflow run from a tag push in this repository. Found by a Codex Security audit (both models). Red: python3 tests/test_ci_homebrew_untrusted_input.py Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ly the real release run Makes tests/test_ci_homebrew_untrusted_input.py green (previous commit). Root cause: the version step substituted github.event.workflow_run.head_branch and the dispatch input into its script text, and the gate accepted any run of a workflow with the release workflow's display name. The values now reach the script as env vars (INPUT_VERSION, HEAD_BRANCH, EVENT_NAME), and the gate requires the run to be .github/workflows/release.yml, from this repository, for a push. Dispatch keeps working as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Homebrew update workflow filters ChangesHomebrew workflow safeguards
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The workflow tightens release-run eligibility and safely passes event inputs through environment variables, with safeguard tests registered in CI. No actionable merge-blocking risk remains after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change narrows which release runs can update Homebrew and handles event values as data rather than executable shell text. No introduced security regression was identified. The publishing job remains privileged, and its live credential scope and runner isolation were not verified. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
Merge receipt for |
|
Review, after the fact: a subagent reviewed The injection is closed. I enumerated every The guard has teeth. Reverting only the workflow hunks and keeping the new test gives 8 failures, and the first one is the marker file, so it proves execution and not a text pattern. Each half reverted independently also fails: Two findings, both medium, both now in #16197:
Two low ones, also in #16197: the sweep's prefix list misses One informational note, pre-existing and not touched here: the cask heredoc at line 163 uses an unquoted delimiter, so its body takes command substitution. Not exploitable while The one thing I could not settle, and it is yours to answer rather than mine: whether any of those 20 dispatches was meant to publish a cask, or whether dispatching Fixed: nothing in this PR (it merged first). Left: findings 1 through 4, all in #16197. — Raindrop g2 🫧 |
* test(ci): pin the homebrew gate's trigger allow-list and version pattern #16140 closed the injection but left three ways to reopen it or to lose a release. The guard did not assert the version regex that keeps the one remaining interpolation safe, did not know the `inputs.version` spelling of a dispatch input, and did not say which triggers the gate may accept. Add, all failing against the current workflow: - the gate allow-lists `github.event.workflow_run.event` and admits exactly push and workflow_dispatch, so neither a widening nor a narrowing is silent - `steps.version.outputs` reaches a script through `env:` like the rest of the attacker-derived values - the version step pins an anchored semver pattern with no wildcard tail - a semver-prefixed payload (`v1.2.3$(...)`), which a bare `$(...)` payload cannot stand in for once the pattern is widened - `inputs.` as an interpolation prefix Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(ci): let the homebrew gate accept a dispatched release run release.yml ships from two triggers: `push: tags: v*` and `workflow_dispatch` (.github/workflows/release.yml:3-7). #16140's gate accepted only `push`, which removes the recovery path the file's own comment was added for. Of the last 30 Release macOS app runs, 20 were dispatches; v0.64.23, v0.64.24 and v0.64.25 all had a failing tag-push run in September, and the dispatch is how a maintainer publishes the cask after one of those. With the gate as written the tap would silently stay on the previous version, which is the exact failure the comment at lines 32-39 describes. Allow-list both triggers with the idiom already used at ci-compile-attribution.yml:55. Dispatching release.yml needs write access and `head_repository.full_name == github.repository` still holds, so this does not widen the trust boundary; `pull_request` stays out. Also move the last interpolation out of a `run:` body. `steps.version.outputs.version` is the branch name minus a `v`, admitted by an anchored semver regex. Safe today, but its safety lives in a regex three steps away: widen that pattern to take `1.2.3-beta.1` and a branch named `v1.2.3$(cmd)` runs in the job holding HOMEBREW_TAP_TOKEN. Passing it through `env:` removes the dependency. Fixes the comment at lines 92-94, which lost a sentence boundary and reads as the opposite of what it means. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* test(ci): update-homebrew must keep triggering-run metadata out of scripts update-homebrew.yml substitutes github.event.workflow_run.head_branch and the dispatch input into its `run:` script as text. The test renders the version step the way the runner does, with a hostile branch name, and checks nothing executes; it also requires the gate to accept only the real release workflow run from a tag push in this repository. Found by a Codex Security audit (both models). Red: python3 tests/test_ci_homebrew_untrusted_input.py Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> * fix(ci): update-homebrew takes run metadata through env and trusts only the real release run Makes tests/test_ci_homebrew_untrusted_input.py green (previous commit). Root cause: the version step substituted github.event.workflow_run.head_branch and the dispatch input into its script text, and the gate accepted any run of a workflow with the release workflow's display name. The values now reach the script as env vars (INPUT_VERSION, HEAD_BRANCH, EVENT_NAME), and the gate requires the run to be .github/workflows/release.yml, from this repository, for a push. Dispatch keeps working as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com> (cherry picked from commit 55c3817)
2 participants
update-homebrew.ymlnow passes the triggering run's branch and the dispatch input to its version step throughenv:instead of substituting them into the script, and its gate accepts only a.github/workflows/release.ymlrun from this repository triggered by a push (manual dispatch unchanged).Found by a Codex Security audit (both models agreed). Commit 1 adds the failing test (it renders the step the way the runner does, with a hostile branch name, and checks nothing executes), commit 2 the fix.
Verification:
python3 tests/test_ci_homebrew_untrusted_input.py,tests/test_release_homebrew_gate.py,tests/test_ci_workflow_run_sources.py,tests/test_ci_workflow_guards_are_wired.py; actionlint reports only the four pre-existing SC2086 infos.Changelog
none
🤖 Generated with Claude Code
Summary by cubic
Fixes a command injection vulnerability in the Homebrew cask update workflow where a hostile branch name from a triggering run could execute arbitrary code with the tap token.
env:instead of being substituted into script text, so values like$(cmd)can't run.workflow_rungate now accepts only the realrelease.ymlrun from this repository triggered by a push; manual dispatch is unchanged.Written for commit f3b70ce. Summary will update on new commits.
Summary by CodeRabbit