controllers: don't allow GET requests with params in the body.

Don't allow GET requests to pass the request params in the body instead
of in the URL. While Rails can handle GET params passed in the body, it
goes against spec and it may cause problems if the response is a redirect
and the client doesn't send the body params when following the redirect.

This may be a breaking change for broken API clients who were sending
GET params in the body instead of in the URL. This can happen when people
use HTTP libraries incorrectly.

app/controllers/application_controller.rb

@@ -2,6 +2,7 @@
 
 class ApplicationController < ActionController::Base
   class PageRemovedError < StandardError; end
+  class RequestBodyNotAllowedError < StandardError; end
 
   include Pundit::Authorization
   helper_method :search_params, :permitted_attributes
@@ -9,6 +10,7 @@ class ApplicationController < ActionController::Base
   self.responder = ApplicationResponder
 
   skip_forgery_protection if: -> { SessionLoader.new(request).has_api_authentication? }
+  before_action :check_get_body
   before_action :reset_current_user
   before_action :set_current_user
   before_action :normalize_search
@@ -120,6 +122,8 @@ class ApplicationController < ActionController::Base
       render_error_page(401, exception, message: exception.message, template: "sessions/new")
     when ActionController::InvalidAuthenticityToken, ActionController::UnpermittedParameters, ActionController::InvalidCrossOriginRequest, ActionController::Redirecting::UnsafeRedirectError
       render_error_page(403, exception, message: exception.message)
+    when RequestBodyNotAllowedError
+      render_error_page(403, exception, message: "Request body not allowed for #{request.method} request")
     when ActiveSupport::MessageVerifier::InvalidSignature, # raised by `find_signed!`
          User::PrivilegeError,
          Pundit::NotAuthorizedError
@@ -193,6 +197,10 @@ class ApplicationController < ActionController::Base
     request.variant = params[:variant].try(:to_sym)
   end
 
+  def check_get_body
+    raise RequestBodyNotAllowedError if request.method.in?(%w[GET HEAD OPTIONS]) && request.body.size > 0
+  end
+
   # allow api clients to force errors for testing purposes.
   def cause_error
     return unless params[:cause_error].present?

test/functional/application_controller_test.rb

@@ -10,6 +10,20 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest
       assert_response 406
     end
 
+    should "return 403 Bad Request for a GET request with a body" do
+      get root_path, headers: { "Content-Type": "application/x-www-form-urlencoded", "Accept": "application/json" }, env: { RAW_POST_DATA: "tags=touhou" }
+
+      assert_response 403
+      assert_equal("ApplicationController::RequestBodyNotAllowedError", response.parsed_body["error"])
+      assert_equal("Request body not allowed for GET request", response.parsed_body["message"])
+    end
+
+    should "return 200 OK for a POST request overriden to be a GET request" do
+      post root_path, headers: { "Content-Type": "application/x-www-form-urlencoded", "Accept": "application/json", "X-Http-Method-Override": "GET" }, env: { RAW_POST_DATA: "tags=touhou" }
+
+      assert_response 200
+    end
+
     context "on a RecordNotFound error" do
       should "return 404 Not Found even with a bad file extension" do
         get post_path("bad.json")