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

No way to affect response when unhandled exceptions thrown from ForwardedHeaderTransformer #26459

Closed
kkroner8451 opened this issue Jan 27, 2021 · 2 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Milestone

Comments

@kkroner8451
Copy link

Affects: \spring-web:5.2.12


We are running a service behind a reverse proxy and have set server.forward-headers-strategy=framework. This then registers a ForwardedHeaderTransformer bean. When an exception is thrown from that class it bubbles up the stack and the connection is eventually closed without modifying/sending a response at all. The net result is that the reverse proxy never gets a response and will treat it as a 502 Bad Gateway which is less than ideal.

The exception we are seeing is being thrown in the ForwardedHeaderTransformer.apply(ServerHttpRequest request) method when the UriCompenentsBuilder.build(true) is called. Ultimately, the URI was not properly encoded which is how we discovered the issue. We are fixing our end to better ensure it is encoded correctly when we build the URI to have the user follow, but obviously we can't prevent users from sending bad request in the first place. Also, any unhandled exception that could go wrong in the ForwardedHeaderTransformer or a developer's custom implementation would still result in the connection being closed without an opportunity to write a response. Looking at how this is called from HttpWebHandlerAdapter.handle(ServerHttpRequest request, ServerHttpResponse response), it seems the expectation is that nothing could ever go wrong in applying the transformer.

The preference would be that we could handle the exception, log it and then return a proper response code with a message indicating that the URI was malformed. I would think this could be achieved by throwing an exception such as ResponseStatusException, or a custom exception with the @ResponseStatus annotation present. This was attempted with no affect. Again, with how the exception handling is the only option is the exception is bubbled up until eventually the connection/channel is closed without modifying the response in HttpServerHandle.onStateChanged(Connection connection, State newState). Alternatively, the ServerHttpResponse could be exposed to the transformer but I could see that not being a great idea from a design perspective.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 27, 2021
@rstoyanchev rstoyanchev self-assigned this Jan 27, 2021
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 27, 2021
@rstoyanchev rstoyanchev added this to the 5.3.4 milestone Jan 27, 2021
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 27, 2021

ForwardedHeaderTransformer is applied ahead of the WebFilter and WebExceptionHandler chain. This is intentional so that the entire chain will see a consistent request but it also means that a ResponseStatusException will not be handled as usual. We can improve this by having the transformer catch and raise a ServerWebInputException (perhaps through a protected method) which would then be handled in HttpWebHandlerAdapter by applying the status code to the response. Does that seem okay?

@kkroner8451
Copy link
Author

That seems like it should work.

@rstoyanchev rstoyanchev added the for: backport-to-5.2.x Marks an issue as a candidate for backport to 5.2.x label Jan 28, 2021
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.2.x Marks an issue as a candidate for backport to 5.2.x labels Jan 28, 2021
rstoyanchev added a commit that referenced this issue Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants