-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: package and utilize generated suppression file #8116
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
base: main
Are you sure you want to change the base?
Conversation
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 introduces a mechanism to package a generated suppressions file with the application during build time by downloading it from a hosted URL. The downloaded file is then loaded alongside the existing base suppression file.
- Adds Maven exec plugin to download suppressions from a hosted URL during the build process
- Extends the suppression loading logic to include the downloaded
generated-suppressions.xmlfile - Refactors the packaged suppression loading method to accept a filename parameter for reusability
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pom.xml | Adds version management for exec-maven-plugin |
| core/pom.xml | Configures exec-maven-plugin to download suppressions file using wget during build |
| core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java | Adds constant for generated suppressions filename and refactors loading logic to support both base and generated suppression files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
|
With this approach, ODC needs to load the generatedSuppressions twice, once off packaged, and again off "hosted" suppressions within the It's slightly more complex, but I wonder if it would be cleaner to make this self contained somewhere within the below to just pre-populate the user's cache in the data directory on disk; to keep all logic and workflow related to these suppressions self-contained, and with consistent language? With this approach it would happen
Lines 260 to 264 in a9f313a
|
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Chad Wilson <[email protected]>
On my machine, a forced update took 148ms to be loaded on 4G connection with a phone. We seem to load all elements in a |
|
My concern is more about evaluation/analysis rather than loading the file. I haven't looked at how smart the algorithm is or how parallelized, but my concern would be that there is an O(n^2) issue here, e.g if every suppression is evaluated for every single possible dependency. We also don't really have any automation to ensure the regexes aren't insanely expensive which might have a multiplicative effect. On a bigger project with, say, 1,000 dependencies (JavaScript!!) and lots of FPs due to use of CLI and generic dependency names, that might be an issue - either slowness or excessive junk/GC production. Aside from perf, I also think it's a bit confusing to have 3 data sources in the code to reason about, which is why I suggest using this file essentially as an initial cache value for HostedSuppressions instead (when working offline, or if stale) since it is not valid/useful to have both the packaged version and a version received remotely. I'm happy to test and suggest a PR to this one if @jeremylong wants to see what it might look like. That'd avoid having to do any de-duplication of individual suppressions since we can generally assume there are very limited duplications between base and hosted/generated/published (especially after Jeremy's adjacent cleanup PRs). I think that helps us as it's already quite tough figuring out why we might not be able to replicate a given FP given combinations of (ODC version, ODC variant, enabled analyzers, enabled data sources, available data sources, hosted suppressions currentness, offline status, available local tools) |
|
I like the idea of the embedded being the initial cache - especially if you don't mind putting together the PR ;). I was also going to make sure there was a mechanism to prevent loading the same rule twice... |
Co-authored-by: Nicolas Humblot <[email protected]>
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.
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/owasp/dependencycheck/analyzer/AbstractSuppressionAnalyzer.java
Outdated
Show resolved
Hide resolved
…SuppressionAnalyzer.java Co-authored-by: Copilot <[email protected]>
|
@chadlwilson mind taking another look at the PR after my updates? We should only be using the hosted suppression file that gets packaged if the file was not able to be downloaded during the update process. |
Description of Change
First of three PRs around cleaning up the differences between the generated and base suppression file. This PR enhances the build to download the current generated suppression file, packages it into the core lib, and loads both the base and generated suppression files during execution.