-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: detect unused suppression sub-entries ✅ #8236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: detect unused suppression sub-entries ✅ #8236
Conversation
Added methods to track used CVEs and vulnerability names, and check for unused sub-entries.
|
Hi I’ve submitted this PR to improve detection of unused suppression rule sub-entries (CVE and vulnerability name tracking). Whenever you have time, I’d really appreciate your review or any feedback to help improve the implementation. Thank you for your guidance and support! 🙏 |
|
Please properly fill in the PR description to accurately summarise what you have changed, why, link the issue you are trying to resolve etc, and address the lint/build/test failures. |
|
@chadlwilson Thanks for the feedback! I've updated the PR description with a detailed summary and linked issue #8197. |
|
Hi @chadlwilson, I’ve fixed the failing test expectations and now all checks are passing. Thank you for reviewing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the unused suppression detection feature by tracking individual CVE and vulnerabilityName entries within suppression rules, rather than treating entire suppression blocks as atomic units. This allows more precise identification of unused suppression entries when dependencies are upgraded and some vulnerabilities no longer apply.
Changes:
- Added tracking of used CVE and vulnerabilityName sub-entries in
SuppressionRuleusing HashSet collections - Implemented methods to mark sub-entries as used during vulnerability suppression processing
- Updated
UnusedSuppressionRuleAnalyzerto detect and report rules with unused sub-entries - Modified tests to reflect the new behavior where rules with unused sub-entries are flagged
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| core/src/main/java/org/owasp/dependencycheck/xml/suppression/SuppressionRule.java | Added fields and methods to track which CVE and vulnerabilityName entries are actually used during suppression processing |
| core/src/main/java/org/owasp/dependencycheck/analyzer/UnusedSuppressionRuleAnalyzer.java | Updated detection logic to flag rules with unused sub-entries and improved error messaging |
| core/src/test/java/org/owasp/dependencycheck/analyzer/UnusedSuppressionRuleAnalyzerTest.java | Fixed formatting, corrected comments, and updated test expectations to verify unused sub-entry detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/org/owasp/dependencycheck/xml/suppression/SuppressionRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/xml/suppression/SuppressionRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/xml/suppression/SuppressionRule.java
Outdated
Show resolved
Hide resolved
| if (!remove && v.getName() != null) { | ||
| for (PropertyType entry : this.vulnerabilityNames) { | ||
| if (entry.matches(v.getName())) { | ||
| markVulnerabilityNameUsed(entry.getValue()); |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The markVulnerabilityNameUsed call has excessive indentation (13 spaces). It should be consistently indented to match the surrounding code at the same nesting level.
| final String message = String.format( | ||
| "Suppression Rule had unused entries (or zero matches): %s", rule); |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message formatting is split across multiple lines with unusual indentation. The second part of the String.format call starts at column 9 instead of being properly aligned. Consider keeping the entire format string on one line or using proper indentation that aligns with the opening parenthesis.
core/src/main/java/org/owasp/dependencycheck/xml/suppression/SuppressionRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/xml/suppression/SuppressionRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/xml/suppression/SuppressionRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/xml/suppression/SuppressionRule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/xml/suppression/SuppressionRule.java
Outdated
Show resolved
Hide resolved
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
Add tracking for used CVE and vulnerability-name suppression entries Track which CVE and vulnerability-name sub-entries are actually matched during suppression This helps identify outdated or ineffective suppression rules and improves suppression file hygiene. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (Vulnerability v : dependency.getVulnerabilities()) { | ||
| boolean remove = false; | ||
| for (String entry : this.cve) { | ||
| for (String entry : this.cve) { |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation: This line has an extra leading space. It should align with the surrounding code at the same indentation level (16 spaces, not 17).
| for (String entry : this.cve) { | |
| for (String entry : this.cve) { |
| private final Set<String> usedCves = new HashSet<>(); | ||
|
|
||
| /** | ||
| * Track used vulnerability-name sub-entries. | ||
| */ | ||
| private final Set<String> usedVulnerabilityNames = new HashSet<>(); |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thread safety issue: These HashSet fields are being modified without synchronization in a class marked @NotThreadSafe, but the class is accessed by analyzers marked @threadsafe that support parallel processing. Multiple threads can call the process() method concurrently, leading to potential race conditions when adding to usedCves and usedVulnerabilityNames sets.
Consider using thread-safe collections like ConcurrentHashMap.newKeySet() instead of HashSet, or add proper synchronization to the markCveUsed() and markVulnerabilityNameUsed() methods.
| public boolean hasUnusedSubEntries() { | ||
| if (!matched) { | ||
| return false; | ||
| } | ||
|
|
||
| for (String c : this.cve) { | ||
| if (!usedCves.contains(c)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| for (PropertyType pt : this.vulnerabilityNames) { | ||
| String name = pt.getValue(); | ||
| if (!usedVulnerabilityNames.contains(name)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete tracking of suppression matches: The hasUnusedSubEntries() method only checks CVE and vulnerabilityName entries, but suppressions can also match based on CWE entries (lines 681-690) and CVSS score thresholds (lines 701-706). When these other suppression types match, they set matched=true but don't track which specific CVE/vulnerabilityName entries were used.
This means if a rule has CVE entries AND a cvssBelow threshold, and only the cvssBelow matches, the CVE entries will incorrectly appear as unused. Consider tracking all suppression types or adjusting the logic to only check for unused sub-entries when the rule uses CVE or vulnerabilityName matching.
| if (!remove && v.getName() != null) { | ||
| for (PropertyType entry : this.vulnerabilityNames) { | ||
| if (entry.matches(v.getName())) { | ||
| markVulnerabilityNameUsed(entry.getValue()); |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation: This line has extra leading whitespace compared to the surrounding code. The line should align with the 'for' statement above it at line 673.
| if ((!rule.isMatched() || rule.hasUnusedSubEntries()) && !rule.isBase()) { | ||
| final String message = String.format( | ||
| "Suppression Rule had unused entries (or zero matches): %s", rule); |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message should be more specific and helpful. The current message "Suppression Rule had unused entries (or zero matches)" is ambiguous. When hasUnusedSubEntries() returns true, it means that some CVE or vulnerabilityName entries within a matched rule are unused - not that there are zero matches. Consider separate messages for the two cases: one for completely unused rules (!rule.isMatched()) and another for rules with unused sub-entries (rule.hasUnusedSubEntries()), to make it clearer what action the user should take.
| /** | ||
| * Returns whether this suppression rule matched any dependency. | ||
| * | ||
| * @return the value of matched | ||
| * @return true if the rule matched at least one dependency | ||
| */ |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation: This JavaDoc comment has extra leading whitespace (8 spaces instead of 4). It should align with the other method JavaDocs in this class.
chadlwilson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion the whole approach here needs reworking.
It has a number of problems in both design and functionality; aside from style.
- no tests
- mutates collections in threaded code so not threadsafe (as copilot notes)
- doesn't work correctly if a vulnerability is suppressed by its CPE (regex or prefix)
- doesn't work correctly if a vulnerability is suppressed by its CWE
- it doesn't tell you specifically which sub-parts of the rule are unmatched making this useless to use in practice
- unnecessarily retains and logs via the earlier crude
matchedstatus, when that should be unnecessary when tracking the entries deliberately vulnerabilityNametracking is buggy/incomplete because it tracks based on the text of the rule, ignoring thePropertyTypewhether it is aregexor not.- marks and tracks matches against base suppression entries unnecessarily which is confusing for debugging (and wasteful on memory usage)
IMO this really needs some more design thought and a more holistic approach for tracking these which might need some refactoring of the core process(Dependency) method to make it easier to understand - otherwise bugs are likely to be introduced later when/if different suppression styles are supported.
|
Thanks for the detailed feedback — I agree the current approach needs redesign (thread safety, CPE/CWE handling, PropertyType usage, and better reporting). I’ll rework this with a new design and proper tests rather than patching the current PR. Appreciate the guidance. |
Fixes #8197
Summary
Improve unused suppression detection by tracking individual CVE and vulnerabilityName (GHSA) entries inside a suppression rule.
Previously, only whole suppression blocks were checked for usage. This change allows detecting unused sub-entries so they can be safely removed when
failBuildOnUnusedSuppressionRuleis enabled.Changes
SuppressionRuleSuppressionRuleUnusedSuppressionRuleAnalyzerMotivation
When dependencies are upgraded, some suppressed vulnerabilities no longer apply. Currently these remain hidden because the suppression block is still considered "used". This change makes suppression maintenance more accurate and easier.
Testing