From 17c423f5afa2d8e6a0b1adfc2f7380489c5db33b Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 25 Sep 2019 17:16:21 +0100 Subject: [PATCH] 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 --- .../reactive/MockServerHttpResponse.java | 10 +- .../springframework/http/ResponseCookie.java | 93 ++++++++++++++++++- .../reactive/ReactorServerHttpResponse.java | 23 ++--- .../reactive/ServletServerHttpResponse.java | 30 +++--- .../reactive/UndertowServerHttpResponse.java | 1 + .../http/ResponseCookieTests.java | 73 +++++++++------ .../reactive/test/MockServerHttpResponse.java | 10 +- 7 files changed, 174 insertions(+), 66 deletions(-) diff --git a/spring-test/src/main/java/org/springframework/mock/http/server/reactive/MockServerHttpResponse.java b/spring-test/src/main/java/org/springframework/mock/http/server/reactive/MockServerHttpResponse.java index 71fc199378b5..4206db148b20 100644 --- a/spring-test/src/main/java/org/springframework/mock/http/server/reactive/MockServerHttpResponse.java +++ b/spring-test/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 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 index 7507de653ca3..65afc80a506b 100644 --- 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 = + 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/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java index 4675d5170452..43842bdb28e0 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java +++ b/spring-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 cookies : getCookies().values()) { + for (ResponseCookie cookie : cookies) { + this.response.addHeader(HttpHeaders.SET_COOKIE, cookie.toString()); } } } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java index f28b404c509b..b82233977e2a 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java +++ b/spring-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 cookies : getCookies().values()) { + for (ResponseCookie cookie : cookies) { + this.response.addHeader(HttpHeaders.SET_COOKIE, cookie.toString()); } } } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java index 1fb094b5b08b..d19f839cb2b8 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java +++ b/spring-web/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 index 0afa3afb961b..79affb0daf34 100644 --- 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/spring-web/src/test/java/org/springframework/mock/http/server/reactive/test/MockServerHttpResponse.java b/spring-web/src/test/java/org/springframework/mock/http/server/reactive/test/MockServerHttpResponse.java index 366dbbdc3c7b..98b92da526d0 100644 --- a/spring-web/src/test/java/org/springframework/mock/http/server/reactive/test/MockServerHttpResponse.java +++ b/spring-web/src/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 cookies : getCookies().values()) { + for (ResponseCookie cookie : cookies) { + getHeaders().add(HttpHeaders.SET_COOKIE, cookie.toString()); + } + } } @Override