diff --git a/lib/brakeman/checks/base_check.rb b/lib/brakeman/checks/base_check.rb index 68e2668e3e..5e6e1a3056 100644 --- a/lib/brakeman/checks/base_check.rb +++ b/lib/brakeman/checks/base_check.rb @@ -432,14 +432,12 @@ def has_immediate_model? exp, out = nil #the current tracker, or else @models should contain an array of the model #names, which is available via tracker.models.keys def model_name? exp - @models ||= @tracker.models.keys - if exp.is_a? Symbol - @models.include? exp + tracker.models.include? exp elsif call? exp and exp.target.nil? and exp.method == :current_user true elsif sexp? exp - @models.include? class_name(exp) + tracker.models.include? class_name(exp) else false end @@ -485,11 +483,11 @@ def self.description def active_record_models return @active_record_models if @active_record_models - @active_record_models = {} + @active_record_models = Brakeman::ClassCollection.new - tracker.models.each do |name, model| + tracker.models.each_class do |model| if model.ancestor? :"ActiveRecord::Base" - @active_record_models[name] = model + @active_record_models << model end end diff --git a/lib/brakeman/checks/check_detailed_exceptions.rb b/lib/brakeman/checks/check_detailed_exceptions.rb index fb8f78db53..887f0138bc 100644 --- a/lib/brakeman/checks/check_detailed_exceptions.rb +++ b/lib/brakeman/checks/check_detailed_exceptions.rb @@ -24,7 +24,7 @@ def check_local_request_config end def check_detailed_exceptions - tracker.controllers.each do |_name, controller| + tracker.controllers.each_class do |controller| controller.methods_public.each do |method_name, definition| src = definition[:src] body = src.body.last diff --git a/lib/brakeman/checks/check_forgery_setting.rb b/lib/brakeman/checks/check_forgery_setting.rb index 6e2bb5a86b..bc65d39633 100644 --- a/lib/brakeman/checks/check_forgery_setting.rb +++ b/lib/brakeman/checks/check_forgery_setting.rb @@ -14,11 +14,11 @@ def run_check tracker.controllers .select { |_, controller| controller.parent == :"ActionController::Base" } - .each do |name, controller| + .each do |_, controller| if controller and not controller.protect_from_forgery? - csrf_warning :controller => name, + csrf_warning :controller => controller.name, :warning_code => :csrf_protection_missing, - :message => msg(msg_code("protect_from_forgery"), " should be called in ", msg_code(name)), + :message => msg(msg_code("protect_from_forgery"), " should be called in ", msg_code(controller.name)), :file => controller.file, :line => controller.top_line elsif version_between? "4.0.0", "100.0.0" and forgery_opts = controller.options[:protect_from_forgery] @@ -27,7 +27,7 @@ def run_check access_arg.value == :exception args = { - :controller => name, + :controller => controller.name, :warning_type => "Cross-Site Request Forgery", :warning_code => :csrf_not_protected_by_raising_exception, :message => msg(msg_code("protect_from_forgery"), " should be configured with ", msg_code("with: :exception")), diff --git a/lib/brakeman/checks/check_model_serialize.rb b/lib/brakeman/checks/check_model_serialize.rb index f919d8372b..acb5364bdc 100644 --- a/lib/brakeman/checks/check_model_serialize.rb +++ b/lib/brakeman/checks/check_model_serialize.rb @@ -17,7 +17,7 @@ def run_check return unless @upgrade_version - tracker.models.each do |_name, model| + tracker.models.each_class do |model| check_for_serialize model end end diff --git a/lib/brakeman/checks/check_nested_attributes.rb b/lib/brakeman/checks/check_nested_attributes.rb index d0f10dcb51..b4bcc2ead2 100644 --- a/lib/brakeman/checks/check_nested_attributes.rb +++ b/lib/brakeman/checks/check_nested_attributes.rb @@ -29,7 +29,7 @@ def run_check end def uses_nested_attributes? - active_record_models.each do |_name, model| + active_record_models.each_class do |model| return true if model.options[:accepts_nested_attributes_for] end diff --git a/lib/brakeman/checks/check_nested_attributes_bypass.rb b/lib/brakeman/checks/check_nested_attributes_bypass.rb index 63c4e806e2..02a6f043bc 100644 --- a/lib/brakeman/checks/check_nested_attributes_bypass.rb +++ b/lib/brakeman/checks/check_nested_attributes_bypass.rb @@ -18,7 +18,7 @@ def run_check end def check_nested_attributes - active_record_models.each do |name, model| + active_record_models.each_class do |model| if opts = model.options[:accepts_nested_attributes_for] opts.each do |args| if args.any? { |a| allow_destroy? a } and args.any? { |a| reject_if? a } diff --git a/lib/brakeman/checks/check_page_caching_cve.rb b/lib/brakeman/checks/check_page_caching_cve.rb index c955403f07..3866707538 100644 --- a/lib/brakeman/checks/check_page_caching_cve.rb +++ b/lib/brakeman/checks/check_page_caching_cve.rb @@ -10,17 +10,13 @@ def run_check gem_version = tracker.config.gem_version(gem_name.to_sym) upgrade_version = '1.2.2' cve = 'CVE-2020-8159' - return unless gem_version and version_between?('0.0.0', '1.2.1', gem_version) - message = msg("Directory traversal vulnerability in ", msg_version(gem_version, gem_name), " ", msg_cve(cve), ". Upgrade to ", msg_version(upgrade_version, gem_name)) - if uses_caches_page? confidence = :high else confidence = :weak end - warn :warning_type => 'Directory Traversal', :warning_code => :CVE_2020_8159, :message => message, @@ -30,8 +26,9 @@ def run_check end def uses_caches_page? - tracker.controllers.any? do |name, controller| - controller.options.has_key? :caches_page + tracker.controllers.each_class do |controller| + return true if controller.options[:caches_page] end + return false end end diff --git a/lib/brakeman/checks/check_skip_before_filter.rb b/lib/brakeman/checks/check_skip_before_filter.rb index 3d74c9b733..fbf4e2e70b 100644 --- a/lib/brakeman/checks/check_skip_before_filter.rb +++ b/lib/brakeman/checks/check_skip_before_filter.rb @@ -13,7 +13,7 @@ class Brakeman::CheckSkipBeforeFilter < Brakeman::BaseCheck @description = "Warn when skipping CSRF or authentication checks by default" def run_check - tracker.controllers.each do |_name, controller| + tracker.controllers.each_class do |controller| controller.skip_filters.each do |filter| process_skip_filter filter, controller end diff --git a/lib/brakeman/checks/check_sql.rb b/lib/brakeman/checks/check_sql.rb index b566aebb71..722621158c 100644 --- a/lib/brakeman/checks/check_sql.rb +++ b/lib/brakeman/checks/check_sql.rb @@ -97,7 +97,7 @@ def find_scope_calls end def ar_scope_calls(symbol_name, &block) - active_record_models.each do |name, model| + active_record_models.each_class do |model| model_args = model.options[symbol_name] if model_args model_args.each do |args| diff --git a/lib/brakeman/checks/check_validation_regex.rb b/lib/brakeman/checks/check_validation_regex.rb index 038d3b4451..1d27b4ed89 100644 --- a/lib/brakeman/checks/check_validation_regex.rb +++ b/lib/brakeman/checks/check_validation_regex.rb @@ -16,7 +16,7 @@ class Brakeman::CheckValidationRegex < Brakeman::BaseCheck FORMAT = Sexp.new(:lit, :format) def run_check - active_record_models.each do |name, model| + active_record_models.each_class do |model| @current_model = model format_validations = model.options[:validates_format_of] diff --git a/lib/brakeman/processors/lib/module_helper.rb b/lib/brakeman/processors/lib/module_helper.rb index f6fbef8d40..adfdb3f016 100644 --- a/lib/brakeman/processors/lib/module_helper.rb +++ b/lib/brakeman/processors/lib/module_helper.rb @@ -32,7 +32,7 @@ def handle_module exp, tracker_class, parent = nil def handle_class exp, collection, tracker_class name = class_name(exp.class_name) - parent = class_name exp.parent_name + parent = class_name(exp.parent_name) if @current_class outer_class = @current_class @@ -43,12 +43,14 @@ def handle_class exp, collection, tracker_class name = (@current_module.name.to_s + "::" + name.to_s).to_sym end - if collection[name] - @current_class = collection[name] + bm_name = Brakeman::ClassName.new(name) + + if collection[bm_name] + @current_class = collection[bm_name] @current_class.add_file @current_file, exp else - @current_class = tracker_class.new name, parent, @current_file, exp, @tracker - collection[name] = @current_class + @current_class = tracker_class.new bm_name, parent, @current_file, exp, @tracker + collection[bm_name] = @current_class end exp.body = process_all! exp.body diff --git a/lib/brakeman/rescanner.rb b/lib/brakeman/rescanner.rb index cf5b255c5f..b720c9f2b7 100644 --- a/lib/brakeman/rescanner.rb +++ b/lib/brakeman/rescanner.rb @@ -205,7 +205,7 @@ def rescan_lib path lib = nil - tracker.libs.each do |_name, library| + tracker.libs.each_class do |library| if library.files.include?(path) lib = library break @@ -277,7 +277,7 @@ def rescan_deleted_template path def rescan_deleted_lib path deleted_lib = nil - tracker.libs.delete_if do |_name, lib| + tracker.libs.delete_if do |lib| if lib.files.include?(path) deleted_lib = lib true @@ -297,7 +297,7 @@ def remove_deleted_file path deleted = false [:controllers, :models, :libs].each do |collection| - tracker.send(collection).delete_if do |_name, data| + tracker.send(collection).delete_if do |data| if data.files.include?(path) deleted = true true @@ -349,7 +349,7 @@ def rescan_mixin lib to_rescan = [] #Rescan controllers that mixed in library - tracker.controllers.each do |_name, controller| + tracker.controllers.each_class do |controller| if controller.includes.include? lib.name controller.files.each do |path| unless @paths.include? path diff --git a/lib/brakeman/tracker.rb b/lib/brakeman/tracker.rb index 89b4823b2a..9c49914da0 100644 --- a/lib/brakeman/tracker.rb +++ b/lib/brakeman/tracker.rb @@ -6,6 +6,7 @@ require 'brakeman/processors/lib/find_all_calls' require 'brakeman/tracker/config' require 'brakeman/tracker/constants' +require 'brakeman/tracker/class_collection' #The Tracker keeps track of all the processed information. class Brakeman::Tracker @@ -16,7 +17,7 @@ class Brakeman::Tracker #Place holder when there should be a model, but it is not #clear what model it will be. - UNKNOWN_MODEL = :BrakemanUnresolvedModel + UNKNOWN_MODEL = Brakeman::ClassName.new(:BrakemanUnresolvedModel) #Creates a new Tracker. # @@ -29,16 +30,16 @@ def initialize(app_tree, processor = nil, options = {}) @config = Brakeman::Config.new(self) @templates = {} - @controllers = {} + @controllers = Brakeman::ClassCollection.new #Initialize models with the unknown model so #we can match models later without knowing precisely what #class they are. - @models = {} + @models = Brakeman::ClassCollection.new @models[UNKNOWN_MODEL] = Brakeman::Model.new(UNKNOWN_MODEL, nil, @app_tree.file_path("NOT_REAL.rb"), nil, self) @routes = {} @initializers = {} @errors = [] - @libs = {} + @libs = Brakeman::ClassCollection.new @constants = Brakeman::Constants.new @checks = nil @processed = nil diff --git a/lib/brakeman/tracker/class_collection.rb b/lib/brakeman/tracker/class_collection.rb new file mode 100644 index 0000000000..263da6613a --- /dev/null +++ b/lib/brakeman/tracker/class_collection.rb @@ -0,0 +1,165 @@ +require 'set' + +module Brakeman + # Represents a class name, including variations with different scopes. + class ClassName + attr_reader :names + + def initialize name + @names = [name.to_sym] + + unless name.to_s.start_with? "::" + name.to_s.split('::').reverse.inject do |full, current| + @names << full.to_sym + current << "::" << full + end + end + end + + def key + @names.first + end + + def include? name + if name.is_a? ClassName + (@names & name.names).any? + else + @names.include? name + end + end + + def == name + return true if self.object_id == name.object_id + + self.include? name + end + + def to_sym + self.key.to_sym + end + + def to_s + self.key.to_s + end + + def inspect + self.to_sym.inspect + end + end + + # Holds a collection of classes indexed by class name. + class ClassCollection + def initialize + @class_index = {} + @classes = [] + end + + # Add class to collection by name. + def []= name, klass + if name.is_a? ClassName + @class_index[name.key] = klass + else + @class_index[name.to_sym] = klass + end + + @classes << klass + end + + # Look up class by name. + # If `strict` is `true`, only the indexed name will be used. + # + # Otherwise, if the name is not in the index, each class + # will attempt to match the name in a more general way. + def [] name, strict = true + return nil if name.nil? # TODO why are we looking up nil class names? + + if name.is_a? ClassName + if klass = @class_index[name.key] + return klass + elsif strict + return nil + end + elsif klass = @class_index[name] + return klass + elsif strict + return nil + end + + @classes.each do |klass| + return klass if klass.name == name + end + + nil + end + + # Add class by name. + def << klass + self[klass.name] = klass + end + + # Delete class by name. + def delete name + deleted = @class_index.delete name + @classes.delete(deleted) + + deleted # To match Hash behavior + end + + # Iterate over all classes and delete when the block returns true. + def delete_if &block + @classes.delete_if do |klass| + if yield(klass) + @class_index.delete klass.name + true + end + end + end + + # Iterate over each (name, class) pair. + def each &block + @class_index.each &block + end + + # Iterate over each class. + def each_class &block + @class_index.each_value &block + end + + # Return true if the collection has any classes. + def any? + raise ArgumentError if block_given? + + !self.empty? + end + + # Return true if the collection is empty. + def empty? + @classes.empty? + end + + # Returns true if the collection contains the given class name. + # + # *Fuzzy* match on class name. + def include? class_name + !!self[class_name, false] + end + + # Return class names in index. + def keys + @class_index.keys + end + + # Return number of classes in collection. + def length + @classes.length + end + + def select &block + @class_index.select &block + end + + def sort_by &block + @class_index.sort_by &block + end + end +end diff --git a/lib/brakeman/tracker/collection.rb b/lib/brakeman/tracker/collection.rb index bb48de66cc..ececa2c4bd 100644 --- a/lib/brakeman/tracker/collection.rb +++ b/lib/brakeman/tracker/collection.rb @@ -20,7 +20,7 @@ def initialize name, parent, file_name, src, tracker end def ancestor? parent, seen={} - seen[self.name] = true + seen[self.name.key] = true if self.parent == parent or seen[self.parent] true diff --git a/lib/brakeman/tracker/controller.rb b/lib/brakeman/tracker/controller.rb index 699b19acde..10ab27c710 100644 --- a/lib/brakeman/tracker/controller.rb +++ b/lib/brakeman/tracker/controller.rb @@ -43,8 +43,8 @@ def before_filter_list processor, method while controller filters = controller.get_before_filters(processor, method) + filters - controller = tracker.controllers[controller.parent] || - tracker.libs[controller.parent] + controller = tracker.controllers[controller.parent, :strict] || + tracker.libs[controller.parent, :strict] end remove_skipped_filters processor, filters, method @@ -76,8 +76,8 @@ def remove_skipped_filters processor, filters, method while controller filters = filters - controller.get_skipped_filters(processor, method) - controller = tracker.controllers[controller.parent] || - tracker.libs[controller.parent] + controller = tracker.controllers[controller.parent, :strict] || + tracker.libs[controller.parent, :strict] end filters diff --git a/lib/brakeman/tracker/model.rb b/lib/brakeman/tracker/model.rb index 8f10056848..f9651f6d63 100644 --- a/lib/brakeman/tracker/model.rb +++ b/lib/brakeman/tracker/model.rb @@ -28,7 +28,7 @@ def unprotected_model? # go up the chain of parent classes to see if any have attr_accessible def parent_classes_protected? seen={} - seen[self.name] = true + seen[self.name.key] = true if @attr_accessible or self.includes.include? :"ActiveModel::ForbiddenAttributesProtection" true diff --git a/test/tests/class_collection.rb b/test/tests/class_collection.rb new file mode 100644 index 0000000000..26a156a849 --- /dev/null +++ b/test/tests/class_collection.rb @@ -0,0 +1,129 @@ +require_relative "../test" +require "brakeman/tracker/class_collection" + +class TestClassCollection < Minitest::Test + def new_model name = @names.pop + Brakeman::Model.new(name, nil, nil, nil, BrakemanTester.new_tracker) + end + + def setup + @collection = Brakeman::ClassCollection.new + @names = ('A'..'Z').map(&:to_sym) + end + + def test_set_and_get + m = new_model + @collection[m.name] = m + + assert_equal m, @collection[m.name] + end + + def test_get_strict + m = new_model(:"A::B") + @collection[m.name] = m + + assert_equal m, @collection[m.name, :strict] + assert_nil @collection[:B, :strict] + end + + def test_shovel + m = new_model + @collection << m + + assert_equal m, @collection[m.name] + end + + def test_delete + m = new_model + @collection << m + + assert_equal m, @collection[m.name] + + @collection.delete m.name + + assert_nil @collection[m.name] + assert @collection.empty? + end + + def test_delete_if + m1 = new_model + m2 = new_model + @collection << m1 + @collection << m2 + + assert_equal 2, @collection.length + + @collection.delete_if do |klass| + klass == m2 + end + + assert_equal 1, @collection.length + assert_nil @collection[m2.name] + assert_equal m1, @collection[m1.name] + end + + def test_any? + refute @collection.any? + + @collection << new_model + + assert @collection.any? + + assert_raises ArgumentError do + @collection.any? do + end + end + end + + def test_empty? + assert @collection.empty? + + @collection << new_model + + refute @collection.empty? + end +end + +class TestClassName < Minitest::Test + + def test_sanity_equality + c = Brakeman::ClassName.new(:Thing) + d = Brakeman::ClassName.new(:Thing) + + assert_equal c, c + assert_equal c, d + end + + def test_equality + c = Brakeman::ClassName.new(:Thing) + d = Brakeman::ClassName.new(:'A::B::C::Thing') + e = Brakeman::ClassName.new(:'C::Thing') + + assert_equal c, d + assert_equal d, c + assert_equal c, e + assert_equal d, e + end + + def test_collection + x = Brakeman::ClassCollection.new + c = Brakeman::Model.new(:Thing, nil, nil, nil, BrakemanTester.new_tracker) + + x << c + + assert_equal c, x[:Thing] + end + + def test_top + a = Brakeman::ClassName.new(:"::Top") + b = Brakeman::ClassName.new(:"A::Top") + + refute_equal a, b + end + + def test_unknown + tracker = BrakemanTester.new_tracker + + assert tracker.models.include? Brakeman::Tracker::UNKNOWN_MODEL + end +end