vulns: add --fix-available and --no-fix-available flags - #23152
Conversation
|
@andrew may be interested in this as per https://github.com/orgs/Homebrew/discussions/6965#discussioncomment-17664522 |
There was a problem hiding this comment.
Pull request overview
This PR extends brew vulns with new filtering options so users can restrict vulnerability results based on whether an upstream fixed version exists, and improves patch resolution classification to recognize OSV-style IDs as security issues.
Changes:
- Add
--fix-available/--no-fix-availableflags tobrew vulns(mutually exclusive) and plumb them into the scanner. - Update the vulnerability scanner to filter findings based on presence/absence of
fixed_versions. - Treat
OSV-YYYY-NNN...identifiers as “security” inPatch.resolves_type, with corresponding test coverage and Sorbet RBI updates.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/vulns/scanner.rb | Add only_fixed / except_fixed options and apply fix-availability filtering during scans. |
| Library/Homebrew/cmd/vulns.rb | Define new CLI switches, enforce mutual exclusion, and pass options through to the scanner. |
| Library/Homebrew/test/vulns/scanner_spec.rb | Add unit tests validating scanner filtering behavior for fix availability. |
| Library/Homebrew/test/cmd/vulns_spec.rb | Add unit tests ensuring CLI flags are passed through and conflicts are rejected. |
| Library/Homebrew/patch.rb | Add OSV_PATTERN and include it in resolves_type security classification. |
| Library/Homebrew/test/patch_spec.rb | Extend resolves-type tests to include OSV identifiers as security issues. |
| Library/Homebrew/sorbet/rbi/dsl/homebrew/cmd/vulns.rbi | Add RBI methods for the new CLI switches (fix_available?, no_fix_available?). |
Files not reviewed (1)
- Library/Homebrew/sorbet/rbi/dsl/homebrew/cmd/vulns.rbi: File type not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
andrew
left a comment
There was a problem hiding this comment.
Thanks for working on this. I found two correctness gaps that should be addressed before merge.
Finding 1 — The fix-availability filter needs to be relative to the matched version/range, rather than checking whether the record has any fixed event. OSV permits multiple status intervals in one range. For example, introduced 0, fixed 1.0.0, introduced 2.0.0 leaves 2.1.0 affected with no fix, but fixed_versions.any? is true, so --fix-available includes it and --no-fix-available hides it. Please make this target-version aware and add coverage for a reopened, open-ended interval.
Finding 2 — Please keep the FormulaAudit/Patches validation in sync with the new OSV identifier classification. It currently accepts only CVE, GHSA, or URL values, so resolves "OSV-2023-298" is still rejected by brew audit/brew style and this new path cannot be used in a formula. Please update the cop and its specs, along with the user-facing documentation/messages, to accept OSV IDs.
424cdfb to
a76ef3e
Compare
There was a problem hiding this comment.
Thanks for the follow-up. The OSV identifier audit/style path is now in sync, and the reopened SEMVER coverage is useful. I found three remaining issues that should be addressed before merge.
Finding 1 — The fix-availability filter still needs to be target-aware for GIT ranges. Scanner queries OSV with ecosystem: "GIT", but fix_available? handles every non-SEMVER range by returning true when that range contains any fixed event. For introduced aaa, fixed bbb, introduced ccc, a target matched in the final open interval is therefore still included by --fix-available and hidden by --no-fix-available. Please avoid treating a record-wide/non-SEMVER fixed event as the fix for the current match. One option is to carry or derive enough match context to identify the GIT interval that OSV matched; if that cannot be determined safely from the response, represent it as unknown/no known fix instead of returning true. Please add a regression using a reopened GIT range whose current matched interval has no closing fixed event.
Finding 2 — last_affected should not be treated as a fixed version. The last_affected branch stores its boundary in Interval#fix, so fix_available? returns true even though the vulnerability has no fixed event and fixed_versions is empty. last_affected is an inclusive affected boundary, not the version containing a fix. Please keep the inclusive upper bound but set fix: nil for last_affected; only the fixed branch should populate Interval#fix. A focused spec should assert that an affected target ending at last_affected has fix_available? == false.
Finding 3 — Please regenerate the manpage and shell completions. The new switches are absent from the generated command documentation and completions. Running ./bin/brew generate-man-completions --no-exit-code produces updates to docs/Manpage.md, manpages/brew.1, and the Bash, Fish, and Zsh completions (20 additions across five files). Please run ./bin/brew generate-man-completions and commit those generated outputs with the source changes.
|
Regarding Finding 3, that likely should be a ci check. Making note of that, will address this all soon. |
aa80fc5 to
2c35518
Compare
|
One thing I noticed was the git hashes provided by OSV.dev for fixes may or may not be in any given release, so this still may have false positive/negatives if the data only has a commit hash but no fixed semver. Edit: I think that's a different way of saying Andrew's Finding 1 above. Will think more on this. |
|
I did include the ci change here, let me know if I should back that out and prep it for a separate PR. |
9dfb73e to
2c1f6c3
Compare
2c1f6c3 to
5ecea1f
Compare
andrew
left a comment
There was a problem hiding this comment.
Thanks, the previous three points are all addressed. A few remaining items.
CI is red on brew style: test/vulns/scanner_spec.rb:634 is 136 chars (limit 118). Shortening the it description to something like "treats a reopened GIT range with no closing fixed event as no fix available" would fix it.
In Vulnerability#fix_available?, the first semver_ranges.each { in_semver_range?(...) } loop that sets version_matched computes the same thing as the second semver_ranges.each { intervals(...).each { in_interval?(...) } } loop, so intervals(events) runs twice per range. The first loop can go, with version_matched ||= matched_semver_range set after the second (or fold both into one pass).
Also in fix_available?, the final ranges.each block after next if ranges.none? is only reachable when the target matched via the versions list but sits outside every SEMVER interval, i.e. the record's versions and ranges disagree. In that path it returns true if any interval anywhere has a fix, which is the record-wide fixed-event behaviour flagged in the first review (a fix at 1.0.0 is not a fix for an affected 2.1.0). It also re-calls non_semver_fix_available? on ranges already checked just above, and calls it on SEMVER ranges too. Dropping that block and the next if ranges.none? guard and falling through to false is simpler and the conservative answer for inconsistent data. Minor nit in the same method: versions.any? && versions.any? { ... } — the outer guard is redundant.
The .github/workflows/autogenerated-files.yml change widening the path trigger to cmd/**, dev-cmd/**, cli/parser.rb, completions.rb and env_config.rb is a good idea but orthogonal to this feature and changes CI behaviour for every command PR. Please back it out here and open it separately so it can land and be reverted independently.
|
CI changes moved to (Draft) #23258, repushing here shortly. |
1a95be8 to
f777b38
Compare
andrew
left a comment
There was a problem hiding this comment.
Thanks, the previous review points are addressed. I found two remaining correctness issues.
Finding 1: fix_available? needs to scope affected entries and ranges to the repository queried. It currently marks every non-SEMVER range as matched (version_matched = true if non_semver_ranges.any?) and returns true for the first one with a fix. OSV records can describe several packages. I reproduced this with CVE-2021-4044: for openssl-3.0.0, the fetched record lists a fixed Node.js range before an OpenSSL range ending in last_affected, and the current code returns fix_available?("openssl-3.0.0") == true based on the Node.js fix. Please pass the target repository context into the matcher, ignore unrelated affected entries and ranges, and add a regression using a multi-package record.
Finding 2: SemVer comparison failures must remain unknown or no known fix. in_interval? intentionally rescues and returns true for vulnerability matching, but fix_available? reuses it and sets fix_found whenever the interval has a fixed event. A target such as openssl-3.0.0 against a SEMVER range from introduced: 0 to fixed: 3.0.1 therefore returns true even though the target cannot be ordered. Please use a strict or tri-state interval check for fix availability, return false for uncomparable targets or bounds, and add a focused regression.
97cf2a3 to
d44c33a
Compare
|
Latest Finding 1: I don't have an example off hand, but in the past I've noticed CVE database is sometimes inconsistent about how it represents that. In some cases, it's the combination of the two (you need both packages installed), and in other cases, it is a disjunction (you need either package installed). I don't think that's something we can address here given it is an upstream metadata issue, but we can address the core part of the finding. |
d44c33a to
0a00d44
Compare
|
I think the 1 failing test is a flaky test. Working on a deterministic version on another branch. |
andrew
left a comment
There was a problem hiding this comment.
Thanks, both previous findings are addressed: in_interval_strict? correctly returns nil on Uncomparable and fix_available? skips those results, and affected_entry_relevant? with the repo_url parameter handles the CVE-2021-4044 case. A few remaining items before merge.
Finding 1: affected_entry_relevant? treats any entry with no GIT ranges as relevant (return true if git_ranges.empty?). A record aliased across ecosystems, for example a GHSA listing both a GIT range for the C library and a SEMVER or ECOSYSTEM range for a PyPI/npm wrapper, would still let the wrapper's fixed event leak through, since the wrapper entry has git_ranges.empty? and its SEMVER bounds are numerically comparable to a tag like 3.0.0. This is the same class as the previous repo-scoping finding in a different shape. Please also skip entries whose package.ecosystem is set to something other than GIT, e.g. return false if (eco = aff.dig("package", "ecosystem")) && eco != "GIT" ahead of the GIT-range check.
Finding 2: affected_entry_relevant? normalises with .downcase.delete_suffix(".git") but not a trailing /. OSV repo values are not consistent about trailing slashes, so https://github.com/openssl/openssl/ would fail to match https://github.com/openssl/openssl. Please add .chomp("/") to both sides of the comparison.
Finding 3: test/cmd/vulns_spec.rb line 145, the regex /only_fixed and except_fixed cannot be both true|conflicts|conflicts with|mutually exclusive/ still references a scanner-level error message that no longer exists. Homebrew's conflicts mechanism raises with "mutually exclusive"; please tighten the assertion to match that or the flag names rather than carrying dead alternatives.
Finding 4: the AI checkbox is ticked but the template asks for an explanation of how AI was used and how the changes were verified. Please add a short note under the checkbox.
|
66b1600 to
f0c8835
Compare
26a5cfa to
88746e9
Compare
|
I attempted to rebase since it's not liking the commit format workflow now. Not sure if that's due to the merge commit @andrew added or something else. I can squash locally and force push if that's helpful. |
|
The Fix is in flight at #23402 (paired with Homebrew/actions#905). Nothing to change on your side; your squashed commit is fine. I'll merge once the workflow fix is in or override the check. |
brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?This PR was developed with AI assistance (Antigravity / Gemini) to identify the appropriate portions of the code base to edit, suggest changes, write corresponding RSpec tests, verify them locally, and assess feedback on the PR.