jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.85k stars 1.91k forks source link

Jetty12: HttpConfiguration#_relativeRedirectAllowed flipped to true per default #11947

Open davido opened 3 months ago

davido commented 3 months ago

Jetty Version

jetty-12

Jetty Environment

ee10

Java Version

Java 21

Question

During migration from Jetty 11 to Jetty 12 (ee10) we started to see some test failures.

Particularly, the Location header wasn't set to absolute URL after the redirect. Particularly, the if statement wasn't true any more after migration to Jetty 12:

jetty-server/src/main/java/org/eclipse/jetty/server/Response.java

  // if relative redirects are not allowed?
  if (!request.getConnectionMetaData().getHttpConfiguration().isRelativeRedirectAllowed())
  {
    // make the location an absolute URI
    location = URIUtil.newURI(uri.getScheme(), Request.getServerName(request), Request.getServerPort(request), location, null);
  }

We've investigated the reason for this test failure and concluded, that there was an undocumented behavioural change in Jetty 12 in HttpConfiguration class in context of this issue #11014 and PR #11019.

To fix the behaviour in our case we have to unflip this option, by setting (see [1] for the whole change):

  private HttpConfiguration defaultConfig(int requestHeaderSize) {
    HttpConfiguration config = new HttpConfiguration();
    config.setRequestHeaderSize(requestHeaderSize);
    config.setRelativeRedirectAllowed(false);
    config.setSendServerVersion(false);
    config.setSendDateHeader(true);
    return config;
  }

Question 1: Can this behavioural change be better documented?

Question 2: Why the default jetty.xml still has default="false"?

jetty-server/src/main/config/etc/jetty.xml

[...]
<Set name="relativeRedirectAllowed"><Property name="jetty.httpConfig.relativeRedirectAllowed" default="false"/></Set>
[...]

[1] https://gerrit-review.googlesource.com/c/gerrit/+/424580

joakime commented 3 months ago

The change in Jetty 12.0.x was done in PR #11019 as a reaction to Issue #11014

joakime commented 3 months ago

@gregw do you remember why you flipped this to true in only the embedded mode and not in the XML or ini ?

jglick commented 2 months ago

This also caused test failures during an attempt to migrate CloudBees CI (based on Jenkins) to Jetty 12. The new behavior seems fine and the fix was straightforward, but it seems worth highlighting as a significant change; https://github.com/jetty/jetty.project/releases/tag/jetty-12.0.5 notes only

RedirectRegexRule and RewritePatternRule should consider relativeRedirectAllowed

which would not have caught my eye. I found this via git blame.