Cache Control only written if not set

Previously Spring Security always wrote cache control headers and relied
on the application to override the values. This can cause problems with
cache control. For example, applications may only set cache control if
the header is not already set. Additionally, setting of Cache-Control
should disable writing of Pragma.

This commit delays writing headers until just before the response is
committed and only writes the Cache Control headers if they do not exist.

Fixes gh-2953

web/src/main/java/org/springframework/security/web/header/HeaderWriterFilter.java

@@ -15,15 +15,17 @@
  */
 package org.springframework.security.web.header;
 
-import org.springframework.util.Assert;
-import org.springframework.web.filter.OncePerRequestFilter;
+import java.io.IOException;
+import java.util.List;
 
 import javax.servlet.FilterChain;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import java.io.IOException;
-import java.util.*;
+
+import org.springframework.security.web.util.OnCommittedResponseWrapper;
+import org.springframework.util.Assert;
+import org.springframework.web.filter.OncePerRequestFilter;
 
 /**
  * Filter implementation to add headers to the current request. Can be useful to add
@@ -58,10 +60,50 @@ public class HeaderWriterFilter extends OncePerRequestFilter {
 			HttpServletResponse response, FilterChain filterChain)
 					throws ServletException, IOException {
 
-		for (HeaderWriter headerWriter : headerWriters) {
-			headerWriter.writeHeaders(request, response);
+		HeaderWriterResponse headerWriterResponse = new HeaderWriterResponse(request,
+				response, this.headerWriters);
+		try {
+			filterChain.doFilter(request, headerWriterResponse);
+		}
+		finally {
+			headerWriterResponse.writeHeaders();
 		}
-		filterChain.doFilter(request, response);
 	}
 
+	static class HeaderWriterResponse extends OnCommittedResponseWrapper {
+		private final HttpServletRequest request;
+		private final List<HeaderWriter> headerWriters;
+
+		HeaderWriterResponse(HttpServletRequest request, HttpServletResponse response,
+				List<HeaderWriter> headerWriters) {
+			super(response);
+			this.request = request;
+			this.headerWriters = headerWriters;
+		}
+
+		/*
+		 * (non-Javadoc)
+		 *
+		 * @see org.springframework.security.web.util.OnCommittedResponseWrapper#
+		 * onResponseCommitted()
+		 */
+		@Override
+		protected void onResponseCommitted() {
+			writeHeaders();
+			this.disableOnResponseCommitted();
+		}
+
+		protected void writeHeaders() {
+			if (isDisableOnResponseCommitted()) {
+				return;
+			}
+			for (HeaderWriter headerWriter : this.headerWriters) {
+				headerWriter.writeHeaders(this.request, getHttpResponse());
+			}
+		}
+
+		private HttpServletResponse getHttpResponse() {
+			return (HttpServletResponse) getResponse();
+		}
+	}
 }

web/src/main/java/org/springframework/security/web/header/writers/CacheControlHeadersWriter.java

@@ -15,14 +15,20 @@
  */
 package org.springframework.security.web.header.writers;
 
+import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.List;
 
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
 import org.springframework.security.web.header.Header;
+import org.springframework.security.web.header.HeaderWriter;
+import org.springframework.util.ReflectionUtils;
 
 /**
- * A {@link StaticHeadersWriter} that inserts headers to prevent caching. Specifically it
- * adds the following headers:
+ * Inserts headers to prevent caching if no cache control headers have been specified.
+ * Specifically it adds the following headers:
  * <ul>
  * <li>Cache-Control: no-cache, no-store, max-age=0, must-revalidate</li>
  * <li>Pragma: no-cache</li>
@@ -32,21 +38,47 @@ import org.springframework.security.web.header.Header;
  * @author Rob Winch
  * @since 3.2
  */
-public final class CacheControlHeadersWriter extends StaticHeadersWriter {
+public final class CacheControlHeadersWriter implements HeaderWriter {
+	private static final String EXPIRES = "Expires";
+	private static final String PRAGMA = "Pragma";
+	private static final String CACHE_CONTROL = "Cache-Control";
+
+	private final Method getHeaderMethod;
+
+	private final HeaderWriter delegate;
 
 	/**
 	 * Creates a new instance
 	 */
 	public CacheControlHeadersWriter() {
-		super(createHeaders());
+		this.delegate = new StaticHeadersWriter(createHeaders());
+		this.getHeaderMethod = ReflectionUtils.findMethod(HttpServletResponse.class,
+				"getHeader", String.class);
+	}
+
+	@Override
+	public void writeHeaders(HttpServletRequest request, HttpServletResponse response) {
+		if (hasHeader(response, CACHE_CONTROL) || hasHeader(response, EXPIRES)
+				|| hasHeader(response, PRAGMA)) {
+			return;
+		}
+		this.delegate.writeHeaders(request, response);
+	}
+
+	private boolean hasHeader(HttpServletResponse response, String headerName) {
+		if (this.getHeaderMethod == null) {
+			return false;
+		}
+		return ReflectionUtils.invokeMethod(this.getHeaderMethod, response,
+				headerName) != null;
 	}
 
 	private static List<Header> createHeaders() {
 		List<Header> headers = new ArrayList<Header>(2);
-		headers.add(new Header("Cache-Control",
+		headers.add(new Header(CACHE_CONTROL,
 				"no-cache, no-store, max-age=0, must-revalidate"));
-		headers.add(new Header("Pragma", "no-cache"));
-		headers.add(new Header("Expires", "0"));
+		headers.add(new Header(PRAGMA, "no-cache"));
+		headers.add(new Header(EXPIRES, "0"));
 		return headers;
 	}
 }

web/src/test/java/org/springframework/security/web/header/HeaderWriterFilterTests.java

@@ -15,21 +15,32 @@
  */
 package org.springframework.security.web.header;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.verify;
-
+import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
+import javax.servlet.FilterChain;
+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.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.verifyNoMoreInteractions;
+import static org.mockito.Mockito.verifyZeroInteractions;
 
 /**
  * Tests for the {@code HeadersFilter}
@@ -60,8 +71,8 @@ public class HeaderWriterFilterTests {
 	@Test
 	public void additionalHeadersShouldBeAddedToTheResponse() throws Exception {
 		List<HeaderWriter> headerWriters = new ArrayList<HeaderWriter>();
-		headerWriters.add(writer1);
-		headerWriters.add(writer2);
+		headerWriters.add(this.writer1);
+		headerWriters.add(this.writer2);
 
 		HeaderWriterFilter filter = new HeaderWriterFilter(headerWriters);
 
@@ -71,9 +82,34 @@ public class HeaderWriterFilterTests {
 
 		filter.doFilter(request, response, filterChain);
 
-		verify(writer1).writeHeaders(request, response);
-		verify(writer2).writeHeaders(request, response);
+		verify(this.writer1).writeHeaders(request, response);
+		verify(this.writer2).writeHeaders(request, response);
 		assertThat(filterChain.getRequest()).isEqualTo(request); // verify the filterChain
 																	// continued
 	}
+
+	// gh-2953
+	@Test
+	public void headersDelayed() throws Exception {
+		HeaderWriterFilter filter = new HeaderWriterFilter(
+				Arrays.<HeaderWriter>asList(this.writer1));
+
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		MockHttpServletResponse response = new MockHttpServletResponse();
+
+		filter.doFilter(request, response, new FilterChain() {
+			@Override
+			public void doFilter(ServletRequest request, ServletResponse response)
+					throws IOException, ServletException {
+				verifyZeroInteractions(HeaderWriterFilterTests.this.writer1);
+
+				response.flushBuffer();
+
+				verify(HeaderWriterFilterTests.this.writer1).writeHeaders(
+						any(HttpServletRequest.class), any(HttpServletResponse.class));
+			}
+		});
+
+		verifyNoMoreInteractions(this.writer1);
+	}
 }

web/src/test/java/org/springframework/security/web/header/writers/CacheControlHeadersWriterTests.java

@@ -15,19 +15,32 @@
  */
 package org.springframework.security.web.header.writers;
 
-import static org.assertj.core.api.Assertions.assertThat;
-
 import java.util.Arrays;
 
+import javax.servlet.http.HttpServletResponse;
+
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.powermock.core.classloader.annotations.PrepareOnlyThisForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
+import org.springframework.util.ReflectionUtils;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.when;
+import static org.powermock.api.mockito.PowerMockito.spy;
 
 /**
  * @author Rob Winch
  *
  */
+@RunWith(PowerMockRunner.class)
+@PrepareOnlyThisForTest(ReflectionUtils.class)
 public class CacheControlHeadersWriterTests {
 
 	private MockHttpServletRequest request;
@@ -38,20 +51,79 @@ public class CacheControlHeadersWriterTests {
 
 	@Before
 	public void setup() {
-		request = new MockHttpServletRequest();
-		response = new MockHttpServletResponse();
-		writer = new CacheControlHeadersWriter();
+		this.request = new MockHttpServletRequest();
+		this.response = new MockHttpServletResponse();
+		this.writer = new CacheControlHeadersWriter();
 	}
 
 	@Test
 	public void writeHeaders() {
-		writer.writeHeaders(request, response);
+		this.writer.writeHeaders(this.request, this.response);
 
-		assertThat(response.getHeaderNames().size()).isEqualTo(3);
-		assertThat(response.getHeaderValues("Cache-Control")).isEqualTo(
+		assertThat(this.response.getHeaderNames().size()).isEqualTo(3);
+		assertThat(this.response.getHeaderValues("Cache-Control")).isEqualTo(
 				Arrays.asList("no-cache, no-store, max-age=0, must-revalidate"));
-		assertThat(response.getHeaderValues("Pragma")).isEqualTo(
-				Arrays.asList("no-cache"));
-		assertThat(response.getHeaderValues("Expires")).isEqualTo(Arrays.asList("0"));
+		assertThat(this.response.getHeaderValues("Pragma"))
+				.isEqualTo(Arrays.asList("no-cache"));
+		assertThat(this.response.getHeaderValues("Expires"))
+				.isEqualTo(Arrays.asList("0"));
+	}
+
+	@Test
+	public void writeHeadersServlet25() {
+		spy(ReflectionUtils.class);
+		when(ReflectionUtils.findMethod(HttpServletResponse.class, "getHeader",
+				String.class)).thenReturn(null);
+		this.response = spy(this.response);
+		doThrow(NoSuchMethodError.class).when(this.response).getHeader(anyString());
+		this.writer = new CacheControlHeadersWriter();
+
+		this.writer.writeHeaders(this.request, this.response);
+
+		assertThat(this.response.getHeaderNames().size()).isEqualTo(3);
+		assertThat(this.response.getHeaderValues("Cache-Control")).isEqualTo(
+				Arrays.asList("no-cache, no-store, max-age=0, must-revalidate"));
+		assertThat(this.response.getHeaderValues("Pragma"))
+				.isEqualTo(Arrays.asList("no-cache"));
+		assertThat(this.response.getHeaderValues("Expires"))
+				.isEqualTo(Arrays.asList("0"));
+	}
+
+	// gh-2953
+	@Test
+	public void writeHeadersDisabledIfCacheControl() {
+		this.response.setHeader("Cache-Control", "max-age: 123");
+
+		this.writer.writeHeaders(this.request, this.response);
+
+		assertThat(this.response.getHeaderNames()).hasSize(1);
+		assertThat(this.response.getHeaderValues("Cache-Control"))
+				.containsOnly("max-age: 123");
+		assertThat(this.response.getHeaderValue("Pragma")).isNull();
+		assertThat(this.response.getHeaderValue("Expires")).isNull();
+	}
+
+	@Test
+	public void writeHeadersDisabledIfPragma() {
+		this.response.setHeader("Pragma", "mock");
+
+		this.writer.writeHeaders(this.request, this.response);
+
+		assertThat(this.response.getHeaderNames()).hasSize(1);
+		assertThat(this.response.getHeaderValues("Pragma")).containsOnly("mock");
+		assertThat(this.response.getHeaderValue("Expires")).isNull();
+		assertThat(this.response.getHeaderValue("Cache-Control")).isNull();
+	}
+
+	@Test
+	public void writeHeadersDisabledIfExpires() {
+		this.response.setHeader("Expires", "mock");
+
+		this.writer.writeHeaders(this.request, this.response);
+
+		assertThat(this.response.getHeaderNames()).hasSize(1);
+		assertThat(this.response.getHeaderValues("Expires")).containsOnly("mock");
+		assertThat(this.response.getHeaderValue("Cache-Control")).isNull();
+		assertThat(this.response.getHeaderValue("Pragma")).isNull();
 	}
 }