Merge pull request #327 from presidentbeef/add_method_for_input_type_output

Factor out dangerous input type in messages and normalize

lib/brakeman/checks/base_check.rb

@@ -516,4 +516,23 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
 
     @active_record_models
   end
+
+  def friendly_type_of input_type
+    if input_type.is_a? Match
+      input_type = input_type.type
+    end
+
+    case input_type
+    when :params
+      "parameter value"
+    when :cookies
+      "cookie value"
+    when :request
+      "request value"
+    when :model
+      "model attribute"
+    else
+      "user input"
+    end
+  end
 end

lib/brakeman/checks/check_content_tag.rb

@@ -45,7 +45,7 @@ class Brakeman::CheckContentTag < Brakeman::CheckCrossSiteScripting
 
     call = result[:call] = result[:call].dup
 
-    args = call.arglist 
+    args = call.arglist
 
     tag_name = args[1]
     content = args[2]
@@ -94,19 +94,12 @@ class Brakeman::CheckContentTag < Brakeman::CheckCrossSiteScripting
     end
 
     if input = has_immediate_user_input?(arg)
-      case input.type
-      when :params
-        message = "Unescaped parameter value in content_tag"
-      when :cookies
-        message = "Unescaped cookie value in content_tag"
-      else
-        message = "Unescaped user input value in content_tag"
-      end
+      message = "Unescaped #{friendly_type_of input} in content_tag"
 
       add_result result
 
       warn :result => result,
-        :warning_type => "Cross Site Scripting", 
+        :warning_type => "Cross Site Scripting",
         :warning_code => :xss_content_tag,
         :message => message,
         :user_input => input.match,
@@ -126,7 +119,7 @@ class Brakeman::CheckContentTag < Brakeman::CheckCrossSiteScripting
         end
 
         warn :result => result,
-          :warning_type => "Cross Site Scripting", 
+          :warning_type => "Cross Site Scripting",
           :warning_code => :xss_content_tag,
           :message => "Unescaped model attribute in content_tag",
           :user_input => match,
@@ -135,28 +128,14 @@ class Brakeman::CheckContentTag < Brakeman::CheckCrossSiteScripting
       end
 
     elsif @matched
-      message = "Unescaped "
-
-      case @matched.type
-      when :model
-        return if tracker.options[:ignore_model_output]
-        message << "model attribute"
-      when :params
-        message << "parameter"
-      when :cookies
-        message << "cookie"
-      when :session
-        message << "session"
-      else
-        message << "user input"
-      end
+      return if @matched.type == :model and tracker.options[:ignore_model_output]
 
-      message << " value in content_tag"
+      message = "Unescaped #{friendly_type_of @matched} in content_tag"
 
       add_result result
 
-      warn :result => result, 
-        :warning_type => "Cross Site Scripting", 
+      warn :result => result,
+        :warning_type => "Cross Site Scripting",
         :warning_code => :xss_content_tag,
         :message => message,
         :user_input => @matched.match,

lib/brakeman/checks/check_cross_site_scripting.rb

@@ -104,16 +104,7 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
     if input = has_immediate_user_input?(out)
       add_result exp
 
-      case input.type
-      when :params
-        message = "Unescaped parameter value"
-      when :cookies
-        message = "Unescaped cookie value"
-      when :request
-        message = "Unescaped request value"
-      else
-        message = "Unescaped user input value"
-      end
+      message = "Unescaped #{friendly_type_of input}"
 
       warn :template => @current_template,
         :warning_type => "Cross Site Scripting",
@@ -194,15 +185,8 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
       message = nil
 
       if @matched
-        case @matched.type
-        when :model
-          unless tracker.options[:ignore_model_output]
-            message = "Unescaped model attribute"
-          end
-        when :params
-          message = "Unescaped parameter value"
-        when :cookies
-          message = "Unescaped cookie value"
+        unless @matched.type and tracker.options[:ignore_model_output]
+          message = "Unescaped #{friendly_type_of @matched}"
         end
 
         if message and not duplicate? exp

lib/brakeman/checks/check_file_access.rb

@@ -48,20 +48,7 @@ class Brakeman::CheckFileAccess < Brakeman::BaseCheck
     end
 
     if match
-      case match.type
-      when :params
-        message = "Parameter"
-      when :cookies
-        message = "Cookie"
-      when :request
-        message = "Request"
-      when :model
-        message = "Model attribute"
-      else
-        message = "User input"
-      end
-
-      message << " value used in file name"
+      message = "#{friendly_type_of(match).capitalize} used in file name"
 
       warn :result => result,
         :warning_type => "File Access",

lib/brakeman/checks/check_link_to.rb

@@ -68,14 +68,7 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
     input = has_immediate_user_input?(argument)
     return false unless input
 
-    case input.type
-    when :params
-      message = "Unescaped parameter value in link_to"
-    when :cookies
-      message = "Unescaped cookie value in link_to"
-    else
-      message = "Unescaped user input value in link_to"
-    end
+    message = "Unescaped #{friendly_type_of input} in link_to"
 
     warn_xss(result, message, input.match, CONFIDENCE[:high])
   end
@@ -96,15 +89,11 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
   # Check if we should warn about the matched result
   def check_matched(result, matched = nil)
     return false unless matched
-    message = nil
+    return false if matched.type == :model and not tracker.options[:ignore_model_output]
 
-    if matched.type == :model and not tracker.options[:ignore_model_output]
-      message = "Unescaped model attribute in link_to"
-    elsif matched.type == :params
-      message = "Unescaped parameter value in link_to"
-    end
+    message = "Unescaped #{friendly_type_of matched} in link_to"
 
-    message ? warn_xss(result, message, @matched.match, CONFIDENCE[:med]) : false
+    warn_xss(result, message, @matched.match, CONFIDENCE[:med])
   end
 
   # Create a warn for this xss

lib/brakeman/checks/check_link_to_href.rb

@@ -42,14 +42,7 @@ class Brakeman::CheckLinkToHref < Brakeman::CheckLinkTo
 
 
     if input = has_immediate_user_input?(url_arg)
-      case input.type
-      when :params
-        message = "Unsafe parameter value in link_to href"
-      when :cookies
-        message = "Unsafe cookie value in link_to href"
-      else
-        message = "Unsafe user input value in link_to href"
-      end
+      message = "Unsafe #{friendly_type_of input} in link_to href"
 
       unless duplicate? result
         add_result result

lib/brakeman/checks/check_render.rb

@@ -47,22 +47,9 @@ class Brakeman::CheckRender < Brakeman::BaseCheck
         return
       end
 
-      message = "Render path contains "
-
-      case input.type
-      when :params
-        message << "parameter value"
-      when :cookies
-        message << "cookie value"
-      when :request
-        message << "request value"
-      when :model
-        #Skip models
-        return
-      else
-        message << "user input value"
-      end
+      return if input.type == :model #skip models
 
+      message = "Render path contains #{friendly_type_of input}"
 
       warn :result => result,
         :warning_type => "Dynamic Render Path",

lib/brakeman/checks/check_symbol_dos.rb

@@ -52,20 +52,7 @@ class Brakeman::CheckSymbolDoS < Brakeman::BaseCheck
     end
 
     if confidence
-      input_type = case input.type
-                   when :params
-                     "parameter value"
-                   when :cookies
-                     "cookies value"
-                   when :request
-                     "request value"
-                   when :model
-                     "model attribute"
-                   else
-                     "user input"
-                   end
-
-      message = "Symbol conversion from unsafe string (#{input_type})"
+      message = "Symbol conversion from unsafe string (#{friendly_type_of input})"
 
       warn :result => result,
         :warning_type => "Denial of Service",

lib/brakeman/checks/check_unsafe_reflection.rb

@@ -38,20 +38,7 @@ class Brakeman::CheckUnsafeReflection < Brakeman::BaseCheck
     end
 
     if confidence
-      input_type = case input.type
-                   when :params
-                     "parameter value"
-                   when :cookies
-                     "cookies value"
-                   when :request
-                     "request value"
-                   when :model
-                     "model attribute"
-                   else
-                     "user input"
-                   end
-
-      message = "Unsafe Reflection method #{method} called with #{input_type}"
+      message = "Unsafe Reflection method #{method} called with #{friendly_type_of input}"
 
       warn :result => result,
         :warning_type => "Remote Code Execution",

lib/brakeman/checks/check_yaml_load.rb

@@ -28,20 +28,7 @@ class Brakeman::CheckYAMLLoad < Brakeman::BaseCheck
     end
 
     if confidence
-      input_type = case input.type
-                   when :params
-                     "parameter value"
-                   when :cookies
-                     "cookies value"
-                   when :request
-                     "request value"
-                   when :model
-                     "model attribute"
-                   else
-                     "user input"
-                   end
-
-      message = "YAML.#{method} called with #{input_type}"
+      message = "YAML.#{method} called with #{friendly_type_of input}"
 
       warn :result => result,
         :warning_type => "Remote Code Execution",

test/tests/test_rails3.rb

@@ -1036,7 +1036,7 @@ class Rails3Tests < Test::Unit::TestCase
     assert_warning :type => :warning,
       :warning_type => "Remote Code Execution",
       :line => 125,
-      :message => /^YAML\.load\ called\ with\ cookies\ value/,
+      :message => /^YAML\.load\ called\ with\ cookie\ value/,
       :confidence => 1,
       :file => /home_controller\.rb/
   end
@@ -1064,7 +1064,7 @@ class Rails3Tests < Test::Unit::TestCase
     assert_warning :type => :warning,
       :warning_type => "Remote Code Execution",
       :line => 131,
-      :message => /^YAML\.load_stream\ called\ with\ cookies\ val/,
+      :message => /^YAML\.load_stream\ called\ with\ cookie\ value/,
       :confidence => 0,
       :file => /home_controller\.rb/
   end

test/tests/test_rails31.rb

@@ -718,7 +718,7 @@ class Rails31Tests < Test::Unit::TestCase
     assert_warning :type => :warning,
       :warning_type => "File Access",
       :line => 109,
-      :message => /^Model attribute\ value\ used\ in\ file\ name/,
+      :message => /^Model attribute\ used\ in\ file\ name/,
       :confidence => 1,
       :file => /users_controller\.rb/
   end