From d38b45615d9c71b7a4f692ab5845b8d06a22d774 Mon Sep 17 00:00:00 2001 From: Samuel Williams Date: Thu, 16 Mar 2023 15:18:46 +1300 Subject: [PATCH] Make query parameters without = have nil values (#2059) (#2060) * Revert "Prefer to use `query_parser` itself as the cache key. (#2058)" This reverts commit 5f90c33e4ccee827cb5df3d8854dc72791345c51. * Revert "Fix handling of cached values in `Rack::Request`. (#2054)" This reverts commit d25feddcbe634d95ec693bfbd710167a11c74069. * Revert "Add `QueryParser#missing_value` for handling missing values + tests. (#2052)" This reverts commit 59d9ba903fdb50cf8db708c8263a7b2a79de83fb. * Revert "Split form/query parsing into two steps (#2038)" This reverts commit 9f059d19647aeaef5c2cc683a333c06120caf939. * 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. # Conflicts: # lib/rack/multipart.rb # lib/rack/request.rb # test/spec_request.rb --- lib/rack/constants.rb | 2 - lib/rack/multipart.rb | 25 ------------ lib/rack/query_parser.rb | 61 ++++++++-------------------- lib/rack/request.rb | 83 ++++++++++----------------------------- test/spec_query_parser.rb | 18 +-------- test/spec_request.rb | 19 +++++---- test/spec_utils.rb | 14 +++---- 7 files changed, 56 insertions(+), 166 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 5cae8f638..3347662ac 100644 --- a/lib/rack/multipart.rb +++ b/lib/rack/multipart.rb @@ -13,31 +13,6 @@ module Rack module Multipart MULTIPART_BOUNDARY = "AaB03x" - # 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) io = env[RACK_INPUT] diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb index c465e57ad..9e8434cf3 100644 --- a/lib/rack/query_parser.rb +++ b/lib/rack/query_parser.rb @@ -39,42 +39,19 @@ def initialize(params_class, _key_space_limit=(not_deprecated = true; nil), para @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 @@ -86,7 +63,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 @@ -97,11 +74,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 @@ -113,14 +96,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 @@ -155,8 +130,6 @@ def missing_value return if k.empty? - v ||= missing_value - if after == '' if k == '[]' && depth != 0 return [v] diff --git a/lib/rack/request.rb b/lib/rack/request.rb index 894216743..99a33aa1e 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_query_parser.rb b/test/spec_query_parser.rb index 5f4708186..dbb8b14ed 100644 --- a/test/spec_query_parser.rb +++ b/test/spec_query_parser.rb @@ -7,25 +7,9 @@ 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 + 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" => ""}) - 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}) diff --git a/test/spec_request.rb b/test/spec_request.rb index f1d5e97ee..169118635 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -574,10 +574,9 @@ def self.req(headers) 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.GET.must_equal "foo" => "bar", "quux" => "bla", "nothing" => nil, "empty" => "" 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", "nothing" => nil, "empty" => "" end it "not truncate query strings containing semi-colons #543 only in POST" do @@ -1535,12 +1534,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 0293ccfdc..c4f9b27fa 100644 --- a/test/spec_utils.rb +++ b/test/spec_utils.rb @@ -159,7 +159,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"). @@ -176,7 +176,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"). @@ -186,19 +186,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", ""] @@ -210,7 +210,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").