From c2e756a944fd3ca2efa58bd285c0e75e0b4794ab Mon Sep 17 00:00:00 2001 From: Jon Dufresne Date: Fri, 25 Feb 2022 10:11:56 -0400 Subject: [PATCH] Remove body content from redirect responses Modern browsers don't render this HTML so it goes unused in practice. The delivered bytes are therefore a small waste (although very small) and unnecessary and could be optimized away. Additionally, the HTML fails validation. Using the W3C v.Nu, we see the following errors: Warning: Consider adding a lang attribute to the html start tag to declare the language of this document. Error: Start tag seen without seeing a doctype first. Expected . Error: Element head is missing a required instance of child element title. These errors may surface in site-wide compliance tests (either internal tests or external contractual tests). Avoid the false positives by removing the HTML. While these warnings and errors could be resolved, it would be simpler on future maintenance to remove the body altogether (especially as it isn't rendered by the browser). As the same string is copied around a few places, this removes multiple touch points to resolve the current validation errors as well as new ones. Many other frameworks and web servers don't include an HTML body on redirect, so there isn't a reason for Rails to do so. By removing the custom Rails HTML, there are fewing "fingerprints" that a malicious bot could use to identify the backend technologies. Application controllers that wish to add a response body after calling redirect_to can continue to do so. --- actionpack/CHANGELOG.md | 7 +++++++ .../lib/action_controller/metal/redirecting.rb | 2 +- .../middleware/actionable_exceptions.rb | 2 +- .../lib/action_dispatch/routing/redirection.rb | 2 +- actionpack/test/controller/integration_test.rb | 2 +- actionpack/test/dispatch/prefix_generation_test.rb | 12 ++---------- actionpack/test/dispatch/routing_test.rb | 12 ++---------- 7 files changed, 15 insertions(+), 24 deletions(-) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 5accaaf2ca780..bb8336e3d0ca9 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,3 +1,10 @@ +* Make `redirect_to` return an empty response body. + + Application controllers that wish to add a response body after calling + `redirect_to` can continue to do so. + + *Jon Dufresne* + * Use non-capturing group for subdomain matching in `ActionDispatch::HostAuthorization` Since we do nothing with the captured subdomain group, we can use a non-capturing group instead. diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb index b3ac74d4aca21..e476c9b4b67d9 100644 --- a/actionpack/lib/action_controller/metal/redirecting.rb +++ b/actionpack/lib/action_controller/metal/redirecting.rb @@ -87,7 +87,7 @@ def redirect_to(options = {}, response_options = {}) self.status = _extract_redirect_to_status(options, response_options) self.location = _enforce_open_redirect_protection(_compute_redirect_to_location(request, options), allow_other_host: allow_other_host) - self.response_body = "You are being redirected." + self.response_body = "" end # Soft deprecated alias for #redirect_back_or_to where the +fallback_location+ location is supplied as a keyword argument instead diff --git a/actionpack/lib/action_dispatch/middleware/actionable_exceptions.rb b/actionpack/lib/action_dispatch/middleware/actionable_exceptions.rb index 33583ef3f6197..d502c65752d38 100644 --- a/actionpack/lib/action_dispatch/middleware/actionable_exceptions.rb +++ b/actionpack/lib/action_dispatch/middleware/actionable_exceptions.rb @@ -30,7 +30,7 @@ def redirect_to(location) uri = URI.parse location if uri.relative? || uri.scheme == "http" || uri.scheme == "https" - body = "You are being redirected." + body = "" else return [400, { "Content-Type" => "text/plain" }, ["Invalid redirection URI"]] end diff --git a/actionpack/lib/action_dispatch/routing/redirection.rb b/actionpack/lib/action_dispatch/routing/redirection.rb index 9fa01f7c3f116..1ad6c9f9913c9 100644 --- a/actionpack/lib/action_dispatch/routing/redirection.rb +++ b/actionpack/lib/action_dispatch/routing/redirection.rb @@ -38,7 +38,7 @@ def serve(req) req.commit_flash - body = %(You are being redirected.) + body = "" headers = { "Location" => uri.to_s, diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb index 2e1a54fbf88da..4677c1192e21a 100644 --- a/actionpack/test/controller/integration_test.rb +++ b/actionpack/test/controller/integration_test.rb @@ -349,7 +349,7 @@ def test_redirect assert_response 302 assert_response :redirect assert_response :found - assert_equal "You are being redirected.", response.body + assert_equal "", response.body assert_kind_of Nokogiri::HTML::Document, html_document assert_equal 1, request_count diff --git a/actionpack/test/dispatch/prefix_generation_test.rb b/actionpack/test/dispatch/prefix_generation_test.rb index e6106766ee118..5ee1a611ca130 100644 --- a/actionpack/test/dispatch/prefix_generation_test.rb +++ b/actionpack/test/dispatch/prefix_generation_test.rb @@ -331,11 +331,7 @@ def setup def verify_redirect(url, status = 301) assert_equal status, response.status assert_equal url, response.headers["Location"] - assert_equal expected_redirect_body(url), response.body - end - - def expected_redirect_body(url) - %(You are being redirected.) + assert_equal "", response.body end end @@ -460,11 +456,7 @@ def app def verify_redirect(url, status = 301) assert_equal status, response.status assert_equal url, response.headers["Location"] - assert_equal expected_redirect_body(url), response.body - end - - def expected_redirect_body(url) - %(You are being redirected.) + assert_equal "", response.body end end end diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb index 69ac8423f227b..ddaf68b2c0559 100644 --- a/actionpack/test/dispatch/routing_test.rb +++ b/actionpack/test/dispatch/routing_test.rb @@ -3914,11 +3914,7 @@ def with_https def verify_redirect(url, status = 301) assert_equal status, @response.status assert_equal url, @response.headers["Location"] - assert_equal expected_redirect_body(url), @response.body - end - - def expected_redirect_body(url) - %(You are being redirected.) + assert_equal "", @response.body end end @@ -4341,11 +4337,7 @@ def app; APP end def verify_redirect(url, status = 301) assert_equal status, @response.status assert_equal url, @response.headers["Location"] - assert_equal expected_redirect_body(url), @response.body - end - - def expected_redirect_body(url) - %(You are being redirected.) + assert_equal "", @response.body end end