From 302307e38df47bee5a3542978eabce4ee1192418 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Wed, 15 Oct 2014 19:03:35 -0700 Subject: [PATCH] Make --separate-models the default and allow --no-separate-models to turn it off again --- OPTIONS.md | 4 ++-- lib/brakeman.rb | 4 ++-- lib/brakeman/options.rb | 4 ++-- test/tests/markdown_output.rb | 4 ++-- test/tests/mass_assign_disable.rb | 2 +- test/tests/rails2.rb | 4 ++-- test/tests/rails3.rb | 18 +++++++++++++++--- test/tests/rails_with_xss_plugin.rb | 3 ++- test/tests/rescanner.rb | 2 +- test/tests/tabs_output.rb | 4 ++-- 10 files changed, 31 insertions(+), 18 deletions(-) diff --git a/OPTIONS.md b/OPTIONS.md index 61f062ff..aa87365d 100644 --- a/OPTIONS.md +++ b/OPTIONS.md @@ -139,9 +139,9 @@ To limit width of the tables output in text reports, use By default, there is no limit. -Brakeman will bundle all warnings about models without `attr_accessible` into one warning. This was problem a mistake. It's more useful to get one warning per model with +Brakeman will warn about each model without `attr_accessible`. In the HTML report it may be nicer to get all models in one warning with - brakeman --separate-models + brakeman --no-separate-models Sometimes you don't need a big report, just the summary: diff --git a/lib/brakeman.rb b/lib/brakeman.rb index 3876855a..0fa8512c 100644 --- a/lib/brakeman.rb +++ b/lib/brakeman.rb @@ -19,7 +19,7 @@ module Brakeman # * :app_path - path to root of Rails app (required) # * :assume_all_routes - assume all methods are routes (default: true) # * :check_arguments - check arguments of methods (default: true) - # * :collapse_mass_assignment - report unprotected models in single warning (default: true) + # * :collapse_mass_assignment - report unprotected models in single warning (default: false) # * :combine_locations - combine warning locations (default: true) # * :config_file - configuration file # * :escape_html - escape HTML by default (automatic) @@ -122,7 +122,7 @@ module Brakeman :safe_methods => Set.new, :min_confidence => 2, :combine_locations => true, - :collapse_mass_assignment => true, + :collapse_mass_assignment => false, :highlight_user_input => true, :ignore_redirect_to_model => true, :ignore_model_output => false, diff --git a/lib/brakeman/options.rb b/lib/brakeman/options.rb index b88381f8..56340d1d 100644 --- a/lib/brakeman/options.rb +++ b/lib/brakeman/options.rb @@ -200,8 +200,8 @@ module Brakeman::Options options[:output_files].push(file) end - opts.on "--separate-models", "Warn on each model without attr_accessible" do - options[:collapse_mass_assignment] = false + opts.on "--[no]-separate-models", "Warn on each model without attr_accessible (Default)" do |combine| + options[:collapse_mass_assignment] = combine end opts.on "--summary", "Only output summary of warnings" do diff --git a/test/tests/markdown_output.rb b/test/tests/markdown_output.rb index 092be123..981ae1a4 100644 --- a/test/tests/markdown_output.rb +++ b/test/tests/markdown_output.rb @@ -7,9 +7,9 @@ class TestMarkdownOutput < Test::Unit::TestCase def test_reported_warnings if Brakeman::Scanner::RUBY_1_9 - assert_equal 170, Report.lines.to_a.count - else assert_equal 171, Report.lines.to_a.count + else + assert_equal 172, Report.lines.to_a.count end end end diff --git a/test/tests/mass_assign_disable.rb b/test/tests/mass_assign_disable.rb index d6709b21..bac3a376 100644 --- a/test/tests/mass_assign_disable.rb +++ b/test/tests/mass_assign_disable.rb @@ -9,7 +9,7 @@ class MassAssignDisableTest < Test::Unit::TestCase end assert_changes - assert_fixed 3 + assert_fixed 4 assert_new 0 end diff --git a/test/tests/rails2.rb b/test/tests/rails2.rb index c0cc5fb1..0157fee6 100644 --- a/test/tests/rails2.rb +++ b/test/tests/rails2.rb @@ -5,7 +5,7 @@ abort "Please run using test/test.rb" unless defined? BrakemanTester -Rails2 = BrakemanTester.run_scan "rails2", "Rails 2", :run_all_checks => true +Rails2 = BrakemanTester.run_scan "rails2", "Rails 2", :run_all_checks => true, :collapse_mass_assignment => true class Rails2Tests < Test::Unit::TestCase include BrakemanTester::FindWarning @@ -1385,7 +1385,7 @@ class Rails2Tests < Test::Unit::TestCase end end -Rails2WithOptions = BrakemanTester.run_scan "rails2", "Rails 2", :collapse_mass_assignment => false, :run_all_checks => true +Rails2WithOptions = BrakemanTester.run_scan "rails2", "Rails 2", :run_all_checks => true class Rails2WithOptionsTests < Test::Unit::TestCase include BrakemanTester::FindWarning diff --git a/test/tests/rails3.rb b/test/tests/rails3.rb index cb87596e..88677e90 100644 --- a/test/tests/rails3.rb +++ b/test/tests/rails3.rb @@ -14,7 +14,7 @@ class Rails3Tests < Test::Unit::TestCase def expected @expected ||= { :controller => 1, - :model => 8, + :model => 9, :template => 38, :generic => 72 } @@ -395,10 +395,22 @@ class Rails3Tests < Test::Unit::TestCase def test_attribute_restriction assert_warning :type => :model, + :warning_code => 19, + :fingerprint => "91d73b1b9d6920156b920729c0146292eb9f10f4ba9515740442dbe82d4dee78", :warning_type => "Attribute Restriction", - :message => /^Mass assignment is not restricted using /, + :line => nil, + :message => /^Mass\ assignment\ is\ not\ restricted\ using\ /, + :confidence => 0, + :relative_path => "app/models/account.rb" + + assert_warning :type => :model, + :warning_code => 19, + :fingerprint => "b325ae8a4570599cde146875ae86427506befae36a3b4a97ce2223930846fec5", + :warning_type => "Attribute Restriction", + :line => nil, + :message => /^Mass\ assignment\ is\ not\ restricted\ using\ /, :confidence => 0, - :file => /account, user\.rb/ + :relative_path => "app/models/user.rb" end def test_attr_protected diff --git a/test/tests/rails_with_xss_plugin.rb b/test/tests/rails_with_xss_plugin.rb index d8460c07..5310f766 100644 --- a/test/tests/rails_with_xss_plugin.rb +++ b/test/tests/rails_with_xss_plugin.rb @@ -4,7 +4,8 @@ RailsWithXssPlugin = BrakemanTester.run_scan( "rails_with_xss_plugin", "RailsWithXssPlugin", :absolute_paths => true, - :run_all_checks => true + :run_all_checks => true, + :collapse_mass_assignment => true ) class RailsWithXssPluginTests < Test::Unit::TestCase diff --git a/test/tests/rescanner.rb b/test/tests/rescanner.rb index 6560b5c8..207b89ed 100644 --- a/test/tests/rescanner.rb +++ b/test/tests/rescanner.rb @@ -208,7 +208,7 @@ class RescannerTests < Test::Unit::TestCase assert_reindex :none assert_changes - assert_new 1 + assert_new 3 assert_fixed 0 end diff --git a/test/tests/tabs_output.rb b/test/tests/tabs_output.rb index dbeccc65..ef6d253c 100644 --- a/test/tests/tabs_output.rb +++ b/test/tests/tabs_output.rb @@ -7,9 +7,9 @@ class TestTabsOutput < Test::Unit::TestCase def test_reported_warnings if Brakeman::Scanner::RUBY_1_9 - assert_equal 108, Report.lines.to_a.count - else assert_equal 109, Report.lines.to_a.count + else + assert_equal 110, Report.lines.to_a.count end end end -- GitLab