Rails 7 redirect options (#1755)

* Handle Rails 7 redirect options

lib/brakeman/checks/check_redirect.rb

@@ -11,8 +11,6 @@ class Brakeman::CheckRedirect < Brakeman::BaseCheck
   @description = "Looks for calls to redirect_to with user input as arguments"
 
   def run_check
-    Brakeman.debug "Finding calls to redirect_to()"
-
     @model_find_calls = Set[:all, :create, :create!, :find, :find_by_sql, :first, :first!, :last, :last!, :new, :sole]
 
     if tracker.options[:rails3]
@@ -41,13 +39,15 @@ def process_result result
     opt = call.first_arg
 
     if method == :redirect_to and
+        not protected_by_raise?(call) and
         not only_path?(call) and
         not explicit_host?(opt) and
         not slice_call?(opt) and
         not safe_permit?(opt) and
+        not disallow_other_host?(call) and
         res = include_user_input?(opt)
 
-      if res.type == :immediate
+      if res.type == :immediate and not allow_other_host?(call)
         confidence = :high
       else
         confidence = :weak
@@ -260,4 +260,25 @@ def safe_permit? exp
 
     false
   end
+
+  def protected_by_raise? call
+    raise_on_redirects? and
+      not allow_other_host? call
+  end
+
+  def raise_on_redirects?
+    @raise_on_redirects ||= true?(tracker.config.rails.dig(:action_controller, :raise_on_open_redirects))
+  end
+
+  def allow_other_host? call
+    opt = call.last_arg
+
+    hash? opt and true? hash_access(opt, :allow_other_host)
+  end
+
+  def disallow_other_host? call
+    opt = call.last_arg
+
+    hash? opt and false? hash_access(opt, :allow_other_host)
+  end
 end

test/apps/rails7/app/controllers/users_controller.rb

@@ -16,4 +16,12 @@ def redirect_param_with_fallback
   def redirect_url_from_param_with_fallback
     redirect_to url_from(params[:redirect_url]) || "/"
   end
+
+  def redirect_with_allow_host
+    redirect_to params[:x], allow_other_host: true # low confidence warning
+  end
+
+  def redirect_with_explicit_not_allow
+    redirect_to params[:x], allow_other_host: false # no warning
+  end
 end

test/apps/rails7/config/application.rb

@@ -18,5 +18,8 @@ class Application < Rails::Application
     #
     # config.time_zone = "Central Time (US & Canada)"
     # config.eager_load_paths << Rails.root.join("extras")
+
+    # Test with this off
+    config.action_controller.raise_on_open_redirects = false
   end
 end

test/tests/config.rb

@@ -53,6 +53,6 @@ def test_rails7_configuration_load_defaults
     assert_equal Sexp.new(:true), tracker.config.rails[:action_controller][:urlsafe_csrf_tokens]
 
     # Check a 7.0 config
-    assert_equal Sexp.new(:true), tracker.config.rails[:action_controller][:raise_on_open_redirects]
+    assert_equal Sexp.new(:true), tracker.config.rails[:action_controller][:wrap_parameters_by_default]
   end
 end

test/tests/rails7.rb

@@ -13,7 +13,7 @@ def expected
       :controller => 0,
       :model => 0,
       :template => 0,
-      :warning => 19
+      :warning => 20
     }
   end
 
@@ -323,4 +323,32 @@ def test_redirect_2
       code: s(:call, nil, :redirect_to, s(:or, s(:call, nil, :url_from, s(:call, s(:params), :[], s(:lit, :redirect_url))), s(:str, "/"))),
       user_input: s(:call, s(:params), :[], s(:lit, :redirect_url))
   end
+
+  def test_redirect_disallow_other_host
+    assert_no_warning check_name: "Redirect",
+      type: :warning,
+      warning_code: 18,
+      fingerprint: "cef6913575a0db9d43acdb945fd8d7f5b1ea3e0a19c457d9c85bf673e67a4a85",
+      warning_type: "Redirect",
+      line: 25,
+      message: /^Possible\ unprotected\ redirect/,
+      confidence: 0,
+      relative_path: "app/controllers/users_controller.rb",
+      code: s(:call, nil, :redirect_to, s(:call, s(:params), :[], s(:lit, :x)), s(:hash, s(:lit, :allow_other_host), s(:false))),
+      user_input: s(:call, s(:params), :[], s(:lit, :x))
+  end
+
+  def test_redirect_allow_other_host
+    assert_warning check_name: "Redirect",
+      type: :warning,
+      warning_code: 18,
+      fingerprint: "a1fdd251c91d225a187a41b9b4acf88412d86a834b597fa58a17d208681b8a00",
+      warning_type: "Redirect",
+      line: 21,
+      message: /^Possible\ unprotected\ redirect/,
+      confidence: 2,
+      relative_path: "app/controllers/users_controller.rb",
+      code: s(:call, nil, :redirect_to, s(:call, s(:params), :[], s(:lit, :x)), s(:hash, s(:lit, :allow_other_host), s(:true))),
+      user_input: s(:call, s(:params), :[], s(:lit, :x))
+  end
 end

test/tests/rails7_redirect.rb

@@ -0,0 +1,31 @@
+require_relative '../test'
+require 'brakeman/rescanner'
+
+class RailsConfiguration < Minitest::Test
+  include BrakemanTester::FindWarning
+  include BrakemanTester::RescanTestHelper
+
+  def report
+    @rescanner.tracker.report.to_hash
+  end
+
+  def test_rails7_default_no_open_redirects
+    before_rescan_of ['config/application.rb'], 'rails7' do
+      replace 'config/application.rb', 'config.action_controller.raise_on_open_redirects = false', 'config.action_controller.raise_on_open_redirects = true'
+    end
+
+    assert_fixed 1
+
+    assert_no_warning check_name: "Redirect",
+      type: :warning,
+      warning_code: 18,
+      fingerprint: "0e6b36e8598a024ef8832d7af1a5b0089f6b00f96c17e2ccdb87aca012e6f76f",
+      warning_type: "Redirect",
+      line: 13,
+      message: /^Possible\ unprotected\ redirect/,
+      confidence: 0,
+      relative_path: "app/controllers/users_controller.rb",
+      code: s(:call, nil, :redirect_to, s(:or, s(:call, s(:params), :[], s(:lit, :redirect_url)), s(:str, "/"))),
+      user_input: s(:call, s(:params), :[], s(:lit, :redirect_url))
+  end
+end