Support for sameSite attribute in WebFlux

Bypass server cookie and write Set-Cookie header directly for Reactor Netty, and Servlet API which do not provide options. For Undertow use the sameSite attribute. Closes gh-23693
spring-projects · Sep 25, 2019 · 17c423f · 17c423f
1 parent b1ed051
commit 17c423f
Show file tree

Hide file tree

Showing 7 changed files with 174 additions and 66 deletions.
diff --git a/...t/src/main/java/org/springframework/mock/http/server/reactive/MockServerHttpResponse.java b/...t/src/main/java/org/springframework/mock/http/server/reactive/MockServerHttpResponse.java
@@ -18,7 +18,7 @@
 
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
-import java.util.Collection;
+import java.util.List;
 import java.util.Optional;
 import java.util.function.Function;
 
@@ -32,6 +32,7 @@
 import org.springframework.core.io.buffer.DataBufferUtils;
 import org.springframework.core.io.buffer.DefaultDataBufferFactory;
 import org.springframework.http.HttpHeaders;
+import org.springframework.http.ResponseCookie;
 import org.springframework.http.server.reactive.AbstractServerHttpResponse;
 import org.springframework.util.Assert;
 import org.springframework.util.MimeType;
@@ -101,8 +102,11 @@ protected void applyHeaders() {
 
 	@Override
 	protected void applyCookies() {
-		getCookies().values().stream().flatMap(Collection::stream)
-				.forEach(cookie -> getHeaders().add(HttpHeaders.SET_COOKIE, cookie.toString()));
+		for (List<ResponseCookie> cookies : getCookies().values()) {
+			for (ResponseCookie cookie : cookies) {
+				getHeaders().add(HttpHeaders.SET_COOKIE, cookie.toString());
+			}
+		}
 	}
 
 	@Override

diff --git a/spring-web/src/main/java/org/springframework/http/ResponseCookie.java b/spring-web/src/main/java/org/springframework/http/ResponseCookie.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2019 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -59,12 +59,18 @@ private ResponseCookie(String name, String value, Duration maxAge, @Nullable Str
 
 		super(name, value);
 		Assert.notNull(maxAge, "Max age must not be null");
+
 		this.maxAge = maxAge;
 		this.domain = domain;
 		this.path = path;
 		this.secure = secure;
 		this.httpOnly = httpOnly;
 		this.sameSite = sameSite;
+
+		Rfc6265Utils.validateCookieName(name);
+		Rfc6265Utils.validateCookieValue(value);
+		Rfc6265Utils.validateDomain(domain);
+		Rfc6265Utils.validatePath(path);
 	}
 
 
@@ -308,4 +314,89 @@ public interface ResponseCookieBuilder {
 		ResponseCookie build();
 	}
 
+
+	private static class Rfc6265Utils {
+
+		private static final String SEPARATOR_CHARS = new String(new char[] {
+				'(', ')', '<', '>', '@', ',', ';', ':', '\\', '"', '/', '[', ']', '?', '=', '{', '}', ' '
+		});
+
+		private static final String DOMAIN_CHARS =
+				"0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ.-";
+
+
+		public static void validateCookieName(String name) {
+			for (int i = 0; i < name.length(); i++) {
+				char c = name.charAt(i);
+				// CTL = <US-ASCII control chars (octets 0 - 31) and DEL (127)>
+				if (c <= 0x1F || c == 0x7F) {
+					throw new IllegalArgumentException(
+							name + ": RFC2616 token cannot have control chars");
+				}
+				if (SEPARATOR_CHARS.indexOf(c) >= 0) {
+					throw new IllegalArgumentException(
+							name + ": RFC2616 token cannot have separator chars such as '" + c + "'");
+				}
+				if (c >= 0x80) {
+					throw new IllegalArgumentException(
+							name + ": RFC2616 token can only have US-ASCII: 0x" + Integer.toHexString(c));
+				}
+			}
+		}
+
+		public static void validateCookieValue(@Nullable String value) {
+			if (value == null) {
+				return;
+			}
+			int start = 0;
+			int end = value.length();
+			if (end > 1 && value.charAt(0) == '"' && value.charAt(end - 1) == '"') {
+				start = 1;
+				end--;
+			}
+			char[] chars = value.toCharArray();
+			for (int i = start; i < end; i++) {
+				char c = chars[i];
+				if (c < 0x21 || c == 0x22 || c == 0x2c || c == 0x3b || c == 0x5c || c == 0x7f) {
+					throw new IllegalArgumentException(
+							"RFC2616 cookie value cannot have '" + c + "'");
+				}
+				if (c >= 0x80) {
+					throw new IllegalArgumentException(
+							"RFC2616 cookie value can only have US-ASCII chars: 0x" + Integer.toHexString(c));
+				}
+			}
+		}
+
+		public static void validateDomain(@Nullable String domain) {
+			if (!StringUtils.hasLength(domain)) {
+				return;
+			}
+			int char1 = domain.charAt(0);
+			int charN = domain.charAt(domain.length() - 1);
+			if (char1 == '.' || char1 == '-' || charN == '.' || charN == '-') {
+				throw new IllegalArgumentException("Invalid first/last char in cookie domain: " + domain);
+			}
+			for (int i = 0, c = -1; i < domain.length(); i++) {
+				int p = c;
+				c = domain.charAt(i);
+				if (DOMAIN_CHARS.indexOf(c) == -1 || (p == '.' && (c == '.' || c == '-')) || (p == '-' && c == '.')) {
+					throw new IllegalArgumentException(domain + ": invalid cookie domain char '" + c + "'");
+				}
+			}
+		}
+
+		public static void validatePath(@Nullable String path) {
+			if (path == null) {
+				return;
+			}
+			for (int i = 0; i < path.length(); i++) {
+				char c = path.charAt(i);
+				if (c < 0x20 || c > 0x7E || c == ';') {
+					throw new IllegalArgumentException(path + ": Invalid cookie path char '" + c + "'");
+				}
+			}
+		}
+	}
+
 }
diff --git a/...web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java b/...web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java
@@ -17,10 +17,9 @@
 package org.springframework.http.server.reactive;
 
 import java.nio.file.Path;
+import java.util.List;
 
 import io.netty.buffer.ByteBuf;
-import io.netty.handler.codec.http.cookie.Cookie;
-import io.netty.handler.codec.http.cookie.DefaultCookie;
 import org.reactivestreams.Publisher;
 import reactor.core.publisher.Flux;
 import reactor.core.publisher.Mono;
@@ -91,21 +90,11 @@ protected void applyHeaders() {
 
 	@Override
 	protected void applyCookies() {
-		for (String name : getCookies().keySet()) {
-			for (ResponseCookie httpCookie : getCookies().get(name)) {
-				Cookie cookie = new DefaultCookie(name, httpCookie.getValue());
-				if (!httpCookie.getMaxAge().isNegative()) {
-					cookie.setMaxAge(httpCookie.getMaxAge().getSeconds());
-				}
-				if (httpCookie.getDomain() != null) {
-					cookie.setDomain(httpCookie.getDomain());
-				}
-				if (httpCookie.getPath() != null) {
-					cookie.setPath(httpCookie.getPath());
-				}
-				cookie.setSecure(httpCookie.isSecure());
-				cookie.setHttpOnly(httpCookie.isHttpOnly());
-				this.response.addCookie(cookie);
+		// Netty Cookie doesn't support sameSite. When this is resolved, we can adapt to it again:
+		// https://github.com/netty/netty/issues/8161
+		for (List<ResponseCookie> cookies : getCookies().values()) {
+			for (ResponseCookie cookie : cookies) {
+				this.response.addHeader(HttpHeaders.SET_COOKIE, cookie.toString());
 			}
 		}
 	}

diff --git a/...web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java b/...web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java
@@ -19,13 +19,13 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.Charset;
+import java.util.List;
 
 import javax.servlet.AsyncContext;
 import javax.servlet.AsyncEvent;
 import javax.servlet.AsyncListener;
 import javax.servlet.ServletOutputStream;
 import javax.servlet.WriteListener;
-import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletResponse;
 
 import org.reactivestreams.Processor;
@@ -142,21 +142,19 @@ protected void applyHeaders() {
 
 	@Override
 	protected void applyCookies() {
-		for (String name : getCookies().keySet()) {
-			for (ResponseCookie httpCookie : getCookies().get(name)) {
-				Cookie cookie = new Cookie(name, httpCookie.getValue());
-				if (!httpCookie.getMaxAge().isNegative()) {
-					cookie.setMaxAge((int) httpCookie.getMaxAge().getSeconds());
-				}
-				if (httpCookie.getDomain() != null) {
-					cookie.setDomain(httpCookie.getDomain());
-				}
-				if (httpCookie.getPath() != null) {
-					cookie.setPath(httpCookie.getPath());
-				}
-				cookie.setSecure(httpCookie.isSecure());
-				cookie.setHttpOnly(httpCookie.isHttpOnly());
-				this.response.addCookie(cookie);
+
+		// Servlet Cookie doesn't support same site:
+		// https://github.com/eclipse-ee4j/servlet-api/issues/175
+
+		// For Jetty, starting 9.4.21+ we could adapt to HttpCookie:
+		// https://github.com/eclipse/jetty.project/issues/3040
+
+		// For Tomcat it seems to be a global option only:
+		// https://tomcat.apache.org/tomcat-8.5-doc/config/cookie-processor.html
+
+		for (List<ResponseCookie> cookies : getCookies().values()) {
+			for (ResponseCookie cookie : cookies) {
+				this.response.addHeader(HttpHeaders.SET_COOKIE, cookie.toString());
 			}
 		}
 	}

diff --git a/...eb/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java b/...eb/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java
@@ -115,6 +115,7 @@ protected void applyCookies() {
 				}
 				cookie.setSecure(httpCookie.isSecure());
 				cookie.setHttpOnly(httpCookie.isHttpOnly());
+				cookie.setSameSiteMode(httpCookie.getSameSite());
 				this.exchange.getResponseCookies().putIfAbsent(name, cookie);
 			}
 		}

diff --git a/spring-web/src/test/java/org/springframework/http/ResponseCookieTests.java b/spring-web/src/test/java/org/springframework/http/ResponseCookieTests.java
@@ -16,12 +16,13 @@
 
 package org.springframework.http;
 
-import java.time.Duration;
+import java.util.Arrays;
 
+import org.hamcrest.Matchers;
 import org.junit.Test;
 
-import static org.hamcrest.CoreMatchers.*;
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
 
 /**
  * Unit tests for {@link ResponseCookie}.
@@ -30,40 +31,60 @@
 public class ResponseCookieTests {
 
 	@Test
-	public void defaultValues() {
+	public void basic() {
+
+		assertEquals("id=", ResponseCookie.from("id", null).build().toString());
 		assertEquals("id=1fWa", ResponseCookie.from("id", "1fWa").build().toString());
-	}
 
-	@Test
-	public void httpOnlyStrictSecureWithDomainAndPath() {
-		assertEquals("id=1fWa; Path=/projects; Domain=spring.io; Secure; HttpOnly; SameSite=strict",
-				ResponseCookie.from("id", "1fWa").domain("spring.io").path("/projects")
-						.httpOnly(true).secure(true).sameSite("strict").build().toString());
+		assertEquals(
+				"id=1fWa; Path=/path; Domain=abc; " +
+						"Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; " +
+						"Secure; HttpOnly; SameSite=None",
+				ResponseCookie.from("id", "1fWa")
+						.domain("abc").path("/path").maxAge(0).httpOnly(true).secure(true).sameSite("None")
+						.build().toString());
 	}
 
 	@Test
-	public void maxAge() {
-
-		Duration maxAge = Duration.ofDays(365);
-		String expires = HttpHeaders.formatDate(System.currentTimeMillis() + maxAge.toMillis());
-		expires = expires.substring(0, expires.indexOf(":") + 1);
+	public void nameChecks() {
 
-		assertThat(ResponseCookie.from("id", "1fWa").maxAge(maxAge).build().toString(), allOf(
-				startsWith("id=1fWa; Max-Age=31536000; Expires=" + expires),
-				endsWith(" GMT")));
+		Arrays.asList("id", "i.d.", "i-d", "+id", "i*d", "i$d", "#id")
+				.forEach(name -> {
+					ResponseCookie.from(name, "value").build();
+					// no exception..
+				});
 
-		assertThat(ResponseCookie.from("id", "1fWa").maxAge(maxAge.getSeconds()).build().toString(), allOf(
-				startsWith("id=1fWa; Max-Age=31536000; Expires=" + expires),
-				endsWith(" GMT")));
+		Arrays.asList("\"id\"", "id\t", "i\td", "i d", "i;d", "{id}", "[id]", "\"", "id\u0091")
+				.forEach(name -> {
+					try {
+						ResponseCookie.from(name, "value").build();
+					}
+					catch (IllegalArgumentException ex) {
+						assertThat(ex.getMessage(), Matchers.containsString("RFC2616 token"));
+					}
+				});
 	}
 
 	@Test
-	public void maxAge0() {
-		assertEquals("id=1fWa; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT",
-				ResponseCookie.from("id", "1fWa").maxAge(Duration.ofSeconds(0)).build().toString());
+	public void valueChecks() {
 
-		assertEquals("id=1fWa; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT",
-				ResponseCookie.from("id", "1fWa").maxAge(0).build().toString());
+		Arrays.asList("1fWa", "", null, "1f=Wa", "1f-Wa", "1f/Wa", "1.f.W.a.")
+				.forEach(value -> {
+					ResponseCookie.from("id", value).build();
+					// no exception..
+				});
+
+		Arrays.asList("1f\tWa", "\t", "1f Wa", "1f;Wa", "\"1fWa", "1f\\Wa", "1f\"Wa", "\"", "1fWa\u0005", "1f\u0091Wa")
+				.forEach(value -> {
+					try {
+						ResponseCookie.from("id", value).build();
+					}
+					catch (IllegalArgumentException ex) {
+						assertThat(ex.getMessage(), Matchers.containsString("RFC2616 cookie value"));
+					}
+				});
 	}
 
+
+
 }
diff --git a/.../test/java/org/springframework/mock/http/server/reactive/test/MockServerHttpResponse.java b/.../test/java/org/springframework/mock/http/server/reactive/test/MockServerHttpResponse.java
@@ -18,7 +18,7 @@
 
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
-import java.util.Collection;
+import java.util.List;
 import java.util.Optional;
 import java.util.function.Function;
 
@@ -32,6 +32,7 @@
 import org.springframework.core.io.buffer.DataBufferUtils;
 import org.springframework.core.io.buffer.DefaultDataBufferFactory;
 import org.springframework.http.HttpHeaders;
+import org.springframework.http.ResponseCookie;
 import org.springframework.http.server.reactive.AbstractServerHttpResponse;
 import org.springframework.util.Assert;
 import org.springframework.util.MimeType;
@@ -101,8 +102,11 @@ protected void applyHeaders() {
 
 	@Override
 	protected void applyCookies() {
-		getCookies().values().stream().flatMap(Collection::stream)
-				.forEach(cookie -> getHeaders().add(HttpHeaders.SET_COOKIE, cookie.toString()));
+		for (List<ResponseCookie> cookies : getCookies().values()) {
+			for (ResponseCookie cookie : cookies) {
+				getHeaders().add(HttpHeaders.SET_COOKIE, cookie.toString());
+			}
+		}
 	}
 
 	@Override