Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #3433 - Multiple cookies with same name are not supported #4390

Merged
merged 6 commits into from
Feb 18, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2019 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2020 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand Down Expand Up @@ -70,10 +70,7 @@ public static Map<String, Cookie> parseCookies(String header) {
value = value.substring(1, value.length() - 1);
}
if (!name.startsWith("$")) {
if (cookie != null) {
cookies.put(cookie.name, cookie.getImmutableCookie());
}

checkSimilarCookieName(cookies, cookie);
cookie = new MutableCookie(name, value);
cookie.version = version;
} else if (name.startsWith("$Version")) {
Expand All @@ -84,10 +81,26 @@ public static Map<String, Cookie> parseCookies(String header) {
cookie.domain = value;
}
}
checkSimilarCookieName(cookies, cookie);
return cookies;
}

/**
* Check if a cookie with identical name had been parsed.
* If yes, the one with the longest string will be kept
* @param cookies : Map of cookies
* @param cookie : Cookie to be checked
*/
private static void checkSimilarCookieName(Map<String, Cookie> cookies, MutableCookie cookie) {
if (cookie != null) {
cookies.put(cookie.name, cookie.getImmutableCookie());
if (cookies.containsKey(cookie.name)){
if (cookie.value.length() > cookies.get(cookie.name).getValue().length()){
cookies.put(cookie.name, cookie.getImmutableCookie());
}
} else {
cookies.put(cookie.name, cookie.getImmutableCookie());
}
}
return cookies;
}

public static Cookie parseCookie(String header) {
Expand Down Expand Up @@ -148,8 +161,6 @@ public static NewCookie parseNewCookie(String header) {
cookie.secure = true;
} else if (param.startsWith("version")) {
cookie.version = Integer.parseInt(value);
} else if (param.startsWith("domain")) {
cookie.domain = value;
} else if (param.startsWith("httponly")) {
cookie.httpOnly = true;
} else if (param.startsWith("expires")) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2019 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2020 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand Down Expand Up @@ -29,6 +29,7 @@
import javax.ws.rs.core.AbstractMultivaluedMap;
import javax.ws.rs.core.Configuration;
import javax.ws.rs.core.MultivaluedMap;
import javax.ws.rs.core.NewCookie;
import javax.ws.rs.ext.RuntimeDelegate;
import javax.ws.rs.ext.RuntimeDelegate.HeaderDelegate;

Expand Down Expand Up @@ -298,6 +299,24 @@ public static void checkHeaderChanges(final Map<String, String> headersSnapshot,
}
}

/**
* Compare two NewCookies having the same name. See documentation RFC.
*
* @param one NewCookie to be compared.
* @param second NewCookie to be compared.
* @return the prefered NewCookie according to rules :
* - the latest expiration date.
* - if same expiration date, the longest path.
*/
public static NewCookie getPreferedNewCookie(NewCookie one, NewCookie second) {
jansupol marked this conversation as resolved.
Show resolved Hide resolved

if (!one.getExpiry().equals(second.getExpiry())){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expiry date can be NULL. Isn't it better to use maxAge value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference between expiry and maxAge.
IE does not support maxAge, so most of the servers set expiry rather than maxAge. It is possible to set one or the other.
NewCookie is what comes from the server, and even though maxAge is recommended, expires seems to be more often. But it seems to be one or the other, so we should compare the later from both.

But yes, we should check for NULL, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well ,regarding check of expiry VS max-age it's good to check them both :)

Regarding setting one or another attribute in request - actually server shall put both. For now I've found that the only browser which does not support max-age attribute is IE.

Per RFC 6265 max-age attribute has precedence over expiry attribute if both are present in request. In my opinion this shall be respected while choosing preferred cookie.

return one.getExpiry().after(second.getExpiry()) ? one : second;
} else {
return one.getPath().length() > second.getPath().length() ? one : second;
}
}

/**
* Convert a message header value, represented as a general object, to it's
* string representation. If the supplied header value is {@code null},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012, 2019 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2020 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand Down Expand Up @@ -587,7 +587,12 @@ public Map<String, NewCookie> getResponseCookies() {
for (String cookie : cookies) {
if (cookie != null) {
NewCookie newCookie = HttpHeaderReader.readNewCookie(cookie);
result.put(newCookie.getName(), newCookie);
String cookieName = newCookie.getName();
if (result.containsKey(cookieName)) {
result.put(cookieName, HeaderUtils.getPreferedNewCookie(result.get(cookieName), newCookie));
} else {
result.put(cookieName, newCookie);
}
}
}
return result;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012, 2019 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2020 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand Down Expand Up @@ -456,7 +456,12 @@ public Map<String, NewCookie> getResponseCookies() {
for (String cookie : HeaderUtils.asStringList(cookies, configuration)) {
if (cookie != null) {
NewCookie newCookie = HttpHeaderReader.readNewCookie(cookie);
result.put(newCookie.getName(), newCookie);
String cookieName = newCookie.getName();
if (result.containsKey(cookieName)) {
result.put(cookieName, HeaderUtils.getPreferedNewCookie(result.get(cookieName), newCookie));
} else {
result.put(cookieName, newCookie);
}
}
}
return result;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2013, 2019 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2013, 2020 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand All @@ -17,16 +17,20 @@
package org.glassfish.jersey.tests.e2e.common.message.internal;

import java.net.URI;
import java.text.ParseException;
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.LinkedList;
import java.util.List;

import javax.ws.rs.core.AbstractMultivaluedMap;
import javax.ws.rs.core.MultivaluedMap;
import javax.ws.rs.core.NewCookie;
import javax.ws.rs.ext.RuntimeDelegate;

import org.glassfish.jersey.message.internal.HeaderUtils;
import org.glassfish.jersey.message.internal.HttpDateFormat;
import org.glassfish.jersey.tests.e2e.common.TestRuntimeDelegate;

import org.junit.Test;
Expand Down Expand Up @@ -180,4 +184,97 @@ public void testAsHeaderString() throws Exception {
final String result = HeaderUtils.asHeaderString(values, null);
assertEquals("value,[null]," + uri.toASCIIString(), result);
}

@Test
public void testgetPreferedNewCookie(){

Date earliestDate;
Date latestDate;

try {
earliestDate = HttpDateFormat.readDate("Fri, 1 Jan 2020 00:00:00 GMT");
jansupol marked this conversation as resolved.
Show resolved Hide resolved
latestDate = HttpDateFormat.readDate("Fri, 1 Jan 2020 01:00:00 GMT");
} catch (ParseException e) {
e.printStackTrace();
earliestDate = new Date(10);
latestDate = new Date(100);
}

NewCookie one = new NewCookie(
"fred",
"valuestring",
"pathstring",
"domainstring",
0,
"commentstring",
0,
latestDate,
false,
false);
NewCookie second = new NewCookie(
"fred",
"valuestring",
"pathstring",
"domainstring",
0,
"commentstring",
0,
earliestDate,
false,
false);

assertEquals(one, HeaderUtils.getPreferedNewCookie(one, second));

NewCookie longPathNewCookie = new NewCookie(
"fred",
"valuestring",
"longestpathstring",
"domainstring",
0,
"commentstring",
0,
latestDate,
false,
false);
NewCookie shortPathNewCookie = new NewCookie(
"fred",
"valuestring",
"shortestpath",
"domainstring",
0,
"commentstring",
0,
latestDate,
false,
false);

assertEquals(longPathNewCookie, HeaderUtils.getPreferedNewCookie(longPathNewCookie, shortPathNewCookie));

NewCookie identicalNewCookie = new NewCookie(
"fred",
"valuestring",
"pathstring",
"domainstring",
0,
"commentstring",
0,
latestDate,
false,
false);
NewCookie identicalNewCookie1 = new NewCookie(
"fred",
"valuestring",
"pathstring",
"domainstring",
0,
"commentstring",
0,
latestDate,
false,
false);

assertEquals(identicalNewCookie, HeaderUtils.getPreferedNewCookie(identicalNewCookie, identicalNewCookie1));

}

}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, 2018 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2020 Oracle and/or its affiliates. All rights reserved.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License v. 2.0, which is available at
Expand Down Expand Up @@ -148,6 +148,32 @@ public void testCreateCookies() {
assertTrue(".sun.com".equals(c.getDomain()));
}

@Test
public void testMultipleCookiesWithSameName(){

String cookieHeader = "kobe=longeststring; kobe=shortstring";
Map<String, Cookie> cookies = HttpHeaderReader.readCookies(cookieHeader);
assertEquals(cookies.size(), 1);
Cookie c = cookies.get("kobe");
assertEquals(c.getVersion(), 0);
assertEquals("kobe", c.getName());
assertEquals("longeststring", c.getValue());

cookieHeader = "bryant=longeststring; bryant=shortstring; fred=shortstring ;fred=longeststring;$Path=/path";
cookies = HttpHeaderReader.readCookies(cookieHeader);
assertEquals(cookies.size(), 2);
c = cookies.get("bryant");
assertEquals(c.getVersion(), 0);
assertEquals("bryant", c.getName());
assertEquals("longeststring", c.getValue());
c = cookies.get("fred");
assertEquals(c.getVersion(), 0);
assertEquals("fred", c.getName());
assertEquals("longeststring", c.getValue());
assertEquals("/path", c.getPath());

}

@Test
public void testNewCookieToString() {
NewCookie cookie = new NewCookie("fred", "flintstone");
Expand Down