Merge branch 'main' into github-reporter

CHANGES.md

@@ -1,3 +1,12 @@
+# 5.0.1 - 2021-04-27
+
+* Detect `::Rails.application.configure` too
+* Set more line numbers on Sexps
+* Support loading `slim/smart`
+* Don't fail if $HOME/$USER are not defined
+* Always ignore slice/only calls for mass assignment
+* Convert splat array arguments to arguments
+
 # 5.0.0 - 2021-01-26
 
 * Ignore `uuid` as a safe attribute

gem_common.rb

@@ -8,6 +8,7 @@ def self.dev_dependencies spec
     end
 
     def self.base_dependencies spec
+      spec.add_dependency "parallel", "~>1.20"
       spec.add_dependency "ruby_parser", "~>3.13"
       spec.add_dependency "ruby_parser-legacy", "~>1.0"
       spec.add_dependency "sexp_processor", "~> 4.7"

lib/brakeman/file_parser.rb

@@ -1,3 +1,5 @@
+require 'parallel'
+
 module Brakeman
   ASTFile = Struct.new(:path, :ast)
 
@@ -13,21 +15,46 @@ def initialize app_tree, timeout
     end
 
     def parse_files list
-      read_files list do |path, contents|
-        if ast = parse_ruby(contents, path.relative)
-          ASTFile.new(path, ast)
+      # Parse the files in parallel.
+      # By default, the parsing will be in separate processes.
+      # So we map the result to ASTFiles and/or Exceptions
+      # then partition them into ASTFiles and Exceptions
+      # and add the Exceptions to @errors
+      #
+      # Basically just a funky way to deal with two possible
+      # return types that are returned from isolated processes.
+      #
+      # Note this method no longer uses read_files
+      @file_list, new_errors = Parallel.map(list) do |file_name|
+        file_path = @app_tree.file_path(file_name)
+        contents = file_path.read
+
+        begin
+          if ast = parse_ruby(contents, file_path.relative)
+            ASTFile.new(file_name, ast)
+          end
+        rescue Exception => e
+          e
         end
+      end.compact.partition do |result|
+        result.is_a? ASTFile
       end
+
+      errors.concat new_errors
     end
 
     def read_files list
       list.each do |path|
         file = @app_tree.file_path(path)
 
-        result = yield file, file.read
+        begin
+          result = yield file, file.read
 
-        if result
-          @file_list << result
+          if result
+            @file_list << result
+          end
+        rescue Exception => e
+          @errors << e
         end
       end
     end
@@ -42,17 +69,12 @@ def parse_ruby input, path
         Brakeman.debug "Parsing #{path}"
         RubyParser.new.parse input, path, @timeout
       rescue Racc::ParseError => e
-        error e.exception(e.message + "\nCould not parse #{path}")
+        raise e.exception(e.message + "\nCould not parse #{path}")
       rescue Timeout::Error => e
-        error Exception.new("Parsing #{path} took too long (> #{@timeout} seconds). Try increasing the limit with --parser-timeout")
+        raise Exception.new("Parsing #{path} took too long (> #{@timeout} seconds). Try increasing the limit with --parser-timeout")
       rescue => e
-        error e.exception(e.message + "\nWhile processing #{path}")
+        raise e.exception(e.message + "\nWhile processing #{path}")
       end
     end
-
-    def error exception
-      @errors << exception
-      nil
-    end
   end
 end

lib/brakeman/processors/alias_processor.rb

@@ -201,11 +201,11 @@ def process_call exp
       res = process_or_simple_operation(exp)
       return res if res
     elsif target == ARRAY_CONST and method == :new
-      return Sexp.new(:array, *exp.args)
+      return Sexp.new(:array, *exp.args).line(exp.line)
     elsif target == HASH_CONST and method == :new and first_arg.nil? and !node_type?(@exp_context.last, :iter)
-      return Sexp.new(:hash)
+      return Sexp.new(:hash).line(exp.line)
     elsif exp == RAILS_TEST or exp == RAILS_DEV
-      return Sexp.new(:false)
+      return Sexp.new(:false).line(exp.line)
     end
 
     #See if it is possible to simplify some basic cases
@@ -243,7 +243,7 @@ def process_call exp
         env[target_var] = target
         return target
       elsif string? target and string_interp? first_arg
-        exp = Sexp.new(:dstr, target.value + first_arg[1]).concat(first_arg.sexp_body(2))
+        exp = Sexp.new(:dstr, target.value + first_arg[1]).concat(first_arg.sexp_body(2)).line(exp.line)
         env[target_var] = exp
       elsif string? first_arg and string_interp? target
         if string? target.last
@@ -294,7 +294,7 @@ def process_call exp
 
   # Painful conversion of Array#join into string interpolation
   def process_array_join array, join_str
-    result = s()
+    result = s().line(array.line)
 
     join_value = if string? join_str
                    join_str.value
@@ -332,11 +332,11 @@ def process_array_join array, join_str
     result.unshift combined_first
 
     # Have to fix up strings that follow interpolation
-    result.reduce(s(:dstr)) do |memo, e|
+    result.reduce(s(:dstr).line(array.line)) do |memo, e|
       if string? e and node_type? memo.last, :evstr
         e.value = "#{join_value}#{e.value}"
       elsif join_value and node_type? memo.last, :evstr and node_type? e, :evstr
-        memo << s(:str, join_value)
+        memo << s(:str, join_value).line(e.line)
       end
 
       memo << e
@@ -347,9 +347,9 @@ def join_item item, join_value
     if item.is_a? String
       "#{item}#{join_value}"
     elsif string? item or symbol? item or number? item
-      s(:str, "#{item.value}#{join_value}")
+      s(:str, "#{item.value}#{join_value}").line(item.line)
     else
-      s(:evstr, item)
+      s(:evstr, item).line(item.line)
     end
   end
 
@@ -690,7 +690,7 @@ def process_op_asgn1 exp
         end
       end
     else
-      new_value = process s(:call, s(:call, target_var, :[], index), exp[3], value)
+      new_value = process s(:call, s(:call, target_var, :[], index), exp[3], value).line(exp.line)
 
       env[match] = new_value
     end

lib/brakeman/processors/base_processor.rb

@@ -8,7 +8,7 @@ class Brakeman::BaseProcessor < Brakeman::SexpProcessor
   include Brakeman::SafeCallHelper
   include Brakeman::Util
 
-  IGNORE = Sexp.new :ignore
+  IGNORE = Sexp.new(:ignore).line(0)
 
   #Return a new Processor.
   def initialize tracker
@@ -216,7 +216,7 @@ def make_render exp, in_view = false
   #
   #And also :layout for inside templates
   def find_render_type call, in_view = false
-    rest = Sexp.new(:hash)
+    rest = Sexp.new(:hash).line(call.line)
     type = nil
     value = nil
     first_arg = call.first_arg
@@ -236,7 +236,7 @@ def find_render_type call, in_view = false
       end
     elsif first_arg.is_a? Symbol or first_arg.is_a? String
       type = :action
-      value = Sexp.new(:lit, first_arg.to_sym)
+      value = Sexp.new(:lit, first_arg.to_sym).line(call.line)
     elsif first_arg.nil?
       type = :default
     elsif not hash? first_arg
@@ -293,6 +293,6 @@ def make_inline_render value, options
     @tracker.processor.process_template(template_name, ast, type, nil, @current_file)
     @tracker.processor.process_template_alias(@tracker.templates[template_name])
 
-    return s(:lit, template_name), options
+    return s(:lit, template_name).line(value.line), options
   end
 end

lib/brakeman/processors/lib/rails4_config_processor.rb

@@ -2,10 +2,11 @@
 
 class Brakeman::Rails4ConfigProcessor < Brakeman::Rails3ConfigProcessor
   APPLICATION_CONFIG = s(:call, s(:call, s(:const, :Rails), :application), :configure)
+  ALT_APPLICATION_CONFIG = s(:call, s(:call, s(:colon3, :Rails), :application), :configure)
 
   # Look for Rails.application.configure do ... end
   def process_iter exp
-    if exp.block_call == APPLICATION_CONFIG
+    if exp.block_call == APPLICATION_CONFIG or exp.block_call == ALT_APPLICATION_CONFIG
       @inside_config = true
       process exp.block if sexp? exp.block
       @inside_config = false

lib/brakeman/scanner.rb

@@ -353,6 +353,9 @@ def index_call_sites
   def parse_ruby_file file
     fp = Brakeman::FileParser.new(tracker.app_tree, tracker.options[:parser_timeout])
     fp.parse_ruby(file.read, file)
+  rescue Exception => e
+    tracker.error(e)
+    nil
   end
 end
 

lib/brakeman/version.rb

@@ -1,3 +1,3 @@
 module Brakeman
-  Version = "5.0.0"
+  Version = "5.0.1"
 end

test/apps/rails6/config/environments/production.rb

@@ -1,4 +1,4 @@
-Rails.application.configure do
+::Rails.application.configure do
   # Settings specified here will take precedence over those in config/application.rb.
 
   # Code is not reloaded between requests.

test/tests/alias_processor.rb

@@ -1151,6 +1151,18 @@ def test_array_join_lots_of_interp
     INPUT
   end
 
+  def test_array_join_line_numbers
+    # Test that line numbers are set for all parts of a joined string
+    original_sexp = RubyParser.new.parse "z = [x, 1].join(' ')"
+    processed_sexp = Brakeman::AliasProcessor.new.process_safely(original_sexp)
+
+    assert_equal s(:lasgn, :z, s(:dstr, "", s(:evstr, s(:call, nil, :x)), s(:str, " 1"))), processed_sexp
+    assert_equal 1, processed_sexp[2].line
+    assert_equal 1, processed_sexp[2][2].line
+    assert_equal 1, processed_sexp[2][2][1].line
+    assert_equal 1, processed_sexp[2][3].line
+  end
+
   def test_ignore_freeze
     assert_alias "blah", <<-INPUT
     x = blah.freeze

test/tests/file_parser.rb

@@ -1,4 +1,5 @@
 require_relative '../test'
+require 'tempfile'
 
 class FileParserTests < Minitest::Test
   def setup
@@ -8,21 +9,27 @@ def setup
   end
 
   def test_parse_error
-    @file_parser.parse_ruby <<-RUBY, "/tmp/BRAKEMAN_FAKE_PATH/test.rb"
-        x =
-    RUBY
+    tempfile = Tempfile.new
+    tempfile.write("x = ")
+    tempfile.close
 
+    @file_parser.parse_files([tempfile.path])
     @tracker.add_errors(@file_parser.errors)
 
     assert_equal 1, @tracker.errors.length
+  ensure
+    tempfile.unlink
   end
 
   def test_parse_error_shows_newer_failure
-    @file_parser.parse_ruby <<-RUBY, "/tmp/BRAKEMAN_FAKE_PATH/test.rb"
+    tempfile = Tempfile.new
+    tempfile.write <<-RUBY
     blah(x: 1)
     thing do
     RUBY
+    tempfile.close
 
+    @file_parser.parse_files([tempfile.path])
     @tracker.add_errors(@file_parser.errors)
 
     assert_equal 1, @tracker.errors.length
@@ -34,6 +41,20 @@ def test_parse_error_shows_newer_failure
     end
   end
 
+  def test_read_files_reports_error
+    tempfile = Tempfile.new
+    tempfile.write("x = ")
+    tempfile.close
+
+    @file_parser.read_files([tempfile.path]) do |path, contents|
+      @file_parser.parse_ruby contents, path
+    end
+
+    @tracker.add_errors(@file_parser.errors)
+
+    assert_equal 1, @tracker.errors.length
+  end
+
   def test_parse_ruby_accepts_file_path
     file_path = Brakeman::FilePath.from_app_tree @tracker.app_tree, "config/test.rb"