Reject path components in internal command names - #23831
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
馃煛 Changes recommended
Internal-command paths remain reachable through validation gaps, and rejected names can still trigger developer-command side effects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens internal command resolution by rejecting command names containing path components.
Changes:
- Adds Ruby and Bash command-name validation.
- Adds regression tests for invalid internal command names.
File summaries
| File | Review |
|---|---|
Library/Homebrew/test/commands_spec.rb |
Adds validator tests, but lacks exact . and .. cases. |
Library/Homebrew/commands.rb |
Adds validation, but Commands.path still bypasses it. |
Library/Homebrew/brew.sh |
Adds dispatch guards, but validation occurs after side effects and still accepts exact . and ... |
Review details
Suppressed comments (2)
Library/Homebrew/brew.sh:597
- The developer-command predicate also accepts
.and.., so it is not in sync withvalid_internal_dev_cmd?and can selectdev-cmd/..shordev-cmd/...sh. Apply the same explicit exclusions in this branch.
elif [[ -z "${HOMEBREW_FORCE_RUBY_COMMAND:-}" && "${HOMEBREW_COMMAND}" != */* &&
-f "${HOMEBREW_LIBRARY}/Homebrew/dev-cmd/${HOMEBREW_COMMAND}.sh" ]]
Library/Homebrew/test/commands_spec.rb:48
- The new examples only pass
../other, so they exercise the slash short-circuit but never the newly added./..branch. Add parameterised cases for the exact dot components for both validators; otherwise removing%w[. ..].include?(cmd)leaves this test suite green.
expect(described_class.valid_internal_cmd?("../other")).to be(false)
end
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
馃挕 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Patrick Linnane <patrick@linnane.io>
p-linnane
force-pushed
the
internal-command-containment
branch
from
September 6, 2026 00:59
bda140a to
76fa15e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both
brew.shandCommands.valid_internal_cmd?resolved internal command names by joining them onto thecmdanddev-cmddirectories, so a name containing path components could select a file outside those directories before any tap trust check ran. Reject names containing/,.or..inbrew.shas soon as the command name is known, before the developer-command classification, and in one Ruby check behindvalid_internal_cmd?,valid_internal_dev_cmd?andCommands.path, with two-way comments keeping both in sync.brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?GPT-6 Astra and Claude Code (Fable 5.1) drafted the implementation and tests; I reviewed the diff, verified the new tests fail without the change and pass with it, and ran
brew lgtm --onlineplus targeted specs.