Get rid of many of Sexp#args and Sexp#arglist calls

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
 

lib/brakeman/checks/check_cross_site_scripting.rb

@@ -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_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_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

lib/brakeman/processors/alias_processor.rb

@@ -299,11 +299,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
 
@@ -311,7 +312,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

@@ -165,7 +165,7 @@ class Brakeman::BaseProcessor < Brakeman::SexpProcessor
   def process_attrasgn exp
     exp = exp.dup
     exp.target = process exp.target
-    exp.arglist = process exp.arglist
+    process_call_args! exp
     exp
   end
 

lib/brakeman/processors/erb_template_processor.rb

@@ -16,10 +16,10 @@ class Brakeman::ErbTemplateProcessor < Brakeman::TemplateProcessor
     if node_type? target, :lvar and target.value == :_erbout
       if method == :concat
         @inside_concat = true
-        exp.arglist = process(exp.arglist)
+        process_call_args! exp
         @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
 
@@ -43,7 +43,7 @@ class Brakeman::ErbTemplateProcessor < Brakeman::TemplateProcessor
         abort "Unrecognized action on _erbout: #{method}"
       end
     elsif target == nil and method == :render
-      exp.arglist = process(exp.arglist)
+      process_call_args! exp
       make_render_in_view exp
     else
       #TODO: Is it really necessary to create a new Sexp here?

lib/brakeman/processors/erubis_template_processor.rb

@@ -14,7 +14,7 @@ class Brakeman::ErubisTemplateProcessor < Brakeman::TemplateProcessor
     #_buf is the default output variable for Erubis
     if node_type?(target, :lvar, :ivar) and (target.value == :_buf or target.value == :@output_buffer)
       if method == :<< or method == :safe_concat
-        exp.arglist = process exp.arglist
+        process_call_args! exp
 
         arg = exp.first_arg
 
@@ -42,7 +42,7 @@ class Brakeman::ErubisTemplateProcessor < Brakeman::TemplateProcessor
         abort "Unrecognized action on buffer: #{method}"
       end
     elsif target == nil and method == :render
-      exp.arglist = process exp.arglist
+      process_call_args! exp
       make_render_in_view exp
     else
       #TODO: Is it really necessary to create a new Sexp here?

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
@@ -91,7 +90,7 @@ class Brakeman::HamlTemplateProcessor < Brakeman::TemplateProcessor
       end
     elsif target == nil and method == :render
       #Process call to render()
-      exp.arglist = process exp.arglist
+      process_call_args! exp
       make_render_in_view exp
     else
       #TODO: Do we really need a new Sexp here?

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/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