Conversation
…e the same product name
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request addresses nondeterministic CPE matching for the Line messaging app. Changes include adding an ORDER BY clause (vendor, product) to multiple CPE database queries to ensure consistent row ordering. A new helper function checks if a software vendor appears within a CPE vendor field. The CPE selection logic is refactored to collect all matching candidates and apply a vendor-match heuristic to select the best one. Additional test cases and test data validate the vendor-matching behavior and deterministic ordering. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/vulnerabilities/nvd/cpe.go (1)
182-188: Consider lowercasingitem.Vendorfor defensive consistency.The function lowercases
software.Vendorbut notitem.Vendor. While CPE vendors in the NVD database are typically lowercase, explicitly lowercasing both sides would make this more robust.♻️ Suggested improvement
func cpeVendorMatchesSoftware(item *IndexedCPEItem, software *fleet.Software) bool { sVendor := strings.ToLower(software.Vendor) - return sVendor != "" && strings.Contains(sVendor, item.Vendor) + return sVendor != "" && strings.Contains(sVendor, strings.ToLower(item.Vendor)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/vulnerabilities/nvd/cpe.go` around lines 182 - 188, cpeVendorMatchesSoftware currently lowercases software.Vendor but not item.Vendor, which can cause mismatches; update the function (cpeVendorMatchesSoftware) to compare lowercased values for both sides by computing e.g. sVendor := strings.ToLower(software.Vendor) and iVendor := strings.ToLower(item.Vendor) and then use strings.Contains(sVendor, iVendor) so the comparison is case-insensitive and more robust when matching IndexedCPEItem.Vendor against fleet.Software.Vendor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/vulnerabilities/nvd/cpe_test.go`:
- Line 1820: Add a Homebrew-to-CPython translation rule so Homebrew python
packages map to vendor "python" instead of Microsoft's python; create a rule
matching software.name /^python(@.*)?$/ with source "homebrew_packages" and a
filter that sets product:["python"] and vendor:["python"] (matching the JSON
snippet in the review) and add it alongside the existing Homebrew translation
rules used by the CPE translation logic referenced in cpe_test.go.
- Line 1341: The test shows Python package "requests" being resolved to
"jenkins:requests" (cpe:2.3:a:jenkins:requests...) due to alphabetical CPE
lookup; add a CPE translation rule that maps the PyPI package name "requests"
(or the ecosystem identifier used in translation code) to the correct CPE vendor
for the Python requests library so the produced CPE becomes
cpe:2.3:a:python:requests:...; update the translation rules data structure (the
one exercised by server/vulnerabilities/nvd/cpe_test.go) to include an explicit
mapping from "requests" (PyPI) -> vendor "python" to ensure deterministic
correct matching.
In `@server/vulnerabilities/nvd/cpe.go`:
- Around line 658-667: The code calls resolveDeprecatedCPE(db, results,
software) when hasDeprecatedMatches is true but passes the entire results slice
(including non-matching or irrelevant deprecated items); instead, build a
filtered slice containing only items that both cpeItemMatchesSoftware(...)
returned true for and are deprecated (preserving original ordering), and pass
that filtered slice to resolveDeprecatedCPE (keep using db and software); update
the block around hasDeprecatedMatches and the references to results so
resolveDeprecatedCPE only examines the relevant deprecatedMatches.
---
Nitpick comments:
In `@server/vulnerabilities/nvd/cpe.go`:
- Around line 182-188: cpeVendorMatchesSoftware currently lowercases
software.Vendor but not item.Vendor, which can cause mismatches; update the
function (cpeVendorMatchesSoftware) to compare lowercased values for both sides
by computing e.g. sVendor := strings.ToLower(software.Vendor) and iVendor :=
strings.ToLower(item.Vendor) and then use strings.Contains(sVendor, iVendor) so
the comparison is case-insensitive and more robust when matching
IndexedCPEItem.Vendor against fleet.Software.Vendor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f9faf04-e094-4a53-8bfe-7ac9d08fe4b0
📒 Files selected for processing (4)
changes/39899-deterministic-cpe-matchingserver/vulnerabilities/nvd/cpe.goserver/vulnerabilities/nvd/cpe_test.goserver/vulnerabilities/nvd/testing_utils.go
There was a problem hiding this comment.
Pull request overview
This PR aims to make NVD CPE matching deterministic when multiple CPE candidates match the same software (notably when multiple CPEs share a product name), reducing run-to-run variability in vulnerability detection.
Changes:
- Adds
ORDER BYto CPE candidate queries and updates selection logic to avoid nondeterministic “first row wins” behavior. - Introduces a vendor-based tiebreak helper and adds/updates tests to cover the deterministic behavior.
- Adds a changelog entry for the bugfix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| server/vulnerabilities/nvd/cpe.go | Adds query ordering + best-match selection logic and a vendor tiebreak helper. |
| server/vulnerabilities/nvd/cpe_test.go | Adds deterministic tie tests and updates integration expectations reflecting the new selection order. |
| server/vulnerabilities/nvd/testing_utils.go | Extends the test CPE dictionary with additional line product entries for ambiguity scenarios. |
| changes/39899-deterministic-cpe-matching | User-visible change note for deterministic CPE matching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #41649 +/- ##
==========================================
+ Coverage 66.36% 66.40% +0.04%
==========================================
Files 2492 2498 +6
Lines 199603 200295 +692
Branches 8826 8826
==========================================
+ Hits 132469 133015 +546
- Misses 55160 55257 +97
- Partials 11974 12023 +49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ksykulev
left a comment
ksykulev
left a comment
There was a problem hiding this comment.
Not a big deal, but the commit message misspelled deteminism. Should be determinism
| // pass cpeItemMatchesSoftware. | ||
| func cpeVendorMatchesSoftware(item *IndexedCPEItem, software *fleet.Software) bool { | ||
| sVendor := strings.ToLower(software.Vendor) | ||
| return sVendor != "" && strings.Contains(sVendor, item.Vendor) |
There was a problem hiding this comment.
Is this reasonable? Are there instances where just doing a strings.Contains might get us into trouble?
Maybe using word boundaries?
if sVendor == "" {
return false
}
pattern := `\b` + regexp.QuoteMeta(item.Vendor) + `\b`
matched, _ := regexp.MatchString(pattern, sVendor)
return matched
Related issue: Resolves #39899
This fix fixes the determinism issue by ordering the results, however, it does not necessarily fix the correctness issue. Another bug opened for that: #41644
That's why you see changes in
cpe_test.gothat may seem incorrect in some cases. In reality the previous behavior was purely by coincidence (based on insert order).Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit