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