From 2e0ca9284a6864cfbbb632d849df3fdd7a7c554e Mon Sep 17 00:00:00 2001
From: Gannon McGibbon <gannon.mcgibbon@gmail.com>
Date: Tue, 22 Jan 2019 11:40:13 -0500
Subject: Revert ensure external redirects are explicitly allowed
---
actionpack/CHANGELOG.md | 6 ---
.../lib/action_controller/metal/force_ssl.rb | 3 +-
.../lib/action_controller/metal/redirecting.rb | 33 +++-------------
.../test/controller/action_pack_assertions_test.rb | 6 +--
actionpack/test/controller/log_subscriber_test.rb | 4 +-
actionpack/test/controller/redirect_test.rb | 44 ++++------------------
.../controllers/active_storage/blobs_controller.rb | 2 +-
.../active_storage/representations_controller.rb | 2 +-
8 files changed, 21 insertions(+), 79 deletions(-)
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 1a1b1034aa..a6344bdb7c 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -11,12 +11,6 @@
*Rafael Mendonça França*
-* Ensure external redirects are explicitly allowed
-
- Add `fallback_location` and `allow_other_host` options to `redirect_to`.
-
- *Gannon McGibbon*
-
* Introduce ActionDispatch::HostAuthorization
This is a new middleware that guards against DNS rebinding attacks by
diff --git a/actionpack/lib/action_controller/metal/force_ssl.rb b/actionpack/lib/action_controller/metal/force_ssl.rb
index 205f84ae36..93fd57b640 100644
--- a/actionpack/lib/action_controller/metal/force_ssl.rb
+++ b/actionpack/lib/action_controller/metal/force_ssl.rb
@@ -13,7 +13,7 @@ module ActionController
ACTION_OPTIONS = [:only, :except, :if, :unless]
URL_OPTIONS = [:protocol, :host, :domain, :subdomain, :port, :path]
- REDIRECT_OPTIONS = [:status, :flash, :alert, :notice, :allow_other_host]
+ REDIRECT_OPTIONS = [:status, :flash, :alert, :notice]
module ClassMethods # :nodoc:
def force_ssl(options = {})
@@ -41,7 +41,6 @@ module ActionController
host: request.host,
path: request.fullpath,
status: :moved_permanently,
- allow_other_host: true,
}
if host_or_options.is_a?(Hash)
diff --git a/actionpack/lib/action_controller/metal/redirecting.rb b/actionpack/lib/action_controller/metal/redirecting.rb
index 8bd003f5ed..67c198d150 100644
--- a/actionpack/lib/action_controller/metal/redirecting.rb
+++ b/actionpack/lib/action_controller/metal/redirecting.rb
@@ -60,7 +60,7 @@ module ActionController
raise AbstractController::DoubleRenderError if response_body
self.status = _extract_redirect_to_status(options, response_options)
- self.location = _compute_safe_redirect_to_location(request, options, response_options)
+ self.location = _compute_redirect_to_location(request, options)
self.response_body = "<html><body>You are being <a href=\"#{ERB::Util.unwrapped_html_escape(response.location)}\">redirected</a>.</body></html>"
end
@@ -88,13 +88,9 @@ module ActionController
# All other options that can be passed to <tt>redirect_to</tt> are accepted as
# options and the behavior is identical.
def redirect_back(fallback_location:, allow_other_host: true, **args)
- referer = request.headers.fetch("Referer", fallback_location)
- response_options = {
- fallback_location: fallback_location,
- allow_other_host: allow_other_host,
- **args,
- }
- redirect_to referer, response_options
+ referer = request.headers["Referer"]
+ redirect_to_referer = referer && (allow_other_host || _url_host_allowed?(referer))
+ redirect_to redirect_to_referer ? referer : fallback_location, **args
end
def _compute_redirect_to_location(request, options) #:nodoc:
@@ -118,23 +114,6 @@ module ActionController
public :_compute_redirect_to_location
private
- def _compute_safe_redirect_to_location(request, options, response_options)
- location = _compute_redirect_to_location(request, options)
- location_options = options.is_a?(Hash) ? options : {}
- if response_options[:allow_other_host] || _url_host_allowed?(location, location_options)
- location
- else
- fallback_location = response_options.fetch(:fallback_location) do
- raise ArgumentError, <<~MSG.squish
- Unsafe redirect #{location.inspect},
- use :fallback_location to specify a fallback
- or :allow_other_host to redirect anyway.
- MSG
- end
- _compute_redirect_to_location(request, fallback_location)
- end
- end
-
def _extract_redirect_to_status(options, response_options)
if options.is_a?(Hash) && options.key?(:status)
Rack::Utils.status_code(options.delete(:status))
@@ -145,8 +124,8 @@ module ActionController
end
end
- def _url_host_allowed?(url, options = {})
- URI(url.to_s).host.in?([request.host, options[:host]])
+ def _url_host_allowed?(url)
+ URI(url.to_s).host == request.host
rescue ArgumentError, URI::Error
false
end
diff --git a/actionpack/test/controller/action_pack_assertions_test.rb b/actionpack/test/controller/action_pack_assertions_test.rb
index c7aae034dd..ecb8c37e6b 100644
--- a/actionpack/test/controller/action_pack_assertions_test.rb
+++ b/actionpack/test/controller/action_pack_assertions_test.rb
@@ -28,13 +28,13 @@ class ActionPackAssertionsController < ActionController::Base
def redirect_to_path() redirect_to "/some/path" end
- def redirect_invalid_external_route() redirect_to "ht_tp://www.rubyonrails.org", allow_other_host: true end
+ def redirect_invalid_external_route() redirect_to "ht_tp://www.rubyonrails.org" end
def redirect_to_named_route() redirect_to route_one_url end
- def redirect_external() redirect_to "http://www.rubyonrails.org", allow_other_host: true; end
+ def redirect_external() redirect_to "http://www.rubyonrails.org"; end
- def redirect_external_protocol_relative() redirect_to "//www.rubyonrails.org", allow_other_host: true; end
+ def redirect_external_protocol_relative() redirect_to "//www.rubyonrails.org"; end
def response404() head "404 AWOL" end
diff --git a/actionpack/test/controller/log_subscriber_test.rb b/actionpack/test/controller/log_subscriber_test.rb
index cbebc6b59c..0562c16284 100644
--- a/actionpack/test/controller/log_subscriber_test.rb
+++ b/actionpack/test/controller/log_subscriber_test.rb
@@ -25,11 +25,11 @@ module Another
end
def redirector
- redirect_to "http://foo.bar/", allow_other_host: true
+ redirect_to "http://foo.bar/"
end
def filterable_redirector
- redirect_to "http://secret.foo.bar/", allow_other_host: true
+ redirect_to "http://secret.foo.bar/"
end
def data_sender
diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb
index 945d2275c0..998498e1b2 100644
--- a/actionpack/test/controller/redirect_test.rb
+++ b/actionpack/test/controller/redirect_test.rb
@@ -49,11 +49,11 @@ class RedirectController < ActionController::Base
end
def url_redirect_with_status
- redirect_to("http://www.example.com", status: :moved_permanently, allow_other_host: true)
+ redirect_to("http://www.example.com", status: :moved_permanently)
end
def url_redirect_with_status_hash
- redirect_to("http://www.example.com", status: 301, allow_other_host: true)
+ redirect_to("http://www.example.com", status: 301)
end
def relative_url_redirect_with_status
@@ -81,27 +81,19 @@ class RedirectController < ActionController::Base
end
def redirect_to_url
- redirect_to "http://www.rubyonrails.org/", allow_other_host: true
- end
-
- def redirect_to_unsafe_url
redirect_to "http://www.rubyonrails.org/"
end
- def redirect_to_relative_unsafe_url
- redirect_to ".br"
- end
-
def redirect_to_url_with_unescaped_query_string
- redirect_to "http://example.com/query?status=new", allow_other_host: true
+ redirect_to "http://example.com/query?status=new"
end
def redirect_to_url_with_complex_scheme
- redirect_to "x-test+scheme.complex:redirect", allow_other_host: true
+ redirect_to "x-test+scheme.complex:redirect"
end
def redirect_to_url_with_network_path_reference
- redirect_to "//www.rubyonrails.org/", allow_other_host: true
+ redirect_to "//www.rubyonrails.org/"
end
def redirect_to_existing_record
@@ -121,12 +113,12 @@ class RedirectController < ActionController::Base
end
def redirect_to_with_block
- redirect_to proc { "http://www.rubyonrails.org/" }, allow_other_host: true
+ redirect_to proc { "http://www.rubyonrails.org/" }
end
def redirect_to_with_block_and_assigns
@url = "http://www.rubyonrails.org/"
- redirect_to proc { @url }, allow_other_host: true
+ redirect_to proc { @url }
end
def redirect_to_with_block_and_options
@@ -253,28 +245,6 @@ class RedirectTest < ActionController::TestCase
assert_redirected_to "http://www.rubyonrails.org/"
end
- def test_redirect_to_unsafe_url
- error = assert_raises(ArgumentError) do
- get :redirect_to_unsafe_url
- end
- assert_equal <<~MSG.squish, error.message
- Unsafe redirect \"http://www.rubyonrails.org/\",
- use :fallback_location to specify a fallback or
- :allow_other_host to redirect anyway.
- MSG
- end
-
- def test_redirect_to_relative_unsafe_url
- error = assert_raises(ArgumentError) do
- get :redirect_to_relative_unsafe_url
- end
- assert_equal <<~MSG.squish, error.message
- Unsafe redirect \"http://test.host.br\",
- use :fallback_location to specify a fallback or
- :allow_other_host to redirect anyway.
- MSG
- end
-
def test_redirect_to_url_with_unescaped_query_string
get :redirect_to_url_with_unescaped_query_string
assert_response :redirect
diff --git a/activestorage/app/controllers/active_storage/blobs_controller.rb b/activestorage/app/controllers/active_storage/blobs_controller.rb
index a8e42d7356..4fc3fbe824 100644
--- a/activestorage/app/controllers/active_storage/blobs_controller.rb
+++ b/activestorage/app/controllers/active_storage/blobs_controller.rb
@@ -9,6 +9,6 @@ class ActiveStorage::BlobsController < ActiveStorage::BaseController
def show
expires_in ActiveStorage.service_urls_expire_in
- redirect_to @blob.service_url(disposition: params[:disposition]), allow_other_host: true
+ redirect_to @blob.service_url(disposition: params[:disposition])
end
end
diff --git a/activestorage/app/controllers/active_storage/representations_controller.rb b/activestorage/app/controllers/active_storage/representations_controller.rb
index d01af5d939..98e11e5dbb 100644
--- a/activestorage/app/controllers/active_storage/representations_controller.rb
+++ b/activestorage/app/controllers/active_storage/representations_controller.rb
@@ -9,6 +9,6 @@ class ActiveStorage::RepresentationsController < ActiveStorage::BaseController
def show
expires_in ActiveStorage.service_urls_expire_in
- redirect_to @blob.representation(params[:variation_key]).processed.service_url(disposition: params[:disposition]), allow_other_host: true
+ redirect_to @blob.representation(params[:variation_key]).processed.service_url(disposition: params[:disposition])
end
end
--
cgit v1.2.3