Allow rescue from parameter parse errors

[Gannon McGibbon + Josh Cheek]

8 files changed, 95 insertions, 8 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 97d7049cba..90cf989100 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,13 @@
+*   Allow rescue from parameter parse errors:
+
+    ```
+    rescue_from ActionDispatch::Http::Parameters::ParseError do
+      head :unauthorized
+    end
+    ```
+
+    *Gannon McGibbon*, *Josh Cheek*
+
 *   Reset Capybara sessions if failed system test screenshot raising an exception.
 
     Reset Capybara sessions if `take_failed_screenshot` raise exception
diff --git a/actionpack/lib/action_controller/metal/params_wrapper.rb b/actionpack/lib/action_controller/metal/params_wrapper.rb
index a678377d4f..7361946de5 100644
--- a/actionpack/lib/action_controller/metal/params_wrapper.rb
+++ b/actionpack/lib/action_controller/metal/params_wrapper.rb
@@ -253,7 +253,10 @@ module ActionController
         # This will display the wrapped hash in the log file.
         request.filtered_parameters.merge! wrapped_filtered_hash
       end
-      super
+    ensure
+      # NOTE: Rescues all exceptions so they
+      # may be caught in ActionController::Rescue.
+      return super
     end
 
     private
diff --git a/actionpack/lib/action_dispatch/http/filter_parameters.rb b/actionpack/lib/action_dispatch/http/filter_parameters.rb
index 9f8f178402..cbb772175c 100644
--- a/actionpack/lib/action_dispatch/http/filter_parameters.rb
+++ b/actionpack/lib/action_dispatch/http/filter_parameters.rb
@@ -41,6 +41,8 @@ module ActionDispatch
       # Returns a hash of parameters with all sensitive data replaced.
       def filtered_parameters
         @filtered_parameters ||= parameter_filter.filter(parameters)
+      rescue ActionDispatch::Http::Parameters::ParseError
+        @filtered_parameters = {}
       end
 
       # Returns a hash of request.env with all sensitive data replaced.
diff --git a/actionpack/lib/action_dispatch/http/mime_negotiation.rb b/actionpack/lib/action_dispatch/http/mime_negotiation.rb
index be129965d1..498b1e6695 100644
--- a/actionpack/lib/action_dispatch/http/mime_negotiation.rb
+++ b/actionpack/lib/action_dispatch/http/mime_negotiation.rb
@@ -7,6 +7,11 @@ module ActionDispatch
     module MimeNegotiation
       extend ActiveSupport::Concern
 
+      RESCUABLE_MIME_FORMAT_ERRORS = [
+        ActionController::BadRequest,
+        ActionDispatch::Http::Parameters::ParseError,
+      ]
+
       included do
         mattr_accessor :ignore_accept_header, default: false
       end
@@ -59,7 +64,7 @@ module ActionDispatch
         fetch_header("action_dispatch.request.formats") do |k|
           params_readable = begin
                               parameters[:format]
-                            rescue ActionController::BadRequest
+                            rescue *RESCUABLE_MIME_FORMAT_ERRORS
                               false
                             end
 
diff --git a/actionpack/lib/action_dispatch/http/parameters.rb b/actionpack/lib/action_dispatch/http/parameters.rb
index 8d7431fd6b..13d0963a33 100644
--- a/actionpack/lib/action_dispatch/http/parameters.rb
+++ b/actionpack/lib/action_dispatch/http/parameters.rb
@@ -111,13 +111,23 @@ module ActionDispatch
           begin
             strategy.call(raw_post)
           rescue # JSON or Ruby code block errors.
-            my_logger = logger || ActiveSupport::Logger.new($stderr)
-            my_logger.debug "Error occurred while parsing request parameters.\nContents:\n\n#{raw_post}"
-
+            log_parse_error_once
             raise ParseError
           end
         end
 
+        def log_parse_error_once
+          @parse_error_logged ||= begin
+            parse_logger = logger || ActiveSupport::Logger.new($stderr)
+            parse_logger.debug <<~MSG.chomp
+              Error occurred while parsing request parameters.
+              Contents:
+
+              #{raw_post}
+            MSG
+          end
+        end
+
         def params_parsers
           ActionDispatch::Request.parameter_parsers
         end
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index 7bc364d370..44f23940d3 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -383,9 +383,6 @@ module ActionDispatch
         end
         self.request_parameters = Request::Utils.normalize_encode_params(pr)
       end
-    rescue Http::Parameters::ParseError # one of the parse strategies blew up
-      self.request_parameters = Request::Utils.normalize_encode_params(super || {})
-      raise
     rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
       raise ActionController::BadRequest.new("Invalid request parameters: #{e.message}")
     end
diff --git a/actionpack/test/controller/params_parse_test.rb b/actionpack/test/controller/params_parse_test.rb
new file mode 100644
index 0000000000..440ab06fd7
--- /dev/null
+++ b/actionpack/test/controller/params_parse_test.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+
+class ParamsParseTest < ActionController::TestCase
+  class UsersController < ActionController::Base
+    def create
+      head :ok
+    end
+  end
+
+  tests UsersController
+
+  def test_parse_error_logged_once
+    log_output = capture_log_output do
+      post :create, body: "{", as: :json
+    end
+    assert_equal <<~LOG, log_output
+      Error occurred while parsing request parameters.
+      Contents:
+
+      {
+    LOG
+  end
+
+  private
+
+    def capture_log_output
+      output = StringIO.new
+      request.set_header "action_dispatch.logger", ActiveSupport::Logger.new(output)
+      yield
+      output.string
+    end
+end
diff --git a/actionpack/test/controller/rescue_test.rb b/actionpack/test/controller/rescue_test.rb
index 4ed79073e5..3c39373e55 100644
--- a/actionpack/test/controller/rescue_test.rb
+++ b/actionpack/test/controller/rescue_test.rb
@@ -70,6 +70,10 @@ class RescueController < ActionController::Base
     render plain: "io error"
   end
 
+  rescue_from ActionDispatch::Http::Parameters::ParseError do
+    render plain: "parse error", status: :bad_request
+  end
+
   before_action(only: :before_action_raises) { raise "umm nice" }
 
   def before_action_raises
@@ -130,6 +134,11 @@ class RescueController < ActionController::Base
     raise ResourceUnavailableToRescueAsString
   end
 
+  def arbitrary_action
+    params
+    render plain: "arbitrary action"
+  end
+
   def missing_template
   end
 
@@ -306,6 +315,23 @@ class RescueControllerTest < ActionController::TestCase
     get :exception_with_no_handler_for_wrapper
     assert_response :unprocessable_entity
   end
+
+  test "can rescue a ParseError" do
+    capture_log_output do
+      post :arbitrary_action, body: "{", as: :json
+    end
+    assert_response :bad_request
+    assert_equal "parse error", response.body
+  end
+
+  private
+
+    def capture_log_output
+      output = StringIO.new
+      request.set_header "action_dispatch.logger", ActiveSupport::Logger.new(output)
+      yield
+      output.string
+    end
 end
 
 class RescueTest < ActionDispatch::IntegrationTest