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

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

Closed
davido opened this issue Jun 23, 2024 · 3 comments
Closed
Assignees
Labels

Comments

@davido
Copy link

davido commented Jun 23, 2024

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
Copy link
Contributor

joakime commented Jun 24, 2024

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

@joakime
Copy link
Contributor

joakime commented Jun 24, 2024

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

@jglick
Copy link
Contributor

jglick commented Jul 25, 2024

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.

gregw added a commit that referenced this issue Nov 20, 2024
Fix #11947 by aligning embedded and XML HttpConfiguration defaults
gregw added a commit that referenced this issue Nov 21, 2024
Fix #11947 by aligning embedded and XML HttpConfiguration defaults
Revert most changes
gregw added a commit that referenced this issue Nov 23, 2024
Fix #11947 by aligning embedded and XML HttpConfiguration defaults
Fixed SIWE
gregw added a commit that referenced this issue Nov 29, 2024
Fix #11947 by aligning embedded and XML HttpConfiguration defaults
@gregw gregw closed this as completed Dec 2, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.1.0 Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

No branches or pull requests

4 participants