Skip to content

Commit

Permalink
Add redirect_back and redirect_back_or_to to open redirect check (#…
Browse files Browse the repository at this point in the history
…1756)

* Add redirect_back and redirect_back_or_to
  • Loading branch information
presidentbeef committed Jan 26, 2023
1 parent 749b664 commit 72327f7
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 7 deletions.
20 changes: 15 additions & 5 deletions lib/brakeman/checks/check_redirect.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ def run_check
@model_find_calls << :find_sole_by
end

@tracker.find_call(:target => false, :method => :redirect_to).each do |res|
methods = [:redirect_to, :redirect_back, :redirect_back_or_to]

@tracker.find_call(:target => false, :methods => methods).each do |res|
process_result res
end
end
Expand All @@ -34,12 +36,20 @@ def process_result result
return unless original? result

call = result[:call]
method = call.method

opt = call.first_arg

if method == :redirect_to and
not protected_by_raise?(call) and
# Location is specified with `fallback_location:`
# otherwise the arguments do not contain a location and
# the call can be ignored
if call.method == :redirect_back
if hash? opt and location = hash_access(opt, :fallback_location)
opt = location
else
return
end
end

if not protected_by_raise?(call) and
not only_path?(call) and
not explicit_host?(opt) and
not slice_call?(opt) and
Expand Down
12 changes: 12 additions & 0 deletions test/apps/rails7/app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,16 @@ def redirect_with_allow_host
def redirect_with_explicit_not_allow
redirect_to params[:x], allow_other_host: false # no warning
end

def redirect_back_with_fallback
redirect_back fallback_location: params[:x]
end

def redirect_back_or_to_with_fallback
redirect_back_or_to params[:x]
end

def redirect_back_or_to_with_fallback_disallow_host
redirect_back_or_to params[:x], allow_other_host: false # no warning
end
end
30 changes: 29 additions & 1 deletion test/tests/rails7.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def expected
:controller => 0,
:model => 0,
:template => 0,
:warning => 20
:warning => 22
}
end

Expand Down Expand Up @@ -351,4 +351,32 @@ def test_redirect_allow_other_host
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

def test_redirect_back
assert_warning check_name: "Redirect",
type: :warning,
warning_code: 18,
fingerprint: "81ee1b43b1a16a2e143669adb3259407bb462f1963d339717662d9271a154909",
warning_type: "Redirect",
line: 29,
message: /^Possible\ unprotected\ redirect/,
confidence: 0,
relative_path: "app/controllers/users_controller.rb",
code: s(:call, nil, :redirect_back, s(:hash, s(:lit, :fallback_location), s(:call, s(:params), :[], s(:lit, :x)))),
user_input: s(:call, s(:params), :[], s(:lit, :x))
end

def test_redirect_back_or_to
assert_warning check_name: "Redirect",
type: :warning,
warning_code: 18,
fingerprint: "e5aed5eb26b588f3cb6f9f7d34c63ceffcb574348c4fd3c8464e11cab16ed3e3",
warning_type: "Redirect",
line: 33,
message: /^Possible\ unprotected\ redirect/,
confidence: 0,
relative_path: "app/controllers/users_controller.rb",
code: s(:call, nil, :redirect_back_or_to, s(:call, s(:params), :[], s(:lit, :x))),
user_input: s(:call, s(:params), :[], s(:lit, :x))
end
end
26 changes: 25 additions & 1 deletion test/tests/rails7_redirect.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def test_rails7_default_no_open_redirects
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_fixed 3

assert_no_warning check_name: "Redirect",
type: :warning,
Expand All @@ -27,5 +27,29 @@ def test_rails7_default_no_open_redirects
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))

assert_no_warning check_name: "Redirect",
type: :warning,
warning_code: 18,
fingerprint: "81ee1b43b1a16a2e143669adb3259407bb462f1963d339717662d9271a154909",
warning_type: "Redirect",
line: 29,
message: /^Possible\ unprotected\ redirect/,
confidence: 0,
relative_path: "app/controllers/users_controller.rb",
code: s(:call, nil, :redirect_back, s(:hash, s(:lit, :fallback_location), s(:call, s(:params), :[], s(:lit, :x)))),
user_input: s(:call, s(:params), :[], s(:lit, :x))

assert_no_warning check_name: "Redirect",
type: :warning,
warning_code: 18,
fingerprint: "e5aed5eb26b588f3cb6f9f7d34c63ceffcb574348c4fd3c8464e11cab16ed3e3",
warning_type: "Redirect",
line: 33,
message: /^Possible\ unprotected\ redirect/,
confidence: 0,
relative_path: "app/controllers/users_controller.rb",
code: s(:call, nil, :redirect_back_or_to, s(:call, s(:params), :[], s(:lit, :x))),
user_input: s(:call, s(:params), :[], s(:lit, :x))
end
end

0 comments on commit 72327f7

Please sign in to comment.