From 61d59576ff4c6532c53f6d8f495cdb1ece33cad5 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 15 Mar 2023 18:33:07 -0700 Subject: [PATCH 1/5] Revert "Prefer to use `query_parser` itself as the cache key. (#2058)" This reverts commit 5f90c33e4ccee827cb5df3d8854dc72791345c51. --- lib/rack/request.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/rack/request.rb b/lib/rack/request.rb index fbdead8ab..86e133cd0 100644 --- a/lib/rack/request.rb +++ b/lib/rack/request.rb @@ -523,7 +523,7 @@ def parseable_data? # behaviour. This includes sub-classes that override query_parser or # expand_params. def cache_key - query_parser + query_parser.class end # Given a current input value, and a validity key, check if the cache @@ -560,8 +560,7 @@ def cache_key # If the cache was not defined for this cache key, then create a new cache: unless cache - cache = Hash.new.compare_by_identity - set_header(key, cache) + set_header(key, cache = {}) end begin From 77f3334e2bec112f5f86816f02f47de16a347433 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 15 Mar 2023 18:33:16 -0700 Subject: [PATCH 2/5] Revert "Fix handling of cached values in `Rack::Request`. (#2054)" This reverts commit d25feddcbe634d95ec693bfbd710167a11c74069. --- lib/rack/request.rb | 154 +++++++++++++------------------------------ test/spec_request.rb | 66 ++----------------- 2 files changed, 49 insertions(+), 171 deletions(-) diff --git a/lib/rack/request.rb b/lib/rack/request.rb index 86e133cd0..e69696450 100644 --- a/lib/rack/request.rb +++ b/lib/rack/request.rb @@ -480,114 +480,25 @@ def parseable_data? PARSEABLE_DATA_MEDIA_TYPES.include?(media_type) end - # Given a current input value, and a validity key, check if the cache - # is valid, and if so, return the cached value. If not, yield the - # current value to the block, and set the cache to the result. - # - # This method does not use cache_key, so it is shared between all - # instance of Rack::Request and it's sub-classes. - private def cache_for(key, validity_key, current_value) - # Get the current value of the validity key and compare it with the input value: - if get_header(validity_key).equal?(current_value) - # If the values are the same, then the cache is valid, so return the cached value. - if has_header?(key) - value = get_header(key) - # If the cached value is an exception, then re-raise it. - if value.is_a?(Exception) - raise value.class, value.message, cause: value.cause - else - # Otherwise, return the cached value. - return value - end - end - end - - # If the cache is not valid, then yield the current value to the block: - value = yield(current_value) - - # Set the validity key to the current value so that we can detect changes: - set_header(validity_key, current_value) - - # Set the cache to the result of the block, and return the result: - set_header(key, value) - rescue => error - # If an exception is raised, then set the cache to the exception, and re-raise it: - set_header(validity_key, current_value) - set_header(key, error) - raise - end - - # This cache key is used by cached values generated by class_cache_for, - # specfically GET and POST. This is to ensure that the cache is not - # shared between instances of different classes which have different - # behaviour. This includes sub-classes that override query_parser or - # expand_params. - def cache_key - query_parser.class - end - - # Given a current input value, and a validity key, check if the cache - # is valid, and if so, return the cached value. If not, yield the - # current value to the block, and set the cache to the result. - # - # This method uses cache_key to ensure that the cache is not shared - # between instances of different classes which have different - # behaviour of the cached operations. - private def class_cache_for(key, validity_key, current_value) - # The cache is organised in the env as: - # env[key][cache_key] = value - # and is valid as long as env[validity_key].equal?(current_value) - - cache_key = self.cache_key - - # Get the current value of the validity key and compare it with the input value: - if get_header(validity_key).equal?(current_value) - # Lookup the cache for the current cache key: - if cache = get_header(key) - if cache.key?(cache_key) - # If the cache is valid, then return the cached value. - value = cache[cache_key] - if value.is_a?(Exception) - # If the cached value is an exception, then re-raise it. - raise value.class, value.message, cause: value.cause - else - # Otherwise, return the cached value. - return value - end - end - end - end - - # If the cache was not defined for this cache key, then create a new cache: - unless cache - set_header(key, cache = {}) - end - - begin - # Yield the current value to the block to generate an updated value: - value = yield(current_value) - - # Only set this after generating the value, so that if an error or other cache depending on the same key, it will be invalidated correctly: - set_header(validity_key, current_value) - return cache[cache_key] = value - rescue => error - set_header(validity_key, current_value) - cache[cache_key] = error - raise - end - end - # Returns the data received in the query string. def GET - class_cache_for(RACK_REQUEST_QUERY_HASH, RACK_REQUEST_QUERY_STRING, query_string) do - expand_params(query_param_list) + if get_header(RACK_REQUEST_QUERY_STRING) == query_string + if query_hash = get_header(RACK_REQUEST_QUERY_HASH) + return query_hash + end end + + set_header(RACK_REQUEST_QUERY_HASH, expand_params(query_param_list)) end def query_param_list - cache_for(RACK_REQUEST_QUERY_PAIRS, RACK_REQUEST_QUERY_STRING, query_string) do - set_header(RACK_REQUEST_QUERY_HASH, nil) - split_query(query_string, '&') + if get_header(RACK_REQUEST_QUERY_STRING) == query_string + get_header(RACK_REQUEST_QUERY_PAIRS) + else + query_pairs = split_query(query_string, '&') + set_header RACK_REQUEST_QUERY_STRING, query_string + set_header RACK_REQUEST_QUERY_HASH, nil + set_header(RACK_REQUEST_QUERY_PAIRS, query_pairs) end end @@ -596,13 +507,33 @@ def query_param_list # This method support both application/x-www-form-urlencoded and # multipart/form-data. def POST - class_cache_for(RACK_REQUEST_FORM_HASH, RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT)) do - expand_params(body_param_list) + if get_header(RACK_REQUEST_FORM_INPUT).equal?(get_header(RACK_INPUT)) + if form_hash = get_header(RACK_REQUEST_FORM_HASH) + return form_hash + end end + + set_header(RACK_REQUEST_FORM_HASH, expand_params(body_param_list)) end def body_param_list - cache_for(RACK_REQUEST_FORM_PAIRS, RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT)) do |rack_input| + if error = get_header(RACK_REQUEST_FORM_ERROR) + raise error.class, error.message, cause: error.cause + end + + begin + rack_input = get_header(RACK_INPUT) + + form_pairs = nil + + # If the form data has already been memoized from the same + # input: + if get_header(RACK_REQUEST_FORM_INPUT).equal?(rack_input) + if form_pairs = get_header(RACK_REQUEST_FORM_PAIRS) + return form_pairs + end + end + if rack_input.nil? form_pairs = [] elsif form_data? || parseable_data? @@ -613,16 +544,19 @@ def body_param_list # form_vars.sub!(/\0\z/, '') # performance replacement: form_vars.slice!(-1) if form_vars.end_with?("\0") - # Removing this line breaks Rail test "test_filters_rack_request_form_vars"! - set_header(RACK_REQUEST_FORM_VARS, form_vars) - + set_header RACK_REQUEST_FORM_VARS, form_vars form_pairs = split_query(form_vars, '&') end else form_pairs = [] end - - form_pairs + + set_header RACK_REQUEST_FORM_INPUT, rack_input + set_header RACK_REQUEST_FORM_HASH, nil + set_header(RACK_REQUEST_FORM_PAIRS, form_pairs) + rescue => error + set_header(RACK_REQUEST_FORM_ERROR, error) + raise end end diff --git a/test/spec_request.rb b/test/spec_request.rb index e525621a7..2a3f792a3 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -1554,19 +1554,12 @@ def initialize(*) rack_input.write(input) rack_input.rewind - form_hash_cache = {} - - req = make_request Rack::MockRequest.env_for( - "/", - "rack.request.form_hash" => form_hash_cache, - "rack.request.form_input" => rack_input, - :input => rack_input - ) - - form_hash = {'foo' => 'bar'}.freeze - form_hash_cache[req.cache_key] = form_hash + req = make_request Rack::MockRequest.env_for("/", + "rack.request.form_hash" => { 'foo' => 'bar' }, + "rack.request.form_input" => rack_input, + :input => rack_input) - req.POST.must_equal form_hash + req.POST.must_equal req.env['rack.request.form_hash'] end it "conform to the Rack spec" do @@ -1964,53 +1957,4 @@ def make_request(env) DelegateRequest.new super(env) end end - - class UpperRequest < Rack::Request - def expand_params(parameters) - parameters.map do |(key, value)| - [key.upcase, value] - end.to_h - end - - # If this is not specified, the behaviour becomes order dependent. - def cache_key - :my_request - end - end - - it "correctly expands parameters" do - env = {"QUERY_STRING" => "foo=bar"} - - request = Rack::Request.new(env) - request.query_param_list.must_equal [["foo", "bar"]] - request.GET.must_equal "foo" => "bar" - - upper_request = UpperRequest.new(env) - upper_request.query_param_list.must_equal [["foo", "bar"]] - upper_request.GET.must_equal "FOO" => "bar" - - env['QUERY_STRING'] = "foo=bar&bar=baz" - - request.GET.must_equal "foo" => "bar", "bar" => "baz" - upper_request.GET.must_equal "FOO" => "bar", "BAR" => "baz" - end - - class BrokenRequest < Rack::Request - def expand_params(parameters) - raise "boom" - end - end - - it "raises an error if expand_params raises an error" do - env = {"QUERY_STRING" => "foo=bar"} - - request = Rack::Request.new(env) - request.GET.must_equal "foo" => "bar" - - broken_request = BrokenRequest.new(env) - lambda { broken_request.GET }.must_raise RuntimeError - - # Subsequnt calls also raise an error: - lambda { broken_request.GET }.must_raise RuntimeError - end end From be3ea6eadd1f0ba0300bf4406d218613f9e09bc5 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 15 Mar 2023 18:33:38 -0700 Subject: [PATCH 3/5] Revert "Add `QueryParser#missing_value` for handling missing values + tests. (#2052)" This reverts commit 59d9ba903fdb50cf8db708c8263a7b2a79de83fb. --- lib/rack.rb | 1 - lib/rack/query_parser.rb | 15 +++------------ test/spec_query_parser.rb | 33 --------------------------------- 3 files changed, 3 insertions(+), 46 deletions(-) delete mode 100644 test/spec_query_parser.rb diff --git a/lib/rack.rb b/lib/rack.rb index d00b96667..69780545e 100644 --- a/lib/rack.rb +++ b/lib/rack.rb @@ -41,7 +41,6 @@ module Rack autoload :MethodOverride, "rack/method_override" autoload :Mime, "rack/mime" autoload :NullLogger, "rack/null_logger" - autoload :QueryParser, "rack/query_parser" autoload :Recursive, "rack/recursive" autoload :Reloader, "rack/reloader" autoload :RewindableInput, "rack/rewindable_input" diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb index 525900f7b..1c05ae828 100644 --- a/lib/rack/query_parser.rb +++ b/lib/rack/query_parser.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require_relative 'bad_request' -require 'uri' module Rack class QueryParser @@ -112,14 +111,6 @@ def normalize_params(params, name, v, _depth=nil) _normalize_params(params, name, v, 0) end - # This value is used by default when a parameter is missing (nil). This - # usually happens when a parameter is specified without an `=value` part. - # The default value is an empty string, but this can be overridden by - # subclasses. - def missing_value - String.new - end - private def _normalize_params(params, name, v, depth) raise ParamsTooDeepError if depth >= param_depth_limit @@ -154,7 +145,7 @@ def missing_value return if k.empty? - v ||= missing_value + v ||= String.new if after == '' if k == '[]' && depth != 0 @@ -216,8 +207,8 @@ def params_hash_has_key?(hash, key) true end - def unescape(string, encoding = Encoding::UTF_8) - URI.decode_www_form_component(string, encoding) + def unescape(s) + Utils.unescape(s) end class Params < Hash diff --git a/test/spec_query_parser.rb b/test/spec_query_parser.rb deleted file mode 100644 index 5f4708186..000000000 --- a/test/spec_query_parser.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -require_relative 'helper' - -separate_testing do - require_relative '../lib/rack/query_parser' -end - -describe Rack::QueryParser do - def query_parser - @query_parser ||= Rack::QueryParser.new(Rack::QueryParser::Params, 8) - end - - it "has a default value" do - assert_equal "", query_parser.missing_value - end - - it "can normalize values with missing values" do - query_parser.parse_nested_query("a=a").must_equal({"a" => "a"}) - query_parser.parse_nested_query("a=").must_equal({"a" => ""}) - query_parser.parse_nested_query("a").must_equal({"a" => ""}) - end - - it "can override default missing value" do - def query_parser.missing_value - nil - end - - query_parser.parse_nested_query("a=a").must_equal({"a" => "a"}) - query_parser.parse_nested_query("a=").must_equal({"a" => ""}) - query_parser.parse_nested_query("a").must_equal({"a" => nil}) - end -end From 0827e96d01321bcd0e33f5f8656a97849be3a60d Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 15 Mar 2023 18:33:56 -0700 Subject: [PATCH 4/5] Revert "Split form/query parsing into two steps (#2038)" This reverts commit 9f059d19647aeaef5c2cc683a333c06120caf939. --- lib/rack/constants.rb | 2 - lib/rack/multipart.rb | 25 ------------ lib/rack/query_parser.rb | 51 ++++++++---------------- lib/rack/request.rb | 83 ++++++++++------------------------------ test/spec_request.rb | 9 ++--- 5 files changed, 41 insertions(+), 129 deletions(-) diff --git a/lib/rack/constants.rb b/lib/rack/constants.rb index 5b0c181e5..13365935b 100644 --- a/lib/rack/constants.rb +++ b/lib/rack/constants.rb @@ -54,13 +54,11 @@ module Rack RACK_RESPONSE_FINISHED = 'rack.response_finished' RACK_REQUEST_FORM_INPUT = 'rack.request.form_input' RACK_REQUEST_FORM_HASH = 'rack.request.form_hash' - RACK_REQUEST_FORM_PAIRS = 'rack.request.form_pairs' RACK_REQUEST_FORM_VARS = 'rack.request.form_vars' RACK_REQUEST_FORM_ERROR = 'rack.request.form_error' RACK_REQUEST_COOKIE_HASH = 'rack.request.cookie_hash' RACK_REQUEST_COOKIE_STRING = 'rack.request.cookie_string' RACK_REQUEST_QUERY_HASH = 'rack.request.query_hash' - RACK_REQUEST_QUERY_PAIRS = 'rack.request.query_pairs' RACK_REQUEST_QUERY_STRING = 'rack.request.query_string' RACK_METHODOVERRIDE_ORIGINAL_METHOD = 'rack.methodoverride.original_method' end diff --git a/lib/rack/multipart.rb b/lib/rack/multipart.rb index 4b02fb3e8..165b4db3a 100644 --- a/lib/rack/multipart.rb +++ b/lib/rack/multipart.rb @@ -19,31 +19,6 @@ class MissingInputError < StandardError include BadRequest end - # Accumulator for multipart form data, conforming to the QueryParser API. - # In future, the Parser could return the pair list directly, but that would - # change its API. - class ParamList # :nodoc: - def self.make_params - new - end - - def self.normalize_params(params, key, value) - params << [key, value] - end - - def initialize - @pairs = [] - end - - def <<(pair) - @pairs << pair - end - - def to_params_hash - @pairs - end - end - class << self def parse_multipart(env, params = Rack::Utils.default_query_parser) unless io = env[RACK_INPUT] diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb index 1c05ae828..1592a01e3 100644 --- a/lib/rack/query_parser.rb +++ b/lib/rack/query_parser.rb @@ -37,42 +37,19 @@ def initialize(params_class, param_depth_limit) @param_depth_limit = param_depth_limit end - # Originally stolen from Mongrel, now with some modifications: + # Stolen from Mongrel, with some small modifications: # Parses a query string by breaking it up at the '&'. You can also use this # to parse cookies by changing the characters used in the second parameter # (which defaults to '&'). - # - # Returns an array of 2-element arrays, where the first element is the - # key and the second element is the value. - def split_query(qs, separator = nil, &unescaper) - pairs = [] - - if qs && !qs.empty? - unescaper ||= method(:unescape) - - qs.split(separator ? (COMMON_SEP[separator] || /[#{separator}] */n) : DEFAULT_SEP).each do |p| - next if p.empty? - pair = p.split('=', 2).map!(&unescaper) - pair << nil if pair.length == 1 - pairs << pair - end - end - - pairs - rescue ArgumentError => e - raise InvalidParameterError, e.message, e.backtrace - end - - # Parses a query string by breaking it up at the '&'. You can also use this - # to parse cookies by changing the characters used in the second parameter - # (which defaults to '&'). - # - # Returns a hash where each value is a string (when a key only appears once) - # or an array of strings (when a key appears more than once). def parse_query(qs, separator = nil, &unescaper) + unescaper ||= method(:unescape) + params = make_params - split_query(qs, separator, &unescaper).each do |k, v| + (qs || '').split(separator ? (COMMON_SEP[separator] || /[#{separator}] */n) : DEFAULT_SEP).each do |p| + next if p.empty? + k, v = p.split('=', 2).map!(&unescaper) + if cur = params[k] if cur.class == Array params[k] << v @@ -84,7 +61,7 @@ def parse_query(qs, separator = nil, &unescaper) end end - params.to_h + return params.to_h end # parse_nested_query expands a query string into structural types. Supported @@ -95,11 +72,17 @@ def parse_query(qs, separator = nil, &unescaper) def parse_nested_query(qs, separator = nil) params = make_params - split_query(qs, separator).each do |k, v| - _normalize_params(params, k, v, 0) + unless qs.nil? || qs.empty? + (qs || '').split(separator ? (COMMON_SEP[separator] || /[#{separator}] */n) : DEFAULT_SEP).each do |p| + k, v = p.split('=', 2).map! { |s| unescape(s) } + + _normalize_params(params, k, v, 0) + end end - params.to_h + return params.to_h + rescue ArgumentError => e + raise InvalidParameterError, e.message, e.backtrace end # normalize_params recursively expands parameters into structural types. If diff --git a/lib/rack/request.rb b/lib/rack/request.rb index e69696450..a3eb9926a 100644 --- a/lib/rack/request.rb +++ b/lib/rack/request.rb @@ -483,22 +483,11 @@ def parseable_data? # Returns the data received in the query string. def GET if get_header(RACK_REQUEST_QUERY_STRING) == query_string - if query_hash = get_header(RACK_REQUEST_QUERY_HASH) - return query_hash - end - end - - set_header(RACK_REQUEST_QUERY_HASH, expand_params(query_param_list)) - end - - def query_param_list - if get_header(RACK_REQUEST_QUERY_STRING) == query_string - get_header(RACK_REQUEST_QUERY_PAIRS) + get_header(RACK_REQUEST_QUERY_HASH) else - query_pairs = split_query(query_string, '&') - set_header RACK_REQUEST_QUERY_STRING, query_string - set_header RACK_REQUEST_QUERY_HASH, nil - set_header(RACK_REQUEST_QUERY_PAIRS, query_pairs) + query_hash = parse_query(query_string, '&') + set_header(RACK_REQUEST_QUERY_STRING, query_string) + set_header(RACK_REQUEST_QUERY_HASH, query_hash) end end @@ -507,16 +496,6 @@ def query_param_list # This method support both application/x-www-form-urlencoded and # multipart/form-data. def POST - if get_header(RACK_REQUEST_FORM_INPUT).equal?(get_header(RACK_INPUT)) - if form_hash = get_header(RACK_REQUEST_FORM_HASH) - return form_hash - end - end - - set_header(RACK_REQUEST_FORM_HASH, expand_params(body_param_list)) - end - - def body_param_list if error = get_header(RACK_REQUEST_FORM_ERROR) raise error.class, error.message, cause: error.cause end @@ -524,36 +503,36 @@ def body_param_list begin rack_input = get_header(RACK_INPUT) - form_pairs = nil - - # If the form data has already been memoized from the same - # input: - if get_header(RACK_REQUEST_FORM_INPUT).equal?(rack_input) - if form_pairs = get_header(RACK_REQUEST_FORM_PAIRS) - return form_pairs + # If the form hash was already memoized: + if form_hash = get_header(RACK_REQUEST_FORM_HASH) + # And it was memoized from the same input: + if get_header(RACK_REQUEST_FORM_INPUT).equal?(rack_input) + return form_hash end end + # Otherwise, figure out how to parse the input: if rack_input.nil? - form_pairs = [] + set_header RACK_REQUEST_FORM_INPUT, nil + set_header(RACK_REQUEST_FORM_HASH, {}) elsif form_data? || parseable_data? - unless form_pairs = Rack::Multipart.extract_multipart(self, Rack::Multipart::ParamList) - form_vars = rack_input.read + unless set_header(RACK_REQUEST_FORM_HASH, parse_multipart) + form_vars = get_header(RACK_INPUT).read # Fix for Safari Ajax postings that always append \0 # form_vars.sub!(/\0\z/, '') # performance replacement: form_vars.slice!(-1) if form_vars.end_with?("\0") set_header RACK_REQUEST_FORM_VARS, form_vars - form_pairs = split_query(form_vars, '&') + set_header RACK_REQUEST_FORM_HASH, parse_query(form_vars, '&') end + + set_header RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT) + get_header RACK_REQUEST_FORM_HASH else - form_pairs = [] + set_header RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT) + set_header(RACK_REQUEST_FORM_HASH, {}) end - - set_header RACK_REQUEST_FORM_INPUT, rack_input - set_header RACK_REQUEST_FORM_HASH, nil - set_header(RACK_REQUEST_FORM_PAIRS, form_pairs) rescue => error set_header(RACK_REQUEST_FORM_ERROR, error) raise @@ -693,28 +672,6 @@ def parse_multipart Rack::Multipart.extract_multipart(self, query_parser) end - def split_query(query, d = '&') - query_parser = query_parser() - unless query_parser.respond_to?(:split_query) - query_parser = Utils.default_query_parser - unless query_parser.respond_to?(:split_query) - query_parser = QueryParser.make_default(0) - end - end - - query_parser.split_query(query, d) - end - - def expand_params(pairs, query_parser = query_parser()) - params = query_parser.make_params - - pairs.each do |key, value| - query_parser.normalize_params(params, key, value) - end - - params.to_params_hash - end - def split_header(value) value ? value.strip.split(/[,\s]+/) : [] end diff --git a/test/spec_request.rb b/test/spec_request.rb index 2a3f792a3..9a94b35fc 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -572,12 +572,11 @@ def self.req(headers) end it "parse the query string" do - request = make_request(Rack::MockRequest.env_for("/?foo=bar&quux=bla¬hing&empty=")) - request.query_string.must_equal "foo=bar&quux=bla¬hing&empty=" - request.GET.must_equal "foo" => "bar", "quux" => "bla", "nothing" => "", "empty" => "" + request = make_request(Rack::MockRequest.env_for("/?foo=bar&quux=bla")) + request.query_string.must_equal "foo=bar&quux=bla" + request.GET.must_equal "foo" => "bar", "quux" => "bla" request.POST.must_be :empty? - request.params.must_equal "foo" => "bar", "quux" => "bla", "nothing" => "", "empty" => "" - request.query_param_list.must_equal [["foo", "bar"], ["quux", "bla"], ["nothing", nil], ["empty", ""]] + request.params.must_equal "foo" => "bar", "quux" => "bla" end it "handles invalid unicode in query string value" do From 8beebd375c48adcd6a0577d5fee6b3b7dda84d52 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Wed, 15 Mar 2023 18:46:12 -0700 Subject: [PATCH 5/5] Make query parameters without = have nil values This was Rack's historical behavior. While it doesn't match URL spec section 5.1.3.3, keeping the historical behavior avoids all of the complexity required to support the URL spec standard by default, but also support frameworks that want to be backwards compatible. This keeps as much of the specs added by the recently reverted commits that make sense. --- lib/rack.rb | 1 + lib/rack/query_parser.rb | 7 +++---- test/spec_query_parser.rb | 17 +++++++++++++++++ test/spec_request.rb | 22 +++++++++++++--------- test/spec_utils.rb | 14 +++++++------- 5 files changed, 41 insertions(+), 20 deletions(-) create mode 100644 test/spec_query_parser.rb diff --git a/lib/rack.rb b/lib/rack.rb index 69780545e..d00b96667 100644 --- a/lib/rack.rb +++ b/lib/rack.rb @@ -41,6 +41,7 @@ module Rack autoload :MethodOverride, "rack/method_override" autoload :Mime, "rack/mime" autoload :NullLogger, "rack/null_logger" + autoload :QueryParser, "rack/query_parser" autoload :Recursive, "rack/recursive" autoload :Reloader, "rack/reloader" autoload :RewindableInput, "rack/rewindable_input" diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb index 1592a01e3..28cbce18f 100644 --- a/lib/rack/query_parser.rb +++ b/lib/rack/query_parser.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require_relative 'bad_request' +require 'uri' module Rack class QueryParser @@ -128,8 +129,6 @@ def normalize_params(params, name, v, _depth=nil) return if k.empty? - v ||= String.new - if after == '' if k == '[]' && depth != 0 return [v] @@ -190,8 +189,8 @@ def params_hash_has_key?(hash, key) true end - def unescape(s) - Utils.unescape(s) + def unescape(string, encoding = Encoding::UTF_8) + URI.decode_www_form_component(string, encoding) end class Params < Hash diff --git a/test/spec_query_parser.rb b/test/spec_query_parser.rb new file mode 100644 index 000000000..dbb8b14ed --- /dev/null +++ b/test/spec_query_parser.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require_relative 'helper' + +separate_testing do + require_relative '../lib/rack/query_parser' +end + +describe Rack::QueryParser do + query_parser ||= Rack::QueryParser.make_default(8) + + it "can normalize values with missing values" do + query_parser.parse_nested_query("a=a").must_equal({"a" => "a"}) + query_parser.parse_nested_query("a=").must_equal({"a" => ""}) + query_parser.parse_nested_query("a").must_equal({"a" => nil}) + end +end diff --git a/test/spec_request.rb b/test/spec_request.rb index 9a94b35fc..9966762ea 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -572,11 +572,11 @@ def self.req(headers) end it "parse the query string" do - request = make_request(Rack::MockRequest.env_for("/?foo=bar&quux=bla")) - request.query_string.must_equal "foo=bar&quux=bla" - request.GET.must_equal "foo" => "bar", "quux" => "bla" + request = make_request(Rack::MockRequest.env_for("/?foo=bar&quux=bla¬hing&empty=")) + request.query_string.must_equal "foo=bar&quux=bla¬hing&empty=" + request.GET.must_equal "foo" => "bar", "quux" => "bla", "nothing" => nil, "empty" => "" request.POST.must_be :empty? - request.params.must_equal "foo" => "bar", "quux" => "bla" + request.params.must_equal "foo" => "bar", "quux" => "bla", "nothing" => nil, "empty" => "" end it "handles invalid unicode in query string value" do @@ -1553,12 +1553,16 @@ def initialize(*) rack_input.write(input) rack_input.rewind - req = make_request Rack::MockRequest.env_for("/", - "rack.request.form_hash" => { 'foo' => 'bar' }, - "rack.request.form_input" => rack_input, - :input => rack_input) + form_hash = {} + + req = make_request Rack::MockRequest.env_for( + "/", + "rack.request.form_hash" => form_hash, + "rack.request.form_input" => rack_input, + :input => rack_input + ) - req.POST.must_equal req.env['rack.request.form_hash'] + req.POST.must_be_same_as form_hash end it "conform to the Rack spec" do diff --git a/test/spec_utils.rb b/test/spec_utils.rb index b771eafae..64e070ed8 100644 --- a/test/spec_utils.rb +++ b/test/spec_utils.rb @@ -141,7 +141,7 @@ def assert_nested_query(exp, act) it "parse nested query strings correctly" do Rack::Utils.parse_nested_query("foo"). - must_equal "foo" => "" + must_equal "foo" => nil Rack::Utils.parse_nested_query("foo="). must_equal "foo" => "" Rack::Utils.parse_nested_query("foo=bar"). @@ -158,7 +158,7 @@ def assert_nested_query(exp, act) Rack::Utils.parse_nested_query("&foo=1&&bar=2"). must_equal "foo" => "1", "bar" => "2" Rack::Utils.parse_nested_query("foo&bar="). - must_equal "foo" => "", "bar" => "" + must_equal "foo" => nil, "bar" => "" Rack::Utils.parse_nested_query("foo=bar&baz="). must_equal "foo" => "bar", "baz" => "" Rack::Utils.parse_nested_query("my+weird+field=q1%212%22%27w%245%267%2Fz8%29%3F"). @@ -168,19 +168,19 @@ def assert_nested_query(exp, act) must_equal "pid=1234" => "1023", "a" => "b" Rack::Utils.parse_nested_query("foo[]"). - must_equal "foo" => [""] + must_equal "foo" => [nil] Rack::Utils.parse_nested_query("foo[]="). must_equal "foo" => [""] Rack::Utils.parse_nested_query("foo[]=bar"). must_equal "foo" => ["bar"] Rack::Utils.parse_nested_query("foo[]=bar&foo"). - must_equal "foo" => "" + must_equal "foo" => nil Rack::Utils.parse_nested_query("foo[]=bar&foo["). - must_equal "foo" => ["bar"], "foo[" => "" + must_equal "foo" => ["bar"], "foo[" => nil Rack::Utils.parse_nested_query("foo[]=bar&foo[=baz"). must_equal "foo" => ["bar"], "foo[" => "baz" Rack::Utils.parse_nested_query("foo[]=bar&foo[]"). - must_equal "foo" => ["bar", ""] + must_equal "foo" => ["bar", nil] Rack::Utils.parse_nested_query("foo[]=bar&foo[]="). must_equal "foo" => ["bar", ""] @@ -192,7 +192,7 @@ def assert_nested_query(exp, act) must_equal "foo" => ["bar"], "baz" => ["1", "2", "3"] Rack::Utils.parse_nested_query("x[y][z]"). - must_equal "x" => { "y" => { "z" => "" } } + must_equal "x" => { "y" => { "z" => nil } } Rack::Utils.parse_nested_query("x[y][z]=1"). must_equal "x" => { "y" => { "z" => "1" } } Rack::Utils.parse_nested_query("x[y][z][]=1").