Merge pull request #23610 from Homebrew/advisory-match-new-history

advisory-match: add --new-history to skip reviewed records

Author: MikeMcQuaid

Library/Homebrew/dev-cmd/advisory-match.rb

@@ -34,10 +34,15 @@ class AdvisoryMatch < AbstractCommand
         switch "--no-history",
                description: "Skip the `FormulaVersions` walk for the `fixed` " \
                             "boundary; use the current `pkg_version` instead."
+        switch "--new-history",
+               depends_on:  "--output=",
+               description: "Walk `FormulaVersions` only for records whose " \
+                            "reviewed ranges are not already in <directory>."
         conflicts "--all", "--index"
         conflicts "--all", "--json"
         conflicts "--index", "--json"
         conflicts "--index", "--output"
+        conflicts "--no-history", "--new-history"
 
         named_args [:formula]
 
@@ -64,7 +69,11 @@ def run
                   status, = matcher.range_status(hit)
                   next if status&.state == :not_applicable
 
-                  first_fixed = matcher.first_fixed_version(formula, hit) unless args.no_history?
+                  record_id = matcher.record_id(formula, hit)
+                  walk_history = !args.no_history? && status&.fixed?
+                  walk_history &&= emitter.history_required?(record_id) if args.new_history?
+                  emitter.record_history_walk if walk_history
+                  first_fixed = matcher.first_fixed_version(formula, hit) if walk_history
                   next if first_fixed == :never_affected
 
                   boundary = first_fixed if first_fixed.is_a?(String)
@@ -142,6 +151,12 @@ def report(matcher, formula, hits)
       # `--output` and text mode write per-record and only accumulate counts;
       # `--json` accumulates the array (single-formula / PR-bot use, so bounded).
       class Emitter
+        sig { params(_record_id: String).returns(T::Boolean) }
+        def history_required?(_record_id) = true
+
+        sig { void }
+        def record_history_walk; end
+
         sig { params(record: T::Hash[Symbol, T.untyped]).void }
         def <<(record); end
 
@@ -159,11 +174,12 @@ def initialize(dir, verbose:)
           @written = T.let(0, Integer)
           @unchanged = T.let(0, Integer)
           @skipped_generated = T.let(0, Integer)
+          @history_walks = T.let(0, Integer)
         end
 
         sig { override.params(record: T::Hash[Symbol, T.untyped]).void }
         def <<(record)
-          path = File.join(@dir, "#{record.fetch(:id)}.json")
+          path = record_path(record.fetch(:id))
           # A record already emitted by `generate-vulns-advisories` (a formula
           # `resolves` patch annotation) is more authoritative than a matched
           # candidate; overwriting it would drop `fix: "patch"` for a derived
@@ -182,6 +198,35 @@ def <<(record)
           @written += 1
         end
 
+        sig { override.params(record_id: String).returns(T::Boolean) }
+        def history_required?(record_id)
+          path = record_path(record_id)
+          return true unless File.file?(path)
+
+          existing = JSON.parse(File.read(path))
+          return true unless existing.is_a?(Hash)
+          return false if existing.dig("database_specific", "source") == "generated"
+
+          affected = existing["affected"]
+          return true unless affected.is_a?(Array)
+          return true if affected.empty?
+
+          affected.any? { |entry| !entry.is_a?(Hash) || !entry["ranges"] }
+        rescue JSON::ParserError
+          true
+        end
+
+        sig { override.void }
+        def record_history_walk
+          @history_walks += 1
+          puts "  #{@history_walks} history walks" if @verbose && (@history_walks % 100).zero?
+        end
+
+        sig { params(record_id: String).returns(String) }
+        def record_path(record_id)
+          File.join(@dir, "#{record_id}.json")
+        end
+
         sig { params(path: String).returns(T.nilable(String)) }
         def existing_source(path)
           JSON.parse(File.read(path)).dig("database_specific", "source")
@@ -192,7 +237,8 @@ def existing_source(path)
         sig { override.void }
         def finish
           Utils::Output.ohai "#{@written} records written to #{@dir} " \
-                             "(#{@unchanged} unchanged, #{@skipped_generated} generated left as-is)"
+                             "(#{@unchanged} unchanged, #{@skipped_generated} generated left as-is, " \
+                             "#{@history_walks} history walks)"
         end
       end
 

Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/advisory_match.rbi

@@ -66,6 +66,110 @@ def stub_osv_hit(cve, fixed:)
     end
   end
 
+  it "only walks history for records not already present with --new-history" do
+    stub_osv_hit("CVE-2024-1234", fixed: "2.28.1")
+
+    Dir.mktmpdir do |dir|
+      path = File.join(dir, "BREW-requests-CVE-2024-1234.json")
+      record = {
+        "schema_version"    => Homebrew::Vulns::OsvExport::SCHEMA_VERSION,
+        "id"                => "BREW-requests-CVE-2024-1234",
+        "modified"          => "2026-01-01T00:00:00Z",
+        "affected"          => [{
+          "package" => { "ecosystem" => "Homebrew", "name" => "requests" },
+          "ranges"  => [{ "type" => "ECOSYSTEM", "events" => [
+            { "introduced" => "0" }, { "fixed" => "2.28.1" }
+          ] }],
+        }],
+        "database_specific" => { "source" => "matched" },
+      }
+      File.write(path, JSON.generate(record))
+
+      matcher = Homebrew::Vulns::Match.new
+      allow(Homebrew::Vulns::Match).to receive(:new).and_return(matcher)
+      expect(matcher).not_to receive(:first_fixed_version)
+
+      expect { cmd_for("requests", "--output", dir, "--new-history").run }
+        .to output(/0 history walks/).to_stdout
+      expect(JSON.parse(File.read(path)).dig("affected", 0, "ranges", 0, "events", 1))
+        .to eq("fixed" => "2.28.1")
+    end
+  end
+
+  it "uses the historical boundary for a new record with --new-history" do
+    stub_osv_hit("CVE-2024-1234", fixed: "2.28.1")
+    matcher = Homebrew::Vulns::Match.new
+    expect(matcher).to receive(:first_fixed_version).and_return("2.28.1")
+    allow(Homebrew::Vulns::Match).to receive(:new).and_return(matcher)
+
+    Dir.mktmpdir do |dir|
+      expect { cmd_for("requests", "--output", dir, "--new-history").run }
+        .to output(/1 history walks/).to_stdout
+      path = File.join(dir, "BREW-requests-CVE-2024-1234.json")
+      expect(JSON.parse(File.read(path)).dig("affected", 0, "ranges", 0, "events", 1))
+        .to eq("fixed" => "2.28.1")
+    end
+  end
+
+  it "walks history when an existing matched record has no ranges" do
+    stub_osv_hit("CVE-2024-1234", fixed: "2.28.1")
+    matcher = Homebrew::Vulns::Match.new
+    expect(matcher).to receive(:first_fixed_version).and_return("2.28.1")
+    allow(Homebrew::Vulns::Match).to receive(:new).and_return(matcher)
+
+    Dir.mktmpdir do |dir|
+      path = File.join(dir, "BREW-requests-CVE-2024-1234.json")
+      File.write(path, JSON.generate({
+        "id"                => "BREW-requests-CVE-2024-1234",
+        "affected"          => [{ "package" => { "ecosystem" => "Homebrew", "name" => "requests" } }],
+        "database_specific" => { "source" => "matched" },
+      }))
+
+      cmd_for("requests", "--output", dir, "--new-history").run
+      expect(JSON.parse(File.read(path)).dig("affected", 0, "ranges", 0, "events", 1))
+        .to eq("fixed" => "2.28.1")
+    end
+  end
+
+  it "walks history when an existing record is malformed" do
+    stub_osv_hit("CVE-2024-1234", fixed: "2.28.1")
+    matcher = Homebrew::Vulns::Match.new
+    expect(matcher).to receive(:first_fixed_version).and_return("2.28.1")
+    allow(Homebrew::Vulns::Match).to receive(:new).and_return(matcher)
+
+    Dir.mktmpdir do |dir|
+      path = File.join(dir, "BREW-requests-CVE-2024-1234.json")
+      File.write(path, "{")
+
+      cmd_for("requests", "--output", dir, "--new-history").run
+      expect(JSON.parse(File.read(path)).dig("affected", 0, "ranges", 0, "events", 1))
+        .to eq("fixed" => "2.28.1")
+    end
+  end
+
+  it "does not walk history for a new record with --no-history" do
+    stub_osv_hit("CVE-2024-1234", fixed: "2.28.1")
+    matcher = Homebrew::Vulns::Match.new
+    expect(matcher).not_to receive(:first_fixed_version)
+    allow(Homebrew::Vulns::Match).to receive(:new).and_return(matcher)
+
+    Dir.mktmpdir do |dir|
+      cmd_for("requests", "--output", dir, "--no-history").run
+    end
+  end
+
+  it "does not count a history walk for a new record that is still affected" do
+    stub_osv_hit("CVE-2024-1234", fixed: "2.32.0")
+    matcher = Homebrew::Vulns::Match.new
+    expect(matcher).not_to receive(:first_fixed_version)
+    allow(Homebrew::Vulns::Match).to receive(:new).and_return(matcher)
+
+    Dir.mktmpdir do |dir|
+      expect { cmd_for("requests", "--output", dir, "--new-history").run }
+        .to output(/0 history walks/).to_stdout
+    end
+  end
+
   it "drops :not_applicable hits instead of emitting them as open ranges" do
     allow(Homebrew::Vulns::OSV).to receive(:query_batch).and_return([[{ "id" => "CVE-2024-1234" }], []])
     allow(Homebrew::Vulns::OSV).to receive(:vulnerability).with("CVE-2024-1234").and_return(
@@ -88,7 +192,11 @@ def stub_osv_hit(cve, fixed:)
                                        "database_specific" => { "source" => "generated" },
                                        "affected"          => [{ "ecosystem_specific" => { "fix" => "patch" } }] }))
 
-      expect { cmd_for("requests", "--output", dir, "--no-history").run }
+      matcher = Homebrew::Vulns::Match.new
+      expect(matcher).not_to receive(:first_fixed_version)
+      allow(Homebrew::Vulns::Match).to receive(:new).and_return(matcher)
+
+      expect { cmd_for("requests", "--output", dir, "--new-history").run }
         .to output(/0 records written.*1 generated left as-is/).to_stdout
       expect(JSON.parse(File.read(path)).dig("affected", 0, "ecosystem_specific", "fix")).to eq "patch"
     end
@@ -160,6 +268,16 @@ def stub_osv_hit(cve, fixed:)
     expect { described_class.new(["--all", "--json"]) }.to raise_error(UsageError, /mutually exclusive/)
   end
 
+  it "requires --output with --new-history" do
+    expect { described_class.new(["requests", "--new-history"]) }
+      .to raise_error(UsageError, /--new-history.*--output/)
+  end
+
+  it "rejects --new-history with --no-history" do
+    expect { described_class.new(["requests", "--output", "out", "--new-history", "--no-history"]) }
+      .to raise_error(UsageError, /mutually exclusive/)
+  end
+
   it "emits the formula-identity index with --index" do
     requests
     core_tap = instance_double(CoreTap, installed?: true, name: "homebrew/core", formula_names: ["requests"])

Library/Homebrew/test/dev-cmd/advisory-match_spec.rb

@@ -562,6 +562,8 @@ def registry_hit(affected_events:, subject_version: "2.31.0", resource: nil, nam
       record = matcher.to_brew_record(requests, hit, now:)
 
       expect(record[:id]).to eq "BREW-requests-CVE-2024-1234"
+      expect(matcher.record_id(requests, hit))
+        .to eq Homebrew::Vulns::OsvExport.record_for(requests, "CVE-2024-1234", now:)[:id]
       expect(record[:upstream]).to eq ["CVE-2024-1234", "GHSA-abcd"]
       expect(record[:severity]).to eq [{ "type" => "CVSS_V3", "score" => "..." }]
       expect(record[:references]).to eq [{ "type" => "ADVISORY", "url" => "https://x" }]

Library/Homebrew/test/vulns/match_spec.rb

@@ -497,7 +497,7 @@ def to_brew_record(formula, hit, first_fixed: nil, now: Time.now.utc)
 
         record = T.let({
           schema_version:    OsvExport::SCHEMA_VERSION,
-          id:                "#{OsvExport::ID_PREFIX}-#{formula.name}-#{hit.canonical_id}",
+          id:                record_id(formula, hit),
           published:         timestamp,
           modified:          timestamp,
           upstream:          vuln.identifiers,
@@ -520,6 +520,11 @@ def to_brew_record(formula, hit, first_fixed: nil, now: Time.now.utc)
         record
       end
 
+      sig { params(formula: Formula, hit: Hit).returns(String) }
+      def record_id(formula, hit)
+        OsvExport.record_id(formula, hit.canonical_id)
+      end
+
       sig {
         params(hit: Hit, status: T.nilable(Vulnerability::RangeStatus)).returns(String)
       }

Library/Homebrew/vulns/match.rb

@@ -59,7 +59,7 @@ def self.run(annotated, dir, first_fixed: nil, now: Time.now.utc)
         annotated.each do |formula, patches|
           Scanner.resolved_ids(patches).each do |vuln_id|
             upstream = upstream_cache.fetch(vuln_id) { upstream_cache[vuln_id] = fetch_upstream(vuln_id) }
-            path = File.join(dir, "#{ID_PREFIX}-#{formula.name}-#{vuln_id}.json")
+            path = File.join(dir, "#{record_id(formula, vuln_id)}.json")
             existing = File.file?(path)
             # A transient OSV outage would otherwise strip summary/severity/etc.
             # from an existing enriched record; leave it untouched instead.
@@ -123,7 +123,7 @@ def self.record_for(formula, vuln_id, patches: formula.serialized_patches,
         timestamp = now.strftime("%Y-%m-%dT%H:%M:%SZ")
         record = T.let({
           schema_version:    SCHEMA_VERSION,
-          id:                "#{ID_PREFIX}-#{formula.name}-#{vuln_id}",
+          id:                record_id(formula, vuln_id),
           published:         timestamp,
           modified:          timestamp,
           upstream:          [vuln_id],
@@ -150,6 +150,11 @@ def self.record_for(formula, vuln_id, patches: formula.serialized_patches,
         record
       end
 
+      sig { params(formula: Formula, vuln_id: String).returns(String) }
+      def self.record_id(formula, vuln_id)
+        "#{ID_PREFIX}-#{formula.name}-#{vuln_id}"
+      end
+
       sig {
         params(formula: Formula, vuln_id: String, patches: T::Array[T::Hash[String, T.untyped]], fixed: String)
           .returns(T::Hash[Symbol, T.untyped])

Library/Homebrew/vulns/osv_export.rb

@@ -344,6 +344,7 @@ _brew_advisory_match() {
       --help
       --index
       --json
+      --new-history
       --no-history
       --output
       --quiet

completions/bash/brew

@@ -339,6 +339,7 @@ __fish_brew_complete_arg 'advisory-match' -l debug -d 'Display any debugging inf
 __fish_brew_complete_arg 'advisory-match' -l help -d 'Show this message'
 __fish_brew_complete_arg 'advisory-match' -l index -d 'Emit the formula-identity index as JSON and exit'
 __fish_brew_complete_arg 'advisory-match' -l json -d 'Output candidate records as a JSON array'
+__fish_brew_complete_arg 'advisory-match' -l new-history -d 'Walk `FormulaVersions` only for records whose reviewed ranges are not already in directory'
 __fish_brew_complete_arg 'advisory-match' -l no-history -d 'Skip the `FormulaVersions` walk for the `fixed` boundary; use the current `pkg_version` instead'
 __fish_brew_complete_arg 'advisory-match' -l output -d 'Write each record to directory as `BREW-formula-id.json`, preserving existing `published`/`ranges` fields'
 __fish_brew_complete_arg 'advisory-match' -l quiet -d 'Make some output more quiet'

completions/fish/brew.fish

@@ -442,7 +442,8 @@ _brew_advisory_match() {
     '--help[Show this message]' \
     '(--all --json --output)--index[Emit the formula-identity index as JSON and exit]' \
     '(--all --index)--json[Output candidate records as a JSON array]' \
-    '--no-history[Skip the `FormulaVersions` walk for the `fixed` boundary; use the current `pkg_version` instead]' \
+    '(--no-history)--new-history[Walk `FormulaVersions` only for records whose reviewed ranges are not already in directory]' \
+    '(--new-history)--no-history[Skip the `FormulaVersions` walk for the `fixed` boundary; use the current `pkg_version` instead]' \
     '(--index)--output[Write each record to directory as `BREW-formula-id.json`, preserving existing `published`/`ranges` fields]' \
     '--quiet[Make some output more quiet]' \
     '--repology[Load the formula to distro-package index from file instead of the published `data/repology.json`]' \