Spring-security: Spring Security not writing http headers when the http stream was flushed by a participant of the filterChain.

[jtoselli]

Summary

Spring Security 4.1.0.RELEASE and 4.1.1.RELEASE: Spring Security does not write HTTP response headers in the following scenario.

Actual Behavior

Starting from Spring Security 4.1.0.RELEASE, the method HeaderWriterFilter#doFilterInternal first does the chain through the filterChain#doFilter method giving it the possibility to flush the response and finally it tries to write the headers. However if the above mentioned happens, the headers will not be sent to the client as the underlying http outputstream was already flushed.
Here's the code:

@Override
 protected void doFilterInternal(HttpServletRequest request,
   HttpServletResponse response, FilterChain filterChain)
     throws ServletException, IOException {

  HeaderWriterResponse headerWriterResponse = new HeaderWriterResponse(request,
    response, this.headerWriters);
  try {
   filterChain.doFilter(request, headerWriterResponse);
  }
  finally {
   headerWriterResponse.writeHeaders();
  }
 }

In the 4.0.1.RELEASE version, it happens the other way (so headers are effectively written and received by the clients) (assuming this filter was the first one in the chain):

 @Override
 protected void doFilterInternal(HttpServletRequest request,
   HttpServletResponse response, FilterChain filterChain)
   throws ServletException, IOException {

  for (HeaderWriter factory : headerWriters) {
   factory.writeHeaders(request, response);
  }
  filterChain.doFilter(request, response);
 }

So in previous versions, the headers are tried to be sent to the client BEFORE the remaining participants of the filterChain could attempt to put data in the stream, however in the new version it can happens (for example) that a JSP view flushes the buffer BEFORE the finally block of the doFilterInternal method is reached

So this is what is happening to summarize:

1. The participants in the filterChain are writting into the http stream (example: a JSP being served through the InternalResourceView class). If the output buffer is full, then the stream is flushed and sent to the client.
2. If that happens, then the headers cannot be sent to the client as the stream was flushed once.
3. The client receives everything except by the headers that the HeaderWriterFilters class should have written.

Expected Behavior

The headers should be written ALWAYS and there should be no way for any other participant in the filterChain to prevent this.

1. The headers has to be written to the http stream
2. The other participants in the chain should be able to write in the stream.
3. The client finally has to be able to receive the headers and the response body.

Configuration

Version

4.1.0.RELEASE, 4.1.1.RELASE

Related

2953

Sample

[Quantas]

We have also noticed this in our applications and had to revert back to 4.0.3.RELEASE as well.

[jtoselli]

Yes, we put a Filter in the chain that writes the headers first (no-cache for example). I will be working in a unit test that probes the issue in the meantime. Thanks Quantas! I'm wondering what was the motivation of swapping the order in which the headers and the filterChain#doFilter happens...

[Quantas]

I think we had a similar thing where we added headers like no-cache. I hope this gets corrected in 4.1.2.RELEASE so we can continue to keep up with updates.

[polacofede]

it seems pretty easy to fix the code to behave as previous version, provided there was no specific reason for this change... Is bit disruptive change that can go unnoticed when upgrading compromising in some way the app security.

[jtoselli]

It seems that the HeaderWriterResponse is doing what is supposed to do but at some point it does not participates in the Filter chain from the very beginning so when he starts playing the response could have been already committed by some other party in the chain.

[jtoselli]

Unit test that mimics the mentioned behaviour.
HeaderWriterFilterNotWrittingHeadersTest.java.txt

[rwinch]

Thanks for the report. This is due to fixes for #2953

This should happen before the response is committed due to using OnCommittedResponseWrapper. In particular it detects if the response is about to be committed and ensures the headers are written before the response is committed. I wonder if we are missing a use case.

Can anyone provide a complete application (not unit test) where this is happening?

[jtoselli]

Hi Rob, of course, I will try to attach the code as soon as I can.

[jtoselli]

Hi Rob, I am working on creating an artifact that mimics our app so the issue can be reproduced. In the meantime, I would expect an exception (like an assertion) to be thrown instead of just not interfering with the flow. That way you can be aware the code could be vulnerable due to missing headers sent to the client and verify the config or apply a workaround.

[rwinch]

@jtoselli I don't think we want an exception in this case. If this were something that happened, it would be something provided by the servlet container

[jtoselli]

@rwinch were you able to reproduce the issue? I'm sorry I didn't have the time to upload my config yet. Let me know if you still need it and I will do my best :)

[rwinch]

@jtoselli Thanks for reaching out. I haven't been able to reproduce this.

I'm asking for a full integration test because the unit test is invalid. This is because the unit test performs a flushBuffer() on the original MockHttpServletRequest. However, if anything within the FilterChain performs flushBuffer() it will be on an HttpServletResponse that is wrapped and ensures that the headers are written before the response is committed.

For example, the following test is more realistic and passes:

package com.example;

import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.util.Arrays;

import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner;

import org.springframework.mock.web.MockFilterChain;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.security.web.header.HeaderWriter;
import org.springframework.security.web.header.HeaderWriterFilter;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;

@RunWith(MockitoJUnitRunner.class)
public class HeaderWriterFilterNotWrittingHeadersTest {
    @Mock
    HeaderWriter writer;
    HeaderWriterFilter subject;
    MockHttpServletRequest req;
    MockHttpServletResponse res;
    MockFilterChain filterChain;

    @Before
    public void setup() throws UnsupportedEncodingException {
        this.subject = new HeaderWriterFilter(Arrays.<HeaderWriter>asList(this.writer));
        this.req = new MockHttpServletRequest();
        this.res = new MockHttpServletResponse();
        this.filterChain = new MockFilterChain() {
            @Override
            public void doFilter(ServletRequest request, ServletResponse response)
                    throws IOException, ServletException {
                response.getWriter().write("lets flush the stream");

                // the HeaderWriter has not been invoked yet
                verifyZeroInteractions(
                        HeaderWriterFilterNotWrittingHeadersTest.this.writer);

                // as soon as we invoke flushBuffer it should invoke the writer
                response.flushBuffer();

                // verify the writer has been invoked
                verify(HeaderWriterFilterNotWrittingHeadersTest.this.writer).writeHeaders(
                        any(HttpServletRequest.class), any(HttpServletResponse.class));

                // invoke the super class to indicate all the verifications have been invoked
                super.doFilter(request, response);
            }
        };
    }

    @Test
    public void testWritingHeaders() throws ServletException, IOException {
        this.subject.doFilter(this.req, this.res, this.filterChain);

        // verify that the MockFilterChain was invoked along w/ its assertions
        assertThat(this.filterChain.getRequest()).isNotNull();
    }
}

[rwinch]

Moving this to the Backlog until someone can reproduce it.

[eballetbaz]

I also reproduced the issue on Tomcat/JBoss when the dispatcher forwards to a JSP
Indeed updating the headers is disabled in org.apache.catalina.core.ApplicationHttpResponse for "included" response. (See javadoc on ApplicationHttpResponse#addHeader: Disallow addHeader() calls on an included response)

[jtoselli]

Thanks for taking care of this issue!

[jazdw]

@rwinch This issue should be re-opened. I believe it has manifested itself again since the commit that closed this issue (57d7ad05f9c8a3e177a97cbde5aa094f8179941f) was reverted (ea3dd336aa3772e9427be050a02d818084007006).

I can set a breakpoint in HeaderWriterFilter.onResponseCommitted() and see that this.isCommitted() returns true, therefore the headers are not written.

Now this seems to be related to the size of the file that I am returning in the response body. I noticed that a 122544 byte file did not have the headers set and a 15385 byte file did.

I believe this behaviour comes down to the logic in OnCommittedResponseWrapper

    private void checkContentLength(long contentLengthToWrite) {
        this.contentWritten += contentLengthToWrite;
        boolean isBodyFullyWritten = this.contentLength > 0
                && this.contentWritten >= this.contentLength;
        int bufferSize = getBufferSize();
        boolean requiresFlush = bufferSize > 0 && this.contentWritten >= bufferSize;
        if (isBodyFullyWritten || requiresFlush) {
            doOnResponseCommitted();
        }
    }

Its entire functionality seems to be based on the premise that as long as we write less than ServletResponse.getBufferSize() bytes then the response will not be committed and we can therefore still add headers at a later point.

However I can see that my Jetty ServletOutputStream implementation HttpOutput has two fields -

• _bufferSize - 32768 which is what getBufferSize() returns
• _commitSize - 8192

It seems that Jetty is committing the response after 8192 bytes are written and hence sometimes the headers are not being written.

edit
I can confirm that starting Jetty with its HttpConfiguration set like so httpConfig.setOutputAggregationSize(httpConfig.getOutputBufferSize()); fixes the issue. This sets _commitSize to the same as _bufferSize i.e. 32768.

Javadoc from HttpConfiguration.setOutputAggregationSize()

Set the max size of the response content write that is copied into the aggregate buffer. Writes that are smaller of this size are copied into the aggregate buffer, while writes that are larger of this size will cause the aggregate buffer to be flushed and the write to be executed without being copied.

Versions

• Spring - 4.3.17.RELEASE
• Spring security - 4.2.6.RELEASE
• Jetty - 9.3.23.v20180228

[sandipchitale]

I am experience this when using Springboot 2.x (embedded tomcat) + Spring security 5.x + Zuul proxy. The header writer gets triggered when

org.springframework.cloud.netflix.zuul.filters.post.SendResponseFilter

writes the response and flushes it, which triggers the:

org.springframework.security.web.header.HeaderWriterFilter.HeaderWriterResponse

but if the response from downstream service is smaller than the (tomcat) response buffer size, the response is committed and the writing headers (adding header to already committed response) is skipped.

[rwinch]

@sandipchitale Thanks for taking the time to provide feedback.

Can you please create a new issue rather than commenting on a closed ticket. This will help us ensure your feedback does not get lost. On the new issue please provide a link to this issue and details on how to reproduce the problem (ideally a sample or test that demonstrates the issue).