Polish HTTP Response Splitting

* Use new test method name convention of
  methodNameWhen<Condition>Then<Expectation>
* Check null Cookie
* Check Cookie.getName() for crlf since we do not want to rely on the
  implementation. For example Cookie could be overriden by extending it.
* Use Crlf as convention instead of CLRF as style guide
* Create new FirewalledResponse before each test to ensure isolation
* Use Mock for HttpServletResponse delegate to keep test in isolation (i.e.
  we do not want our tests to fail if MockHttpServletRequest changes an
  Exception error message)

Issue gh-3910

web/src/main/java/org/springframework/security/web/firewall/FirewalledResponse.java

@@ -58,18 +58,24 @@ class FirewalledResponse extends HttpServletResponseWrapper {
 
 	@Override
 	public void addCookie(Cookie cookie) {
+		if(cookie != null) {
+			validateCRLF(SET_COOKIE_HEADER, cookie.getName());
 			validateCRLF(SET_COOKIE_HEADER, cookie.getValue());
 			validateCRLF(SET_COOKIE_HEADER, cookie.getPath());
 			validateCRLF(SET_COOKIE_HEADER, cookie.getDomain());
 			validateCRLF(SET_COOKIE_HEADER, cookie.getComment());
+		}
 		super.addCookie(cookie);
 	}
 
 	void validateCRLF(String name, String value) {
-		if (name != null && CR_OR_LF.matcher(name).find()
+		if (hasCrlf(name) || hasCrlf(value)) {
-				|| value != null && CR_OR_LF.matcher(value).find()) {
 			throw new IllegalArgumentException(
 					"Invalid characters (CR/LF) in header " + name);
 		}
 	}
+
+	private boolean hasCrlf(String value) {
+		return value != null && CR_OR_LF.matcher(value).find();
+	}
 }

web/src/test/java/org/springframework/security/web/firewall/FirewalledResponseTests.java

@@ -15,15 +15,17 @@
  */
 package org.springframework.security.web.firewall;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.fail;
-
 import javax.servlet.http.Cookie;
+import javax.servlet.http.HttpServletResponse;
 
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
-import org.springframework.mock.web.MockHttpServletResponse;
+
+import static org.assertj.core.api.Assertions.fail;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 
 /**
  * @author Luke Taylor
@@ -31,116 +33,131 @@ import org.springframework.mock.web.MockHttpServletResponse;
  * @author Gabriel Lavoie
  */
 public class FirewalledResponseTests {
-	private MockHttpServletResponse response = new MockHttpServletResponse();
-	private FirewalledResponse fwResponse = new FirewalledResponse(response);
-
 	@Rule
 	public ExpectedException expectedException = ExpectedException.none();
 
+	private HttpServletResponse response;
+	private FirewalledResponse fwResponse;
+
+	@Before
+	public void setup() {
+		response = mock(HttpServletResponse.class);
+		fwResponse = new FirewalledResponse(response);
+	}
+
 	@Test
-	public void acceptRedirectLocationWithoutCRLF() throws Exception {
+	public void sendRedirectWhenValidThenNoException() throws Exception {
 		fwResponse.sendRedirect("/theURL");
-		assertThat(response.getRedirectedUrl()).isEqualTo("/theURL");
+
+		verify(response).sendRedirect("/theURL");
 	}
 
 	@Test
-	public void validateNullSafetyForRedirectLocation() throws Exception {
+	public void sendRedirectWhenNullThenDelegateInvoked() throws Exception {
-		// Exception from MockHttpServletResponse, exception not described in servlet spec.
-		expectedException.expect(IllegalArgumentException.class);
-		expectedException.expectMessage("Redirect URL must not be null");
-
 		fwResponse.sendRedirect(null);
+
+		verify(response).sendRedirect(null);
 	}
 
 	@Test
-	public void rejectsRedirectLocationContainingCRLF() throws Exception {
+	public void sendRedirectWhenHasCrlfThenThrowsException() throws Exception {
-		expectedException.expect(IllegalArgumentException.class);
+		expectCrlfValidationException();
-		expectedException.expectMessage("Invalid characters (CR/LF)");
 
 		fwResponse.sendRedirect("/theURL\r\nsomething");
 	}
 
 	@Test
-	public void acceptHeaderValueWithoutCRLF() throws Exception {
+	public void addHeaderWhenValidThenDelegateInvoked() throws Exception {
 		fwResponse.addHeader("foo", "bar");
-		assertThat(response.getHeader("foo")).isEqualTo("bar");
+
+		verify(response).addHeader("foo", "bar");
 	}
 
 	@Test
-	public void validateNullSafetyForHeaderValue() throws Exception {
+	public void addHeaderWhenNullValueThenDelegateInvoked() throws Exception {
-		// Exception from MockHttpServletResponse, exception not described in servlet spec.
-		expectedException.expect(IllegalArgumentException.class);
-		expectedException.expectMessage("Header value must not be null");
-
 		fwResponse.addHeader("foo", null);
+
+		verify(response).addHeader("foo", null);
 	}
 
 	@Test
-	public void rejectHeaderValueContainingCRLF() {
+	public void addHeaderWhenHeaderValueHasCrlfThenException() {
-		expectCRLFValidationException();
+		expectCrlfValidationException();
 
 		fwResponse.addHeader("foo", "abc\r\nContent-Length:100");
 	}
 
 	@Test
-	public void rejectHeaderNameContainingCRLF() {
+	public void addHeaderWhenHeaderNameHasCrlfThenException() {
-		expectCRLFValidationException();
+		expectCrlfValidationException();
 
 		fwResponse.addHeader("abc\r\nContent-Length:100", "bar");
 	}
 
 	@Test
-	public void acceptCookieWithoutCRLF() {
+	public void addCookieWhenValidThenDelegateInvoked() {
 		Cookie cookie = new Cookie("foo", "bar");
 		cookie.setPath("/foobar");
 		cookie.setDomain("foobar");
 		cookie.setComment("foobar");
 
 		fwResponse.addCookie(cookie);
+
+		verify(response).addCookie(cookie);
+	}
+	@Test
+	public void addCookieWhenNullThenDelegateInvoked() {
+		fwResponse.addCookie(null);
+
+		verify(response).addCookie(null);
 	}
 
 	@Test
-	public void rejectCookieNameContainingCRLF() {
+	public void addCookieWhenCookieNameContainsCrlfThenException() {
-		// This one is thrown by the Cookie class constructor from javax.servlet-api,
+		// Constructor validates the name
-		// no need to cover in FirewalledResponse.
+		Cookie cookie = new Cookie("valid-since-constructor-validates", "bar") {
-		expectedException.expect(IllegalArgumentException.class);
+			@Override
-		Cookie cookie = new Cookie("foo\r\nbar", "bar");
+			public String getName() {
+				return "foo\r\nbar";
 			}
 
-	@Test
+		};
-	public void rejectCookieValueContainingCRLF() {
+		expectCrlfValidationException();
-		expectCRLFValidationException();
 
-		Cookie cookie = new Cookie("foo", "foo\r\nbar");
 		fwResponse.addCookie(cookie);
 	}
 
 	@Test
-	public void rejectCookiePathContainingCRLF() {
+	public void addCookieWhenCookieValueContainsCrlfThenException() {
-		expectCRLFValidationException();
+		Cookie cookie = new Cookie("foo", "foo\r\nbar");
+		expectCrlfValidationException();
 
+		fwResponse.addCookie(cookie);
+	}
+
+	@Test
+	public void addCookieWhenCookiePathContainsCrlfThenException() {
 		Cookie cookie = new Cookie("foo", "bar");
 		cookie.setPath("/foo\r\nbar");
+		expectCrlfValidationException();
 
 		fwResponse.addCookie(cookie);
 	}
 
 	@Test
-	public void rejectCookieDomainContainingCRLF() {
+	public void addCookieWhenCookieDomainContainsCrlfThenException() {
-		expectCRLFValidationException();
-
 		Cookie cookie = new Cookie("foo", "bar");
 		cookie.setDomain("foo\r\nbar");
+		expectCrlfValidationException();
 
 		fwResponse.addCookie(cookie);
 	}
 
 	@Test
-	public void rejectCookieCommentContainingCRLF() {
+	public void addCookieWhenCookieCommentContainsCrlfThenException() {
-		expectCRLFValidationException();
-
 		Cookie cookie = new Cookie("foo", "bar");
 		cookie.setComment("foo\r\nbar");
+		expectCrlfValidationException();
 
 		fwResponse.addCookie(cookie);
 	}
@@ -156,7 +173,7 @@ public class FirewalledResponseTests {
 		validateLineEnding("foo\nbar", "bar");
 	}
 
-	private void expectCRLFValidationException() {
+	private void expectCrlfValidationException() {
 		expectedException.expect(IllegalArgumentException.class);
 		expectedException.expectMessage("Invalid characters (CR/LF)");
 	}