From c454e0b546106b877a7431d494295beee23a01bf Mon Sep 17 00:00:00 2001 From: Chad Wilson <29788154+chadlwilson@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:10:58 +0800 Subject: [PATCH] fix: correct opt-in `ServletEnv` charset mismapping when parsing query strings Also unifies the query parsing on Rack 2.2's proper validated parser and compares against DefaultEnv. --- CHANGELOG.md | 1 + .../ruby/rack/handler/servlet/servlet_env.rb | 106 ++++++++---------- .../rack/handler/servlet_env_parsing_spec.rb | 90 +++++++++++++++ src/spec/ruby/rack/handler/servlet_spec.rb | 77 ------------- 4 files changed, 136 insertions(+), 138 deletions(-) create mode 100644 src/spec/ruby/rack/handler/servlet_env_parsing_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 52d36f62a..2cd4136b0 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/main/ruby/rack/handler/servlet/servlet_env.rb b/src/main/ruby/rack/handler/servlet/servlet_env.rb index 4fd358054..03a2ffd7e 100644 --- a/src/main/ruby/rack/handler/servlet/servlet_env.rb +++ b/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 diff --git a/src/spec/ruby/rack/handler/servlet_env_parsing_spec.rb b/src/spec/ruby/rack/handler/servlet_env_parsing_spec.rb new file mode 100644 index 000000000..be21a70e2 --- /dev/null +++ b/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 diff --git a/src/spec/ruby/rack/handler/servlet_spec.rb b/src/spec/ruby/rack/handler/servlet_spec.rb index 769814d16..a10b23c72 100644 --- a/src/spec/ruby/rack/handler/servlet_spec.rb +++ b/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