Skip to content
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

Add criticality filtering to cli #257

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshmfrankel
Copy link

Why?

When utilizing bundler-audit on CI, it can be helpful
to filter criticality to find higher criticality gems.

What?

  • Adds filter (--filter or -f) command line argument that
    accepts array of values (low, medium, high)
  • Filters within Audit::Scanner to remove filtered out advisories

scan_sources(options,&block)
scan_specs(options,&block)

return self
self
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary return

@@ -68,13 +71,10 @@ def initialize(root=Dir.pwd,gemfile_lock='Gemfile.lock')
def scan(options={},&block)
return enum_for(__method__,options) unless block

ignore = Set[]
ignore += options[:ignore] if options[:ignore]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong but I don't see the above variable assignment being used


def check
update if options[:update]

scanner = Scanner.new
vulnerable = false

scanner.scan(:ignore => options.ignore) do |result|
scanner.scan(ignore: options[:ignore], filter: options[:filter]) do |result|
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed when multiple array options are passed the second one defined (the new one filter) returned as an Enumerable when calling with dot syntax options.filter #=> Enumerable. Instead using symbol key syntax it returns as expected

@joshmfrankel
Copy link
Author

I'm unsure of why this is happening but it seems as though the mock user path expects the directory /tmp/ but receives /data/. From looking through other PRs this seems to be effecting them as well:

1) Bundler::Audit::Database path should prefer the user repo, iff it's as up to date, or more up to date than the vendored one
     Failure/Error: expect(subject).to eq mocked_user_path
     
       expected: "/home/travis/build/rubysec/bundler-audit/tmp/ruby-advisory-db"
            got: "/home/travis/build/rubysec/bundler-audit/data/ruby-advisory-db"
     
       (compared using ==)

@reedloden
Copy link
Member

@joshmfrankel Can you rebase? The spec failure should be fixed.

Why?

When utilizing bundler-audit on CI, it can be helpful
to filter criticality to find higher criticality gems.

What?

* Adds filter (--filter or -f) command line argument that
accepts array of values
* Filters within Audit::Scanner to remove filtered out advisories
@joshmfrankel joshmfrankel force-pushed the joshmfrankel/add-criticality-filtering-to-cli branch from fc0919e to ebc6516 Compare June 21, 2020 10:23
@joshmfrankel
Copy link
Author

joshmfrankel commented Jun 21, 2020

Rebased with upstream

Looks like that did the trick. Thanks @reedloden

@postmodern
Copy link
Member

I am a bit concerned that filtering by Criticality may cause low severity vulnerabilities to go unpatched/unmitigated. I'd settle instead for sorting the advisories by criticality, and possibly color coding their text based on Criticality.

@postmodern
Copy link
Member

Also checkout the lib/bundler/audit/cli/formats/text.rb in the 0.8.0 branch. 0.8.0 is going to be merged/released soon.

@ryanwi
Copy link

ryanwi commented Sep 24, 2021

I support this feature/cli flag as well. bundler-audit is awesome and I encourage teams to add it to their CI pipeline order to provide a forcing function to patch vulnerable gems. But, there are a couple of use cases where only failing on a certain level would be useful

  1. some teams have many vulnerabilities, so only failing CI for critical vulnerabilities gives teams a workable starting point before expanding to fail on all vulnerabilities.
  2. some teams don't want to be slowed down by a CI blocker for non-critical vulnerabilities

npm audit and yarn audit support a level flag to handle something like this use case as well.

@postmodern
Copy link
Member

@ryanwi I could see a severity filter being useful for triaging bugs, where you only want to look at the Critical vulns first, then the Highs, then the Mediums, etc. Although, I want to discourage people from thinking that Low Severity vulnerabilities are harmless. The CVSS Score that denotes Severity is a crude metric for how serious a vulnerability is. An unpatched Medium or Low severity vulnerability could still be exploited by an attacker.

@JeffroeBodine
Copy link

This feature would be great for us. Currently we have build breakers setup to stop builds and deploys any time we run into a CVE. It most cases the CVE criticality starts out as Unknown and then later gets updated with a more accurate level. If we had the ability to filter on different levels or have different exit codes, we'd be able to tweak our build breakers to only fire on critical and high for instance but have another CI job to alert on medium, low, none, or unknown.

@postmodern
Copy link
Member

This feature would be great for us. Currently we have build breakers setup to stop builds and deploys any time we run into a CVE. It most cases the CVE criticality starts out as Unknown and then later gets updated with a more accurate level. If we had the ability to filter on different levels or have different exit codes, we'd be able to tweak our build breakers to only fire on critical and high for instance but have another CI job to alert on medium, low, none, or unknown.

Although, you would risk potentially passing a build with a CVE that's currently Unknown, but then later is updated to be Critical. I think it's safer to view every CVE as potentially critical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants