Skip to content

ExceptionTranslationFilter should not try to handle when response is committed #5273

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.

Sign up for GitHub

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

Jump to bottom
Closed
philwebb opened this issue Apr 30, 2018 · 11 comments
Closed

ExceptionTranslationFilter should not try to handle when response is committed #5273

philwebb opened this issue Apr 30, 2018 · 11 comments
Assignees
@rwinch
Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Milestone
5.0.5

Comments

@philwebb
Copy link
Member

Originally raised in spring-projects/spring-boot#12995.

If you try use GlobalMethodSecurity on Jersey resources you will face exception below:

java.lang.IllegalStateException: Cannot call sendError() after the response has been committed
	at org.apache.catalina.connector.ResponseFacade.sendError(ResponseFacade.java:456) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at javax.servlet.http.HttpServletResponseWrapper.sendError(HttpServletResponseWrapper.java:120) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.springframework.session.web.http.OnCommittedResponseWrapper.sendError(OnCommittedResponseWrapper.java:117) ~[spring-session-core-2.0.2.RELEASE.jar:2.0.2.RELEASE]
	at javax.servlet.http.HttpServletResponseWrapper.sendError(HttpServletResponseWrapper.java:120) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at javax.servlet.http.HttpServletResponseWrapper.sendError(HttpServletResponseWrapper.java:120) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.springframework.security.web.util.OnCommittedResponseWrapper.sendError(OnCommittedResponseWrapper.java:119) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at javax.servlet.http.HttpServletResponseWrapper.sendError(HttpServletResponseWrapper.java:120) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.springframework.security.web.util.OnCommittedResponseWrapper.sendError(OnCommittedResponseWrapper.java:119) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.authentication.www.BasicAuthenticationEntryPoint.commence(BasicAuthenticationEntryPoint.java:61) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.authentication.DelegatingAuthenticationEntryPoint.commence(DelegatingAuthenticationEntryPoint.java:95) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.access.ExceptionTranslationFilter.sendStartAuthentication(ExceptionTranslationFilter.java:210) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.access.ExceptionTranslationFilter.handleSpringSecurityException(ExceptionTranslationFilter.java:182) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.access.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:138) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.session.SessionManagementFilter.doFilter(SessionManagementFilter.java:137) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.authentication.AnonymousAuthenticationFilter.doFilter(AnonymousAuthenticationFilter.java:111) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter.doFilter(SecurityContextHolderAwareRequestFilter.java:170) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.savedrequest.RequestCacheAwareFilter.doFilter(RequestCacheAwareFilter.java:63) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.authentication.www.BasicAuthenticationFilter.doFilterInternal(BasicAuthenticationFilter.java:158) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:116) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.csrf.CsrfFilter.doFilterInternal(CsrfFilter.java:100) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.header.HeaderWriterFilter.doFilterInternal(HeaderWriterFilter.java:66) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.context.SecurityContextPersistenceFilter.doFilter(SecurityContextPersistenceFilter.java:105) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.context.request.async.WebAsyncManagerIntegrationFilter.doFilterInternal(WebAsyncManagerIntegrationFilter.java:56) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:334) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:215) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.security.web.FilterChainProxy.doFilter(FilterChainProxy.java:178) ~[spring-security-web-5.0.4.RELEASE.jar:5.0.4.RELEASE]
	at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:357) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:270) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:99) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.springframework.web.filter.HttpPutFormContentFilter.doFilterInternal(HttpPutFormContentFilter.java:109) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.springframework.web.filter.HiddenHttpMethodFilter.doFilterInternal(HiddenHttpMethodFilter.java:81) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.springframework.session.web.http.SessionRepositoryFilter.doFilterInternal(SessionRepositoryFilter.java:146) ~[spring-session-core-2.0.2.RELEASE.jar:2.0.2.RELEASE]
	at org.springframework.session.web.http.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:81) ~[spring-session-core-2.0.2.RELEASE.jar:2.0.2.RELEASE]
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:200) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107) ~[spring-web-5.0.5.RELEASE.jar:5.0.5.RELEASE]
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:198) ~[tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:496) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:140) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:81) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:87) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:342) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:803) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:66) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:790) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1459) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [na:1.8.0_171]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [na:1.8.0_171]
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61) [tomcat-embed-core-8.5.29.jar:8.5.29]
	at java.lang.Thread.run(Thread.java:748) [na:1.8.0_171]
@philwebb
Copy link
Member Author

There's some analysis by Andy in spring-projects/spring-boot#12995 (comment)

@philwebb philwebb mentioned this issue Apr 30, 2018
Closed
@rwinch
Copy link
Member

rwinch commented Apr 30, 2018

Thanks for the report @andreysaksonov and forwarding it on @philwebb

I'm not sure what we can do to improve the user experience. If the response is already committed BasicAuthenticationEntryPoint cannot prompt the user for their credentials in which case we expect an IllegalStateException

@rwinch rwinch self-assigned this Apr 30, 2018
@rwinch
Copy link
Member

rwinch commented Apr 30, 2018

I'm closing this as works as designed. This is not a Spring Security specific issue. Generally speaking, you cannot have an error in the error handling within the container because it is going to produce an IllegalStateException due to the response being committed. You can work around this by using a try/catch in the error handling.

If you have better ideas on what to do, please let me know. However, I do not see a better way to solve this problem.

@rwinch rwinch closed this as completed Apr 30, 2018
@wilkinsona
Copy link
Member

IMO, letting the IllegalStateException happen is rather nasty. It means the user gets a 500 when they should be getting a 4xx response. Can’t Spring Security be more defensive and check for the response having been committed before it tries to send an error?

@rwinch
Copy link
Member

rwinch commented Apr 30, 2018 •
edited
Loading

@wilkinsona Thanks for the response. To me the user should get a 500 as this is a programming error. What is your suggestion on what to do if the response is committed?

@rwinch rwinch reopened this Apr 30, 2018
@wilkinsona
Copy link
Member

I’d just let the error that’s already been sent and that marked the response as committed make it back to the client. It might not be the ideal response, but I think it’d be better than the current behaviour.

@rwinch
Copy link
Member

rwinch commented Apr 30, 2018 •
edited
Loading

Thanks for your fast response.

I disagree that the suggestion is an improvement. The outlined scenario is a programming error within the application. Specifically, an unauthorized resource was requested and there was no way for the framework to handle the exception because the response was already committed. The developers of the application need to know to fix this and a 500 sent to the client is the appropriate response.

Perhaps an alternative is to have Spring Security re-throw the original AccessDeniedException if the response is already committed. Thoughts?

@wilkinsona
Copy link
Member

a programming error within the application

There’s no code in the application that’s catching exceptions or sending an error. It’s Jersey that’s doing so. And, FWIW, what it is doing doesn’t seem unreasonable to me.

Jersey has caught an exception and, as a result, has sent an error to the client. The exception’s then re-thrown. Spring Security has caught this exception and assumed that it can and should send an error. It does this without checking that something else hasn’t already done so which is where things are going wrong.

Perhaps an alternative is to have Spring Security re-throw the original AccessDeniedException if the response is already committed. Thoughts?

That sounds good to me. If, when it catches an exception, Spring Security determines it can’t do anything because the response is committed, rethrowing the exception seems like a reasonable thing to do as it should make things behave as if Security hadn’t caught the exception in the first place.

Oh and I should have said this above: thanks for reconsidering closing this as working as designed.

@rwinch rwinch added type: bug A general bug in: web An issue in web modules (web, webmvc) and removed Works as Designed labels Apr 30, 2018
@rwinch rwinch added this to the 5.0.5 milestone Apr 30, 2018
@rwinch rwinch changed the title [spring-security+jersey] java.lang.IllegalStateException: Cannot call sendError() after the response has been committed ExceptionTranslationFilter should not try to handle when response is committed Apr 30, 2018
rwinch added a commit that referenced this issue Apr 30, 2018
@rwinch
ExceptionTranslationFilter does not handle committed responses
32f5fb5 
Fixes: gh-5273
@rwinch rwinch closed this as completed in 9bb841a Apr 30, 2018
@rwinch
Copy link
Member

rwinch commented Apr 30, 2018

Thanks for following up again so quickly. I'm glad we found a solution that made sense for both of us. I do think this is an improvement on where things were at, so thanks for prodding me to reconsider this.

I updated Spring Security to avoid trying to handle the exception in the event that the response is committed. The changes are in 5.0.5.BUILD-SNAPSHOT and 5.1.0.BUILD-SNAPSHOT.

@wilkinsona
Copy link
Member

wilkinsona commented May 1, 2018 •
edited
Loading

Unfortunately, the change that has been made doesn't fully address the problem. Trying it with the sample application that was supplied with the Boot issue shows that a 500 is still returned:

$ http :8080/api/test/
HTTP/1.1 500
Cache-Control: no-cache, no-store, max-age=0, must-revalidate
Connection: close
Content-Type: application/json;charset=UTF-8
Date: Tue, 01 May 2018 07:41:24 GMT
Expires: 0
Pragma: no-cache
Transfer-Encoding: chunked
X-Content-Type-Options: nosniff
X-Frame-Options: DENY
X-XSS-Protection: 1; mode=block

{
    "error": "Internal Server Error",
    "message": "Access is denied",
    "path": "/api/test/",
    "status": 500,
    "timestamp": "2018-05-01T07:41:24.689+0000"
}

There is an improvement as the message now reads "Access is denied" so I think the change made in 9bb841a was the right thing to do. To fully address the issue, I've now learned that Jersey can be configured so that it doesn't use sendError and uses setStatus instead. This will allow Security's filters to send an error with the desired 401 response status.

@rwinch
Copy link
Member

rwinch commented May 1, 2018

Thanks for following up with the proposed changes in Boot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rwinch rwinch

Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Projects
None yet
Milestone
5.0.5
Development

No branches or pull requests
3 participants
@rwinch @philwebb @wilkinsona