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

ContextHandler sends redirect on BaseResponse instead of Wrapped Response object from Handler chain #9285

Closed
venkatvb opened this issue Feb 1, 2023 · 2 comments · Fixed by #9286
Assignees
Labels
Bug For general bugs on Jetty side merge-12-needed

Comments

@venkatvb
Copy link

venkatvb commented Feb 1, 2023

Jetty version(s)
11.0.13

Java version/vendor (use: java -version)
openjdk version "1.8.0_352"

OS type/version
MacOS 12.6.1

Description

Issue: ContextHandler sends redirect on BaseResponse instead of Wrapped Response object from Handler chain. This doesn't preserve any wrapping done by up stream handlers & renders the overridden functionality useless.

Use case: In my case, I have hosted embedded Jetty server behind a reverse proxy. All of the requests would have a proxy-prefix. In order to handle the redirects, I've implemented a ProxyRedirecteHandler to re-wire the proxy url location. But as the ContextHandler sends redirect to the base response instead of wrapped response object, I'm not getting my overridden sendRedirect method invoked.

The ContextHandler expects the request must end with /, otherwise it sends a redirect (302) to the same request URI but with a trailing /. Code reference.

// context request must end with /
baseRequest.setHandled(true);
String queryString = baseRequest.getQueryString();
baseRequest.getResponse().sendRedirect(
    HttpServletResponse.SC_MOVED_TEMPORARILY,
    baseRequest.getRequestURI() + (queryString == null ? "/" : ("/?" + queryString)),
    true);

How to reproduce?
I'm created a minimal repo to reproduce the issue: https://github.com/venkatvb/jetty-proxy-redirect-issue-repro

Can run the org.jettyissue.ProxyRedirectTest behind a simple reverse proxy server.

Behind a proxy (base-path: shs, port: 3000)

venkaar@147ddae4c87f spark % curl -IL "http://localhost:3000/shs/demo"
HTTP/1.1 302 Found
X-Powered-By: Express
date: Wed, 01 Feb 2023 23:34:55 GMT
connection: close
location: http://localhost:3000/demo/ --> Notice the redirect location doesn't have proxy-base - as it didn't went through the ProxyRedirectHandler
server: Jetty(11.0.13)

HTTP/1.1 404 Not Found
X-Powered-By: Express
Content-Security-Policy: default-src 'none'
X-Content-Type-Options: nosniff
Content-Type: text/html; charset=utf-8
Content-Length: 145
Date: Wed, 01 Feb 2023 23:34:59 GMT
Connection: keep-alive

Without a proxy (port: 5002)

venkaar@147ddae4c87f spark % curl -IL "http://localhost:5002/demo"

HTTP/1.1 302 Found
Date: Wed, 01 Feb 2023 23:37:12 GMT
Location: http://localhost:5002/demo/
Content-Length: 0
Server: Jetty(11.0.13)

HTTP/1.1 200 OK
Date: Wed, 01 Feb 2023 23:37:22 GMT
Content-Type: text/html
Content-Length: 39
Server: Jetty(11.0.13)
@venkatvb venkatvb added the Bug For general bugs on Jetty side label Feb 1, 2023
@gregw gregw self-assigned this Feb 2, 2023
@gregw
Copy link
Contributor

gregw commented Feb 2, 2023

We need to be concerned about breaking #5605 fixing this.

gregw added a commit that referenced this issue Feb 2, 2023
Use the servlet response sendRedirect method.
Always close the connection if there is content.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@venkatvb
Copy link
Author

venkatvb commented Feb 2, 2023

Thank much @gregw for looking into the issue and raising a pull request to fix!

gregw added a commit that referenced this issue Feb 4, 2023
Use the servlet response sendRedirect method.
Always close the connection if there is content.

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this issue Feb 6, 2023
…x-documentation-operations-logging

* upstream/jetty-12.0.x: (42 commits)
  fixed style
  cleanup TODOs for decoration
  Issue jetty#9300 - Rename RetainableByteBufferPool to ByteBufferPool
  Removed TODOs that will not be done.
  Rename Handler Nested & Collection (jetty#9305)
  fix surefire jpms configuration
  fix merge
  fix merge
  Bump maven.surefire.plugin.version from 3.0.0-M5 to 3.0.0-M8 (jetty#9255)
  Rename RetainableByteBufferPool to ByteBufferPool
  Fixed merge
  Fix jetty#9285 use possibly wrapper response for redirection (jetty#9286)
  Issue jetty#9293 - Jetty 12 - Relax JPMS dependencies (quic) (jetty#9307)
  Issue jetty#9293 - Jetty 12 - Relax JPMS dependencies (fcgi) (jetty#9306)
  Jetty 10 - Configurable Unsafe Host Header (jetty#9283)
  Issue jetty#9293 - Jetty 12 - Relax JPMS dependencies. (jetty#9296)
  Issue jetty#9293 - Jetty 12 - Relax JPMS dependencies. (jetty#9299)
  fix dependency
  add used dependency
  this dependency is used in test scope
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side merge-12-needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants