DEV: Improve handling of invalid requests (#15841)

Our discourse_public_exceptions middleware is designed to catch bubbled exceptions from lower in the stack, and then use `ApplicationController.rescue_with_handler` to render an appropriate error response.

When the request itself is invalid, we had an escape-hatch to skip re-dispatching the request to ApplicationController. However, it was possible to work around this by 'layering' the errors. For example, if you made a request which resulted in a 404, but **also** had some other invalidity, the escape hatch would not be triggered.

This commit ensures that these kind of 'layered' errors are properly handled, without logging warnings. It also adds detection for invalid JSON bodies and badly-formed multipart requests.

The user-facing behavior is unchanged. This commit simply prevents warnings being logged for invalid requests.

lib/middleware/discourse_public_exceptions.rb

@@ -4,6 +4,11 @@
 # we need to handle certain exceptions here
 module Middleware
   class DiscoursePublicExceptions < ::ActionDispatch::PublicExceptions
+    INVALID_REQUEST_ERRORS = Set.new([
+      Rack::QueryParser::InvalidParameterError,
+      ActionController::BadRequest,
+      ActionDispatch::Http::Parameters::ParseError,
+    ])
 
     def initialize(path)
       super
@@ -18,12 +23,7 @@ module Middleware
       exception = env["action_dispatch.exception"]
       response = ActionDispatch::Response.new
 
-      # Special handling for invalid params, in this case we can not re-dispatch
-      # the Request object has a "broken" .params which can not be accessed
-      exception = nil if Rack::QueryParser::InvalidParameterError === exception
-
-      # We also can not dispatch bad requests as no proper params
-      exception = nil if ActionController::BadRequest === exception
+      exception = nil if INVALID_REQUEST_ERRORS.include?(exception)
 
       if exception
         begin
@@ -38,6 +38,13 @@ module Middleware
             return [400, { "Cache-Control" => "private, max-age=0, must-revalidate" }, ["Invalid MIME type"]]
           end
 
+          # Or badly formatted multipart requests
+          begin
+            request.POST
+          rescue EOFError
+            return [400, { "Cache-Control" => "private, max-age=0, must-revalidate" }, ["Invalid request"]]
+          end
+
           if ApplicationController.rescue_with_handler(exception, object: fake_controller)
             body = response.body
             if String === body
@@ -46,6 +53,7 @@ module Middleware
             return [response.status, response.headers, body]
           end
         rescue => e
+          return super if INVALID_REQUEST_ERRORS.include?(e.class)
           Discourse.warn_exception(e, message: "Failed to handle exception in exception app middleware")
         end
 

spec/integration/invalid_request_spec.rb

@@ -0,0 +1,39 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe 'invalid requests', type: :request do
+  before do
+    @orig_logger = Rails.logger
+    Rails.logger = @fake_logger = FakeLogger.new
+  end
+
+  after do
+    Rails.logger = @orig_logger
+  end
+
+  it "handles NotFound with invalid json body" do
+    post "/latest.json", params: "{some: malformed: json", headers: { "content-type" => "application/json" }
+    expect(response.status).to eq(404)
+    expect(@fake_logger.warnings.length).to eq(0)
+    expect(@fake_logger.errors.length).to eq(0)
+  end
+
+  it "handles EOFError when multipart request is malformed" do
+    post "/latest.json", params: "somecontent", headers: {
+      "content-type" => "multipart/form-data; boundary=abcde",
+      "content-length" => "1"
+    }
+    expect(response.status).to eq(400)
+    expect(@fake_logger.warnings.length).to eq(0)
+    expect(@fake_logger.errors.length).to eq(0)
+  end
+
+  it "handles invalid parameters" do
+    post "/latest.json", params: { "foo" => "\255bar" }
+    expect(response.status).to eq(404)
+    expect(@fake_logger.warnings.length).to eq(0)
+    expect(@fake_logger.errors.length).to eq(0)
+  end
+
+end