Deprecate key_space_limit

It was determined that as this limit did not affect nested parameter hashes, it didn't actually prevent an attacker from using more than limited number of bytes for parameter keys, so this limit isn't actually doing anything useful. It is confusing people when it gets in the way of desired large parameter requests.

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].

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
 

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)

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"

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("/",

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