Merge branch 'master' into simple_helper_scanning

Conflicts:
	test/tests/test_rails31.rb

README.md

 ![Brakeman Logo](http://brakemanscanner.org/images/logo_medium.png)
 
-![Travis CI Status](https://secure.travis-ci.org/presidentbeef/brakeman.png) [![Code Climate](https://codeclimate.com/badge.png)](https://codeclimate.com/github/presidentbeef/brakeman)
+[![Travis CI Status](https://secure.travis-ci.org/presidentbeef/brakeman.png)](https://travis-ci.org/presidentbeef/brakeman) [![Code Climate](https://codeclimate.com/badge.png)](https://codeclimate.com/github/presidentbeef/brakeman)
 
 # Brakeman
 

lib/brakeman/checks/base_check.rb

@@ -61,7 +61,7 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
   #Process calls and check if they include user input
   def process_call exp
     process exp.target if sexp? exp.target
-    process_all exp.args
+    process_call_args exp
 
     target = exp.target
 
@@ -104,6 +104,12 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
     exp
   end
 
+  #Does not actually process string interpolation, but notes that it occurred.
+  def process_string_interp exp
+    @string_interp = Match.new(:interp, exp)
+    process_default exp
+  end
+
   private
 
   #Report a warning
@@ -138,7 +144,7 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
 
   # go up the chain of parent classes to see if any have attr_accessible
   def parent_classes_protected? model
-    if model[:attr_accessible]
+    if model[:attr_accessible] or model[:includes].include? :"ActiveModel::ForbiddenAttributesProtection"
       true
     elsif parent = tracker.models[model[:parent]]
       parent_classes_protected? parent
@@ -153,21 +159,28 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
 
     @mass_assign_disabled = false
 
-    if version_between?("3.1.0", "4.0.0") and
+    if version_between?("3.1.0", "3.9.9") and
       tracker.config[:rails] and
       tracker.config[:rails][:active_record] and
       tracker.config[:rails][:active_record][:whitelist_attributes] == Sexp.new(:true)
 
+      @mass_assign_disabled = true
+    elsif version_between?("4.0.0", "4.9.9")
+      #May need to revisit dependng on what Rails 4 actually does/has
       @mass_assign_disabled = true
     else
       matches = tracker.check_initializers(:"ActiveRecord::Base", :send)
 
       if matches.empty?
+        #Check for
+        #  class ActiveRecord::Base
+        #    attr_accessible nil
+        #  end
         matches = tracker.check_initializers([], :attr_accessible)
 
         matches.each do |result|
-          if result[1] == "ActiveRecord" and result[2] == :Base
-            arg = result[-1].first_arg
+          if result.module == "ActiveRecord" and result.result_class == :Base
+            arg = result.call.first_arg
 
             if arg.nil? or node_type? arg, :nil
               @mass_assign_disabled = true
@@ -176,9 +189,10 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
           end
         end
       else
+        #Check for ActiveRecord::Base.send(:attr_accessible, nil)
         matches.each do |result|
-          if call? result[-1]
-            call = result[-1]
+          call = result.call
+          if call? call
             if call.first_arg == Sexp.new(:lit, :attr_accessible) and call.second_arg == Sexp.new(:nil)
               @mass_assign_disabled = true
               break
@@ -188,6 +202,23 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
       end
     end
 
+    #There is a chance someone is using Rails 3.x and the `strong_parameters`
+    #gem and still using hack above, so this is a separate check for
+    #including ActiveModel::ForbiddenAttributesProtection in
+    #ActiveRecord::Base in an initializer.
+    if not @mass_assign_disabled and version_between?("3.1.0", "3.9.9") and tracker.config[:gems][:strong_parameters]
+      matches = tracker.check_initializers([], :include)
+
+      matches.each do |result|
+        call = result.call
+        if call? call
+          if call.first_arg == Sexp.new(:colon2, Sexp.new(:const, :ActiveModel), :ForbiddenAttributesProtection)
+            @mass_assign_disabled = true
+          end
+        end
+      end
+    end
+
     @mass_assign_disabled
   end
 
@@ -220,17 +251,6 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
     false
   end
 
-  #Ignores ignores
-  def process_ignore exp
-    exp
-  end
-
-  #Does not actually process string interpolation, but notes that it occurred.
-  def process_string_interp exp
-    @string_interp = Match.new(:interp, exp)
-    exp
-  end
-
   #Checks if an expression contains string interpolation.
   #
   #Returns Match with :interp type if found.

lib/brakeman/checks/check_cross_site_scripting.rb

@@ -60,7 +60,7 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
 
     json_escape_on = false
     initializers = tracker.check_initializers :ActiveSupport, :escape_html_entities_in_json=
-    initializers.each {|result| json_escape_on = true?(result[-1].first_arg) }
+    initializers.each {|result| json_escape_on = true?(result.call.first_arg) }
 
     if !json_escape_on or version_between? "0.0.0", "2.0.99"
       @known_dangerous << :to_json
@@ -231,7 +231,6 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
     end
 
     method = exp.method
-    args = exp.arglist
 
     #Ignore safe items
     if (target.nil? and (@ignore_methods.include? method or method.to_s =~ IGNORE_LIKE)) or
@@ -252,7 +251,7 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
     elsif @inspect_arguments and params? exp
       @matched = Match.new(:params, exp)
     elsif @inspect_arguments
-      process args
+      process_call_args exp
     end
   end
 

lib/brakeman/checks/check_link_to.rb

@@ -23,7 +23,7 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
     @known_dangerous = []
     #Ideally, I think this should also check to see if people are setting
     #:escape => false
-    methods = tracker.find_call :target => false, :method => :link_to 
+    methods = tracker.find_call :target => false, :method => :link_to
 
     @models = tracker.models.keys
     @inspect_arguments = tracker.options[:check_arguments]
@@ -40,23 +40,24 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
     #an ignored method call by the code above.
     call = result[:call] = result[:call].dup
 
-    args = call.args
+    first_arg = call.first_arg
+    second_arg = call.second_arg
 
     @matched = false
 
     #Skip if no arguments(?) or first argument is a hash
-    return if args.first.nil? or hash? args.first
+    return if first_arg.nil? or hash? first_arg
 
     if version_between? "2.0.0", "2.2.99"
-      check_argument result, args.first
+      check_argument result, first_arg
 
-      if args.second and not hash? args.second
-        check_argument result, args.second
+      if second_arg and not hash? second_arg
+        check_argument result, second_arg
       end
-    elsif args.second
+    elsif second_arg
       #Only check first argument if there is a second argument
       #in Rails 2.3.x
-      check_argument result, args.first
+      check_argument result, first_arg
     end
   end
 
@@ -75,7 +76,7 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
 
       add_result result
       warn :result => result,
-        :warning_type => "Cross Site Scripting", 
+        :warning_type => "Cross Site Scripting",
         :message => message,
         :user_input => input.match,
         :confidence => CONFIDENCE[:high],
@@ -94,7 +95,7 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
         end
 
         warn :result => result,
-          :warning_type => "Cross Site Scripting", 
+          :warning_type => "Cross Site Scripting",
           :message => "Unescaped model attribute in link_to",
           :user_input => match,
           :confidence => confidence,
@@ -111,8 +112,8 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
       if message
         add_result result
 
-        warn :result => result, 
-          :warning_type => "Cross Site Scripting", 
+        warn :result => result,
+          :warning_type => "Cross Site Scripting",
           :message => message,
           :user_input => @matched.match,
           :confidence => CONFIDENCE[:med],
@@ -137,6 +138,7 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
 
     #Bare records create links to the model resource,
     #not a string that could have injection
+    #TODO: Needs test? I think this is broken?
     if model_name? target and context == [:call, :arglist]
       return exp
     end

lib/brakeman/checks/check_link_to_href.rb

@@ -34,7 +34,7 @@ class Brakeman::CheckLinkToHref < Brakeman::CheckLinkTo
     #an ignored method call by the code above.
     call = result[:call] = result[:call].dup
     @matched = false
-    url_arg = process call.args.second
+    url_arg = process call.second_arg
 
     #Ignore situations where the href is an interpolated string
     #with something before the user input

lib/brakeman/checks/check_mail_to.rb

@@ -34,7 +34,7 @@ class Brakeman::CheckMailTo < Brakeman::BaseCheck
     Brakeman.debug "Checking calls to mail_to for javascript encoding"
 
     tracker.find_call(:target => false, :method => :mail_to).each do |result|
-      result[:call].arglist.each do |arg|
+      result[:call].each_arg do |arg|
         if hash? arg
           if option = hash_access(arg, :encode)
             return result if symbol? option and option.value == :javascript

lib/brakeman/checks/check_mass_assignment.rb

@@ -78,13 +78,14 @@ class Brakeman::CheckMassAssignment < Brakeman::BaseCheck
 
   #Want to ignore calls to Model.new that have no arguments
   def check_call call
-    args = process_all! call.args
+    process_call_args call
+    first_arg = call.first_arg
 
-    if args.empty? #empty new()
+    if first_arg.nil? #empty new()
       false
-    elsif hash? args.first and not include_user_input? args.first
+    elsif hash? first_arg and not include_user_input? first_arg
       false
-    elsif all_literals? args
+    elsif all_literal_args? call
       false
     else
       true
@@ -93,17 +94,30 @@ class Brakeman::CheckMassAssignment < Brakeman::BaseCheck
 
   LITERALS = Set[:lit, :true, :false, :nil, :string]
 
-  def all_literals? args
-    args.all? do |arg|
-      if sexp? arg
-        if arg.node_type == :hash
-          all_literals? arg
-        else
-          LITERALS.include? arg.node_type
-        end
+  def all_literal_args? exp
+    if call? exp
+      exp.each_arg do |arg|
+        return false unless literal? arg
+      end
+
+      true
+    else
+      exp.all? do |arg|
+        literal? arg
+      end
+    end
+
+  end
+
+  def literal? exp
+    if sexp? exp
+      if exp.node_type == :hash
+        all_literal_args? exp
       else
-        true
+        LITERALS.include? exp.node_type
       end
+    else
+      true
     end
   end
 end

lib/brakeman/checks/check_select_tag.rb

@@ -38,7 +38,7 @@ class Brakeman::CheckSelectTag < Brakeman::BaseCheck
     add_result result
 
     #Only concerned if user input is supplied for :prompt option
-    last_arg = result[:call].arglist.last
+    last_arg = result[:call].last_arg
 
     if hash? last_arg
       prompt_option = hash_access last_arg, :prompt

lib/brakeman/checks/check_select_vulnerability.rb

@@ -35,7 +35,7 @@ class Brakeman::CheckSelectVulnerability < Brakeman::BaseCheck
   def process_result result
     return if duplicate? result
 
-    third_arg = result[:call].args[2]
+    third_arg = result[:call].third_arg
 
     #Check for user input in options parameter
     if sexp? third_arg and include_user_input? third_arg

lib/brakeman/checks/check_send.rb

@@ -16,10 +16,10 @@ class Brakeman::CheckSend < Brakeman::BaseCheck
   end
 
   def process_result result
-    args = process_all! result[:call].args
+    process_call_args result[:call]
     target = process result[:call].target
 
-    if input = has_immediate_user_input?(args.first)
+    if input = has_immediate_user_input?(result[:call].first_arg)
       warn :result => result,
         :warning_type => "Dangerous Send",
         :message => "User controlled method execution",

lib/brakeman/checks/check_session_settings.rb

@@ -42,7 +42,7 @@ class Brakeman::CheckSessionSettings < Brakeman::BaseCheck
   #in Rails 3.x apps
   def process_call exp
     if tracker.options[:rails3] and exp.target == @session_settings and exp.method == :session_store
-        check_for_issues exp.args.second, "#{tracker.options[:app_path]}/config/initializers/session_store.rb"
+        check_for_issues exp.second_arg, "#{tracker.options[:app_path]}/config/initializers/session_store.rb"
     end
       
     exp

lib/brakeman/checks/check_skip_before_filter.rb

@@ -35,10 +35,11 @@ class Brakeman::CheckSkipBeforeFilter < Brakeman::BaseCheck
   def skip_verify_except? filter
     return false unless call? filter
 
-    args = filter.args
+    first_arg = filter.first_arg
+    last_arg = filter.last_arg
 
-    if symbol? args.first and args.first.value == :verify_authenticity_token and hash? args.last
-      if hash_access(args.last, :except)
+    if symbol? first_arg and first_arg.value == :verify_authenticity_token and hash? last_arg
+      if hash_access(last_arg, :except)
         return true
       end
     end

lib/brakeman/checks/check_sql.rb

@@ -173,37 +173,36 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
 
     call = result[:call]
     method = call.method
-    args = call.args
 
     dangerous_value = case method
                       when :find
-                        check_find_arguments args.second
+                        check_find_arguments call.second_arg
                       when :exists?
-                        check_find_arguments args.first
+                        check_find_arguments call.first_arg
                       when :named_scope, :scope
-                        check_scope_arguments call.arglist
+                        check_scope_arguments call
                       when :find_by_sql, :count_by_sql
-                        check_by_sql_arguments args.first
+                        check_by_sql_arguments call.first_arg
                       when :calculate
-                        check_find_arguments args[2]
+                        check_find_arguments call.third_arg
                       when :last, :first, :all
-                        check_find_arguments args.first
+                        check_find_arguments call.first_arg
                       when :average, :count, :maximum, :minimum, :sum
-                        if args.length > 2
-                          unsafe_sql?(args.first) or check_find_arguments(args.last)
+                        if call.length > 5
+                          unsafe_sql?(call.first_arg) or check_find_arguments(call.last_arg)
                         else
-                          check_find_arguments args.last
+                          check_find_arguments call.last_arg
                         end
                       when :where, :having
                         check_query_arguments call.arglist
                       when :order, :group, :reorder
                         check_order_arguments call.arglist
                       when :joins
-                        check_joins_arguments args.first
+                        check_joins_arguments call.first_arg
                       when :from, :select
-                        unsafe_sql? args.first
+                        unsafe_sql? call.first_arg
                       when :lock
-                        check_lock_arguments args.first
+                        check_lock_arguments call.first_arg
                       else
                         Brakeman.debug "Unhandled SQL method: #{method}"
                       end
@@ -226,8 +225,8 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
         :confidence => confidence
     end
 
-    if check_for_limit_or_offset_vulnerability args.last
-      if include_user_input? args.last
+    if check_for_limit_or_offset_vulnerability call.last_arg
+      if include_user_input? call.last_arg
         confidence = CONFIDENCE[:high]
       else
         confidence = CONFIDENCE[:low]
@@ -259,9 +258,8 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
     unsafe_sql? arg
   end
 
-  def check_scope_arguments args
-    return unless node_type? args, :arglist
-    scope_arg = args[2] #first arg is name of scope
+  def check_scope_arguments call
+    scope_arg = call.second_arg #first arg is name of scope
 
     if node_type? scope_arg, :iter
       unsafe_sql? scope_arg.block
@@ -488,9 +486,8 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
 
     target = exp.target
     method = exp.method
-    args = exp.args
 
-    if string? target or string? args.first
+    if string? target or string? exp.first_arg
       if STRING_METHODS.include? method
         return exp
       end

lib/brakeman/checks/check_validation_regex.rb

@@ -13,40 +13,64 @@ class Brakeman::CheckValidationRegex < Brakeman::BaseCheck
   @description = "Report uses of validates_format_of with improper anchors"
 
   WITH = Sexp.new(:lit, :with)
+  FORMAT = Sexp.new(:lit, :format)
 
   def run_check
     active_record_models.each do |name, model|
       @current_model = name
       format_validations = model[:options][:validates_format_of]
+
       if format_validations
         format_validations.each do |v|
-          process_validator v
+          process_validates_format_of v
+        end
+      end
+
+      validates = model[:options][:validates]
+
+      if validates
+        validates.each do |v|
+          process_validates v
         end
       end
     end
   end
 
   #Check validates_format_of
-  def process_validator validator
+  def process_validates_format_of validator
     if value = hash_access(validator.last, WITH)
       check_regex value, validator
     end
   end
 
+  #Check validates ..., :format => ...
+  def process_validates validator
+    hash_arg = validator.last
+    return unless hash? hash_arg
+
+    value = hash_access(hash_arg, FORMAT)
+
+    if hash? value
+      value = hash_access(value, WITH)
+    end
+
+    if value
+      check_regex value, validator
+    end
+  end
+
   #Issue warning if the regular expression does not use
   #+\A+ and +\z+
   def check_regex value, validator
     return unless regexp? value
 
     regex = value.value.inspect
-    if regex =~ /^\/(.{2}).*(.{2})\/(m|i|x|n|e|u|s|o)*\z/
-      if $1 != "\\A" or ($2 != "\\Z" and $2 != "\\z")
-        warn :model => @current_model,
-          :warning_type => "Format Validation", 
-          :message => "Insufficient validation for '#{get_name validator}' using #{value.value.inspect}. Use \\A and \\z as anchors",
-          :line => value.line,
-          :confidence => CONFIDENCE[:high] 
-      end
+    unless regex =~ /\A\/\\A.*\\(z|Z)\/(m|i|x|n|e|u|s|o)*\z/
+      warn :model => @current_model,
+      :warning_type => "Format Validation",
+      :message => "Insufficient validation for '#{get_name validator}' using #{regex}. Use \\A and \\z as anchors",
+      :line => value.line,
+      :confidence => CONFIDENCE[:high]
     end
   end
 

lib/brakeman/checks/check_without_protection.rb

@@ -33,7 +33,7 @@ class Brakeman::CheckWithoutProtection < Brakeman::BaseCheck
   #All results should be Model.new(...) or Model.attributes=() calls
   def process_result res
     call = res[:call]
-    last_arg = call.args.last
+    last_arg = call.last_arg
 
     if hash? last_arg and not call.original_line and not duplicate? res
 

lib/brakeman/processors/alias_processor.rb

@@ -301,11 +301,12 @@ class Brakeman::AliasProcessor < Brakeman::SexpProcessor
     tar_variable = exp.target
     target = exp.target = process(exp.target)
     method = exp.method
-    args = exp.args
+    index_arg = exp.first_arg
+    value_arg = exp.second_arg
 
     if method == :[]=
-      index = exp.first_arg = process(args.first)
-      value = exp.second_arg = process(args.second)
+      index = exp.first_arg = process(index_arg)
+      value = exp.second_arg = process(value_arg)
       match = Sexp.new(:call, target, :[], index)
       env[match] = value
 
@@ -313,7 +314,7 @@ class Brakeman::AliasProcessor < Brakeman::SexpProcessor
         env[tar_variable] = hash_insert target.deep_clone, index, value
       end
     elsif method.to_s[-1,1] == "="
-      value = exp.first_arg = process(args.first)
+      value = exp.first_arg = process(index_arg)
       #This is what we'll replace with the value
       match = Sexp.new(:call, target, method.to_s[0..-2].to_sym)
 

lib/brakeman/processors/base_processor.rb

@@ -6,17 +6,20 @@ class Brakeman::BaseProcessor < Brakeman::SexpProcessor
   include Brakeman::ProcessorHelper
   include Brakeman::Util
 
-  attr_reader :ignore
+  IGNORE = Sexp.new :ignore
 
   #Return a new Processor.
   def initialize tracker
     super()
     @last = nil
     @tracker = tracker
-    @ignore = Sexp.new :ignore
     @current_template = @current_module = @current_class = @current_method = nil
   end
 
+  def ignore
+    IGNORE
+  end
+
   def process_class exp
     current_class = @current_class
     @current_class = class_name exp[1]
@@ -178,7 +181,7 @@ class Brakeman::BaseProcessor < Brakeman::SexpProcessor
 
   #Generates :render node from call to render.
   def make_render exp, in_view = false 
-    render_type, value, rest = find_render_type exp.args, in_view
+    render_type, value, rest = find_render_type exp, in_view
     rest = process rest
     result = Sexp.new(:render, render_type, value, rest)
     result.line(exp.line)
@@ -192,14 +195,14 @@ class Brakeman::BaseProcessor < Brakeman::SexpProcessor
   #:template, :text, :update, :xml
   #
   #And also :layout for inside templates
-  def find_render_type args, in_view = false
+  def find_render_type call, in_view = false
     rest = Sexp.new(:hash)
     type = nil
     value = nil
-    first_arg = args.first
+    first_arg = call.first_arg
 
-    if args.length == 1 and first_arg == Sexp.new(:lit, :update)
-      return :update, nil, Sexp.new(:arglist, *args[0..-2]) #TODO HUH?
+    if call.second_arg.nil? and first_arg == Sexp.new(:lit, :update)
+      return :update, nil, Sexp.new(:arglist, *call.args[0..-2]) #TODO HUH?
     end
 
     #Look for render :action, ... or render "action", ...
@@ -228,10 +231,12 @@ class Brakeman::BaseProcessor < Brakeman::SexpProcessor
       types_in_hash << :layout
     end
 
+    last_arg = call.last_arg
+
     #Look for "type" of render in options hash
     #For example, render :file => "blah"
-    if hash? args.last
-      hash_iterate(args.last) do |key, val|
+    if hash? last_arg
+      hash_iterate(last_arg) do |key, val|
         if symbol? key and types_in_hash.include? key.value
           type = key.value
           value = val

lib/brakeman/processors/controller_processor.rb

@@ -64,12 +64,13 @@ class Brakeman::ControllerProcessor < Brakeman::BaseProcessor
     end
 
     method = exp.method
-    args = exp.args
+    first_arg = exp.first_arg
+    last_arg = exp.last_arg
 
     #Methods called inside class definition
     #like attr_* and other settings
     if @current_method.nil? and target.nil? and @controller
-      if args.empty?
+      if first_arg.nil? #No args
         case method
         when :private, :protected, :public
           @visibility = method
@@ -81,21 +82,21 @@ class Brakeman::ControllerProcessor < Brakeman::BaseProcessor
       else
         case method
         when :include
-          @controller[:includes] << class_name(args.first) if @controller
+          @controller[:includes] << class_name(first_arg) if @controller
         when :before_filter
           @controller[:options][:before_filters] ||= []
-          @controller[:options][:before_filters] << args
+          @controller[:options][:before_filters] << exp.args
         when :layout
-          if string? args.last
+          if string? last_arg
             #layout "some_layout"
 
-            name = args.last.value.to_s
+            name = last_arg.value.to_s
             if @app_tree.layout_exists?(name)
               @controller[:layout] = "layouts/#{name}"
             else
               Brakeman.debug "[Notice] Layout not found: #{name}"
             end
-          elsif node_type? args.last, :nil, :false
+          elsif node_type? last_arg, :nil, :false
             #layout :false or layout nil
             @controller[:layout] = false
           end

lib/brakeman/processors/erb_template_processor.rb

@@ -19,7 +19,7 @@ class Brakeman::ErbTemplateProcessor < Brakeman::TemplateProcessor
         exp.arglist = process(exp.arglist)
         @inside_concat = false
 
-        if exp.args.length > 2
+        if exp.second_arg
           raise Exception.new("Did not expect more than a single argument to _erbout.concat")
         end
 

lib/brakeman/processors/gem_processor.rb

@@ -27,12 +27,13 @@ class Brakeman::GemProcessor < Brakeman::BaseProcessor
 
   def process_call exp
     if exp.target == nil and exp.method == :gem
-      args = exp.args
+      gem_name = exp.first_arg
+      gem_version = exp.second_arg
 
-      if string? args.second
-        @tracker.config[:gems][args.first.value.to_sym] = args.second.value
+      if string? gem_version
+        @tracker.config[:gems][gem_name.value.to_sym] = gem_version.value
       else
-        @tracker.config[:gems][args.first.value.to_sym] = ">=0.0.0"
+        @tracker.config[:gems][gem_name.value.to_sym] = ">=0.0.0"
       end
     end
 

lib/brakeman/processors/haml_template_processor.rb

@@ -36,14 +36,13 @@ class Brakeman::HamlTemplateProcessor < Brakeman::TemplateProcessor
             when :options, :buffer
               exp
             when :open_tag
-              process(exp.arglist)
-              exp
+              process_call_args exp
             else
               arg = exp.first_arg
 
               if arg
                 @inside_concat = true
-                out = exp.arglist[1] = process(arg)
+                out = exp.first_arg = process(arg)
                 @inside_concat = false
               else
                 raise Exception.new("Empty _hamlout.#{method}()?")
@@ -78,7 +77,7 @@ class Brakeman::HamlTemplateProcessor < Brakeman::TemplateProcessor
       #Has something to do with values of blocks?
     elsif sexp? target and method == :<< and is_buffer_target? target
       @inside_concat = true
-      out = exp.arglist[1] = process(exp.arglist[1])
+      out = exp.first_arg = process(exp.first_arg)
       @inside_concat = false
 
       if out.node_type == :str #ignore plain strings

lib/brakeman/processors/lib/find_all_calls.rb

@@ -46,7 +46,7 @@ class Brakeman::FindAllCalls < Brakeman::BaseProcessor
     end
 
     method = exp.method
-    process_all exp.args
+    process_call_args exp
 
     call = { :target => target, :method => method, :call => exp, :nested => @in_target, :chain => get_chain(exp) }
     

lib/brakeman/processors/lib/find_call.rb

@@ -84,7 +84,7 @@ class Brakeman::FindCall < Brakeman::BaseProcessor
     target = get_target exp.target
     method = exp.method
 
-    process_all exp.args
+    process_call_args exp
 
     if match(@find_targets, target) and match(@find_methods, method)
 

lib/brakeman/processors/lib/processor_helper.rb

@@ -19,6 +19,16 @@ module Brakeman::ProcessorHelper
     exp
   end
 
+  #Process the arguments of a method call. Does not store results.
+  #
+  #This method is used because Sexp#args and Sexp#arglist create new objects.
+  def process_call_args exp
+    exp.each_arg do |a|
+      process a if sexp? a
+    end
+
+    exp
+  end
   #Sets the current module.
   def process_module exp
     module_name = class_name(exp.class_name).to_s

lib/brakeman/processors/lib/rails2_route_processor.rb

@@ -215,7 +215,7 @@ class Brakeman::Rails2RoutesProcessor < Brakeman::BaseProcessor
   #   something.resources :blah
   # end
   def process_with_options exp
-    @with_options = exp.block_call.args.last
+    @with_options = exp.block_call.last_arg
     @nested = Sexp.new(:lvar, exp.block_args.value)
 
     self.current_controller = check_for_controller_name exp.block_call.args

lib/brakeman/processors/lib/rails3_route_processor.rb

@@ -72,9 +72,7 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
 
   #TODO: Need test for this
   def process_root exp
-    args = exp.args
-
-    if value = hash_access(args.first, :to)
+    if value = hash_access(exp.first_arg, :to)
       if string? value
         add_route_from_string value
       end
@@ -84,27 +82,29 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
   end
 
   def process_match exp
-    args = exp.args
+    first_arg = exp.first_arg
+    second_arg = exp.second_arg
+    last_arg = exp.last_arg
 
     #Check if there is an unrestricted action parameter
     action_variable = false
 
-    if string? args.first
-      matcher = args.first.value
+    if string? first_arg
+      matcher = first_arg.value
 
       if matcher == ':controller(/:action(/:id(.:format)))' or
         matcher.include? ':controller' and matcher.include? ':action' #Default routes
-        @tracker.routes[:allow_all_actions] = args.first
+        @tracker.routes[:allow_all_actions] = first_arg
         return exp
       elsif matcher.include? ':action'
         action_variable = true
-      elsif args[1].nil? and in_controller_block? and not matcher.include? ":"
+      elsif second_arg.nil? and in_controller_block? and not matcher.include? ":"
         add_route matcher
       end
     end
 
-    if hash? args.last
-      hash_iterate args.last do |k, v|
+    if hash? last_arg
+      hash_iterate last_arg do |k, v|
         if string? k
           if string? v
             add_route_from_string v
@@ -153,13 +153,13 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
   end
 
   def process_verb exp
-    args = exp.args
-    first_arg = args.first
+    first_arg = exp.first_arg
+    second_arg = exp.second_arg
 
-    if symbol? first_arg and not hash? args.second
+    if symbol? first_arg and not hash? second_arg
       add_route first_arg
-    elsif hash? args.second
-      hash_iterate args.second do |k, v|
+    elsif hash? second_arg
+      hash_iterate second_arg do |k, v|
         if symbol? k and k.value == :to
           if string? v
             add_route_from_string v
@@ -194,12 +194,15 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
   end
 
   def process_resources exp
-    if exp.args and exp.args.second and exp.args.second.node_type == :hash
-      self.current_controller = exp.first_arg.value
+    first_arg = exp.first_arg
+    second_arg = exp.second_arg
+
+    if second_arg and second_arg.node_type == :hash
+      self.current_controller = first_arg.value
       #handle hash
       add_resources_routes
     elsif exp.args.all? { |s| symbol? s }
-      exp.args.each do |s|
+      exp.each_arg do |s|
         self.current_controller = s.value
         add_resources_routes
       end
@@ -211,7 +214,7 @@ class Brakeman::Rails3RoutesProcessor < Brakeman::BaseProcessor
 
   def process_resource exp
     #Does resource even take more than one controller name?
-    exp.args.each do |s|
+    exp.each_arg do |s|
       if symbol? s
         self.current_controller = pluralize(s.value.to_s)
         add_resource_routes

lib/brakeman/processors/model_processor.rb

@@ -60,12 +60,12 @@ class Brakeman::ModelProcessor < Brakeman::BaseProcessor
     end
 
     method = exp.method
-    args = exp.args
+    first_arg = exp.first_arg
 
     #Methods called inside class definition
     #like attr_* and other settings
     if @current_method.nil? and target.nil?
-      if args.empty?
+      if first_arg.nil?
         case method
         when :private, :protected, :public
           @visibility = method
@@ -77,16 +77,16 @@ class Brakeman::ModelProcessor < Brakeman::BaseProcessor
       else
         case method
         when :include
-          @model[:includes] << class_name(args.first) if @model
+          @model[:includes] << class_name(first_arg) if @model
         when :attr_accessible
           @model[:attr_accessible] ||= []
-          args = args.map do |e|
+          args = []
+
+          exp.each_arg do |e|
             if node_type? e, :lit
-              e.value
-            else
-              nil
+              args << e.value
             end
-          end.compact
+          end
 
           @model[:attr_accessible].concat args
         else

lib/brakeman/processors/template_alias_processor.rb

 require 'set'
 require 'brakeman/processors/alias_processor'
 require 'brakeman/processors/lib/render_helper'
+require 'brakeman/tracker'
 
 #Processes aliasing in templates.
 #Handles calls to +render+.
@@ -37,6 +38,9 @@ class Brakeman::TemplateAliasProcessor < Brakeman::AliasProcessor
     name
   end
 
+  UNKNOWN_MODEL_CALL = Sexp.new(:call, Sexp.new(:const, Brakeman::Tracker::UNKNOWN_MODEL), :new)
+  FORM_BUILDER_CALL = Sexp.new(:call, Sexp.new(:const, :FormBuilder), :new)
+
   #Looks for form methods and iterating over collections of Models
   def process_call_with_block exp
     process_default exp
@@ -55,14 +59,14 @@ class Brakeman::TemplateAliasProcessor < Brakeman::AliasProcessor
           if model == target.target
             env[Sexp.new(:lvar, arg)] = Sexp.new(:call, model, :new)
           else
-            env[Sexp.new(:lvar, arg)] = Sexp.new(:call, Sexp.new(:const, Brakeman::Tracker::UNKNOWN_MODEL), :new)
+            env[Sexp.new(:lvar, arg)] = UNKNOWN_MODEL_CALL
           end
 
           process block if sexp? block
         end
       elsif FORM_METHODS.include? method
         if arg.is_a? Symbol
-          env[Sexp.new(:lvar, arg)] = Sexp.new(:call, Sexp.new(:const, :FormBuilder), :new)
+          env[Sexp.new(:lvar, arg)] = FORM_BUILDER_CALL
 
           process block if sexp? block
         end

lib/brakeman/util.rb

@@ -96,12 +96,17 @@ module Brakeman::Util
     nil
   end
 
+  #These are never modified
+  PARAMS_SEXP = Sexp.new(:params)
+  SESSION_SEXP = Sexp.new(:session)
+  COOKIES_SEXP = Sexp.new(:cookies)
+
   #Adds params, session, and cookies to environment
   #so they can be replaced by their respective Sexps.
   def set_env_defaults
-    @env[PARAMETERS] = Sexp.new(:params)
-    @env[SESSION] = Sexp.new(:session)
-    @env[COOKIES] = Sexp.new(:cookies)
+    @env[PARAMETERS] = PARAMS_SEXP
+    @env[SESSION] = SESSION_SEXP
+    @env[COOKIES] = COOKIES_SEXP
   end
 
   #Check if _exp_ represents a hash: s(:hash, {...})

lib/ruby_parser/bm_sexp.rb

@@ -147,13 +147,15 @@ class Sexp
   #s(:call, s(:call, nil, :x, s(:arglist)), :y, s(:arglist, s(:lit, 1)))
   #                        ^- method
   def method
-    expect :call, :attrasgn, :super, :zsuper
+    expect :call, :attrasgn, :super, :zsuper, :result
 
     case self.node_type
     when :call, :attrasgn
       self[2]
     when :super, :zsuper
       :super
+    when :result
+      self.last
     end
   end
 
@@ -219,6 +221,35 @@ class Sexp
     end
   end
 
+  def each_arg replace = false
+    expect :call, :attrasgn, :super, :zsuper
+    range = nil
+
+    case self.node_type
+    when :call, :attrasgn
+      if self[3]
+        range = (3...self.length)
+      end
+    when :super, :zsuper
+      if self[1]
+        range = (1...self.length)
+      end
+    end
+
+    if range
+      range.each do |i|
+        res = yield self[i]
+        self[i] = res if replace
+      end
+    end
+
+    self
+  end
+
+  def each_arg! &block
+    self.each_arg true, &block
+  end
+
   #Returns first argument of a method call.
   def first_arg
     expect :call, :attrasgn
@@ -243,6 +274,26 @@ class Sexp
     self[4] = exp
   end
 
+  def third_arg
+    expect :call, :attrasgn
+    self[5]
+  end
+
+  def third_arg= exp
+    expect :call, :attrasgn
+    self[5] = exp
+  end
+
+  def last_arg
+    expect :call, :attrasgn
+
+    if self[3]
+      self[-1]
+    else
+      nil
+    end
+  end
+
   #Returns condition of an if expression:
   #
   #    s(:if,
@@ -443,6 +494,27 @@ class Sexp
     expect :class
     self[2]
   end
+
+  #Returns the call Sexp in a result returned from FindCall
+  def call
+    expect :result
+
+    self.last
+  end
+
+  #Returns the module the call is inside
+  def module
+    expect :result
+
+    self[1]
+  end
+
+  #Return the class the call is inside
+  def result_class
+    expect :result
+
+    self[2]
+  end
 end
 
 #Invalidate hash cache if the Sexp changes

test/apps/rails3.1/app/models/account.rb

+class Account < ActiveRecord::Base
+  validates :username, :length => 6..20, :format => /([a-z][0-9])+/i
+  validates :phone, :format => { :with => /(\d{3})-(\d{3})-(\d{4})/, :on => :create }, :presence => true 
+  validates :first_name, :format => /\w+/
+end

test/apps/rails3/app/controllers/other_controller.rb

@@ -43,4 +43,13 @@ class OtherController < ApplicationController
   def test_mail_to
     @user = User.find(current_user)
   end
+
+  def test_command_injection_locals
+    `#{some_command}`
+    system("ls #{some_files}")
+  end
+
+  def test_mass_assign_with_strong_params
+    Bill.create(params[:charge])
+  end
 end

test/apps/rails3/app/models/bill.rb

+class Bill < ActiveRecord::Base
+  include ActiveModel::ForbiddenAttributesProtection
+end

test/tests/test_mass_assign_disable.rb

@@ -36,4 +36,30 @@ class MassAssignDisableTest < Test::Unit::TestCase
       end
     RUBY
   end
+
+  def test_strong_parameters_in_initializer
+    init = "config/initializers/mass_assign.rb"
+    gemfile = "Gemfile"
+    config = "config/application.rb"
+
+    before_rescan_of [init, gemfile, config], "rails3.2" do
+      write_file init, <<-RUBY
+        class ActiveRecord::Base
+          include ActiveModel::ForbiddenAttributesProtection
+        end
+      RUBY
+
+      append gemfile, "gem 'strong_parameters'"
+
+      replace config, "config.active_record.whitelist_attributes = true",
+        "config.active_record.whitelist_attributes = false"
+    end
+
+    #We disable whitelist, but add strong_parameters globally, so
+    #there should be no change.
+    assert_reindex :none
+    assert_changes
+    assert_fixed 0
+    assert_new 0
+  end
 end

test/tests/test_rails3.rb

@@ -15,7 +15,7 @@ class Rails3Tests < Test::Unit::TestCase
       :controller => 1,
       :model => 5,
       :template => 30,
-      :warning => 31
+      :warning => 33
     }
   end
 
@@ -63,6 +63,24 @@ class Rails3Tests < Test::Unit::TestCase
       :file => /home_controller\.rb/
   end
 
+  def test_command_injection_non_user_input_backticks
+    assert_warning :type => :warning,
+      :warning_type => "Command Injection",
+      :line => 48,
+      :message => /^Possible\ command\ injection/,
+      :confidence => 1,
+      :file => /other_controller\.rb/
+  end
+
+  def test_command_injection_non_user_input_system
+    assert_warning :type => :warning,
+      :warning_type => "Command Injection",
+      :line => 49,
+      :message => /^Possible\ command\ injection/,
+      :confidence => 1,
+      :file => /other_controller\.rb/
+  end
+
   def test_file_access_concatenation
     assert_warning :type => :warning,
       :warning_type => "File Access",
@@ -571,6 +589,15 @@ class Rails3Tests < Test::Unit::TestCase
       :file => /account\.rb/
   end
 
+  def test_mass_assign_with_strong_params
+    assert_no_warning :type => :warning,
+      :warning_type => "Mass Assignment",
+      :line => 53,
+      :message => /^Unprotected\ mass\ assignment/,
+      :confidence => 0,
+      :file => /other_controller\.rb/
+  end
+
   def test_translate_bug
     assert_warning :type => :warning,
       :warning_type => "Cross Site Scripting",

test/tests/test_rails31.rb

@@ -12,7 +12,7 @@ class Rails31Tests < Test::Unit::TestCase
 
   def expected
     @expected ||= {
-      :model => 0,
+      :model => 3,
       :template => 22,
       :controller => 1,
       :warning => 48 }
@@ -730,4 +730,31 @@ class Rails31Tests < Test::Unit::TestCase
       :confidence => 1,
       :file => /product\.rb/
   end
+
+  def test_validates_format
+    assert_warning :type => :model,
+      :warning_type => "Format Validation",
+      :line => 2,
+      :message => /^Insufficient\ validation\ for\ 'username'\ u/,
+      :confidence => 0,
+      :file => /account\.rb/
+  end
+
+  def test_validates_format_with
+    assert_warning :type => :model,
+      :warning_type => "Format Validation",
+      :line => 3,
+      :message => /^Insufficient\ validation\ for\ 'phone'\ usin/,
+      :confidence => 0,
+      :file => /account\.rb/
+  end
+
+  def test_validates_format_with_short_regex
+    assert_warning :type => :model,
+      :warning_type => "Format Validation",
+      :line => 4,
+      :message => /^Insufficient\ validation\ for\ 'first_name'/,
+      :confidence => 0,
+      :file => /account\.rb/
+  end
 end

test/tests/test_sexp.rb

@@ -18,6 +18,7 @@ class SexpTests < Test::Unit::TestCase
     assert_equal s(:arglist), exp.arglist
     assert_nil exp.first_arg
     assert_nil exp.second_arg
+    assert_nil exp.last_arg
   end
 
   def test_method_call_with_args
@@ -29,6 +30,7 @@ class SexpTests < Test::Unit::TestCase
     assert_equal s(:arglist, s(:lit, 1), s(:lit, 2), s(:lit, 3)), exp.arglist
     assert_equal s(:lit, 1), exp.first_arg
     assert_equal s(:lit, 2), exp.second_arg
+    assert_equal s(:lit, 3), exp.last_arg
   end
 
   def test_method_call_no_target
@@ -40,6 +42,7 @@ class SexpTests < Test::Unit::TestCase
     assert_equal s(:arglist, s(:lit, 1), s(:lit, 2), s(:lit, 3)), exp.arglist
     assert_equal s(:lit, 1), exp.first_arg
     assert_equal s(:lit, 2), exp.second_arg
+    assert_equal s(:lit, 3), exp.last_arg
   end
 
   def test_method_call_set_target
@@ -55,6 +58,7 @@ class SexpTests < Test::Unit::TestCase
 
     assert_equal s(:lit, 1), exp.first_arg
     assert_equal s(:lit, 2), exp.second_arg
+    assert_equal s(:lit, 2), exp.last_arg
     assert_equal s(:arglist, s(:lit, 1), s(:lit, 2)), exp.arglist
     assert_equal s(s(:lit, 1), s(:lit, 2)), exp.args
   end
@@ -69,6 +73,7 @@ class SexpTests < Test::Unit::TestCase
     assert_equal s(s(:lit, 1), s(:lit, 2)), exp.args
     assert_equal s(:lit,1), exp.first_arg
     assert_equal s(:lit, 2), exp.second_arg
+    assert_equal s(:lit, 2), exp.last_arg
   end
 
   def test_method_call_with_block
@@ -277,4 +282,25 @@ class SexpTests < Test::Unit::TestCase
     assert_equal s(:iasgn, :@x, s(:lit, 1)), exp.iasgn(true)
     assert_equal nil, exp.iasgn #Was deleted
   end
+
+  def test_each_arg
+    exp = parse "blah 1, 2, 3"
+
+    args = []
+    exp.each_arg do |a|
+      args << a.value
+    end
+
+    assert_equal [1,2,3], args
+  end
+
+  def test_each_arg!
+    exp = parse "blah 1, 2"
+    exp.each_arg! do |a|
+      s(:lit, a.value + 1)
+    end
+
+    assert_equal s(:lit, 2), exp.first_arg
+    assert_equal s(:lit, 3), exp.second_arg
+  end
 end