diff --git a/lib/brakeman/processors/alias_processor.rb b/lib/brakeman/processors/alias_processor.rb index 3df863f2e9..574118ae26 100644 --- a/lib/brakeman/processors/alias_processor.rb +++ b/lib/brakeman/processors/alias_processor.rb @@ -706,9 +706,11 @@ def process_if exp def early_return? exp return true if node_type? exp, :return + return true if call? exp and [:fail, :raise].include? exp.method if node_type? exp, :block, :rlist - node_type? exp.last, :return + node_type? exp.last, :return or + (call? exp and [:fail, :raise].include? exp.method) else false end diff --git a/test/apps/rails5/app/controllers/mixed_controller.rb b/test/apps/rails5/app/controllers/mixed_controller.rb index ac1c175682..4ca9c79e17 100644 --- a/test/apps/rails5/app/controllers/mixed_controller.rb +++ b/test/apps/rails5/app/controllers/mixed_controller.rb @@ -15,4 +15,10 @@ def another_early_return Statistics::AdminWithdrawal.send("export_#{bank_name}_#inc!") end + + def yet_another_early_return + scope_name = params[:scope].presence + fail ActiveRecord::RecordNotFound unless ['safe', 'also_safe'].include?(scope_name) + Model.public_send(scope_name) + end end diff --git a/test/tests/alias_processor.rb b/test/tests/alias_processor.rb index 12100cb353..1ea8db93f3 100644 --- a/test/tests/alias_processor.rb +++ b/test/tests/alias_processor.rb @@ -883,6 +883,26 @@ def test_branch_array_include_return_more OUTPUT end + def test_branch_array_include_fail + assert_output <<-INPUT, <<-OUTPUT + fail unless ['a', 'b'].include? x + x + INPUT + fail unless ['a', 'b'].include? x + 'a' + OUTPUT + end + + def test_branch_array_include_raise + assert_output <<-INPUT, <<-OUTPUT + raise Z unless ['a', 'b'].include? x + x + INPUT + raise Z unless ['a', 'b'].include? x + 'a' + OUTPUT + end + def test_case_basic assert_output <<-INPUT, <<-OUTPUT z = 3 diff --git a/test/tests/rails5.rb b/test/tests/rails5.rb index 1d9670d961..475bcdb81f 100644 --- a/test/tests/rails5.rb +++ b/test/tests/rails5.rb @@ -94,6 +94,19 @@ def test_dangerous_send_with_early_return :user_input => s(:call, s(:call, s(:params), :[], s(:lit, :filename)), :first) end + def test_dangerous_send_with_fail + assert_no_warning :type => :warning, + :warning_code => 23, + :fingerprint => "e39bb04370762208b68068f4dc823ec897b75bb50f4a5dee02e329e2b6dda733", + :warning_type => "Dangerous Send", + :line => 22, + :message => /^User\ controlled\ method\ execution/, + :confidence => 0, + :relative_path => "app/controllers/mixed_controller.rb", + :code => s(:call, s(:const, :Model), :public_send, s(:call, s(:call, s(:params), :[], s(:lit, :scope)), :presence)), + :user_input => s(:call, s(:call, s(:params), :[], s(:lit, :scope)), :presence) + end + def test_no_symbol_denial_of_service assert_no_warning :type => :warning, :warning_code => 59,