Add check for potential HTTP verb confusion

Fixes #1432

lib/brakeman/checks/check_verb_confusion.rb

@@ -0,0 +1,75 @@
+require 'brakeman/checks/base_check'
+
+class Brakeman::CheckVerbConfusion < Brakeman::BaseCheck
+  Brakeman::Checks.add self
+
+  @description = "Check for uses of `request.get?` that might have unintentional behavior"
+
+  #Process calls
+  def run_check
+    calls = tracker.find_call(target: :request, methods: [:get?]) 
+
+    calls.each do |call|
+      process_result call
+    end
+  end
+
+  def process_result result
+    @current_result = result
+    @matched_call = result[:call]
+    klass = tracker.find_class(result[:location][:class])
+
+    # TODO: abstract into tracker.find_location ?
+    if klass.nil?
+      Brakeman.debug "No class found: #{result[:location][:class]}"
+      return
+    end
+
+    method = klass.get_method(result[:location][:method])
+
+    if method.nil?
+      Brakeman.debug "No method found: #{result[:location][:method]}"
+      return
+    end
+
+    process method[:src]
+  end
+
+  def process_if exp
+    if exp.condition == @matched_call
+      # Found `if request.get?`
+
+      # Do not warn if there is an `elsif` clause
+      if node_type? exp.else_clause, :if
+        return exp
+      end
+
+      warn_about_result @current_result, exp
+    end
+
+    exp
+  end
+
+  def warn_about_result result, code
+    return unless original? result
+
+    confidence = :weak
+    message = msg('Potential HTTP verb confusion. ',
+                  msg_code('HEAD'),
+                  ' is routed like ',
+                  msg_code('GET'),
+                  ' but ',
+                  msg_code('request.get?'),
+                  ' will return ',
+                  msg_code('false')
+                 )
+
+    warn :result => result,
+      :warning_type => "HTTP Verb Confusion",
+      :warning_code => :http_verb_confusion,
+      :message => message,
+      :code => code,
+      :user_input => result[:call],
+      :confidence => confidence
+  end
+end

lib/brakeman/warning_codes.rb

@@ -119,6 +119,7 @@ module Brakeman::WarningCodes
     :CVE_2020_8159 => 115,
     :CVE_2020_8166 => 116,
     :erb_template_injection => 117,
+    :http_verb_confusion => 118,
 
     :custom_check => 9090,
   }

test/apps/rails6/app/controllers/accounts_controller.rb

@@ -0,0 +1,10 @@
+class AccountspsController < ApplicationController
+  def login
+    if request.get?
+      # Do something benign
+    else
+      # Do something sensitive because it's a POST
+      # but actually it could be a HEAD :(
+    end
+  end
+end

test/apps/rails6/config/routes.rb

@@ -4,4 +4,8 @@
   # https://github.com/presidentbeef/brakeman/issues/1410
   x = ->(args) {
   }
+
+  match "/login/oauth/authorize",
+  :to => "users#login",
+  :via => [:get, :post]
 end

test/tests/rails6.rb

@@ -13,7 +13,7 @@ def expected
       :controller => 0,
       :model => 0,
       :template => 4,
-      :generic => 21
+      :generic => 22
     }
   end
 
@@ -418,4 +418,17 @@ def test_template_injection_1
       :code => s(:call, s(:const, :ERB), :new, s(:params)),
       :user_input => s(:params)
   end
+
+  def test_http_verb_confusion_1
+    assert_warning :type => :warning,
+      :warning_code => 118,
+      :fingerprint => "e364e0c4ed8e75644e4eb434f944b4cfe3e60d55be22333820f34147fb85afbf",
+      :warning_type => "HTTP Verb Confusion",
+      :line => 3,
+      :message => /^Potential\ HTTP\ verb\ confusion\.\ `HEAD`\ is/,
+      :confidence => 2,
+      :relative_path => "app/controllers/accounts_controller.rb",
+      :code => s(:if, s(:call, s(:call, nil, :request), :get?), nil, nil),
+      :user_input => s(:call, s(:call, nil, :request), :get?)
+  end
 end