Review ERROR query-string handling

[joakime]

Jetty version(s)
12.0.0

Java version/vendor (use: java -version)
Any

OS type/version
Any

Description
@gregw is the query string merged? like in the case of forward/include?

Scenario 1:

1. Original request to /foo?a=b
2. During handling, servlet calls sendError(500)
3. Error mapping exists for status code 500, pointing to location /error
4. The ERROR dispatch to /error is a servlet.

Scenario 2:

1. Original request to /foo?a=b
2. During handling, servlet calls sendError(500)
3. Error mapping exists for status code 500, pointing to location /error?c=d
4. The ERROR dispatch to /error is a servlet.

Scenario 3:

1. Original request to /foo?a=b
2. During handling, servlet calls AsyncContext.dispatch("/does-not-exist")
3. Results in a 404
4. Error mapping exists for status code 404, pointing to location /error
5. The ERROR dispatch to /error is a servlet.

Scenario 4:

1. Original request to /foo?a=b
2. During handling, servlet calls AsyncContext.dispatch("/does-not-exist")
3. Results in a 404
4. Error mapping exists for status code 404, pointing to location /error?c=d
5. The ERROR dispatch to /error is a servlet.

What should HttpServletRequest.getQueryString() return in each case?

For the two AsyncContext.dispatch() scenarios, this is basically a double dispatch. (original, requested-bad-page, error)

[gregw]

Kind of duplicate of #10130

[joakime]

The original code for ErrorRequest.getQueryString() didn't do merging.

https://github.com/eclipse/jetty.project/blob/ec2dbe73a8fb29243820ad822074808c422d056c/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/Dispatcher.java#L633-L643

[gregw]

Whether the error page exists or is a servlet does not matter.

Original query string and parameters must be maintained. This is a MUST before a 12.0.0 release

IFF error page uris are allowed to contain query strings, then these should be merged. But I'm not sure if we have previously supported this. Can be done later.

[gregw]

The previous code suggests that an error page uri can have a query string. In that case it probably should be merged, but it is going to be s very rare case, so we can keep the previous behavior for now.

But no matter what, the original query parameters must be preserved on map and string form.

[joakime]

Opened PR #10132

[joakime]

Whether the error page exists or is a servlet does not matter.

I think it does.

If the original request into the system is /foo?a=b and that dispatch would result in a 404.
That means the ?a=b query section is preserved for the ERROR dispatch, right?

Now, how about a request into the system is /foo?a=b and that IS served / handled.
But during that handling the servlet does AsyncContext.dispatch("/bar?x=y") and that doesn't exist (would result in a 404)
And that 404 error-page handled by a servlet (lets say something simple like /error)
The ERROR dispatch for that 404 would have what as the query string?

• ?a=b is the original request, but not the one that triggered the 404.
• ?x=y is the request that triggered the 404, so the resulting query string should be ?x=y (not the original ?a=b)

This testcase doesn't exist AFAICT

[gregw]

I'm pretty sure that at no time will the query string every be just x=y, as it will be merged in the async dispatch to be a=b&x=y

Besides, which servlet request is fed to the error logic is a matter for sync handling. Here we have a request passed in and a URI used to construct the dispatcher. The job is to combine. For error dispatching, we have previously used the URI query if not null, else the request query. We need to do at least that in 12.

However, on consideration, we should probably merge query string and parameters. But not important to do that before the release as we have never done it before

[gregw]

@sbordet @joakime At the very minimum, we need to change the getQueryString to only return the query string from the _uri if it is not null. It should not null it out if it is null.

But in EE9, we use the forward mechanism for error dispatches, so the query strings are actually merged. Moreover the parameters are recalculated to match.
So we need to do the same in EE10. Thus we need to look at how forward is merging query string and parameters and do the same for an error dispatch.

[joakime]

PR #10132 (the revert) has been merged.

Will work on ERROR dispatch param merging next.

[gregw]

@joakime I think it might be as simple as to add something like:

        @Override
        public String getQueryString()
        {
            String targetQuery = (_uri == null) ? null : _uri.getQuery();
            if (getRequest() instanceof HttpServletRequest httpServletRequest)
            {
                if (targetQuery == null)
                    return httpServletRequest.getQueryString();

                if (httpServletRequest.getQueryString() == null)
                    return  targetQuery;

                return UrlEncoded.encode(getParams(), StandardCharsets.ISO_8859_1, false);
            }
            return targetQuery;
        }

to ParameterRequestWrapper and to remove the getQueryString method from both extended implementations.

I have some concerns about the efficiency of getParams() in this class as it will often take a needless copy of the map if there is no query parameter in the new URI... but that's another PR.
The getQueryString() doesn't cache the result as it is rare that this will be called for anything but debugging, and then when it is called it will often be cheap, and when it is not cheap then getParams() caches most of the work and finally I can't see any common use-case where getQueryString() is called multiple times.

[joakime]

@gregw this would be new functionality in Jetty.

While researching, I came across a past issue where we discussed this #2074 (it's a long issue discussion, but it does help with explaining why the codebase does what it does) - even has a link to a servlet spec discussion on this too - jakartaee/servlet#308

And when looking at Jetty 10 tests, I can see we don't ever merge the req.getQueryString() result.

https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherForwardTest.java#L130-L176

See the check in "servlet2" on the getQueryString()

[gregw]

Plus my code is wrong as it would put body params into the merged query. Let's think about this....

[gregw]

@joakime after reading #2074 (good research) I see I am just repeating the panic of 2020. We are correctly merging the actual parameters and we deliberately decided not to merge the actual queryString and just go with simple replacement.
So I don't think we need to do anything other than put a comment in the code that we don't merge the queryString on purpose to comply with defacto standard behaviour as set by other implementations.

[joakime]

Merged PR #10144