Skip to content

Commit

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

This reverts commit 5f90c33.

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

This reverts commit d25fedd.

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

This reverts commit 59d9ba9.

* Revert "Split form/query parsing into two steps (#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.
  • Loading branch information
@jeremyevans
jeremyevans committed Mar 16, 2023
1 parent 5f90c33 commit 77cf057
Show file tree
Hide file tree
Showing 7 changed files with 64 additions and 297 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 @@ -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]
Expand Down
61 changes: 17 additions & 44 deletions lib/rack/query_parser.rb
View file
Expand Up @@ -38,42 +38,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
Expand All @@ -85,7 +62,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 @@ -96,11 +73,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 @@ -112,14 +95,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 @@ -154,8 +129,6 @@ def missing_value

return if k.empty?

v ||= missing_value

if after == ''
if k == '[]' && depth != 0
return [v]
Expand Down
178 changes: 34 additions & 144 deletions lib/rack/request.rb
View file
Expand Up @@ -480,115 +480,14 @@ 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
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
cache = Hash.new.compare_by_identity
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)
end
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_HASH)
else
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 @@ -597,33 +496,46 @@ 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 error = get_header(RACK_REQUEST_FORM_ERROR)
raise error.class, error.message, cause: error.cause
end
end

def body_param_list
cache_for(RACK_REQUEST_FORM_PAIRS, RACK_REQUEST_FORM_INPUT, get_header(RACK_INPUT)) do |rack_input|
begin
rack_input = get_header(RACK_INPUT)

# 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")

# Removing this line breaks Rail test "test_filters_rack_request_form_vars"!
set_header(RACK_REQUEST_FORM_VARS, form_vars)

form_pairs = split_query(form_vars, '&')
set_header RACK_REQUEST_FORM_VARS, 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

form_pairs
rescue => error
set_header(RACK_REQUEST_FORM_ERROR, error)
raise
end
end

Expand Down Expand Up @@ -760,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

0 comments on commit 77cf057

Please sign in to comment.