From 64be371749756abc574c511c7d112d3b4f52041c Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Mon, 7 Feb 2022 13:16:57 +0000
Subject: [PATCH] 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 | 20 +++++++---
 spec/integration/invalid_request_spec.rb      | 39 +++++++++++++++++++
 2 files changed, 53 insertions(+), 6 deletions(-)
 create mode 100644 spec/integration/invalid_request_spec.rb

diff --git a/lib/middleware/discourse_public_exceptions.rb b/lib/middleware/discourse_public_exceptions.rb
index fcc2b5ed268..9a9ea11571c 100644
--- a/lib/middleware/discourse_public_exceptions.rb
+++ b/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
 
diff --git a/spec/integration/invalid_request_spec.rb b/spec/integration/invalid_request_spec.rb
new file mode 100644
index 00000000000..2ac20d2d7a7
--- /dev/null
+++ b/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