fix: correct opt-in ServletEnv charset mismapping when parsing query strings

CHANGELOG.md

@@ -1,5 +1,6 @@
 ## 1.3.1 (UNRELEASED)
 
+- fix: correct opt-in `ServletEnv` charset mismapping when parsing query strings
 - fix: ensure `rack.` internal headers are stripped in responses
 - chore: remove ancient dead Rails 2-era adapter code
 - fix: ensure ErrorApp does not mutate shared headers constant

src/main/ruby/rack/handler/servlet/servlet_env.rb

@@ -56,13 +56,17 @@ def load_env_key(env, key)
         def load_parameters
           get_only = ! POST_PARAM_METHODS.include?( @servlet_env.getMethod )
           # we only need to really do this for POSTs but we'll handle all
-          query_hash, form_hash = {}, {}
-          # NOTE: HttpServletRequest#getParameterMap behaves differently than
-          # Rack - preserves all parameters (at least on Tomcat 6/7) - nothing
-          # gets "lost" (like with Rack), most notable differences :
-          # - multi values are kept even when they do not end with '[]'
-          # - if there's a query param and the same param name is in the (POST)
-          #   body, both are kept and present as a multi-value
+          query_params, form_params = query_parser.make_params, query_parser.make_params
+          # NOTE: HttpServletRequest#getParameterMap merges query-string and
+          # (POST) body parameters and exposes *every* raw value per name -
+          # including repeated names that do not end with '[]' and names that
+          # appear in both the query string and the body. We rely on that
+          # completeness only to reconstruct which values came from the query
+          # string vs the body (see the length comparison below), so GET and
+          # POST each end up matching what Rack would have parsed. The values
+          # themselves are still normalized by Rack (#normalize_params) - i.e.
+          # repeated non-'[]' names collapse to the last value, '[]' yields an
+          # Array, etc. - so the resulting params are not "multi-valued" here.
           @servlet_env.getParameterMap.each do |key, val| # String, String[]
             val = [''] if val.nil? # e.g. buggy Jetty 6
             val = [''] if val.length == 1 && val[0].nil?
@@ -76,22 +80,22 @@ def load_parameters
                     get_vals << v; true
                   end
                 end
-                store_parameter(key, get_vals, query_hash)
-                store_parameter(key, post_vals, form_hash)
+                store_parameter(query_params, key, get_vals)
+                store_parameter(form_params, key, post_vals)
               else
-                store_parameter(key, val, query_hash)
+                store_parameter(query_params, key, val)
               end
             else # POST param :
-              store_parameter(key, val, form_hash)
+              store_parameter(form_params, key, val)
             end
           end
           # Rack::Request#GET
           @env[ QUERY_STRING ] = query_string
-          @env[ QUERY_HASH ] = query_hash
+          @env[ QUERY_HASH ] = query_params.to_h
           # Rack::Request#POST
           # TODO should recreate the input e.g. multipart/form-data ...
           @env[ FORM_INPUT ] = @env['rack.input']
-          @env[ FORM_HASH ] = form_hash
+          @env[ FORM_HASH ] = form_params.to_h
         end
 
         def [](key)
@@ -102,45 +106,21 @@ def [](key)
         end
         public :[]
 
-        # @private
-        KEY_SEP = /([^\[\]]+)(?:\[(.*)\])?/
-
-        # Store the parameter into the given Hash.
-        # By default this is performed in a Rack compatible way and thus
-        # some parameter values might get "lost" - it only accepts multiple
-        # values for a paramater if it ends with '[]'.
+        # Store the servlet parameter values under the given (raw) name into the
+        # Rack params, reusing Rack's own QueryParser#normalize_params
         #
-        # @param key the param name
-        # @param val the value(s) in a array-like structure
-        # @param hash the Hash to store the name, value pair
-        def store_parameter(key, val, hash)
-          # emulating Rack::Utils.parse_nested_query behavior
-
-          if match = key.match(KEY_SEP)
-            n_key = match[1]; sub = match[2]
-          else
-            n_key = key; sub = nil # normalized-key[ sub-key ]
-          end
-
-          if sub
-            if sub.empty? # e.g. foo[]=1&foo[]=2
-              if arr = hash[ n_key ]
-                return mark_parameter_error "expected Array (got #{arr.class}) for param `#{n_key}'" unless arr.is_a?(Array)
-                hash[ n_key ] = arr + val.to_a; return
-              end
-              hash[ n_key ] = val.to_a # String[]
-            else # foo[bar]=rrr&foo[baz]=zzz
-              if hsh = hash[ n_key ]
-                return mark_parameter_error "expected Hash (got #{hsh.class}) for param `#{n_key}'" unless hsh.is_a?(Hash)
-                store_parameter(sub, val, hsh)
-              else
-                hash[ n_key ] = { sub => val[ val.length - 1 ] }
-              end
-            end
-          else
-            # for 'foo=bad&foo=bar' does { 'foo' => 'bar' }
-            hash[ n_key ] = val[ val.length - 1 ] # last
-          end
+        # Servlet parameter values arrive already split into an Array (unlike
+        # Rack which sees each name=value pair separately) so the values are
+        # replayed one by one; a ParameterTypeError aborts the offending name
+        # and is re-raised lazily on QUERY_HASH access (see #[]).
+        #
+        # @param params the (Rack::QueryParser::Params) accumulator
+        # @param key the (raw) param name, possibly with `[]`/`[nested]` syntax
+        # @param val the value(s) in an array-like structure
+        def store_parameter(params, key, val)
+          val.each { |v| query_parser.normalize_params(params, key, v, query_parser.param_depth_limit) }
+        rescue ::Rack::Utils::ParameterTypeError => e
+          @parameter_error = e
         end
 
         COOKIE_STRING = "rack.request.cookie_string".freeze
@@ -166,25 +146,29 @@ def load_cookies
 
         private
 
+        # The Rack query parser used for both building the params accumulators
+        # (#make_params) and nesting each value (#normalize_params). Rack's
+        # default parser is a memoized singleton, so this is cheap to call.
+        def query_parser
+          ::Rack::Utils.default_query_parser
+        end
+
         def query_string
           @query_string ||= @servlet_env.getQueryString.to_s
         end
 
         def query_values(key)
-          # Rack::Utils.parse_nested_query does not return all values for a multi-key
-          # HttpUtils.parseQueryString although deprecated does what we need here :
-          # handles multiple values sent by the query string as a string array ...
+          # returns all query-string values for a (possibly repeated) param
+          # name as an Array, or nil when the name is not in the query string
           ( @query_string_table ||= parse_query_string )[key]
         end
 
         def parse_query_string
-          Java::JavaxServletHttp::HttpUtils.parseQueryString(query_string)
-        end
-
-        def mark_parameter_error(msg)
-          raise Rack::Utils::ParameterTypeError, msg
-        rescue Rack::Utils::ParameterTypeError => e
-          @parameter_error = e
+          # Rack::Utils.parse_query yields a String for single and an Array for
+          # repeated names - normalize to always-Array for query_values' callers
+          ::Rack::Utils.parse_query(query_string, '&').each_with_object({}) do |(key, value), table|
+            table[key] = value.is_a?(Array) ? value : [ value ]
+          end
         end
 
       end

src/spec/ruby/rack/handler/servlet_env_parsing_spec.rb

@@ -0,0 +1,90 @@
+#--
+# This source code is available under the MIT license.
+# See the file LICENSE.txt for details.
+#++
+
+require File.expand_path('../../spec_helper', File.dirname(__FILE__))
+
+require 'rack'
+require 'rack/handler/servlet'
+
+# Differential coverage: the "pure" ServletEnv maps servlet-parsed parameters
+# (HttpServletRequest#getParameterMap) into the Rack env, whereas DefaultEnv
+# lets Rack itself parse the QUERY_STRING. ServletEnv's whole reason to exist
+# is to produce the *same* params Rack would - so here we drive the same query
+# string through both and assert Rack::Request#GET comes out identical.
+#
+# The decoded values handed to addParameter are written as literals (simulating
+# what a servlet container decodes getParameterMap to), NOT computed via Rack's
+# unescaper - otherwise the escaping comparison would be circular.
+describe 'Rack::Handler::Servlet ServletEnv vs DefaultEnv (parsing parity)' do
+
+  before do
+    @servlet_context = mock_servlet_context
+  end
+
+  # Build Rack::Request#GET from a query string using the given env class.
+  # +params+ are the (already url-decoded) name/value pairs the servlet
+  # container would expose via getParameterMap; DefaultEnv ignores them.
+  def get_params(env_class, query_string, params = [])
+    request = org.springframework.mock.web.MockHttpServletRequest.new(@servlet_context)
+    request.setMethod('GET')
+    request.setRequestURI('/path')
+    request.setQueryString(query_string)
+    params.each { |name, value| request.addParameter(name, value) }
+    response = org.springframework.mock.web.MockHttpServletResponse.new
+    servlet_env = org.jruby.rack.servlet.ServletRackEnvironment.new(request, response, @rack_context)
+    Rack::Request.new(env_class.create(servlet_env)).GET
+  end
+
+  DefaultEnv = Rack::Handler::Servlet::DefaultEnv
+  ServletEnv = Rack::Handler::Servlet::ServletEnv
+
+  # [ description, query_string, decoded getParameterMap pairs ]
+  PARITY_CASES = [
+    [ 'plain params',            'a=1&b=2',                 [ %w(a 1), %w(b 2) ] ],
+    [ 'space via +',             'a=x+y',                   [ [ 'a', 'x y' ] ] ],
+    [ 'space via %20',           'a=x%20y',                 [ [ 'a', 'x y' ] ] ],
+    [ 'encoded & = #',           'a=%26%3D%23',             [ [ 'a', '&=#' ] ] ],
+    [ 'encoded = in value',      'a=b%3Dc',                 [ [ 'a', 'b=c' ] ] ],
+    [ 'utf-8 name and value',    'caf%C3%A9=%C3%BC',        [ [ "café", "ü" ] ] ],
+    [ 'repeated flat key',       'a=1&a=2',                 [ %w(a 1), %w(a 2) ] ],
+    [ 'array [] key',            'a%5B%5D=1&a%5B%5D=2',     [ [ 'a[]', '1' ], [ 'a[]', '2' ] ] ],
+    [ 'hash [k] key',            'a%5Bb%5D=1',              [ [ 'a[b]', '1' ] ] ],
+    [ 'deep [k][j] nesting',     'a%5Bb%5D%5Bc%5D=x',       [ [ 'a[b][c]', 'x' ] ] ],
+    [ 'hash-in-array a[][b]',    'a%5B%5D%5Bb%5D=1',        [ [ 'a[][b]', '1' ] ] ],
+    [ 'numeric-index hash',      'huh%5B1%5D=b&huh%5B0%5D=a', [ [ 'huh[1]', 'b' ], [ 'huh[0]', 'a' ] ] ],
+    [ 'bracket-in-bracket meh[]','foo%5Bmeh%5B%5D%5D=x&foo%5Bmeh%5B%5D%5D=42', [ [ 'foo[meh[]]', 'x' ], [ 'foo[meh[]]', '42' ] ] ],
+    [ 'unbalanced brackets',     'foo]=0&bar[=1&baz_=2&[meh=3', [ [ 'foo]', '0' ], [ 'bar[', '1' ], [ 'baz_', '2' ], [ '[meh', '3' ] ] ],
+    [ 'empty value',             'a=',                      [ [ 'a', '' ] ] ],
+  ]
+  # NOTE: not a parity case - ServletEnv intentionally does NOT treat ';' as a
+  # value separator (getParameterMap keeps 'b;la'), whereas Rack 2.x's parser
+  # still splits on ';'. See the ServletEnv-specific semicolon spec in
+  # servlet_spec.rb. (Rack 3.x dropped ';' so it would be parity there.)
+
+  PARITY_CASES.each do |desc, query_string, params|
+    it "matches DefaultEnv (real Rack) for #{desc}" do
+      default_get = get_params(DefaultEnv, query_string)
+      servlet_get = get_params(ServletEnv, query_string, params)
+      expect(servlet_get).to eq(default_get)
+    end
+  end
+
+  it 'raises the same ParameterTypeError as Rack on a type clash' do
+    # foo[]=0 then foo[bar]=1 - array then hash for the same name
+    default_err = nil
+    begin; get_params(DefaultEnv, 'foo%5B%5D=0&foo%5Bbar%5D=1'); rescue => e; default_err = e; end
+    servlet_err = nil
+    begin
+      get_params(ServletEnv, 'foo%5B%5D=0&foo%5Bbar%5D=1', [ [ 'foo[]', '0' ], [ 'foo[bar]', '1' ] ])
+    rescue => e
+      servlet_err = e
+    end
+
+    expect(default_err).to be_a(::Rack::Utils::ParameterTypeError)
+    expect(servlet_err).to be_a(::Rack::Utils::ParameterTypeError)
+    expect(servlet_err.message).to eq(default_err.message)
+  end
+
+end

src/spec/ruby/rack/handler/servlet_spec.rb

@@ -404,83 +404,6 @@ def getAttributeNames
       expect { env.fetch('attr4') }.to raise_error # KeyError
     end
 
-    it "parses strange request parameters (Rack-compat)" do
-      servlet_request = @servlet_request
-      servlet_request.setMethod 'GET'
-      servlet_request.setContextPath '/'
-      servlet_request.setPathInfo '/path'
-      servlet_request.setRequestURI '/home/path'
-
-      servlet_request.setQueryString 'foo]=0&bar[=1&baz_=2&[meh=3'
-      servlet_request.addParameter('foo]', '0')
-      servlet_request.addParameter('bar[', '1')
-      servlet_request.addParameter('baz_', '2')
-      servlet_request.addParameter('[meh', '3')
-
-      env = servlet.create_env(@servlet_env)
-      rack_request = Rack::Request.new(env)
-
-      # { "foo" => "0", "bar[" => "1", "baz_" => "2", "meh" => "3" }
-
-      expect(rack_request.GET['foo']).to eql('0')
-      expect(rack_request.GET['baz_']).to eql('2')
-      expect(rack_request.GET['meh']).to eql('3')
-
-      expect(rack_request.query_string).to eql 'foo]=0&bar[=1&baz_=2&[meh=3'
-    end
-
-    it "parses nestedx request parameters (Rack-compat)" do
-      servlet_request = @servlet_request
-      servlet_request.setMethod 'GET'
-      servlet_request.setContextPath '/'
-      servlet_request.setPathInfo '/path'
-      servlet_request.setRequestURI '/home/path'
-
-      servlet_request.setQueryString 'foo[bar]=0&foo[baz]=1&foo[bar]=2&foo[meh[]]=x&foo[meh[]]=42&huh[1]=b&huh[0]=a'
-      servlet_request.addParameter('foo[bar]', '0')
-      servlet_request.addParameter('foo[baz]', '1')
-      servlet_request.addParameter('foo[bar]', '2')
-      servlet_request.addParameter('foo[meh[]]', 'x')
-      servlet_request.addParameter('foo[meh[]]', '42')
-      servlet_request.addParameter('huh[1]', 'b')
-      servlet_request.addParameter('huh[0]', 'a')
-
-      env = servlet.create_env(@servlet_env)
-      rack_request = Rack::Request.new(env)
-
-      # params = { "foo" => { "bar" => "2", "baz" => "1", "meh" => [ nil, nil ] }, "huh" => { "1" => "b", "0" => "a" } }
-      #    expect(rack_request.GET).to eql(params)
-
-      expect(rack_request.GET['foo']['bar']).to eql('2')
-      expect(rack_request.GET['foo']['baz']).to eql('1')
-      expect(rack_request.params['foo']['meh']).to be_a Array
-      expect(rack_request.params['huh']).to eql({ "1" => "b", "0" => "a" })
-
-      expect(rack_request.POST).to eql Hash.new
-
-      expect(rack_request.query_string).to eql 'foo[bar]=0&foo[baz]=1&foo[bar]=2&foo[meh[]]=x&foo[meh[]]=42&huh[1]=b&huh[0]=a'
-    end
-
-    it "raises if nested request parameters are broken (Rack-compat)" do
-      servlet_request = @servlet_request
-      servlet_request.setMethod 'GET'
-      servlet_request.setContextPath '/'
-      servlet_request.setPathInfo '/path'
-      servlet_request.setRequestURI '/home/path'
-      servlet_request.setQueryString 'foo[]=0&foo[bar]=1'
-      servlet_request.addParameter('foo[]', '0')
-      servlet_request.addParameter('foo[bar]', '1')
-
-      env = servlet.create_env(@servlet_env)
-      rack_request = Rack::Request.new(env)
-
-      expect { rack_request.GET }.to raise_error(Rack::Utils::ParameterTypeError, "expected Hash (got Array) for param `foo'")
-      expect(rack_request.POST).to eq({})
-      expect { rack_request.params }.to raise_error(Rack::Utils::ParameterTypeError, "expected Hash (got Array) for param `foo'")
-
-      expect(rack_request.query_string).to eq 'foo[]=0&foo[bar]=1'
-    end
-
   end
 
   shared_examples "(eager)rack-env" do