From 195a93aa79353692b608f48b84ce6d8172514f7f Mon Sep 17 00:00:00 2001 From: Eli Block <3347571+eliblock@users.noreply.github.com> Date: Tue, 13 Jul 2021 15:30:53 -0700 Subject: [PATCH 1/3] feat: sarif output includes suppressed findings --- lib/brakeman/report/report_sarif.rb | 21 +++++++++++++++++-- test/tests/sarif_output.rb | 32 ++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/lib/brakeman/report/report_sarif.rb b/lib/brakeman/report/report_sarif.rb index 7fe37d1284..9714d60964 100644 --- a/lib/brakeman/report/report_sarif.rb +++ b/lib/brakeman/report/report_sarif.rb @@ -48,7 +48,7 @@ def rules end def results - @results ||= all_warnings.map do |warning| + @results ||= tracker.checks.all_warnings.map do |warning| rule_id = render_id warning result_level = infer_level warning message_text = render_message warning.message.to_s @@ -72,6 +72,23 @@ def results ], } + if @ignore_filter && @ignore_filter.ignored?(warning) + result[:suppressions] = [ + { + :kind => 'external', + :justification => @ignore_filter.note_for(warning), + :location => { + :physicalLocation => { + :artifactLocation => { + :uri => Brakeman::FilePath.from_app_tree(@app_tree, @ignore_filter.file).relative, + :uriBaseId => '%SRCROOT%', + }, + }, + }, + } + ] + end + result end end @@ -85,7 +102,7 @@ def check_descriptions # Returns a de-duplicated set of warnings, used to generate rules def unique_warnings_by_warning_code - @unique_warnings_by_warning_code ||= all_warnings.uniq { |w| w.warning_code } + @unique_warnings_by_warning_code ||= tracker.checks.all_warnings.uniq { |w| w.warning_code } end def render_id warning diff --git a/test/tests/sarif_output.rb b/test/tests/sarif_output.rb index a55f1be550..b49412c4e5 100644 --- a/test/tests/sarif_output.rb +++ b/test/tests/sarif_output.rb @@ -3,7 +3,8 @@ class SARIFOutputTests < Minitest::Test def setup - @@sarif ||= JSON.parse(Brakeman.run("#{TEST_PATH}/apps/rails3.2").report.to_sarif) + @@sarif ||= JSON.parse(Brakeman.run(File.join(TEST_PATH, 'apps', 'rails3.2')).report.to_sarif) # has no brakeman.ignore + @@sarif_with_ignore ||= JSON.parse(Brakeman.run(File.join(TEST_PATH, 'apps', 'rails4')).report.to_sarif) # has ignored warnings end def test_log_shape @@ -96,4 +97,33 @@ def test_results_shape end end end + + def test_with_ignore_has_one_suppressed_finding + assert_equal( + 1, + @@sarif_with_ignore.dig('runs', 0, 'results'). + map { |f| 1 if f['suppressions'] }.compact.sum + ) + end + + def test_with_ignore_results_suppression_shape + @@sarif_with_ignore.dig('runs', 0, 'results').each do |finding| + suppressions = finding['suppressions'] + next unless suppressions + + # Each finding with suppressions has exactly one... + assert_equal 1, suppressions.count + assert suppression = suppressions[0] + + # ...external suppression... + assert_equal 'external', suppression['kind'] + + # ...with a valid physical location... + assert suppression['location']['physicalLocation']['artifactLocation']['uri'] + assert_equal '%SRCROOT%', suppression['location']['physicalLocation']['artifactLocation']['uriBaseId'] + + # ...and a justification (will be nil if no notes is set). + assert suppression['justification'] + end + end end From 52bfc9b187dcc898f631124bc841823de6117ca5 Mon Sep 17 00:00:00 2001 From: Eli Block <3347571+eliblock@users.noreply.github.com> Date: Tue, 13 Jul 2021 18:04:43 -0700 Subject: [PATCH 2/3] refactor: IgnoreConfig uses FilePath --- lib/brakeman.rb | 10 ++++++---- lib/brakeman/commandline.rb | 2 +- lib/brakeman/report/ignore/config.rb | 8 ++++---- lib/brakeman/report/report_sarif.rb | 2 +- test/tests/brakeman.rb | 6 +++--- test/tests/ignore.rb | 12 +++++++++--- 6 files changed, 24 insertions(+), 16 deletions(-) diff --git a/lib/brakeman.rb b/lib/brakeman.rb index a127b6524b..b2edc4bb0d 100644 --- a/lib/brakeman.rb +++ b/lib/brakeman.rb @@ -527,12 +527,14 @@ def self.load_brakeman_dependency name, allow_fail = false # Returns an array of alert fingerprints for any ignored warnings without # notes found in the specified ignore file (if it exists). - def self.ignore_file_entries_with_empty_notes file + def self.ignore_file_entries_with_empty_notes file, options return [] unless file require 'brakeman/report/ignore/config' - config = IgnoreConfig.new(file, nil) + app_tree = Brakeman::AppTree.from_options(options) + + config = IgnoreConfig.new(Brakeman::FilePath.from_app_tree(app_tree, file), nil) config.read_from_file config.already_ignored_entries_with_empty_notes.map { |i| i[:fingerprint] } end @@ -543,9 +545,9 @@ def self.filter_warnings tracker, options app_tree = Brakeman::AppTree.from_options(options) if options[:ignore_file] - file = options[:ignore_file] + file = Brakeman::FilePath.from_app_tree(app_tree, options[:ignore_file]) elsif app_tree.exists? "config/brakeman.ignore" - file = app_tree.expand_path("config/brakeman.ignore") + file = Brakeman::FilePath.from_app_tree(app_tree, "config/brakeman.ignore") elsif not options[:interactive_ignore] return end diff --git a/lib/brakeman/commandline.rb b/lib/brakeman/commandline.rb index 0656db8a16..2a585f1d4b 100644 --- a/lib/brakeman/commandline.rb +++ b/lib/brakeman/commandline.rb @@ -126,7 +126,7 @@ def regular_report options ensure_ignore_notes_failed = false if tracker.options[:ensure_ignore_notes] - fingerprints = Brakeman::ignore_file_entries_with_empty_notes tracker.ignored_filter&.file + fingerprints = Brakeman::ignore_file_entries_with_empty_notes tracker.ignored_filter&.file, options unless fingerprints.empty? ensure_ignore_notes_failed = true diff --git a/lib/brakeman/report/ignore/config.rb b/lib/brakeman/report/ignore/config.rb index 670573bfe2..3f3c51c33c 100644 --- a/lib/brakeman/report/ignore/config.rb +++ b/lib/brakeman/report/ignore/config.rb @@ -100,14 +100,14 @@ def already_ignored_entries_with_empty_notes # Read configuration to file def read_from_file file = @file - if File.exist? file + if File.exist? file.absolute begin @already_ignored = JSON.parse(File.read(file), :symbolize_names => true)[:ignored_warnings] rescue => e - raise e, "\nError[#{e.class}] while reading brakeman ignore file: #{file}\n" + raise e, "\nError[#{e.class}] while reading brakeman ignore file: #{file.relative}\n" end else - Brakeman.notify "[Notice] Could not find ignore configuration in #{file}" + Brakeman.notify "[Notice] Could not find ignore configuration in #{file.relative}" @already_ignored = [] end @@ -134,7 +134,7 @@ def save_to_file warnings, file = @file :brakeman_version => Brakeman::Version } - File.open file, "w" do |f| + File.open file.absolute, "w" do |f| f.puts JSON.pretty_generate(output) end end diff --git a/lib/brakeman/report/report_sarif.rb b/lib/brakeman/report/report_sarif.rb index 9714d60964..554d641889 100644 --- a/lib/brakeman/report/report_sarif.rb +++ b/lib/brakeman/report/report_sarif.rb @@ -80,7 +80,7 @@ def results :location => { :physicalLocation => { :artifactLocation => { - :uri => Brakeman::FilePath.from_app_tree(@app_tree, @ignore_filter.file).relative, + :uri => @ignore_filter.file.relative, :uriBaseId => '%SRCROOT%', }, }, diff --git a/test/tests/brakeman.rb b/test/tests/brakeman.rb index 64654d9cb5..8a6460e00e 100644 --- a/test/tests/brakeman.rb +++ b/test/tests/brakeman.rb @@ -387,13 +387,13 @@ def test_ensure_latest end def test_ignore_file_entries_with_empty_notes - assert Brakeman.ignore_file_entries_with_empty_notes(nil).empty? + assert Brakeman.ignore_file_entries_with_empty_notes(nil, {}).empty? ignore_file_missing_notes = Tempfile.new('brakeman.ignore') ignore_file_missing_notes.write IGNORE_WITH_MISSING_NOTES_JSON ignore_file_missing_notes.close assert_equal( - Brakeman.ignore_file_entries_with_empty_notes(ignore_file_missing_notes.path).to_set, + Brakeman.ignore_file_entries_with_empty_notes(ignore_file_missing_notes.path, {:app_path => "/tmp" }).to_set, [ '006ac5fe3834bf2e73ee51b67eb111066f618be46e391d493c541ea2a906a82f', ].to_set @@ -403,7 +403,7 @@ def test_ignore_file_entries_with_empty_notes ignore_file_with_notes = Tempfile.new('brakeman.ignore') ignore_file_with_notes.write IGNORE_WITH_NOTES_JSON ignore_file_with_notes.close - assert Brakeman.ignore_file_entries_with_empty_notes(ignore_file_with_notes.path).empty? + assert Brakeman.ignore_file_entries_with_empty_notes(ignore_file_with_notes.path, {:app_path => "/tmp" }).empty? ignore_file_with_notes.unlink end diff --git a/test/tests/ignore.rb b/test/tests/ignore.rb index 45a8bf4010..7cd2c171b2 100644 --- a/test/tests/ignore.rb +++ b/test/tests/ignore.rb @@ -16,7 +16,8 @@ def setup end def make_config file = @config_file.path - c = Brakeman::IgnoreConfig.new file, report.warnings + app_tree = Brakeman::AppTree.from_options({:app_path => app_path}) + c = Brakeman::IgnoreConfig.new Brakeman::FilePath.from_app_tree(app_tree, file), report.warnings c.read_from_file c.filter_ignored c @@ -27,7 +28,11 @@ def teardown end def report - @@report ||= Brakeman.run(File.join(TEST_PATH, "apps", "rails5.2")) + @@report ||= Brakeman.run(app_path) + end + + def app_path + @@app_path ||= File.join(TEST_PATH, "apps", "rails5.2") end def test_sanity @@ -181,7 +186,8 @@ def test_bad_ignore_json_error_message file.write "{[ This is bad json cuz I don't have a closing square bracket, bwahahaha...}" file.close begin - c = Brakeman::IgnoreConfig.new file.path, report.warnings + app_tree = Brakeman::AppTree.from_options({:app_path => app_path}) + c = Brakeman::IgnoreConfig.new Brakeman::FilePath.from_app_tree(app_tree, file.path), report.warnings c.read_from_file rescue => e # The message should clearly show that there was a problem parsing the json From 82c30e80f7c96c2a6daf9878242e161d2e8503e8 Mon Sep 17 00:00:00 2001 From: Eli Block <3347571+eliblock@users.noreply.github.com> Date: Thu, 15 Jul 2021 12:53:12 -0700 Subject: [PATCH 3/3] refactor: select rather than map compact --- test/tests/sarif_output.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/tests/sarif_output.rb b/test/tests/sarif_output.rb index b49412c4e5..934994c041 100644 --- a/test/tests/sarif_output.rb +++ b/test/tests/sarif_output.rb @@ -102,7 +102,7 @@ def test_with_ignore_has_one_suppressed_finding assert_equal( 1, @@sarif_with_ignore.dig('runs', 0, 'results'). - map { |f| 1 if f['suppressions'] }.compact.sum + select { |f| f['suppressions'] }.count ) end