Describe the bug My project is using Spring 5.1.13.RELEASE. It doesn't use Spring Boot. When I upgrade the Spring Security of the project to the latest version 5.8.4 along with Spring 5.3.28 I have an issue with the CSRF filter of type OncePerRequestFilter for ASYNC request. This filter doesn't support to override the methods shouldNotFilterAsyncDispatch() and shouldNotFilterErrorDispatch(). Also, it's marked as final and I can't override its behavior.
The same for other filters which inherit from OncePerRequestFilter such as org.springframework.security.web.header.HeaderWriterFilter
Expected behavior CsrfFilter should not be marked as final or it should introduce an attribute in the request (like current implementation of org.springframework.security.web.csrf.CsrfFilter#skipRequest() method) so that we can determine if we should let this filter skip this async request or not
Comment From: sjohnr
@phanvuliem, thanks for reaching out!
skip this async request
Can you please explain your use case in more detail or give an example?
Comment From: phanvuliem
My use case is that I create a custom filter which will dispatch the incoming request to ASYNC context if the criteria matches. This filter is put before the Spring security filters in the web.xml context like this
<filter>
<filter-name>myCustomFilter</filter-name>
<filter-class>org.springframework.web.filter.DelegatingFilterProxy</filter-class>
<async-supported>true</async-supported>
</filter>
<filter-mapping>
<filter-name>myCustomFilter</filter-name>
<servlet-name>my-servlet</servlet-name>
</filter-mapping>
<filter>
<filter-name>springSecurityFilterChain</filter-name>
<filter-class>org.springframework.web.filter.DelegatingFilterProxy</filter-class>
<async-supported>true</async-supported>
</filter>
<filter-mapping>
<filter-name>springSecurityFilterChain</filter-name>
<servlet-name>my-servlet</servlet-name>
<dispatcher>REQUEST</dispatcher>
<dispatcher>ASYNC</dispatcher>
<dispatcher>ERROR</dispatcher>
</filter-mapping>
Before I upgrade the version, the OncePerRequestFilter filter doesn't have this check
skipDispatch(httpRequest)
if (skipDispatch(httpRequest) || shouldNotFilter(httpRequest)) {
// Proceed without invoking this filter...
filterChain.doFilter(request, response);
}
else if (hasAlreadyFilteredAttribute) {
// original code from Spring 5.1.13.RELEASE
}
with the new skipDispatch() as below method from OncePerRequestFilter and the close for inheritance from CsrfFilter as current I can't modify the behavior to let the filter execute on my ASYNC context.
private boolean skipDispatch(HttpServletRequest request) {
if (isAsyncDispatch(request) && shouldNotFilterAsyncDispatch()) {
return true;
}
if (request.getAttribute(WebUtils.ERROR_REQUEST_URI_ATTRIBUTE) != null && shouldNotFilterErrorDispatch()) {
return true;
}
return false;
}
This leads to the situation that spring security does not support some security features on an async dispatch. The current implementation implies that we need to go through all the security filters first before dispatching the request in ASYNC context. And this is not my case. I want the security filters to be affected in the ASYNC context for the request. In this case it's the CsrfFilter and HeaderWriterFilter.
Here is the snippet from my code for my filter
// filter main method
public void doFilter(final ServletRequest arg0, final ServletResponse arg1, final FilterChain chain)
throws IOException, ServletException {
final HttpServletRequest httpRequest = (HttpServletRequest) arg0;
final HttpServletResponse httpResponse = (HttpServletResponse) arg1;
if (shouldSkip()) {
chain.doFilter(arg0, arg1);
return;
}
final MyTask wrTask = new MyTask(httpRequest, httpResponse);
httpRequest.startAsync();
threadPoolTaskExecutor.submit(wrTask);
}
// the thread which dispatch the request in ASYNC context
private class MyTask implements Callable<MyResult> {
private final HttpServletRequest httpRequest;
private final HttpServletResponse httpResponse;
MyTask(final HttpServletRequest httpRequest, final HttpServletResponse httpResponse) {
super();
this.httpRequest = httpRequest;
this.httpResponse = httpResponse;
}
@Override
public MyResult call() throws Exception {
if (httpRequest.isAsyncStarted()) {
httpRequest.getAsyncContext().dispatch();
}
return MyResult.buildResult(httpRequest);
}
}
Comment From: phanvuliem
Just received the feedback from spring-web that they won't adapt the flexibility of the OncePerRequestFilter: https://github.com/spring-projects/spring-framework/issues/30881. how should we do in this case?
Comment From: sjohnr
My use case is that I create a custom filter which will dispatch the incoming request to ASYNC context if the criteria matches.
Sorry @phanvuliem, but that did not clear it up for me. I was looking for some description of the task(s) you're performing asynchronously so I could visualize your application better.
Can you remind me of why you can't set up ignoringRequestMatchers(...) rules on http.csrf() or call CsrfFilter.skipRequest(request) in your filter?
Comment From: phanvuliem
Hi @sjohnr ,
At first, I want CSRF filter handle my request. The reason why I can't set up ignoringRequestMatchers(...) rules on http.csrf() or call CsrfFilter.skipRequest(request) in my filter is because the request I'm handling is dispatched by ASYNC dispatcher, not REQUEST. In OncePerRequest it filters out this request before letting doFilterInternal of CsrfFilter does its job.
Currently I have to move my filter after the Spring Security filters so that CSRF filter can work but this work around also leads to a huge change in the my filter implementation so a support from CsrfFilter to allow me to override the behavior of shouldNotFilterAsyncDispatch() and shouldNotFilterErrorDispatch() is more than welcome.
Comment From: sjohnr
Thanks for the additional info, @phanvuliem.
The fact that your workaround is to move your filter behind the Spring Security filter chain suggests that your goal is somewhat counter to the design of Spring Security from its inception (though perhaps I'm wrong, since I have not been on the project for most of that time). From my point of view, your filter is part of your application, which should sit behind the Spring Security filter chain. Otherwise, some of what Spring Security does might eventually leak into your filter, or worse you might find you're just not protected by some aspect of its protection.
To change Spring Security in the way you're suggesting may have larger impacts, including setting a precedent toward bending the design in a direction it wasn't intended to go. Again, I could be wrong here and would need to zoom out and look wider at the picture to determine that. Either way, I certainly agree with the Framework team that a change like this wouldn't make it easier to understand.
I'm going to leave this open for now so we can continue discussing, but I'm not yet convinced the suggested change is the right path forward. I currently feel that moving your filter after the Spring Security filter chain is a much stronger solution.
Comment From: sjohnr
Since there doesn't seem to be anything else to discuss at the moment, I'm going to close this. If you disagree with my reasoning, please provide a minimal sample of an application that can be used to demonstrate the need for this enhancement, and we can re-open if needed.
Comment From: phanvuliem
I don't have any better idea to deny. Agree with your proposal @sjohnr
Comment From: phanvuliem
Hi again, sorry for my late response. I have no further point to deny your argument so I admit the current situation. Will come back to it in case I find a stronger reason for it.