Skip to content

Harden tap redirect trust and rollback - #24198

Merged
MikeMcQuaid merged 1 commit into
mainfrom
redirect-trust-provenance
Oct 9, 2026
Merged

MikeMcQuaid merged 1 commit into
mainfrom
redirect-trust-provenance

Conversation

@p-linnane

Copy link
Copy Markdown
Contributor

Homebrew renamed taps based on any redirecting to line in Git's stderr. Over SSH that line comes from the remote server, so any tap host could relabel its checkout as an official, implicitly trusted tap or as another tap.

  • Rename a tap only when Git's resolved origin URL (including insteadOf) is HTTPS or SSH on github.com.
  • Never rename into or out of an official tap, and don't let an untrusted tap rename into existing tap, formula, cask or command trust.
  • When brew update rolls back a refused redirect, keep local changes and developer branches, and report a failed rollback without skipping other taps.
  • Fix a stale cached remote in brew tap --custom-remote and an ArgumentError when a core tap is redirected to a third-party tap.

GitHub renames of untrusted taps still work; renames from other hosts now need brew untap and brew tap.


  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

GPT-6 Astra and Claude Code (Opus 5.5) drafted the implementation and tests, verified the new tests fail without the change and pass with it, and ran brew lgtm plus targeted specs; I reviewed the diff.


Signed-off-by: Patrick Linnane <patrick@linnane.io>
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0834aa0e-cbfc-4a68-83bd-c6d17ca3b184
📥 Commits

Reviewing files that changed from the base of the PR and between 96d7c8b and a6a3ceb.

📒 Files selected for processing (7)
  • Library/Homebrew/cmd/update-report.rb
  • Library/Homebrew/cmd/update.sh
  • Library/Homebrew/tap.rb
  • Library/Homebrew/test/cmd/update-report_spec.rb
  • Library/Homebrew/test/cmd/update_spec.rb
  • Library/Homebrew/test/tap_spec.rb
  • docs/Homebrew-Security-and-Supply-Chain.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (20)
  • GitHub Check: docker (arm64 Ubuntu 24.04)
  • GitHub Check: docker (x86_64 Ubuntu 24.04)
  • GitHub Check: docker (x86_64 Ubuntu 26.04)
  • GitHub Check: docker (arm64 Ubuntu 26.04)
  • GitHub Check: tests (macOS, 2/2)
  • GitHub Check: tests (generic OS, 1/2)
  • GitHub Check: tests (generic OS, 2/2)
  • GitHub Check: tests (macOS, 1/2)
  • GitHub Check: workflow_syntax
  • GitHub Check: tests (load-only macOS)
  • GitHub Check: tests (load-only Linux)
  • GitHub Check: tests (Linux, 2/2)
  • GitHub Check: tests (online, 2/2)
  • GitHub Check: syntax
  • GitHub Check: tests (no Sorbet)
  • GitHub Check: tests (online, 1/2)
  • GitHub Check: docs
  • GitHub Check: tests (Linux, 1/2)
  • GitHub Check: Analyze
  • GitHub Check: autogenerated
🧰 Additional context used
📚 Code guidelines (2)
docs/AGENTS.md — auto-discovered
Generated by CodeRabbit — auto-discovered
📓 Path-based instructions (3)
Source excerpt: Use the system Homebrew `brew bundle exec ...`

📄 CodeRabbit inference engine (docs/AGENTS.md)

Files:

  • docs/Homebrew-Security-and-Supply-Chain.md
Test command behavior and its observable effects, including stdout, stderr, exit status, and state changes as relevant.

📄 CodeRabbit inference engine (Generated by CodeRabbit)

Files:

  • Library/Homebrew/test/cmd/update_spec.rb
  • Library/Homebrew/test/cmd/update-report_spec.rb
Implement CLI commands as `Homebrew::Cmd` classes inheriting from `AbstractCommand`; declare help text and argument metadata in `cmd_args`, and put command behavior in `run`.

📄 CodeRabbit inference engine (Generated by CodeRabbit)

Files:

  • Library/Homebrew/cmd/update.sh
  • Library/Homebrew/cmd/update-report.rb
🪛 ast-grep (0.45.3)
Library/Homebrew/test/cmd/update-report_spec.rb

[warning] 166-169: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: expect(update_report).to receive(:odie).with(satisfy do |message|
rollback_stash = Utils.popen_read("git", "-C", tap.path, "rev-parse", "refs/stash").chomp
message.include?(user_stash) && message.include?(rollback_stash)
end).and_raise(SystemExit)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures

(hardcoded-secret-rsa-passphrase-ruby)


[warning] 166-169: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: receive(:odie).with(satisfy do |message|
rollback_stash = Utils.popen_read("git", "-C", tap.path, "rev-parse", "refs/stash").chomp
message.include?(user_stash) && message.include?(rollback_stash)
end)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures

(hardcoded-secret-rsa-passphrase-ruby)

🔇 Additional comments (8)
Library/Homebrew/tap.rb (2)

580-583: Check the exit status of git remote get-url before trusting source_remote.

Utils.popen_read returns an empty string when git remote get-url origin fails. That happens when the tap has no origin remote or when the path is not a Git repository. An empty source_remote makes github_source false, so the code refuses the redirect. That result is safe. However, the refusal message does not show that the remote lookup failed. Only same-name redirects continue. This is fail-closed behaviour, so the impact is low. No change is required unless better diagnostics are wanted.


872-878: LGTM!

docs/Homebrew-Security-and-Supply-Chain.md (1)

79-83: LGTM!

Library/Homebrew/cmd/update.sh (1)

1030-1030: LGTM!

Library/Homebrew/test/cmd/update_spec.rb (1)

336-336: LGTM!

Also applies to: 350-353, 445-449

Library/Homebrew/cmd/update-report.rb (1)

102-170: LGTM!

Library/Homebrew/test/cmd/update-report_spec.rb (1)

37-37: LGTM!

Also applies to: 49-58, 103-201

Library/Homebrew/test/tap_spec.rb (1)

586-612: 🩺 Stability & Availability

The spec harness already restores ENV after each example. config.around snapshots ENV with ENV.to_hash at Library/Homebrew/test/spec_helper.rb:314-331, and its ensure calls ENV.replace(@__env) at Library/Homebrew/test/spec_helper.rb:397-398. The proposed with_env change is not needed.


📝 Summary

Summary by CodeRabbit

  • Security
    • Refused tap redirects that could change an official tap’s identity or conflict with existing trust settings. Redirects for renamed taps are allowed only when the destination meets the documented GitHub connection requirements.
    • Updated guidance explains how to proceed when a third-party tap rename is refused.
  • Bug Fixes
    • Prevented duplicate processing of tap redirects and improved recovery of local changes when a redirect is denied.
    • Corrected update reporting to capture the tap’s branch and revision details.

Walkthrough

Tap redirects now check the source remote and trust entries before changing a tap’s identity. Update processing passes repository branch context and restores repository state when it denies a redirect.

Changes

Tap Redirects

Layer / File(s) Summary
Redirect safety checks
Library/Homebrew/tap.rb, Library/Homebrew/test/tap_spec.rb, docs/Homebrew-Security-and-Supply-Chain.md
Redirect handling checks the actual Git origin, tap identity, and existing trust entries before applying a redirect. Tests and security guidance cover redirects that are allowed or refused.
Update branch context
Library/Homebrew/cmd/update.sh, Library/Homebrew/test/cmd/update_spec.rb
Update processing exports each repository’s upstream branch as HOMEBREW_UPDATE_BRANCH. The delegated-command test checks the branch and revision values.
Denied redirect recovery
Library/Homebrew/cmd/update-report.rb, Library/Homebrew/test/cmd/update-report_spec.rb
Update reporting processes each redirected tap path once. For denied redirects, it restores the target branch and remote-tracking ref while preserving local changes where possible. Tests cover alternate checked-out branches, stash conflicts, and rollback errors.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Update as homebrew-update
  participant Report as UpdateReport
  participant Tap as Tap
  participant Trust as Trust entries
  participant Git as Git repository
  Update->>Report: Pass repository branch context
  Report->>Tap: Apply redirected remote
  Tap->>Git: Read origin fetch URL
  Tap->>Trust: Check matching destination entries
  Tap-->>Report: Return redirect denial
  Report->>Git: Restore target branch and remote-tracking ref
Loading

Suggested reviewers: mikemcquaid

Merge Risk: ⚪ Minimal · up to a6a3c

The inspected redirect and rollback paths show no actionable issue requiring a fix before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a6a3c

The redirect policy strengthens protection against unintended trust inheritance. No introduced security defect was established, but recovery remains dependent on Git state and transport configuration, with some failures requiring manual repair.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The independently influenced source is a tap remote's redirect output. The sensitive outcomes are local checkout identity, origin and Git state, and whether formula, cask or command contents inherit trust. The inspected flow does not establish additional service, tenant or credential authority.

Trust Boundaries and Controls

  • observed — Remote-provided destination text is separated from locally resolved source identity. Environment allow/forbid checks remain in place; the new policy additionally blocks non-GitHub name changes, official identity transitions and matching destination trust for untrusted sources before moving the checkout or setting origin.

Resilience and Maintainability Implications

  • observed — Normal update concurrency is constrained by the existing update lock, but denied-redirect recovery consists of separate stash, reset and ref-update operations rather than one transaction. Caught failures provide manual recovery guidance, and the redirect file is removed when the handler exits; this is not evidence of automatic crash recovery.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main changes to tap redirect trust and rollback behavior.
Description check ✅ Passed The description directly explains the tap redirect security changes, rollback behavior, related fixes, tests, and validation steps.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Oct 9, 2026
Merged via the queue into main with commit bea322a Oct 9, 2026
54 checks passed
@MikeMcQuaid
MikeMcQuaid deleted the redirect-trust-provenance branch October 9, 2026 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants