Forward with query duplicates parameter values

[fripoli]

I'm using Spring boot with Jetty and Tuckey URL Rewrite.

The URL rewrite is doing a forward including all query parameters that were sent to the original request.

For example

request /en/product?productId=123
forward /product?productId=123

The query parameter map on the request then become a map of productId with the value [123,123]

Is that the expected behaviour?

I can see there are some testing around this forward functionality here but none of them covers this case

[sbordet]

DispatcherForwardTest.testQueryReplacedByForwardWithQuery() shows that the query string given to the forward hides the query string in the request, so you should see just productId=123 from the forward.

What version of Jetty are you running ?

Can you add a test case to DispatcherForwardTest that reproduces the issue ?

[fripoli]

I'm using spring boot 1.5.6.RELEASE which is using jetty 9.4.6.v20170531

On the test you mentioned, the forwarded url has the same parameter as as the original request but with another value ... not sure why this would be the case, but ok ...

In my case, the forward is not changing any values, I'm simply removing the language part of the url so it can be handled by the controllers as if it wasn't there ... so the parameters are the same and so are their values

[sbordet]

Changing the test to use the same query string in the forward results in the same behavior (i.e. the query string is replaced in the forward, not merged).
We don't do any logic on the value of a parameter.
Works correctly in the test case, so we need a reproducible test case as small as possible.

[fripoli]

The problem is that on line 164 of that test, when it does

checkThat(req.getParameter("a"),Matchers.equalTo("3 three"));

the getParameter("a") method is returning the index 0 of the list that has all values for the query parameter a and that is why the test passes

Spring, is doing

String[] paramValues = request.getParameterValues(name);
if (paramValues != null) {
    arg = (paramValues.length == 1 ? paramValues[0] : paramValues);
}

and that makes all values to be returned

[sbordet]

Ah, good point about index 0. I'll review the test and the behavior and report back.

[fripoli]

Do you still need more info on this?

[joakime]

@janbartel @gregw I've been able to replicate the issues reported here.

See test cases in commit 7022220
and test results at https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/activity?branch=jetty-9.4.x-2074-duplicate-forward-query-entry

Currently, the fault lies in MultiMap.addAllValues(), in that it will cause the duplicate values.

Take this testcase ...

https://github.com/eclipse/jetty.project/blob/7022220904d1d985ee4597481c958a1b91960c80/jetty-util/src/test/java/org/eclipse/jetty/util/MultiMapTest.java#L565-L577

This will result in a new "merged" MultiMap with a single entry "id", but with 2 values ["123", "123"] (The assertion on line 577 fails)

We either need to make MultiMap.addAllValues() not add a value if it's not unique (which is what I'm leaning towards, as addAllValues() is only used by the Request object to merge maps.)

Or we make Request merge query values following the servlet spec logic.

[joakime]

One way to look at this is from the URI point of view.

Given that the original URI is http://machine/one?id=123
And the forward URI is /two?id=123
Should the resulting URI be ...

1. http://machine/two?id=123 (we merge unique name/value pairs)
2. http://machine/two?id=123&id=123 (we just concatenate the requested forward to the end of the original query)

The decision here needs to be thoughtful as to not trip over HPP (HTTP Parameter Pollution) security concerns.

[gregw]

I'm not 100% sure.... but I do recall that it is intended behaviour that a parameter that is both in the original URI and in the forward URI has both values! So I do think it is the expected behaviour.

[fripoli]

original URI /one?foo=123
forward URI /two?bar=345

resulting URI should be /two?bar=345

[joakime]

The Servlet Spec 3.1 Final Section 9.1.1 seems to indicate that there should be no merging.
But the old/original URI queryString should be present only as a Request attribute under name javax.servlet.forward.query_string (aka RequestDispatcher.FORWARD_QUERY_STRING).

If we change this in jetty-9.4.x this will definitely need to be a Compliance configurable. @gregw think about this one too while you noodle out the HttpCompliance changes in Jetty 10.0.x

[joakime]

This means that the RequestDispatcher shouldn't be using Request.mergeQueryParameters...

https://github.com/eclipse/jetty.project/blob/7022220904d1d985ee4597481c958a1b91960c80/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java#L2456

[sbordet]

The existence of mergeQueryParameters() has a reason, and I believe @gregw and I went through all the cases at the time I implemented it.
@fripoli do you have an authoritative source for the behavior you would like? Because I'm not sure it's the correct one, even if it may be desirable.

[fripoli]

It's what I imagined as desirable ... but if what @joakime described from the spec is correct, that is the behaviour

[gregw]

My recollection is that the query merge behaviour was driven by TCK test suite. As we are currently in the process of rerunning the TCK, let's hold action on this one until we can check it's results.

@fripoli I'm 99% sure that the resulting URI for your example should be /two?foo=123&bar=345.

[joakime]

After reading that OWASP link for HTTP Parameter Pollution, I'm not sure that merging for RequestDispatcher is the right thing anymore (from a security and behavior point of view).

Also, in the use case that opened this issue, I think we are still in violation of Section 9.1.1 of the spec.

Parameters specified in the query string used to create the RequestDispatcher take precedence over other parameters of the same name passed to the included servlet. The parameters associated with a RequestDispatcher are scoped to apply only for the duration of the include or forward call.

To me, that means if the parameter name exists in the RequestDispatcher forward call, that parameter replace/overwrites the values of the original URI. not merging/adding values.

[gregw]

I'm not sure how this relates to a HPP attack vector? An application that a)trusts user input; b) does not validate user input; c) does not correctly handle multiple vs single parameters; is going to be HPP vulnerable regardless of the use of forwarding. Now forwarding may well be one way that an application mixes in trusted parameters with user supplied parameters, but it's not the only way. In short, I think that HPP is a framework/application issue and that there is little that the container can do - other than check for correct adherence to RFCs and standards for formatting and escaping of URIs and form content.

Now that is not to say that we should not revisit the parameter merging we are doing here to check we are interpreting the spec correctly, I just don't see it as a security issue.

[janbartel]

@joakime but also see Servlet Spec section 9.4.1 Query String:

The request dispatching mechanism is responsible for aggregating query string
parameters when forwarding or including requests.

There is no description of what "aggregation" means, but could be taken to imply that existing query params are merged with any query params specified to the request dispatcher, as jetty is doing.

The section that you cite, 9.1.1 could be taken to mean that if the query param is present in the request dispatcher call, it should override any existing param of the same name in the calling request.

[janbartel]

Just to follow up and be clear: the query string that is passed to the forwarded-to servlet does not contain duplicate entries of the same name - jetty correctly gives precedence to query params present in the call to the request dispatcher; the param map from within the forwarded-to servlet does contain multiple values for the same name, but the value provided to the request dispatcher will always be at index 0.

In other words, given an original request to "/one?id=123", and request dispatch forward to "/two?id=123", from within the forwarded-to servlet we have:

1. request query string is "/two?id=123"
2. request.getParameterValues("id") -> ["123", 123"] where index 0 is from the original request and 1 is from the forward

The above behaviour has been true at least as far back as jetty-8.2, I haven't looked any further back.

I'm not clear whether the OP is arguing that request.getParameterValues() should never return multiple values from within the forward-to servlet (ie even if the values are different), or only never return duplicate values?

[SerCeMan]

Hey, @joakime! Thank you for working on this! I also faced the same problem with the async support in spring framework, so I created a minimal reproducer for the issue: https://github.com/SerCeMan/jettyredirect.

Returning a forward: ... asynchronously results in duplicating the parameters:

$ curl http://localhost:8070/page1?str=test
params: test,test

Is there anything I can do to help to move this issue forward? As far as I can understand from the comments, there is an ambiguity in the spec, please let me know if I should file a bug in the spring project instead.

[joakime]

The existence of mergeQueryParameters() has a reason, and I believe @gregw and I went through all the cases at the time I implemented it.

I think that the method mergeQueryParameters() is wrongly named, as what it really does is appendQueryParameters()

[sbordet]

I think that the method mergeQueryParameters() is wrongly named, as what it really does is appendQueryParameters()

Nope, query parameters are not appended, they are merged. Values for the same key are appended, but the keys are merged, so mergeQueryParameters() is the right name.

[janbartel]

@olamy are there any tck tests for the handling of query params for forwards an includes??

[olamy]

@janbartel I cannot see any dedicated to this particular case.

[highfestiva]

This breaking change may or may not follow spec, but for me this means reverting to an older version or in the longer term replacing with a different server to keep legacy code running. I suggest you fix according to least astonishment asap and ignore vague spec wordings.

[sbordet]

@highfestiva what older Jetty version shows a behavior that's good for you?

[joakime]

I cannot find an issue filed at https://github.com/eclipse-ee4j/servlet-api asking for clarification on this.

So I asked jakartaee/servlet#308

[gregw]

@sbordet @janbartel @joakime @fripoli While working on #4721 I have noticed something very wrong with our merging of query parameters!

When we merge the actual parameter maps during a forward we keep all parameter values with:
https://github.com/eclipse/jetty.project/blob/jetty-9.4.27.v20200227/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java#L2454-L2464

But when we merge the query strings, we remove the values from the old string that are named in the new string:
https://github.com/eclipse/jetty.project/blob/jetty-9.4.27.v20200227/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java#L2469-L2496

So whatever is the correct behaviour, somehow we are doing one for the map and the other for the string, so that cannot be right!

[gregw]

Servlet specification 9.1.1 says:

Parameters specified in the query string used to create the RequestDispatcher take
precedence over other parameters of the same name passed to the included servlet. 

So "take precedence over other parameters of the same name" is a little bit vague and could be interpreted either as "are ordered before parameter values of the same name" or "replace parameters of the same name".

I'll experiment with #4721 and check against the TCK

[gregw]

I can only find one TCK test regarding this at https://github.com/eclipse-ee4j/jakartaee-tck/blob/8.0/src/com/sun/ts/tests/servlet/spec/requestdispatcher/URLClient.java#L463-L478
which says:

   * @test_Strategy: 
   * 1. Create servlets TestServlet and HttpTestServlet; 
   * 2. Send request to TestServlet with ?testname=getQueryStringForwardTest; 
   * 3. In TestServlet, access HttpTestServlet using RequestDispatcher.forward, with ?testname=getQueryStringTestForward; 
   * 4. Verify in HttpTestServlet, that getQueryString returns correct QueryString testname=getQueryStringTestForward according to 8.4.2

So this suggests that at least for parameters with same name and same value, that they are not duplicated. However, I can't find what document the 8.4.2 relates to as it is not 3.0, 3.1 or 4.0 versions of the spec document. I also can't see any tests that indicate what should be done if the values are different, but have the same name. I'll look to see if they have anything on form content that might help.

[joakime]

Should probably have this conversation about what the spec intends over at jakartaee/servlet#308

[gregw]

@joakime sure, but doing basic research first.
So I have created a test that does a POST with query params, body params and forward query params as follows:

POST query	POST body	forward query
unique1=post%20query	unique2=post%20body	unique3=forward%20query
name=post%20query	name=post%20body	name=forward%20query
same=same	same=same	same=same
list=one	list=two	list=three
list=two	list=three	list=four

I tried this on various servers and got:

Result: jetty/9.4.27.v20200227
  query=unique3=forward%20query&name=forward%20query&same=same&list=three&list=four&unique1=post%20query
    unique3=forward%20query
    name=forward%20query
    same=same
    list=three
    list=four
    unique1=post%20query
  params
    Action=[Test]
    list=[three, four, one, two]
    name=[forward query, post query, post body]
    same=[same, same, same]
    unique1=[post query]
    unique2=[post body]
    unique3=[forward query]

Result: Apache Tomcat/8.0.12
  query=unique3=forward%20query&name=forward%20query&same=same&list=three&list=four
    unique3=forward%20query
    name=forward%20query
    same=same
    list=three
    list=four
  params
    Action=[Test]
    list=[three, four, one, two]
    name=[forward query, post query, post body]
    same=[same, same, same]
    unique1=[post query]
    unique2=[post body]
    unique3=[forward query]

Result: Apache Tomcat/9.0.33
  query=unique3=forward%20query&name=forward%20query&same=same&list=three&list=four
    unique3=forward%20query
    name=forward%20query
    same=same
    list=three
    list=four
  params
    Action=[Test]
    list=[three, four, one, two]
    name=[forward query, post query, post body]
    same=[same, same, same]
    unique1=[post query]
    unique2=[post body]
    unique3=[forward query]

Result: WildFly Servlet 19.0.0.Final (WildFly Core 11.0.0.Final) - 2.0.30.Final
  query=unique3=forward%20query&name=forward%20query&same=same&list=three&list=four
    unique3=forward%20query
    name=forward%20query
    same=same
    list=three
    list=four
  params
    Action=[Test]
    list=[three, four, one, two]
    name=[forward query, post query, post body]
    same=[same, same, same]
    unique1=[post query]
    unique2=[post body]
    unique3=[forward query]

These are all pretty much the same and kind of agree with the strange interpretation of the spec (my panic is subsiding a little). The only difference is that Jetty includes the unique1 query parameter in the final query parameter, while tomcat and firefly don't even merge the query string and just use the string from the forward. Hmmm I'll have to test what they do if there is no query in the forward...

[gregw]

If the forward does not have a query, then all servers keep the original query string in the merge.

[gregw]

Considering the difference is very minor, I think we should keep 9.4 as is and change in 10 to act like the other servers with regards to the query string (replaced by forward not merged).