diff --git a/README.rdoc b/README.rdoc index 66cde310c..3bdaa9741 100644 --- a/README.rdoc +++ b/README.rdoc @@ -178,15 +178,8 @@ Several parameters can be modified on Rack::Utils to configure \Rack behaviour. e.g: - Rack::Utils.key_space_limit = 128 + Rack::Utils.param_depth_limit = 3 -=== key_space_limit - -The default number of bytes to allow all parameters keys in a given parameter hash to take up. -Does not affect nested parameter hashes, so doesn't actually prevent an attacker from using -more than this many bytes for parameter keys. - -Defaults to 65536 characters. === param_depth_limit @@ -214,6 +207,10 @@ Set to 0 for no limit. Can also be set via the +RACK_MULTIPART_PART_LIMIT+ environment variable. +=== key_space_limit + +No longer has an effect, deprecated. + == Changelog See {CHANGELOG.md}[https://github.com/rack/rack/blob/master/CHANGELOG.md]. diff --git a/lib/rack/query_parser.rb b/lib/rack/query_parser.rb index 28670fc18..9a85a4cae 100644 --- a/lib/rack/query_parser.rb +++ b/lib/rack/query_parser.rb @@ -14,15 +14,22 @@ class ParameterTypeError < TypeError; end # sequence. class InvalidParameterError < ArgumentError; end - def self.make_default(key_space_limit, param_depth_limit) - new Params, key_space_limit, param_depth_limit + def self.make_default(_key_space_limit=(not_deprecated = true; nil), param_depth_limit) + unless not_deprecated + warn("`first argument `key_space limit` is deprecated and no longer has an effect. Please call with only one argument, which will be required in a future version of Rack", uplevel: 1) + end + + new Params, param_depth_limit end - attr_reader :key_space_limit, :param_depth_limit + attr_reader :param_depth_limit + + def initialize(params_class, _key_space_limit=(not_deprecated = true; nil), param_depth_limit) + unless not_deprecated + warn("`second argument `key_space limit` is deprecated and no longer has an effect. Please call with only two arguments, which will be required in a future version of Rack", uplevel: 1) + end - def initialize(params_class, key_space_limit, param_depth_limit) @params_class = params_class - @key_space_limit = key_space_limit @param_depth_limit = param_depth_limit end @@ -120,15 +127,11 @@ def normalize_params(params, name, v, depth) end def make_params - @params_class.new @key_space_limit - end - - def new_space_limit(key_space_limit) - self.class.new @params_class, key_space_limit, param_depth_limit + @params_class.new end def new_depth_limit(param_depth_limit) - self.class.new @params_class, key_space_limit, param_depth_limit + self.class.new @params_class, param_depth_limit end private @@ -154,8 +157,7 @@ def unescape(s) end class Params - def initialize(limit) - @limit = limit + def initialize @size = 0 @params = {} end @@ -165,8 +167,6 @@ def [](key) end def []=(key, value) - @size += key.size if key && !@params.key?(key) - raise RangeError, 'exceeded available parameter key space' if @size > @limit @params[key] = value end diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb index 3a23a9d17..7d7719c0b 100644 --- a/lib/rack/utils.rb +++ b/lib/rack/utils.rb @@ -23,9 +23,10 @@ module Utils class << self attr_accessor :default_query_parser end - # The default number of bytes to allow parameter keys to take up. - # This helps prevent a rogue client from flooding a Request. - self.default_query_parser = QueryParser.make_default(65536, 32) + # The default amount of nesting to allowed by hash parameters. + # This helps prevent a rogue client from triggering a possible stack overflow + # when parsing parameters. + self.default_query_parser = QueryParser.make_default(32) module_function @@ -70,11 +71,12 @@ def self.param_depth_limit=(v) end def self.key_space_limit - default_query_parser.key_space_limit + warn("`Rack::Utils.key_space_limit` is deprecated as this value no longer has an effect. It will be removed in a future version of Rack", uplevel: 1) + 65536 end def self.key_space_limit=(v) - self.default_query_parser = self.default_query_parser.new_space_limit(v) + warn("`Rack::Utils.key_space_limit=` is deprecated and no longer has an effect. It will be removed in a future version of Rack", uplevel: 1) end if defined?(Process::CLOCK_MONOTONIC) diff --git a/test/spec_multipart.rb b/test/spec_multipart.rb index f4de71cf8..87902a1f5 100644 --- a/test/spec_multipart.rb +++ b/test/spec_multipart.rb @@ -98,17 +98,6 @@ def multipart_file(name) params['user_sid'].encoding.must_equal Encoding::UTF_8 end - it "raise RangeError if the key space is exhausted" do - env = Rack::MockRequest.env_for("/", multipart_fixture(:content_type_and_no_filename)) - - old, Rack::Utils.key_space_limit = Rack::Utils.key_space_limit, 1 - begin - lambda { Rack::Multipart.parse_multipart(env) }.must_raise(RangeError) - ensure - Rack::Utils.key_space_limit = old - end - end - it "parse multipart form webkit style" do env = Rack::MockRequest.env_for '/', multipart_fixture(:webkit) env['CONTENT_TYPE'] = "multipart/form-data; boundary=----WebKitFormBoundaryWLHCs9qmcJJoyjKR" @@ -219,7 +208,7 @@ def initialize(*) @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)} end end - query_parser = Rack::QueryParser.new c, 65536, 100 + query_parser = Rack::QueryParser.new c, 100 env = Rack::MockRequest.env_for("/", multipart_fixture(:text)) params = Rack::Multipart.parse_multipart(env, query_parser) params[:files][:type].must_equal "text/plain" diff --git a/test/spec_request.rb b/test/spec_request.rb index 111f49121..c2c71dc4b 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -295,7 +295,7 @@ def initialize(*) @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)} end end - parser = Rack::QueryParser.new(c, 65536, 100) + parser = Rack::QueryParser.new(c, 100) c = Class.new(Rack::Request) do define_method(:query_parser) do parser @@ -316,32 +316,6 @@ def initialize(*) req.params.must_equal "foo" => "bar", "quux" => "b;la;wun=duh" end - it "limit the keys from the GET query string" do - env = Rack::MockRequest.env_for("/?foo=bar") - - old, Rack::Utils.key_space_limit = Rack::Utils.key_space_limit, 1 - begin - req = make_request(env) - lambda { req.GET }.must_raise RangeError - ensure - Rack::Utils.key_space_limit = old - end - end - - it "limit the key size per nested params hash" do - nested_query = Rack::MockRequest.env_for("/?foo%5Bbar%5D%5Bbaz%5D%5Bqux%5D=1") - plain_query = Rack::MockRequest.env_for("/?foo_bar__baz__qux_=1") - - old, Rack::Utils.key_space_limit = Rack::Utils.key_space_limit, 3 - begin - exp = { "foo" => { "bar" => { "baz" => { "qux" => "1" } } } } - make_request(nested_query).GET.must_equal exp - lambda { make_request(plain_query).GET }.must_raise RangeError - ensure - Rack::Utils.key_space_limit = old - end - end - it "limit the allowed parameter depth when parsing parameters" do env = Rack::MockRequest.env_for("/?a#{'[a]' * 40}=b") req = make_request(env) @@ -388,7 +362,7 @@ def initialize(*) @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)} end end - parser = Rack::QueryParser.new(c, 65536, 100) + parser = Rack::QueryParser.new(c, 100) c = Class.new(Rack::Request) do define_method(:query_parser) do parser @@ -438,20 +412,6 @@ def initialize(*) req.params.must_equal "foo" => "bar", "quux" => "bla" end - it "limit the keys from the POST form data" do - env = Rack::MockRequest.env_for("", - "REQUEST_METHOD" => 'POST', - :input => "foo=bar&quux=bla") - - old, Rack::Utils.key_space_limit = Rack::Utils.key_space_limit, 1 - begin - req = make_request(env) - lambda { req.POST }.must_raise RangeError - ensure - Rack::Utils.key_space_limit = old - end - end - it "parse POST data with explicit content type regardless of method" do req = make_request \ Rack::MockRequest.env_for("/", diff --git a/test/spec_utils.rb b/test/spec_utils.rb index f753f85b6..a26739276 100644 --- a/test/spec_utils.rb +++ b/test/spec_utils.rb @@ -114,7 +114,7 @@ def assert_nested_query(exp, act) ex = { "foo" => nil } ex["foo"] = ex - params = Rack::Utils::KeySpaceConstrainedParams.new(65536) + params = Rack::Utils::KeySpaceConstrainedParams.new params['foo'] = params params.to_params_hash.to_s.must_equal ex.to_s end @@ -123,6 +123,32 @@ def assert_nested_query(exp, act) Rack::Utils.parse_nested_query(nil).must_equal({}) end + it "should warn using deprecated Rack::Util.key_space_limit=" do + begin + warn_arg = nil + Rack::Utils.define_singleton_method(:warn) do |*args| + warn_arg = args.first + end + Rack::Utils.key_space_limit = 65536 + warn_arg.must_equal("`Rack::Utils.key_space_limit=` is deprecated and no longer has an effect. It will be removed in a future version of Rack") + ensure + Rack::Utils.singleton_class.send(:remove_method, :warn) + end + end + + it "should warn using deprecated Rack::Util.key_space_limit" do + begin + warn_arg = nil + Rack::Utils.define_singleton_method(:warn) do |*args| + warn_arg = args.first + end + Rack::Utils.key_space_limit + warn_arg.must_equal("`Rack::Utils.key_space_limit` is deprecated as this value no longer has an effect. It will be removed in a future version of Rack") + ensure + Rack::Utils.singleton_class.send(:remove_method, :warn) + end + end + it "raise an exception if the params are too deep" do len = Rack::Utils.param_depth_limit @@ -259,7 +285,7 @@ def initialize(*) @params = Hash.new{|h, k| h[k.to_s] if k.is_a?(Symbol)} end end - Rack::Utils.default_query_parser = Rack::QueryParser.new(param_parser_class, 65536, 100) + Rack::Utils.default_query_parser = Rack::QueryParser.new(param_parser_class, 100) h1 = Rack::Utils.parse_query(",foo=bar;,", ";,") h1[:foo].must_equal "bar" h2 = Rack::Utils.parse_nested_query("x[y][][z]=1&x[y][][w]=2")