Welcome file redirects do not honor the relativeRedirectAllowed option

[2is10]

Jetty version(s)

9.4.43.v20210629

Java version/vendor (use: java -version)

openjdk version "11.0.12" 2021-07-20 LTS

OS type/version

macOS Big Sur 11.6 (20G165)

Description

Last year #5029 Relative Redirection introduced a new relativeRedirectAllowed option. (Thanks!) Unfortunately, at least some of the redirects that Jetty itself initiates do not yet honor that option.

For instance, org.eclipse.jetty.server.ResourceService.sendWelcome (the method that redirects to add a missing trailing slash if necessary) builds an absolute redirect URL. But org.eclipse.jetty.server.Response only honors HttpConfiguration.isRelativeRedirectAllowed() if the redirect URL is relative (includes no scheme).

This behavior is problematic for us because in some environments, we use a reverse proxy or load balancer in front of Jetty for SSL termination. As a result, https: requests look to Jetty like http: requests, so it ends up redirecting some https: requests to http: in order to add a trailing slash.

How to reproduce?

1. Allow relative redirects by calling httpConfiguration.setRelativeRedirectAllowed(true).
2. Register a servlet at a path like "/docs/*".
3. Issue a GET /docs request.
4. Notice that the Location header in the response contains an absolute URL (containing a scheme and host) instead of just a relative URL such as just "/docs/".

[joakime]

I don't quite follow your steps to reproduce.
What does the servlet at step 2 do?

[2is10]

The servlet could just serve static files from a directory tree.

var holder = new ServletHolder(DefaultServlet.class);
holder.setAsyncSupported(true);
holder.setInitParameter("resourceBase", "/tmp/docs");
holder.setInitParameter("pathInfoOnly", "true");
holder.setInitParameter("dirAllowed", "false");
context.addServlet(holder, "/docs/*");

[joakime]

So, the addition of the missing slash is because as far as HTTP is concerned, a request for /docs is a different resource than /docs/.

This distinction matters when you are using a case insensitive file system, where this is possible ...

$ ls -la resource-base/
total 12
drwxrwxr-x  3 joakim joakim 4096 Sep 21 15:29 ./
drwxrwxr-x 21 joakim joakim 4096 Sep 21 15:29 ../
drwxrwxr-x  2 joakim joakim 4096 Sep 21 15:29 docs/
-rw-rw-r--  1 joakim joakim    0 Sep 21 15:29 DOCS

This would mean requesting /docs would return the file.
But requesting /docs/ would be handled by the directory.

The setting HttpConfiguration.setRelativeRedirectAllowed(true) currently only impacts the use of HttpServletResponse.sendRedirect().

https://github.com/eclipse/jetty.project/blob/f12c5b668cb16816db126a4674ad4b1ee79e3421/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java#L563-L591

Which the ResourceService.sendWelcome() uses, but it also uses the HttpServletResponse.encodeURL(String) (which is recommended per spec)

https://github.com/eclipse/jetty.project/blob/f12c5b668cb16816db126a4674ad4b1ee79e3421/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java#L407

Perhaps the HttpConfiguration.setRelativeRedirectAllowed(true) configuration should also impact HttpServletResponse.encodeURL(String) behavior, so it can change behavior wholesale, even for places like ResourceService that uses the HttpServletResponse properly?
@gregw WDYT?

I'm a bit leery of changing the behavior of HttpServletResponse.encodeURL(String) as that's essentially breaks that API and its contract.
Other 3rd party libraries that depend on this behavior will have a conniption if this can be made to not work per the API contract.
Also, what other things will we break if we allow that? (eg: the jessionid url behavior that seems to be in encodeURL)

[2is10]

@joakime The main reason for the code that adds a trailing slash is for relative URLs in a welcome page to resolve properly in the browser. For example, if a docs/index.html page contains <a href="dogs.html">, the trailing slash is needed in the page’s URL for that link to correctly resolve to /docs/dogs.html instead of just /dogs.html.

The simplest solution for this specific redirect, I think, would be to build the destination URI starting from request.getRequestURI() (just the path) instead of request.getRequestURL().

ResourceService.java:392 could change from:

StringBuffer buf = request.getRequestURL();

to:

StringBuilder buf = new StringBuilder(request.getRequestURI());

In general-purpose code like Response.encodeURL or Response.sendRedirect, it’s probably more conservative to resolve a relative URL to an absolute URL if necessary than to relativize a provided absolute URL by removing part of it.

[gregw]

@joakime the issue is not so much with encodeURL or sendRedirect, but as @2is10 says, it is that the ResourceService starts with getRequestURL() rather than getRequestURI().
So I think this is fixable at low risk. I'm just pondering if we should use the URI only if isRelativeRedirectAllowed is true or all the time. I'm tending to think the former for 9.4 to minimize behaviour changes and the later for jetty-10 and beyond. fiddling with it now.... (also nice to get rid of the StringBuffer usage and update to StringBuilder).

[gregw]

The ResourceService does sendRedirect in 3 places, and 2 of them use relative URIs, including when we strip an unnecessary trailing /, so I think it is OK to make it always use relative for the addition of a required /. PR coming... stand by....

[gregw]

PR #6883 created. Can we please review/test ASAP as we are trying to catch the current release train!

[2is10]

Thanks @joakime and @gregw for the timely responses and quick fix!