vulns: include OSS-Fuzz patch fixes as GIT ecosystem, add additional "fixed" filtering (released vs. patched), and --list-skipped - #23511
Conversation
|
I can decompose this into 3 separate PRs if you'd like, just thought I'd share where I am on it. Will mark as ready for review once CI passes and I look everything over one more time. cc: @andrew |
|
Note to self: Wondering if we should factor in outdated status here, too... i.e. if outdated and released fix is already available within brew, that should be an easy fix. |
| __fish_brew_complete_arg 'vulns' -l debug -d 'Display any debugging information' | ||
| __fish_brew_complete_arg 'vulns' -l deps -d 'Also check the dependencies of named formulae' | ||
| __fish_brew_complete_arg 'vulns' -l fix-available -d 'Only report vulnerabilities that have a fix available. Note that this may exclude vulnerabilities with fixes available if we cannot determine that the fix is included in the version under consideration' | ||
| __fish_brew_complete_arg 'vulns' -l fix-available -d 'Only report vulnerabilities that have a released version fix available. Shortcut for `--fix-type=released`' |
There was a problem hiding this comment.
We may want to have this be an alias for --fix-type=any, but thought this would be more actionable for a majority of end users.
| fixed = "04656d7450e229622546fd2b11496aa58c44181d" | ||
| v = vuln("id" => "OSV-2023-298", | ||
| "affected" => [{ "package" => { "name" => "cairo", "ecosystem" => "OSS-Fuzz" }, | ||
| "ranges" => [{ "type" => "GIT", |
There was a problem hiding this comment.
This made the output more actionable for myself, though @andrew may have reasoning for excluding non-GIT ecosystems.
| def non_semver_fix_available?(target, range) | ||
| sig { params(val: String).returns(T::Boolean) } | ||
| def self.commit_sha?(val) | ||
| val.match?(/\A[0-9a-f]{6,40}\z/i) && !val.match?(/\A\d+\z/) |
There was a problem hiding this comment.
This feels brittle... if we don't like this, I can figure out another approach.
| end | ||
|
|
||
| ecosystem = aff.dig("package", "ecosystem") | ||
| ecosystem.blank? || ecosystem == "GIT" || ecosystem == "OSS-Fuzz" |
There was a problem hiding this comment.
I think we'll want to lift this to a set of GIT ecosystems once we have a third...
|
I consider my comments non-blocking, just areas where I'm particularly looking for feedback. |
There was a problem hiding this comment.
Pull request overview
This pull request updates brew vulns to better classify and filter vulnerability “fixes” (released versions vs unreleased commit-SHA patches), includes OSS-Fuzz records that provide GIT ranges, and adds an option to list skipped packages for transparency.
Changes:
- Add
--fix-typewith released/patch/any/none/unreleased filtering, and redefine--fix-available/--no-fix-availableas shortcuts for released/unreleased filtering. - Treat OSS-Fuzz affected entries with
GITranges as relevant for repo matching (instead of being skipped by ecosystem checks). - Add
--list-skipped(and document-v/--verbose) to print names of skipped packages in text output, plus update completions/docs/manpage and add unit tests.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| manpages/brew.1 | Documents new brew vulns flags and updated fix semantics. |
| docs/Manpage.md | Mirrors manpage updates for --fix-type and skipped listing options. |
| completions/zsh/_brew | Adds completions for --fix-type / --list-skipped and updates descriptions. |
| completions/fish/brew.fish | Adds completions for --fix-type / --list-skipped and updates descriptions. |
| completions/bash/brew | Adds --fix-type / --list-skipped to bash completion options list. |
| Library/Homebrew/vulns/vulnerability.rb | Adds SHA-vs-release fix classification, new fix-type predicates, and OSS-Fuzz GIT-range relevance handling. |
| Library/Homebrew/vulns/scanner.rb | Replaces only/exclude-fixed booleans with fix_type filtering and tracks skipped formula names. |
| Library/Homebrew/vulns/output.rb | Adds list_skipped: to text output to optionally print skipped package names. |
| Library/Homebrew/cmd/vulns.rb | Adds CLI flags, conflict rules, fix_type parsing/validation, and passes list_skipped to text output. |
| Library/Homebrew/sorbet/rbi/dsl/homebrew/cmd/vulns.rbi | Regenerates RBI to include new args (fix_type, list_skipped). |
| Library/Homebrew/test/vulns/vulnerability_spec.rb | Adds tests for released-vs-patch fix classification and OSS-Fuzz GIT ranges. |
| Library/Homebrew/test/vulns/scanner_spec.rb | Updates tests for new fix_type filtering behavior and adds coverage for released vs patch filters. |
| Library/Homebrew/test/vulns/output_spec.rb | Adds coverage for listing skipped package names in text output. |
| Library/Homebrew/test/cmd/vulns_spec.rb | Updates option-passing tests for fix_type, validates --fix-type, and checks --list-skipped plumbing. |
| Library/Homebrew/test/formula_installer_spec.rb | Minor test refactor to use described_class. |
Files not reviewed (1)
- Library/Homebrew/sorbet/rbi/dsl/homebrew/cmd/vulns.rbi: File type not supported
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
72884ba to
ccbb6ef
Compare
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
Thanks for this. Re: splitting it up, I'm happy keeping it as one PR as long as you don't mind a longer review thread. The OSS-Fuzz fix and --fix-type split are both things I want.
Main asks are the RBI regeneration and swapping the SHA regex for the range type; everything else is minor.
(PR title says --show-skipped, flag is --list-skipped.)
| def non_semver_fix_available?(target, range) | ||
| sig { params(val: String).returns(T::Boolean) } | ||
| def self.commit_sha?(val) | ||
| val.match?(/\A[0-9a-f]{6,40}\z/i) && !val.match?(/\A\d+\z/) |
There was a problem hiding this comment.
Agreed it's brittle, and I think the regex is the wrong signal. The OSV schema already encodes this: type: GIT range events are commits, type: SEMVER / type: ECOSYSTEM events are versions. So:
- In
non_semver_fix_available_of_type?,is_sha = (range["type"] == "GIT")instead of pattern-matching the value.rangeis already in scope. - In the
semver_ranges.eachloop insidefix_available_of_type?, drop the SHA check entirely; a SEMVERfixedis a version by definition, so that branch reduces tofix_found = true if type != :patch.
Then commit_sha? can go.
| return false unless last_event.key?("fixed") | ||
|
|
||
| last_event.key?("fixed") | ||
| fix = last_event["fixed"].to_s |
There was a problem hiding this comment.
Same empty-string case Copilot flagged, missed in this fallback path: {"fixed" => ""} gives is_sha = false, so :released (and :any) return true. Needs a return false if fix.blank? after the .to_s.
Goes away for :released/:patch if the range-type approach above is taken, but :any would still return true on a blank fix without the guard.
| end | ||
|
|
||
| ecosystem = aff.dig("package", "ecosystem") | ||
| ecosystem.blank? || ecosystem == "GIT" || ecosystem == "OSS-Fuzz" |
There was a problem hiding this comment.
This is slightly broader than the PR description: OSS-Fuzz entries with no GIT ranges at all are now also treated as relevant (previously rejected). I think that's fine since OSS-Fuzz records essentially always carry a GIT range, just flagging that it's not strictly "OSS-Fuzz records containing GIT ranges". Happy to keep as-is.
| sig { | ||
| params(findings: T::Array[Finding], checked: Integer, skipped: Integer, | ||
| params(findings: T::Array[Finding], checked: Integer, | ||
| skipped: T.any(Integer, T::Array[Formula], T::Array[String]), |
There was a problem hiding this comment.
The three-way union only exists so the handful of Results.new(..., skipped: 0) test call sites keep working. I'd rather make this T::Array[String] only, derive the count from .size, and update those callers to skipped: []. It's about six sites in test/cmd/vulns_spec.rb and test/vulns/output_spec.rb. Not blocking if you'd prefer to keep the diff small.
| description: "Filter findings by fix type: `released` (official version release), " \ | ||
| "`patch` (unreleased commit SHA), `any` (either), `none` (neither), " \ | ||
| "`unreleased` (no released version fix)." | ||
| switch "--list-skipped", |
There was a problem hiding this comment.
This is silently a no-op with --json since it's only wired into Output.text. Given the goal is actionable output for tooling, adding skipped_formulae to the JSON payload would be more useful than a doc caveat. Either is fine though.
kmarekspartz
changed the title
--show-skipped--list-skipped
- Add `--fix-type` option (`released`, `patch`, `any`, `none`, `unreleased`) to filter findings by fix type. - Update `--fix-available` and `--no-fix-available` as shortcuts for `--fix-type=released` and `--fix-type=unreleased`. - Add `--list-skipped` (`-v` / `--verbose`) flag to list packages skipped during scanning due to missing or unsupported source URLs. - Evaluate `GIT` ranges attached to `OSS-Fuzz` ecosystem records when matching git repository URLs. - Update shell completions, manpages, and unit tests.
ccbb6ef to
00e04e7
Compare
|
Addressed trivial fixes (rbi regen, unrelated spec change, return if fix.blank?) will take on more in the coming days. |
brewcommands to reproduce the bug?brew install cairo && brew vulns --no-fix-available | grep Fixedpreviously printedFixed in: <commit_sha>lines because OSV records withpackage.ecosystemset to"OSS-Fuzz"were bypassed by anecosystem != "GIT"check despite containing matchingGITranges.
brew lgtm(style, typechecking and tests) locally?What does this PR do?
--fix-typefiltering tobrew vulns:--fix-type=<type>with choices:released,patch,any,none, andunreleased.--fix-availableand--no-fix-availableto act as shortcuts for--fix-type=releasedand--fix-type=unreleased.--list-skipped(-v/--verbose):GITRange Evaluation forOSS-FuzzRecords:affected_entry_relevant?to evaluateGITranges regardless of whetherpackage.ecosystemis"GIT"or"OSS-Fuzz".Maintenance:
docs/Manpage.mdandmanpages/brew.1.--fix-type,--list-skipped, fix classification, andOSS-FuzzGITranges acrossVulnerability,Scanner,Output, andCmd::Vulns.Why are these changes included?
Running
brew vulns --no-fix-availablepreviously reported vulnerabilities listing git commit SHA patches underFixed in: <commit_sha>becauseaffected_entry_relevant?bypassedOSS-Fuzzrecords containingGITranges due to anecosystem != "GIT"check.Furthermore, because a commit SHA patch is an unreleased git commit, maintainers cannot resolve the vulnerability simply by bumping the formula version.
This PR fixes range evaluation and introduces
--fix-typeto make fix classification explicit:--fix-available(shortcut for--fix-type=released): Filters for vulnerabilities with an official version release fix available (excluding commit SHA patches).--no-fix-available(shortcut for--fix-type=unreleased): Filters for vulnerabilities without an official version release fix (including those with commit SHA patches).--fix-type=patch: Explicitly filters for vulnerabilities that have git commit SHA patches available.--fix-type=any/--fix-type=none: Provides broad fix filtering across all fix types (nonereproducesmain's--no-fix-availablebehavior).--list-skipped(-v/--verbose): Provides transparency into which specific formulae were skipped during scanning due to missing or unsupported source URLs.These changes enable actionable feedback to contributors about whether formulae need version bumps, patches applied, or source URLs identified.
AI Tool Used: Gemini 3.6 Flash (Medium) via Antigravity CLI.
Verification: Verified all typechecks, style linting, and unit tests locally via
brew lgtm --online.I did manual code review as well.