Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merging query parameters on forward dispatch #308

Open
joakime opened this issue Feb 28, 2020 · 6 comments
Open

Merging query parameters on forward dispatch #308

joakime opened this issue Feb 28, 2020 · 6 comments

Comments

@joakime
Copy link

joakime commented Feb 28, 2020

I'm looking for clarification about how (query) parameters are supposed to be handled during a RequestDispatcher and forward scenario. (I can find no prior issues about this)

When checking Servlet Spec 3.1 (Final) Section 9.1.1 It seems to indicate that there should be no merging of query parameters.
And that the old/original URI queryString should be present only as a HttpServletRequest attribute under name javax.servlet.forward.query_string (aka RequestDispatcher.FORWARD_QUERY_STRING).

Original Request URI:      /product?id=123
RequestDispatcher Path:    /alt/product?id=123
Parameters at forwarded:   [id] = [ "123", "123" ]
Equiv query at forwarded:  ?id=123&id=123

The original request had ?id=123, the RequestDispatcher was used with "/alt/product?id=123" and the request was .forward(), but the parameters at the forwarded destination shows 1 parameter named "id" with 2 values ["123", "123"].

This shows a behavior where the parameters are merge during the RequestDispatcher + forward.

It might make more sense if you see it like this ...

Original Request URI:      /product?id=123
RequestDispatcher Path:    /alt/product?id=456
Parameters at forwarded:   [id] = [ "123", "456" ]
Equiv query at forwarded:  ?id=123&id=456

The original request had ?id=123, the RequestDispatcher was used with "/alt/product?id=456" (different id value) and the request was .forward(), but the parameters at the forwarded destination shows 1 parameter named "id" with 2 values ["123", "456"].

This seems odd, why are the parameters merged?
Is this merging actually dictated by the spec?
The TCK does not appear to have any kind of tests for this behavior, so it feels like an undefined behavior between the various Servlet containers.

Here's another scenario ...

Original Request URI:      /product?id=123
RequestDispatcher Path:    /outofbusiness
Parameters at forwarded:   [id] = "123"
Equiv query at forwarded:  ?id=123

The original request had ?id=123, the RequestDispatcher was used with "/outofbusiness" (no id value) and the request was .forward(), but the parameters at the forwarded destination shows 1 parameter named "id" with a single value ["123"].
Why is this parameter preserved through the forwarded dispatch?

@markt-asf
Copy link
Contributor

I'm working from the Servlet 4.0 spec.
Section 9.1.1 is only part of the story.

Sections 9.4.1 / 9.7.1 state that the query string parameters should be "aggregated". i.e. combine both. See section 3.1 for a fuller explanation of "aggregated" in a similar context.

I take "precedence" in 9.1.1 to mean "appear first" in the aggregation.

On this basis the behaviour you describe is as expected.

@joakime
Copy link
Author

joakime commented Mar 13, 2020

@gregw
Copy link
Contributor

gregw commented Mar 30, 2020

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]

So all the servers pretty much agree except for the handling of the actual query string itself. Jetty aggregates the query string as well as the parameters, while undertow and tomcat just use the query string only from the forward and only do the aggregation in the parameter map. Spec is kind of silent on this one, but unless anybody else thinks that jetty is actually right here, I think we'll change to match the other servers.

@gregw
Copy link
Contributor

gregw commented Mar 30, 2020

@markt-asf If you agree Jetty is wrong here, then please close this one.

@stuartwdouglas
Copy link
Contributor

Either way we should probably add a TCK test

@markt-asf
Copy link
Contributor

I don't think Jetty is wrong. It is just a different interpretation of the spec. Some consistency / clarity would be good here. In addition to some TCK tests we should probably clarify the specification text as well. I think that will need to wait for 5.1 but I'd lean towards keeping this issue open until we do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants