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

RelativeRedirectResponseWrapper does not commit response in sendRedirect #29050

Closed

Conversation

yuezk
Copy link
Contributor

@yuezk yuezk commented Aug 31, 2022

According to the Servlet spec 5.5 Convenience Methods, sendRedirect() should commit the response.

These methods will have the side effect of committing the response, if it has not
already been committed, and terminating it. No further output to the client should
be made by the servlet after these methods are called. If data is written to the
response after these methods are called, the data is ignored.

Currently, it could cause a problem where the server couldn't redirect the user to the URL passed in. Though it might be rare.

We encountered the problem aforementioned. We configured the ForwardedHeaderFilter with relativeRedirect set to true, inside the ForwardedHeaderFilter, it will use RelativeRedirectResponseWrapper to wrap the response. I our application, we called sendRedirect() to redirect to the home page when the user logged in. But we found that the server responds with a 404 status code and the Location response header.

We found that the authentication filter didn't stop the filter chain when logged in, it continues, and the endpoint for handling the login process doesn't actually exist as we are using the filter to intercept the matched login request and perform the login process. So, in the later process, it couldn't find the handler and set the status code to 404. Thus this problem.

image

We called sendRedirect() in successHandler.onAuthenticationSuccess(request, response, authentication), and the filter continues after it.

The filter we are using is SpnegoAuthenticationProcessingFilter from https://github.com/spring-projects/spring-security-kerberos/blob/0c9be90a7480edd48276a3703258258beeef59ff/spring-security-kerberos-web/src/main/java/org/springframework/security/kerberos/web/authentication/SpnegoAuthenticationProcessingFilter.java#L169-L175

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 31, 2022
@bclozel
Copy link
Member

bclozel commented Aug 31, 2022

Could we take a step back here?
Could you share a sample application that shows the incorrect behavior? Ideally, a Spring Boot application created on https://start.spring.io with the ForwardedHeaderFilter and a custom filter (not the authentication one) that reproduces the behavior you're seeing.

Even if we were to flush the buffer where it's suggested, it would not prevent some code to further write to the response, still leading to an incorrect HTTP message (a redirect+location response with invalid HTML content?).

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Aug 31, 2022
@yuezk yuezk force-pushed the fix_relative_redirect_filter branch from b5482c0 to 78d3236 Compare August 31, 2022 16:03
@yuezk
Copy link
Contributor Author

yuezk commented Aug 31, 2022

I will try to provide minimal reproducible code.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 31, 2022
@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Aug 31, 2022
@yuezk
Copy link
Contributor Author

yuezk commented Sep 1, 2022

@bclozel Here is the demo repo: https://github.com/yuezk/RelativeRedirectFilter-bug. It uses RelativeRedirectFilter instead of the ForwardedHeaderFilter, but the problem is the same.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 1, 2022
@yuezk
Copy link
Contributor Author

yuezk commented Sep 1, 2022

Even if we were to flush the buffer where it's suggested, it would not prevent some code to further write to the response, still leading to an incorrect HTTP message (a redirect+location response with invalid HTML content?).

We also need to call resetBuffer() first to make sure the redirect response won't contain the body. Below are the examples from the popular Java EE implementations

@yuezk yuezk force-pushed the fix_relative_redirect_filter branch from e4f3ccc to 4de3c2c Compare September 1, 2022 10:06
@yuezk yuezk force-pushed the fix_relative_redirect_filter branch from 4de3c2c to 2898235 Compare September 1, 2022 10:42
@yuezk yuezk changed the title fix: commit the response in sendRedirect() method Commit the response in sendRedirect() method Sep 2, 2022
@bclozel bclozel self-assigned this Sep 7, 2022
@bclozel bclozel added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Sep 7, 2022
@bclozel bclozel changed the title Commit the response in sendRedirect() method RelativeRedirectResponseWrapper does not commit response in sendRedirect Sep 7, 2022
@bclozel bclozel added this to the 5.3.23 milestone Sep 7, 2022
bclozel added a commit that referenced this pull request Sep 7, 2022
@bclozel bclozel closed this in 298c9a6 Sep 7, 2022
@bclozel
Copy link
Member

bclozel commented Sep 7, 2022

Thanks @yuezk , this will be shipped in the next maintenance release.

@yuezk yuezk deleted the fix_relative_redirect_filter branch September 8, 2022 01:30
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) type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants