Skip to content

Commit

Permalink
Issue #3421 Duplicate session set-cookie (#3426)
Browse files Browse the repository at this point in the history 
Added Response.replaceCookieuse replaceCookie in sessions
unit tests

Signed-off-by: Greg Wilkins <gregw@webtide.com>
  • Loading branch information
@gregw @janbartel
gregw authored and janbartel committed Mar 6, 2019
1 parent 1175553 commit dbf0d2e
Show file tree
Hide file tree
Showing 7 changed files with 224 additions and 35 deletions.
2 changes: 1 addition & 1 deletion ...-security/src/main/java/org/eclipse/jetty/security/authentication/LoginAuthenticator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ protected HttpSession renewSession(HttpServletRequest request, HttpServletRespon
s.renewId(request);
s.setAttribute(Session.SESSION_CREATED_SECURE, Boolean.TRUE);
if (s.isIdChanged() && (response instanceof Response))
((Response)response).addCookie(s.getSessionHandler().getSessionCookie(s, request.getContextPath(), request.isSecure()));
((Response)response).replaceCookie(s.getSessionHandler().getSessionCookie(s, request.getContextPath(), request.isSecure()));
if (LOG.isDebugEnabled())
LOG.debug("renew {}->{}", oldId, s.getId());
}
Expand Down
4 changes: 2 additions & 2 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1527,7 +1527,7 @@ public String changeSessionId()
if (getRemoteUser() != null)
s.setAttribute(Session.SESSION_CREATED_SECURE, Boolean.TRUE);
if (s.isIdChanged() && _sessionHandler.isUsingCookies())
_channel.getResponse().addCookie(_sessionHandler.getSessionCookie(s, getContextPath(), isSecure()));
_channel.getResponse().replaceCookie(_sessionHandler.getSessionCookie(s, getContextPath(), isSecure()));
}

return session.getId();
Expand Down Expand Up @@ -1570,7 +1570,7 @@ public HttpSession getSession(boolean create)
_session = _sessionHandler.newHttpSession(this);
HttpCookie cookie = _sessionHandler.getSessionCookie(_session,getContextPath(),isSecure());
if (cookie != null)
_channel.getResponse().addCookie(cookie);
_channel.getResponse().replaceCookie(cookie);

return _session;
}
Expand Down
136 changes: 119 additions & 17 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.Iterator;
import java.util.List;
import java.util.ListIterator;
import java.util.Locale;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Supplier;
Expand Down Expand Up @@ -194,6 +196,94 @@ public void addCookie(HttpCookie cookie)
cookie.isHttpOnly());
}

/**
* Replace (or add) a cookie.
* Using name, path and domain, look for a matching set-cookie header and replace it.
* @param cookie The cookie to add/replace
*/
public void replaceCookie(HttpCookie cookie)
{
for (ListIterator<HttpField> i = _fields.listIterator(); i.hasNext();)
{
HttpField field = i.next();

if (field.getHeader() == HttpHeader.SET_COOKIE)
{
String old_set_cookie = field.getValue();
String name = cookie.getName();
if (!old_set_cookie.startsWith(name) || old_set_cookie.length()<= name.length() || old_set_cookie.charAt(name.length())!='=')
continue;

String domain = cookie.getDomain();
if (domain!=null)
{
if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965)
{
StringBuilder buf = new StringBuilder();
buf.append(";Domain=");
quoteOnlyOrAppend(buf,domain,isQuoteNeededForCookie(domain));
domain = buf.toString();
}
else
{
domain = ";Domain="+domain;
}
if (!old_set_cookie.contains(domain))
continue;
}
else if (old_set_cookie.contains(";Domain="))
continue;

String path = cookie.getPath();
if (path!=null)
{
if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance()==CookieCompliance.RFC2965)
{
StringBuilder buf = new StringBuilder();
buf.append(";Path=");
quoteOnlyOrAppend(buf,path,isQuoteNeededForCookie(path));
path = buf.toString();
}
else
{
path = ";Path="+path;
}
if (!old_set_cookie.contains(path))
continue;
}
else if (old_set_cookie.contains(";Path="))
continue;

if (getHttpChannel().getHttpConfiguration().getResponseCookieCompliance() == CookieCompliance.RFC2965)
i.set(new HttpField(HttpHeader.CONTENT_ENCODING.SET_COOKIE, newRFC2965SetCookie(
cookie.getName(),
cookie.getValue(),
cookie.getDomain(),
cookie.getPath(),
cookie.getMaxAge(),
cookie.getComment(),
cookie.isSecure(),
cookie.isHttpOnly(),
cookie.getVersion())
));
else
i.set(new HttpField(HttpHeader.CONTENT_ENCODING.SET_COOKIE, newRFC6265SetCookie(
cookie.getName(),
cookie.getValue(),
cookie.getDomain(),
cookie.getPath(),
cookie.getMaxAge(),
cookie.isSecure(),
cookie.isHttpOnly()
)));
return;
}
}

// Not replaced, so add normally
addCookie(cookie);
}

@Override
public void addCookie(Cookie cookie)
{
Expand Down Expand Up @@ -257,6 +347,18 @@ public void addSetRFC6265Cookie(
final long maxAge,
final boolean isSecure,
final boolean isHttpOnly)
{
String set_cookie = newRFC6265SetCookie(name, value, domain, path, maxAge, isSecure, isHttpOnly);

// add the set cookie
_fields.add(HttpHeader.SET_COOKIE, set_cookie);

// Expire responses with set-cookie headers so they do not get cached.
_fields.put(__EXPIRES_01JAN1970);

}

private String newRFC6265SetCookie(String name, String value, String domain, String path, long maxAge, boolean isSecure, boolean isHttpOnly)
{
// Check arguments
if (name == null || name.length() == 0)
Expand All @@ -272,11 +374,11 @@ public void addSetRFC6265Cookie(
StringBuilder buf = __cookieBuilder.get();
buf.setLength(0);
buf.append(name).append('=').append(value==null?"":value);

// Append path
if (path!=null && path.length()>0)
buf.append(";Path=").append(path);

// Append domain
if (domain!=null && domain.length()>0)
buf.append(";Domain=").append(domain);
Expand All @@ -301,15 +403,9 @@ public void addSetRFC6265Cookie(
buf.append(";Secure");
if (isHttpOnly)
buf.append(";HttpOnly");

// add the set cookie
_fields.add(HttpHeader.SET_COOKIE, buf.toString());

// Expire responses with set-cookie headers so they do not get cached.
_fields.put(__EXPIRES_01JAN1970);

return buf.toString();
}

/**
* Format a set cookie value
*
Expand All @@ -333,6 +429,17 @@ public void addSetRFC2965Cookie(
final boolean isSecure,
final boolean isHttpOnly,
int version)
{
String set_cookie = newRFC2965SetCookie(name, value, domain, path, maxAge, comment, isSecure, isHttpOnly, version);

// add the set cookie
_fields.add(HttpHeader.SET_COOKIE, set_cookie);

// Expire responses with set-cookie headers so they do not get cached.
_fields.put(__EXPIRES_01JAN1970);
}

private String newRFC2965SetCookie(String name, String value, String domain, String path, long maxAge, String comment, boolean isSecure, boolean isHttpOnly, int version)
{
// Check arguments
if (name == null || name.length() == 0)
Expand All @@ -347,7 +454,7 @@ public void addSetRFC2965Cookie(
quoteOnlyOrAppend(buf,name,quote_name);

buf.append('=');

// Append the value
boolean quote_value=isQuoteNeededForCookie(value);
quoteOnlyOrAppend(buf,value,quote_value);
Expand Down Expand Up @@ -413,12 +520,7 @@ else if (version>1)
buf.append(";Comment=");
quoteOnlyOrAppend(buf,comment,isQuoteNeededForCookie(comment));
}

// add the set cookie
_fields.add(HttpHeader.SET_COOKIE, buf.toString());

// Expire responses with set-cookie headers so they do not get cached.
_fields.put(__EXPIRES_01JAN1970);
return buf.toString();
}


Expand Down
2 changes: 1 addition & 1 deletion jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1658,7 +1658,7 @@ public void doScope(String target, Request baseRequest, HttpServletRequest reque
HttpCookie cookie = access(existingSession,request.isSecure());
// Handle changed ID or max-age refresh, but only if this is not a redispatched request
if ((cookie != null) && (request.getDispatcherType() == DispatcherType.ASYNC || request.getDispatcherType() == DispatcherType.REQUEST))
baseRequest.getResponse().addCookie(cookie);
baseRequest.getResponse().replaceCookie(cookie);
}

if (LOG.isDebugEnabled())
Expand Down
37 changes: 35 additions & 2 deletions jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.hamcrest.CoreMatchers.allOf;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
Expand All @@ -37,7 +38,6 @@
import java.io.InputStreamReader;
import java.io.LineNumberReader;
import java.io.PrintWriter;
import java.net.HttpCookie;
import java.net.Inet4Address;
import java.net.InetAddress;
import java.net.InetSocketAddress;
Expand All @@ -59,6 +59,7 @@
import javax.servlet.http.HttpSession;

import org.eclipse.jetty.http.CookieCompliance;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpHeader;
Expand Down Expand Up @@ -1019,7 +1020,7 @@ public void testAddCookie_JavaxServletHttp() throws Exception
@Test
public void testAddCookie_JavaNet() throws Exception
{
HttpCookie cookie = new HttpCookie("foo", URLEncoder.encode("bar;baz", UTF_8.toString()));
java.net.HttpCookie cookie = new java.net.HttpCookie("foo", URLEncoder.encode("bar;baz", UTF_8.toString()));
cookie.setPath("/secure");

assertEquals("foo=\"bar%3Bbaz\";$Path=\"/secure\"", cookie.toString());
Expand Down Expand Up @@ -1061,6 +1062,38 @@ public void testCookiesWithReset() throws Exception
assertFalse(set.hasMoreElements());
}

@Test
public void testReplaceHttpCookie()
{
Response response = getResponse();

response.replaceCookie(new HttpCookie("Foo","123456"));
response.replaceCookie(new HttpCookie("Foo","123456", "A", "/path"));
response.replaceCookie(new HttpCookie("Foo","123456", "B", "/path"));

response.replaceCookie(new HttpCookie("Bar","123456"));
response.replaceCookie(new HttpCookie("Bar","123456",null, "/left"));
response.replaceCookie(new HttpCookie("Bar","123456", null, "/right"));

response.replaceCookie(new HttpCookie("Bar","value", null, "/right"));
response.replaceCookie(new HttpCookie("Bar","value",null, "/left"));
response.replaceCookie(new HttpCookie("Bar","value"));

response.replaceCookie(new HttpCookie("Foo","value", "B", "/path"));
response.replaceCookie(new HttpCookie("Foo","value", "A", "/path"));
response.replaceCookie(new HttpCookie("Foo","value"));

assertThat(Collections.list(response.getHttpFields().getValues("Set-Cookie")),
contains(
"Foo=value",
"Foo=value;Path=/path;Domain=A",
"Foo=value;Path=/path;Domain=B",
"Bar=value",
"Bar=value;Path=/left",
"Bar=value;Path=/right"
));
}

@Test
public void testFlushAfterFullContent() throws Exception
{
Expand Down

0 comments on commit dbf0d2e

Please sign in to comment.