Skip to content

Commit

Permalink
Make query parameters without = have nil values (rack#2059)
Browse files Browse the repository at this point in the history 
* Revert "Prefer to use `query_parser` itself as the cache key. (rack#2058)"

This reverts commit 5f90c33.

* Revert "Fix handling of cached values in `Rack::Request`. (rack#2054)"

This reverts commit d25fedd.

* Revert "Add `QueryParser#missing_value` for handling missing values + tests. (rack#2052)"

This reverts commit 59d9ba9.

* Revert "Split form/query parsing into two steps (rack#2038)"

This reverts commit 9f059d1.

* 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
  • Loading branch information
@ioquatix
ioquatix committed Mar 16, 2023
1 parent 51e7a0f commit 99f2779
Show file tree
Hide file tree
Showing 7 changed files with 56 additions and 166 deletions.
2 changes: 0 additions & 2 deletions lib/rack/constants.rb
View file
Expand Up @@ -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
25 changes: 0 additions & 25 deletions lib/rack/multipart.rb
View file
Expand Up @@ -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]
Expand Down
61 changes: 17 additions & 44 deletions lib/rack/query_parser.rb
View file
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -155,8 +130,6 @@ def missing_value

return if k.empty?

v ||= missing_value

if after == ''
if k == '[]' && depth != 0
return [v]
Expand Down
83 changes: 20 additions & 63 deletions lib/rack/request.rb
View file
Expand Up @@ -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

Expand All @@ -507,53 +496,43 @@ 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

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
Expand Down Expand Up @@ -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
Expand Down
18 changes: 1 addition & 17 deletions test/spec_query_parser.rb
View file
Expand Up @@ -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})
Expand Down
19 changes: 11 additions & 8 deletions test/spec_request.rb
View file
Expand Up @@ -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&nothing&empty="))
request.query_string.must_equal "foo=bar&quux=bla&nothing&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
Expand Down Expand Up @@ -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
Expand Down
14 changes: 7 additions & 7 deletions test/spec_utils.rb
View file
Expand Up @@ -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").
Expand All @@ -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").
Expand All @@ -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", ""]

Expand All @@ -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").
Expand Down

0 comments on commit 99f2779

Please sign in to comment.