Affects: main
The way to confirm:
Modify two lines in CookieIntegrationTests and the test will fail.
@ParameterizedHttpServerTest
public void basicTest(HttpServer httpServer) throws Exception {
startServer(httpServer);
URI url = new URI("http://localhost:" + port);
// Modified line 1:add lang=zh-CN
String header = "SID=31d4d96e407aad42; lang=en-US; lang=zh-CN";
ResponseEntity<Void> response = new RestTemplate().exchange(
RequestEntity.get(url).header("Cookie", header).build(), Void.class);
Map<String, List<HttpCookie>> requestCookies = this.cookieHandler.requestCookies;
assertThat(requestCookies.size()).isEqualTo(2);
List<HttpCookie> list = requestCookies.get("SID");
assertThat(list.size()).isEqualTo(1);
assertThat(list.iterator().next().getValue()).isEqualTo("31d4d96e407aad42");
list = requestCookies.get("lang");
// Modified line 2:add this line
assertThat(list.size()).isEqualTo(2);
assertThat(list.iterator().next().getValue()).isEqualTo("en-US");
List<String> headerValues = response.getHeaders().get("Set-Cookie");
assertThat(headerValues.size()).isEqualTo(2);
List<String> cookie0 = splitCookie(headerValues.get(0));
assertThat(cookie0.remove("SID=31d4d96e407aad42")).as("SID").isTrue();
assertThat(cookie0.stream().map(String::toLowerCase))
.containsExactlyInAnyOrder("path=/", "secure", "httponly");
List<String> cookie1 = splitCookie(headerValues.get(1));
assertThat(cookie1.remove("lang=en-US")).as("lang").isTrue();
assertThat(cookie1.stream().map(String::toLowerCase))
.containsExactlyInAnyOrder("path=/", "domain=example.com");
}
According to rfc 6265，server should be able to handle cookies with the same name instead of parsing the first one and ignoring the others. I think it is more reasonable for spring-web to give all values to application and let application decide how to handle multiple values.
By the way, the bottom frameworks Reactor-netty, Jetty, and Tomcat can parse multiple cookie values with the same name in their newer versions, wish spring can support soon.
Related Issues
- https://github.com/reactor/reactor-netty/issues/1641
Comment From: aooohan
I don't think this is an issue with the spring framework.You can debug it at org.springframework.http.server.reactive.AbstractServerHttpRequest#initCookies
Comment From: jsonwan
I don't think this is an issue with the spring framework.You can debug it at org.springframework.http.server.reactive.AbstractServerHttpRequest#initCookies
This method only get the first cookie value when multi cookies with the same name exist, because the implementation class ReactorServerHttpRequest.initCookies() use HttpServerRequest.cookies() to get cookies, which can be replaced by HttpServerRequest.allCookies() to get all the values. Besides, the return type of this method is MultiValueMap, which can be used to save cookies with same name. I guess may be designed so.
Comment From: aooohan
I don't think this is an issue with the spring framework.You can debug it at org.springframework.http.server.reactive.AbstractServerHttpRequest#initCookies
This method only get the first cookie value when multi cookies with the same name exist, because the implementation class ReactorServerHttpRequest.initCookies() use HttpServerRequest.cookies() to get cookies, which can be replaced by HttpServerRequest.allCookies() to get all the values. Besides, the return type of this method is MultiValueMap, which can be used to save cookies with same name. I guess may be designed so.
Yes, you're right, I just took a closer look at the ReactorServerHttpRequest implementation and reactor-netty's HttpServerRequest does provide a way to get all cookies, HttpServerRequest#allCookies().
Comment From: aooohan
@jsonwan Can I propose a pr to solve this issue?
Comment From: jsonwan
@jsonwan Can I propose a pr to solve this issue?
Yes, I tried to solve this problem by using HttpServerRequest.allCookies() instead of cookies() and completing the test cases, however, Jetty, Reactor-netty and Tomcat test cases passed but the undertow test cases failed because undertow does not provide a method to get all cookies. Wish for your help. Related undertow issue: https://issues.redhat.com/browse/UNDERTOW-1676#section-4.2.2
Comment From: aooohan
@jsonwan Can I propose a pr to solve this issue?
Yes, I tried to solve this problem by using HttpServerRequest.allCookies() instead of cookies() and completing the test cases, however, Jetty, Reactor-netty and Tomcat test cases passed but the undertow test cases failed because undertow does not provide a method to get all cookies. Wish for your help. Related undertow issue: https://issues.redhat.com/browse/UNDERTOW-1676#section-4.2.2
Undertow does not offer the ability to parse multiple cookies, there is nothing we can do about it. I think we just need to solve the ReactorServerHttpRequest problem, but of course, I think it's a problem, and it's up to the maintainers of the spring framework to decide if it ends up being what we think it is
Comment From: akvone
We also stumbled upon this. Currently we fixed that by using nativeRequest:
HttpServerRequest nativeRequest = ((AbstractServerHttpRequest) serverWebExchange.getRequest()).getNativeRequest();
Map<CharSequence, List<Cookie>> allCookies = nativeRequest.allCookies();
By the way, it seems that default implementation of HttpServletRequest#getCookies returns all cookies.
Comment From: jhoeller
Note that we are upgrading to the Servlet 6.0 based Undertow 2.3 for Spring Framework 6.0 RC4 (#29435), aiming to retain runtime compatibility with Undertow 2.2. This allows us to rely on the new Undertow 2.2 cookies API, hopefully helping here.
Comment From: sdeleuze
I confirm @aooohan analysis, Undertow handling of multiple cookies with the same name looks incorrect. They may have mistakenly interpreted "e.g., that were set with different Path or Domain attributes" as "i.e. , that were set with different Path or Domain attributes" ("for example" in the spec versus "that is" in their implementation description).
As a consequence, I am going to fix this bug with Reactor Netty and ignore the related test with Undertow (until they fix this bug).