Security-related HTTP headers not written if response is committed during INCLUDE dispatch and RequestDispatcher is not obtained via HttpServletRequest object

[mjagus]

Summary

In #5499 a bug was fixed which caused security-related headers to not appear in HTTP response if that response was commited during INCLUDE dispatch. The solution was to wrap both HttpServletRequest and RequestDispatcher object in order to intercept RequestDispatcher::include call. That fix doesn't work if RequestDispatcher is obtained via some other means than HttpServletRequest::getRequestDispatcher method.

In my particular case it's the SiteMesh 2.x servlet filter that obtains dispatcher via ServletContext::getRequestDispatcher and then calls include method on it. The returned dispatcher is unfortunately not a wrapper created by HeaderWriterFilter which means that no security headers are added to HTTP response.

Actual Behavior

Assuming that security-related headers are enabled in spring-security:

1. HeaderWriterFilter is invoked and both request and response objects are wrapped.
2. After that some 3rd party library calls RequestDispatcher::include on dispatcher obtained via some other means than HttpServletRequest::getRequestDispatcher method.
3. Response is flushed during that INCLUDE dispatch call.

This results in security-related headers not being added to HTTP response.

Expected Behavior

The way of obtaining RequestDispatcher object should not be a factor when adding security-related headers to response.

Configuration

Tomcat 8.5.34
SiteMesh 2.4.2
Spring-based web application which uses JSPs and decorates them using SiteMesh.

Version

5.1.2

Sample

spring-security-gh6414.zip

Attached is a Zip file containing a simple Spring Boot application. Simply run the de.chschu.spring.security.gh6414.Application main class.

The response of http://localhost:8080/positive will have the headers, while http://localhost:8080/negative (which simply includes the other one) will not.

[jzheaux]

Thanks for the report, @mjagus! Would you be able to post a simple example that reproduces the issue?

[mjagus]

@jzheaux I added sample application to the main post. It's a modified sample application from #5499 that obtains RequestDispatcher from ServletContext object instead of HttpServletRequest.

[jzheaux]

@mjagus Awesome, thanks.

I'll do a bit more digging, but a brief look at the Servlet Spec doesn't point to a straight-forward way to intercept ServletContext's request dispatcher facilities. Do you have any recommendations?

[jzheaux]

@mjagus Looking at the SiteMesh code, I see that it uses the servlet context for forwards. Does SiteMesh use it for includes as well somewhere?

[mjagus]

@jzheaux It does use includes as well. For example here: https://github.com/sitemesh/sitemesh2/blob/master/src/java/com/opensymphony/sitemesh/compatability/OldDecorator2NewDecorator.java
This is actually the code that causes the problem in our app.

As for suggestions, at first I thought that HeaderWriterFilter should be changed to apply headers immediately just before passing control to another filter in the chain but after some digging I found out that it used to work that way and was changed because of #2953.
Currently I'm out of ideas and I feel it's impossible to fix easily with the current approach of wrapping request and response objects. Maybe HeaderWriterFilter needs to be reverted to pre-#2953 state and CacheControlHeadersWriter needs to be treated differently from the rest of writers? Just throwing ideas out there.

[jzheaux]

So, treating CacheControlHeadersWriter differently (like writing it late while the others are written early) would still prevent you from getting cache control headers in a ServletContext#include. How important is that in your situation?

The code could possibly expose a boolean like writeHeadersEagerly. This would mean you could get all headers, trading that for the limitations reported in #2953.

How well would something like this work for your usecase:

List<HeaderWriter> earlyWriters = ...;
HeaderWriterFilter earlyFilter = new HeaderWriterFilter(earlyWriters);
earlyFilter.setWriteHeadersEagerly(true);

http
    .addFilterBefore(earlyFilter, HeaderWriterFilter.class)
    // .headers() added by default

Where earlyWriters is a list of header writers that you need written early in the request (which might exclude the cache control writer, for example). These could hypothetically be wrapped in a DelegatingRequestMatcherHeaderWriter so that they'd only be written early if the request matches some criteria.

Or, another application of that might be:

http
    .addObjectPostProcessor(new ObjectPostProcessor<HeaderWriterFilter>() {
        public HeaderWriterFilter postProcess(HeaderWriterFilter filter) {
            filter.setWriteHeadersEagerly(true);
            return filter;
        }
    }

Again with the understanding that this would mean cache control is written early.

Agreed that we can only take the wrapping approach so far, especially since servlet context isn't request-scoped, making it impossible to manage inside a filter chain. You could potentially wrap it yourself and subclass WebAppContext in SiteMesh, though that's just me digging a bit through that code.

[jzheaux]

@mjagus I've written up a ticket for this #6501. Would you be interested in submitting a PR?

[mjagus]

Hi @jzheaux !

Making such thing configurable seems like a great idea, but I think it would be way better to configure it on a HeaderWriter level instead. Our application uses a mix of JSP views and REST endpoints which may need to be cached. So potentially we would still need to add Cache-Control related headers using deferred approach (just so we can avoid #2953) while applying all the other headers eagerly.

I imagine example Java Config would look something like this (assuming deferred approach would be the default one in order to maintain backward compatibility):

http.headers()
    .frameOptions().eager(true).and()
    .xssProtection().eager(true).and()
    .contentTypeOptions().eager(true).and()
    .cacheControl();

and similar XML config:

<sec:headers>
    <sec:frame-options eager="true" />
    <sec:xss-protection eager="true />
    <sec:content-type-options eager="true" />
    <sec:cache-control />
</sec:headers>

I see that PR is already made but maybe there's still some time for discussion? Please tell me what you think.

As for potential workarounds, we are currently instrumenting SiteMesh clasess using AspectJ to redirect include calls to HttpServletRequest object instead of ServletContext just so we can trigger Spring Security logic. This solution is a bit dirty but I'm hoping we can remove it later when the eagerness of applying security headers becomes configurable.