Duplicate headers when security filter is invoked for async dispatches

[wilkinsona]

Summary

When the security filter is configured with REQUEST and ASYNC dispatcher types several headers that are set by Spring Security are duplicated. This is similar to #4199, although it affects more than just the headers related to caching.

Actual Behavior

If request handling starts an AsyncContext and then calls dispatch a number of headers will be duplicated:

http --auth user:password localhost:8080/async
HTTP/1.1 200
Cache-Control: no-cache, no-store, max-age=0, must-revalidate
Cache-Control: no-cache, no-store, max-age=0, must-revalidate
Content-Length: 7
Content-Type: text/plain;charset=UTF-8
Date: Tue, 14 Feb 2017 14:19:49 GMT
Expires: 0
Expires: 0
Pragma: no-cache
Pragma: no-cache
Strict-Transport-Security: max-age=31536000 ; includeSubDomains
X-Content-Type-Options: nosniff
X-Content-Type-Options: nosniff
X-Frame-Options: DENY
X-Frame-Options: DENY
X-XSS-Protection: 1; mode=block

request

Expected Behavior

No header duplication occurs

Configuration

This can be reproduced using Spring Boot 1.5.1.RELEASE with its default security configuration.

Version

4.2.1.RELEASE.

The problem also occurs with 4.1.4.RELEASE (Spring Boot 1.4.4.RELEASE) although the duplication is not as bad:

http --auth user:secret localhost:8080/async
HTTP/1.1 200
Cache-Control: no-cache, no-store, max-age=0, must-revalidate
Content-Length: 7
Content-Type: text/plain;charset=UTF-8
Date: Tue, 14 Feb 2017 14:33:54 GMT
Expires: 0
Pragma: no-cache
Strict-Transport-Security: max-age=31536000 ; includeSubDomains
X-Content-Type-Options: nosniff
X-Content-Type-Options: nosniff
X-Frame-Options: DENY
X-Frame-Options: DENY
X-XSS-Protection: 1; mode=block

Sample

https://github.com/wilkinsona/duplicate-security-headers

[rwinch]

@wilkinsona Thanks for reporting this and proving a sample!

Spring Security's HeaderWriterFilter (which writes the headers) extends Spring Framework's OncePerRequestFilter and returns the default of true for shouldNotFilterAsyncDispatch. As far as I understand it this would mean HeaderWriterFilter should not be invoked again (but it is).

I've created a very simple example that removes Spring Security from the equation and demonstrates that OncePerRequestFilter is executed twice in https://github.com/rwinch/duplicate-security-headers/tree/no-security

@rstoyanchev Do you have any insights as to what Spring Security is doing wrong, if this is a bug in OncePerRequestFilter, etc?

@wilkinsona While the root cause appears something a bit more complex, I think we can help mitigate this issue by changing some/most of the security HeaderWriter implementations to set a header instead of appending a header. I will need to give this a bit of thought over the night.

[rwinch]

I removed the 4.2.2 label because this currently appears to be an issue with OncePerRequestFilter

[wilkinsona]

The problem appears to be that WebAsyncManager.hasConcurrentResult() is returning false so the filter is failing to identify that it's an async dispatch.

[wilkinsona]

A workaround, that requires Servlet 3.0, is to override isAsyncDispatch and query the request's dispatcher type:

@Override
protected boolean isAsyncDispatch(HttpServletRequest request) {
    return request.getDispatcherType() == DispatcherType.ASYNC;
}

[rwinch]

@wilkinsona Thanks for the additional details. I agree, but think this is likely a change for OncePerRequestFilter.

@rstoyanchev what are your thoughts? Should I create a JIRA?

[rstoyanchev]

The implicit expectation is that in a Spring application the Servlet 3 async support is consumed through Spring's abstraction, i.e. the DeferredResult or Callable controller method return values. It can also be used at a lower level through the AsyncWebManager. Out of curiosity what's using Serlvet async API directly?

Regarding the isAsyncDispatch check I'm guessing at the time it was a convenient way to avoid a hard dependency on Servlet 3 which is no longer a concern. So yes please create an SPR ticket and I'll take it from there.

[wilkinsona]

Out of curiosity what's using Serlvet async API directly?

IIRC, it was just me writing a quick and dirty app to test some changes in Boot 2.0 to the default filter dispatcher types. Sorry, I wasn't aware that using the Servlet API directly was something of a no-no.

[rstoyanchev]

Okay no worries, it's something we should update anyway.

The API that underlies DeferredResult is relatively straight-forward and provides a slightly higher level, and more focused abstraction. There is an overview here and you can trace some usages. Hopefully it gives enough reason to prefer it :) .. It's used for a variety of cases besides just returning DeferredResult like SSE support and also Spring Cloud uses it for RxJava support.

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[wilkinsona]

I'm not sure what feedback is required here. @rwinch, did you create a Framework issue? isAsyncDispatch() is still checking hasConcurrentResult() and my reading of the above is that @rstoyanchev was open to changing it to check the request's dispatcher type now that a Servlet 3 dependency is OK.

[rwinch]

Thanks for the nudge @wilkinsona. I'm going to close this in favor of spring-projects/spring-framework#26282