aboutsummaryrefslogtreecommitdiffstats
path: root/actionpack
diff options
context:
space:
mode:
Diffstat (limited to 'actionpack')
-rw-r--r--actionpack/CHANGELOG.md8
-rw-r--r--actionpack/lib/action_controller/metal/mime_responds.rb8
-rw-r--r--actionpack/lib/action_dispatch/http/request.rb5
-rw-r--r--actionpack/lib/action_dispatch/request/utils.rb15
-rw-r--r--actionpack/lib/action_dispatch/routing/route_set.rb1
-rw-r--r--actionpack/lib/action_dispatch/testing/assertions.rb2
-rw-r--r--actionpack/test/controller/integration_test.rb2
-rw-r--r--actionpack/test/controller/live_stream_test.rb6
-rw-r--r--actionpack/test/controller/parameters/mutators_test.rb4
-rw-r--r--actionpack/test/controller/render_test.rb4
-rw-r--r--actionpack/test/dispatch/request_test.rb16
-rw-r--r--actionpack/test/dispatch/show_exceptions_test.rb4
12 files changed, 53 insertions, 22 deletions
diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index a3b4f6e989..4f95a9bab9 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,11 @@
+* Catch invalid UTF-8 querystring values and respond with BadRequest
+
+ Check querystring params for invalid UTF-8 characters, and raise an
+ ActionController::BadRequest error if present. Previously these strings
+ would typically trigger errors further down the stack.
+
+ *Grey Baker*
+
* Parse RSS/ATOM responses as XML, not HTML.
*Alexander Kaupanin*
diff --git a/actionpack/lib/action_controller/metal/mime_responds.rb b/actionpack/lib/action_controller/metal/mime_responds.rb
index 58df5c539e..6e346fadfe 100644
--- a/actionpack/lib/action_controller/metal/mime_responds.rb
+++ b/actionpack/lib/action_controller/metal/mime_responds.rb
@@ -91,11 +91,11 @@ module ActionController #:nodoc:
# and accept Rails' defaults, life will be much easier.
#
# If you need to use a MIME type which isn't supported by default, you can register your own handlers in
- # config/initializers/mime_types.rb as follows.
+ # +config/initializers/mime_types.rb+ as follows.
#
# Mime::Type.register "image/jpg", :jpg
#
- # Respond to also allows you to specify a common block for different formats by using any:
+ # Respond to also allows you to specify a common block for different formats by using +any+:
#
# def index
# @people = Person.all
@@ -151,7 +151,7 @@ module ActionController #:nodoc:
# format.html.none { render "trash" }
# end
#
- # Variants also support common `any`/`all` block that formats have.
+ # Variants also support common +any+/+all+ block that formats have.
#
# It works for both inline:
#
@@ -174,7 +174,7 @@ module ActionController #:nodoc:
# request.variant = [:tablet, :phone]
#
# which will work similarly to formats and MIME types negotiation. If there will be no
- # :tablet variant declared, :phone variant will be picked:
+ # +:tablet+ variant declared, +:phone+ variant will be picked:
#
# respond_to do |format|
# format.html.none
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index c6ab4dbc9a..35e3ac304f 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -338,7 +338,10 @@ module ActionDispatch
# Override Rack's GET method to support indifferent access
def GET
fetch_header("action_dispatch.request.query_parameters") do |k|
- set_header k, Request::Utils.normalize_encode_params(super || {})
+ rack_query_params = super || {}
+ # Check for non UTF-8 parameter values, which would cause errors later
+ Request::Utils.check_param_encoding(rack_query_params)
+ set_header k, Request::Utils.normalize_encode_params(rack_query_params)
end
rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
raise ActionController::BadRequest.new("Invalid query parameters: #{e.message}", e)
diff --git a/actionpack/lib/action_dispatch/request/utils.rb b/actionpack/lib/action_dispatch/request/utils.rb
index a8151a8224..bb3df3c311 100644
--- a/actionpack/lib/action_dispatch/request/utils.rb
+++ b/actionpack/lib/action_dispatch/request/utils.rb
@@ -13,6 +13,21 @@ module ActionDispatch
end
end
+ def self.check_param_encoding(params)
+ case params
+ when Array
+ params.each { |element| check_param_encoding(element) }
+ when Hash
+ params.each_value { |value| check_param_encoding(value) }
+ when String
+ unless params.valid_encoding?
+ # Raise Rack::Utils::InvalidParameterError for consistency with Rack.
+ # ActionDispatch::Request#GET will re-raise as a BadRequest error.
+ raise Rack::Utils::InvalidParameterError, "Non UTF-8 value: #{params}"
+ end
+ end
+ end
+
class ParamEncoder # :nodoc:
# Convert nested Hash to HashWithIndifferentAccess.
#
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 339e2b7c4a..5f54ea130b 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -1,5 +1,4 @@
require 'action_dispatch/journey'
-require 'forwardable'
require 'active_support/concern'
require 'active_support/core_ext/object/to_query'
require 'active_support/core_ext/hash/slice'
diff --git a/actionpack/lib/action_dispatch/testing/assertions.rb b/actionpack/lib/action_dispatch/testing/assertions.rb
index 715d94d406..fae266273e 100644
--- a/actionpack/lib/action_dispatch/testing/assertions.rb
+++ b/actionpack/lib/action_dispatch/testing/assertions.rb
@@ -12,7 +12,7 @@ module ActionDispatch
include Rails::Dom::Testing::Assertions
def html_document
- @html_document ||= if @response.content_type.to_s =~ /xml$/
+ @html_document ||= if @response.content_type.to_s =~ /xml\z/
Nokogiri::XML::Document.parse(@response.body)
else
Nokogiri::HTML::Document.parse(@response.body)
diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb
index 99f6f540a8..d0a1d1285f 100644
--- a/actionpack/test/controller/integration_test.rb
+++ b/actionpack/test/controller/integration_test.rb
@@ -463,7 +463,7 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest
def test_get_xml_rss_atom
%w[ application/xml application/rss+xml application/atom+xml ].each do |mime_string|
with_test_route_set do
- get "/get", {}, {"HTTP_ACCEPT" => mime_string}
+ get "/get", headers: {"HTTP_ACCEPT" => mime_string}
assert_equal 200, status
assert_equal "OK", status_message
assert_response 200
diff --git a/actionpack/test/controller/live_stream_test.rb b/actionpack/test/controller/live_stream_test.rb
index 4d1c23cbee..4224ac2a1b 100644
--- a/actionpack/test/controller/live_stream_test.rb
+++ b/actionpack/test/controller/live_stream_test.rb
@@ -388,8 +388,14 @@ module ActionController
end
def test_exception_callback_when_committed
+ current_threads = Thread.list
+
capture_log_output do |output|
get :exception_with_callback, format: 'text/event-stream'
+
+ # Wait on the execution of all threads
+ (Thread.list - current_threads).each(&:join)
+
assert_equal %(data: "500 Internal Server Error"\n\n), response.body
assert_match 'An exception occurred...', output.rewind && output.read
assert_stream_closed
diff --git a/actionpack/test/controller/parameters/mutators_test.rb b/actionpack/test/controller/parameters/mutators_test.rb
index 6c57c4caeb..744d8664be 100644
--- a/actionpack/test/controller/parameters/mutators_test.rb
+++ b/actionpack/test/controller/parameters/mutators_test.rb
@@ -62,15 +62,11 @@ class ParametersMutatorsTest < ActiveSupport::TestCase
end
test "select! retains permitted status" do
- jruby_skip "https://github.com/jruby/jruby/issues/3137"
-
@params.permit!
assert @params.select! { |k| k != "person" }.permitted?
end
test "select! retains unpermitted status" do
- jruby_skip "https://github.com/jruby/jruby/issues/3137"
-
assert_not @params.select! { |k| k != "person" }.permitted?
end
diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb
index 82c7ebf568..256ebf6a07 100644
--- a/actionpack/test/controller/render_test.rb
+++ b/actionpack/test/controller/render_test.rb
@@ -629,13 +629,13 @@ class HttpCacheForeverTest < ActionController::TestCase
def test_cache_with_public
get :cache_me_forever, params: {public: true}
- assert_equal "max-age=#{100.years.to_i}, public", @response.headers["Cache-Control"]
+ assert_equal "max-age=#{100.years}, public", @response.headers["Cache-Control"]
assert_not_nil @response.etag
end
def test_cache_with_private
get :cache_me_forever
- assert_equal "max-age=#{100.years.to_i}, private", @response.headers["Cache-Control"]
+ assert_equal "max-age=#{100.years}, private", @response.headers["Cache-Control"]
assert_not_nil @response.etag
assert_response :success
end
diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb
index dfedc8ae25..e9896a71f4 100644
--- a/actionpack/test/dispatch/request_test.rb
+++ b/actionpack/test/dispatch/request_test.rb
@@ -977,13 +977,17 @@ class RequestParameters < BaseRequestTest
test "parameters not accessible after rack parse error of invalid UTF8 character" do
request = stub_request("QUERY_STRING" => "foo%81E=1")
+ assert_raises(ActionController::BadRequest) { request.parameters }
+ end
- 2.times do
- assert_raises(ActionController::BadRequest) do
- # rack will raise a Rack::Utils::InvalidParameterError when parsing this query string
- request.parameters
- end
- end
+ test "parameters containing an invalid UTF8 character" do
+ request = stub_request("QUERY_STRING" => "foo=%81E")
+ assert_raises(ActionController::BadRequest) { request.parameters }
+ end
+
+ test "parameters containing a deeply nested invalid UTF8 character" do
+ request = stub_request("QUERY_STRING" => "foo[bar]=%81E")
+ assert_raises(ActionController::BadRequest) { request.parameters }
end
test "parameters not accessible after rack parse error 1" do
diff --git a/actionpack/test/dispatch/show_exceptions_test.rb b/actionpack/test/dispatch/show_exceptions_test.rb
index cbb12a2209..15dd702161 100644
--- a/actionpack/test/dispatch/show_exceptions_test.rb
+++ b/actionpack/test/dispatch/show_exceptions_test.rb
@@ -93,13 +93,13 @@ class ShowExceptionsTest < ActionDispatch::IntegrationTest
assert_kind_of AbstractController::ActionNotFound, env["action_dispatch.exception"]
assert_equal "/404", env["PATH_INFO"]
assert_equal "/not_found_original_exception", env["action_dispatch.original_path"]
- [404, { "Content-Type" => "text/plain" }, ["YOU FAILED BRO"]]
+ [404, { "Content-Type" => "text/plain" }, ["YOU FAILED"]]
end
@app = ActionDispatch::ShowExceptions.new(Boomer.new, exceptions_app)
get "/not_found_original_exception", headers: { 'action_dispatch.show_exceptions' => true }
assert_response 404
- assert_equal "YOU FAILED BRO", body
+ assert_equal "YOU FAILED", body
end
test "returns an empty response if custom exceptions app returns X-Cascade pass" do
generated by cgit v1.2.3 (git 2.25.1) at 2024-10-03 02:13:35 +0000