aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--actionpack/CHANGELOG.md6
-rw-r--r--actionpack/lib/action_controller/metal/flash.rb8
-rw-r--r--actionpack/lib/action_controller/metal/force_ssl.rb5
-rw-r--r--actionpack/lib/action_controller/metal/redirecting.rb43
-rw-r--r--actionpack/test/controller/action_pack_assertions_test.rb6
-rw-r--r--actionpack/test/controller/log_subscriber_test.rb4
-rw-r--r--actionpack/test/controller/redirect_test.rb44
-rw-r--r--actiontext/app/javascript/actiontext/index.js1
-rw-r--r--actiontext/lib/templates/installer.rb29
-rw-r--r--actiontext/package.json4
10 files changed, 110 insertions, 40 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 1457794354..2000be688f 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,9 @@
+* 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/flash.rb b/actionpack/lib/action_controller/metal/flash.rb
index 380f2e9591..a4861dc2c0 100644
--- a/actionpack/lib/action_controller/metal/flash.rb
+++ b/actionpack/lib/action_controller/metal/flash.rb
@@ -44,18 +44,18 @@ module ActionController #:nodoc:
end
private
- def redirect_to(options = {}, response_status_and_flash = {}) #:doc:
+ def redirect_to(options = {}, response_options_and_flash = {}) #:doc:
self.class._flash_types.each do |flash_type|
- if type = response_status_and_flash.delete(flash_type)
+ if type = response_options_and_flash.delete(flash_type)
flash[flash_type] = type
end
end
- if other_flashes = response_status_and_flash.delete(:flash)
+ if other_flashes = response_options_and_flash.delete(:flash)
flash.update(other_flashes)
end
- super(options, response_status_and_flash)
+ super(options, response_options_and_flash)
end
end
end
diff --git a/actionpack/lib/action_controller/metal/force_ssl.rb b/actionpack/lib/action_controller/metal/force_ssl.rb
index 26e6f72b66..205f84ae36 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]
+ REDIRECT_OPTIONS = [:status, :flash, :alert, :notice, :allow_other_host]
module ClassMethods # :nodoc:
def force_ssl(options = {})
@@ -40,7 +40,8 @@ module ActionController
protocol: "https://",
host: request.host,
path: request.fullpath,
- status: :moved_permanently
+ 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 2804a06a58..8bd003f5ed 100644
--- a/actionpack/lib/action_controller/metal/redirecting.rb
+++ b/actionpack/lib/action_controller/metal/redirecting.rb
@@ -55,12 +55,12 @@ module ActionController
# Statements after +redirect_to+ in our controller get executed, so +redirect_to+ doesn't stop the execution of the function.
# To terminate the execution of the function immediately after the +redirect_to+, use return.
# redirect_to post_url(@post) and return
- def redirect_to(options = {}, response_status = {})
+ def redirect_to(options = {}, response_options = {})
raise ActionControllerError.new("Cannot redirect to nil!") unless options
raise AbstractController::DoubleRenderError if response_body
- self.status = _extract_redirect_to_status(options, response_status)
- self.location = _compute_redirect_to_location(request, options)
+ self.status = _extract_redirect_to_status(options, response_options)
+ self.location = _compute_safe_redirect_to_location(request, options, response_options)
self.response_body = "<html><body>You are being <a href=\"#{ERB::Util.unwrapped_html_escape(response.location)}\">redirected</a>.</body></html>"
end
@@ -88,9 +88,13 @@ 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["Referer"]
- redirect_to_referer = referer && (allow_other_host || _url_host_allowed?(referer))
- redirect_to redirect_to_referer ? referer : fallback_location, **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
end
def _compute_redirect_to_location(request, options) #:nodoc:
@@ -114,18 +118,35 @@ module ActionController
public :_compute_redirect_to_location
private
- def _extract_redirect_to_status(options, response_status)
+ 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))
- elsif response_status.key?(:status)
- Rack::Utils.status_code(response_status[:status])
+ elsif response_options.key?(:status)
+ Rack::Utils.status_code(response_options[:status])
else
302
end
end
- def _url_host_allowed?(url)
- URI(url.to_s).host == request.host
+ def _url_host_allowed?(url, options = {})
+ URI(url.to_s).host.in?([request.host, options[: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 ecb8c37e6b..c7aae034dd 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" end
+ def redirect_invalid_external_route() redirect_to "ht_tp://www.rubyonrails.org", allow_other_host: true end
def redirect_to_named_route() redirect_to route_one_url end
- def redirect_external() redirect_to "http://www.rubyonrails.org"; end
+ def redirect_external() redirect_to "http://www.rubyonrails.org", allow_other_host: true; end
- def redirect_external_protocol_relative() redirect_to "//www.rubyonrails.org"; end
+ def redirect_external_protocol_relative() redirect_to "//www.rubyonrails.org", allow_other_host: true; 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 0562c16284..cbebc6b59c 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/"
+ redirect_to "http://foo.bar/", allow_other_host: true
end
def filterable_redirector
- redirect_to "http://secret.foo.bar/"
+ redirect_to "http://secret.foo.bar/", allow_other_host: true
end
def data_sender
diff --git a/actionpack/test/controller/redirect_test.rb b/actionpack/test/controller/redirect_test.rb
index 998498e1b2..945d2275c0 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)
+ redirect_to("http://www.example.com", status: :moved_permanently, allow_other_host: true)
end
def url_redirect_with_status_hash
- redirect_to("http://www.example.com", status: 301)
+ redirect_to("http://www.example.com", status: 301, allow_other_host: true)
end
def relative_url_redirect_with_status
@@ -81,19 +81,27 @@ 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"
+ redirect_to "http://example.com/query?status=new", allow_other_host: true
end
def redirect_to_url_with_complex_scheme
- redirect_to "x-test+scheme.complex:redirect"
+ redirect_to "x-test+scheme.complex:redirect", allow_other_host: true
end
def redirect_to_url_with_network_path_reference
- redirect_to "//www.rubyonrails.org/"
+ redirect_to "//www.rubyonrails.org/", allow_other_host: true
end
def redirect_to_existing_record
@@ -113,12 +121,12 @@ class RedirectController < ActionController::Base
end
def redirect_to_with_block
- redirect_to proc { "http://www.rubyonrails.org/" }
+ redirect_to proc { "http://www.rubyonrails.org/" }, allow_other_host: true
end
def redirect_to_with_block_and_assigns
@url = "http://www.rubyonrails.org/"
- redirect_to proc { @url }
+ redirect_to proc { @url }, allow_other_host: true
end
def redirect_to_with_block_and_options
@@ -245,6 +253,28 @@ 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/actiontext/app/javascript/actiontext/index.js b/actiontext/app/javascript/actiontext/index.js
index c149eda952..0e9251018a 100644
--- a/actiontext/app/javascript/actiontext/index.js
+++ b/actiontext/app/javascript/actiontext/index.js
@@ -1,4 +1,3 @@
-import * as Trix from "trix"
import { AttachmentUpload } from "./attachment_upload"
addEventListener("trix-attachment-add", event => {
diff --git a/actiontext/lib/templates/installer.rb b/actiontext/lib/templates/installer.rb
index e7c6c2623e..990e41ca00 100644
--- a/actiontext/lib/templates/installer.rb
+++ b/actiontext/lib/templates/installer.rb
@@ -1,3 +1,13 @@
+require "pathname"
+require "json"
+
+APPLICATION_PACK_PATH = Pathname.new("app/javascript/packs/application.js")
+JS_PACKAGE_PATH = Pathname.new("#{__dir__}/../../package.json")
+
+JS_PACKAGE = JSON.load(JS_PACKAGE_PATH)
+JS_DEPENDENCIES = JS_PACKAGE["peerDependencies"].dup.merge \
+ JS_PACKAGE["name"] => "^#{JS_PACKAGE["version"]}"
+
say "Copying actiontext.scss to app/assets/stylesheets"
copy_file "#{__dir__}/actiontext.scss", "app/assets/stylesheets/actiontext.scss"
@@ -8,14 +18,15 @@ say "Copying blob rendering partial to app/views/active_storage/blobs/_blob.html
copy_file "#{__dir__}/../../app/views/active_storage/blobs/_blob.html.erb",
"app/views/active_storage/blobs/_blob.html.erb"
-say "Installing JavaScript dependency"
-run "yarn add @rails/actiontext"
-
-APPLICATION_PACK_PATH = "app/javascript/packs/application.js"
+say "Installing JavaScript dependencies"
+run "yarn add #{JS_DEPENDENCIES.map { |name, version| "#{name}@#{version}" }.join(" ")}"
-if File.exist?(APPLICATION_PACK_PATH) && File.read(APPLICATION_PACK_PATH) !~ /import "@rails\/actiontext"/
- say "Adding import to default JavaScript pack"
- append_to_file APPLICATION_PACK_PATH, <<-EOS
-import "@rails/actiontext"
-EOS
+if APPLICATION_PACK_PATH.exist?
+ JS_DEPENDENCIES.keys.each do |name|
+ line = %[require("#{name}")]
+ unless APPLICATION_PACK_PATH.read.include? line
+ say "Adding #{name} to #{APPLICATION_PACK_PATH}"
+ append_to_file APPLICATION_PACK_PATH, "#{line}\n"
+ end
+ end
end
diff --git a/actiontext/package.json b/actiontext/package.json
index ec8f35fd3c..ee4666b85c 100644
--- a/actiontext/package.json
+++ b/actiontext/package.json
@@ -21,7 +21,9 @@
],
"license": "MIT",
"dependencies": {
- "trix": "^1.0.0",
"@rails/activestorage": "^6.0.0-alpha"
+ },
+ "peerDependencies": {
+ "trix": "^1.0.0"
}
}
generated by cgit v1.2.3 (git 2.25.1) at 2025-10-24 01:09:23 +0000