Jetty 12.0.x cookie cache rewrite (#8047)

Cookies from requests on same connection bleed into each other

* Rewrote CookieCache

Co-authored-by: Jan Bartel <janb@webtide.com>
This commit is contained in:
Greg Wilkins 2022-05-24 16:44:53 +10:00 committed by GitHub
parent cb496f7447
commit e72f6c96c4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 395 additions and 62 deletions

View File

@ -14,28 +14,27 @@
package org.eclipse.jetty.http;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.ListIterator;
import org.eclipse.jetty.util.StringUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* Cookie parser
* <p>Optimized stateful cookie parser. Cookies fields are added with the
* {@link #addCookieField(String)} method and parsed on the next subsequent
* call to {@link #getCookies(HttpFields)}.
* <p>Optimized stateful cookie parser.
* If the added fields are identical to those last added (as strings), then the
* cookies are not re parsed.
*
*/
public class CookieCache extends CookieCutter
public class CookieCache
{
protected static final Logger LOG = LoggerFactory.getLogger(CookieCache.class);
protected final List<String> _rawFields = new ArrayList<>();
protected final List<HttpCookie> _cookieList = new ArrayList<>();
private int _addedFields;
private boolean _parsed = false;
private boolean _set = false;
protected List<HttpCookie> _cookieList;
private final CookieCutter _cookieCutter;
public CookieCache()
{
@ -44,75 +43,88 @@ public class CookieCache extends CookieCutter
public CookieCache(CookieCompliance compliance, ComplianceViolation.Listener complianceListener)
{
super(compliance, complianceListener);
}
private void addCookieField(String rawField)
{
if (_set)
throw new IllegalStateException();
if (rawField == null)
return;
rawField = rawField.trim();
if (rawField.length() == 0)
return;
if (_rawFields.size() > _addedFields)
_cookieCutter = new CookieCutter(compliance, complianceListener)
{
if (rawField.equals(_rawFields.get(_addedFields)))
@Override
protected void addCookie(String cookieName, String cookieValue, String cookieDomain, String cookiePath, int cookieVersion, String cookieComment)
{
_addedFields++;
return;
_cookieList.add(new HttpCookie(cookieName, cookieValue));
}
while (_rawFields.size() > _addedFields)
{
_rawFields.remove(_addedFields);
}
}
_rawFields.add(_addedFields++, rawField);
_parsed = false;
};
}
public List<HttpCookie> getCookies(HttpFields headers)
{
// TODO this could be done a lot better with a single iteration and not creating a new list etc.
_set = false;
_addedFields = 0;
boolean building = false;
ListIterator<String> raw = _rawFields.listIterator();
// For each of the headers
for (HttpField field : headers)
{
if (HttpHeader.COOKIE.equals(field.getHeader()))
addCookieField(field.getValue());
// skip non cookie headers
if (!HttpHeader.COOKIE.equals(field.getHeader()))
continue;
// skip blank cookie headers
String value = field.getValue();
if (StringUtil.isBlank(value))
continue;
// If we are building a new cookie list
if (building)
{
// just add the raw string to the list to be parsed later
_rawFields.add(value);
continue;
}
// otherwise we are checking against previous cookies.
// Is there a previous raw cookie to compare with?
if (!raw.hasNext())
{
// No, so we will flip to building state and add to the raw fields we already have.
building = true;
_rawFields.add(value);
continue;
}
// If there is a previous raw cookie and it is the same, then continue checking
if (value.equals(raw.next()))
continue;
// otherwise there is a difference in the previous raw cookie field
// so switch to building mode and remove all subsequent raw fields
// then add the current raw field to be built later.
building = true;
raw.remove();
while (raw.hasNext())
{
raw.next();
raw.remove();
}
_rawFields.add(value);
}
while (_rawFields.size() > _addedFields)
// If we are not building, but there are still more unmatched raw fields, then a field was deleted
if (!building && raw.hasNext())
{
_rawFields.remove(_addedFields);
_parsed = false;
// switch to building mode and delete the unmatched raw fields
building = true;
while (raw.hasNext())
{
raw.next();
raw.remove();
}
}
if (_parsed)
return _cookieList;
// If we ended up in building mode, reparse the cookie list from the raw fields.
if (building)
{
_cookieList = new ArrayList<>();
_cookieCutter.parseFields(_rawFields);
}
parseFields(_rawFields);
_parsed = true;
return _cookieList;
return _cookieList == null ? Collections.emptyList() : _cookieList;
}
@Override
protected void addCookie(String name, String value, String domain, String path, int version, String comment)
{
try
{
// TODO probably should only do name & value now. Version is not longer a thing!
HttpCookie cookie = new HttpCookie(name, value, domain, path, -1, false, false, comment, version);
_cookieList.add(cookie);
}
catch (Exception e)
{
LOG.debug("Unable to add Cookie name={}, value={}, domain={}, path={}, version={}, comment={}",
name, value, domain, path, version, comment, e);
}
}
}

View File

@ -243,6 +243,11 @@ public class HttpCookie
return builder.toString();
}
public String toString()
{
return "%x@%s".formatted(hashCode(), asString());
}
private static void quoteOnlyOrAppend(StringBuilder buf, String s, boolean quote)
{
if (quote)

View File

@ -0,0 +1,178 @@
//
// ========================================================================
// Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//
package org.eclipse.jetty.server;
import java.util.List;
import java.util.ListIterator;
import org.eclipse.jetty.http.CookieCache;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpHeader;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.sameInstance;
public class CookieCacheTest
{
CookieCache _cache;
HttpFields.Mutable _fields;
@BeforeEach
public void prepare() throws Exception
{
_cache = new CookieCache();
_fields = HttpFields.build();
}
@Test
public void testNoFields() throws Exception
{
List<HttpCookie> cookies = _cache.getCookies(_fields.asImmutable());
assertThat(cookies, empty());
cookies = _cache.getCookies(_fields.asImmutable());
assertThat(cookies, empty());
}
@Test
public void testNoCookies() throws Exception
{
_fields.put("Some", "Header");
_fields.put("Other", "header");
List<HttpCookie> cookies = _cache.getCookies(_fields.asImmutable());
assertThat(cookies, empty());
cookies = _cache.getCookies(_fields.asImmutable());
assertThat(cookies, empty());
}
@Test
public void testOneCookie() throws Exception
{
_fields.put("Some", "Header");
_fields.put("Cookie", "name=value");
_fields.put("Other", "header");
List<HttpCookie> cookies0 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies0, hasSize(1));
HttpCookie cookie = cookies0.get(0);
assertThat(cookie.getName(), is("name"));
assertThat(cookie.getValue(), is("value"));
List<HttpCookie> cookies1 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies1, hasSize(1));
assertThat(cookies1, sameInstance(cookies0));
}
@Test
public void testDeleteCookie() throws Exception
{
_fields.put("Some", "Header");
_fields.put("Cookie", "name=value");
_fields.put("Other", "header");
List<HttpCookie> cookies0 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies0, hasSize(1));
HttpCookie cookie = cookies0.get(0);
assertThat(cookie.getName(), is("name"));
assertThat(cookie.getValue(), is("value"));
_fields.remove(HttpHeader.COOKIE);
List<HttpCookie> cookies1 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies1, empty());
assertThat(cookies1, not(sameInstance(cookies0)));
}
@Test
public void testChangedCookie() throws Exception
{
_fields.put("Some", "Header");
_fields.put("Cookie", "name=value");
_fields.put("Other", "header");
List<HttpCookie> cookies0 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies0, hasSize(1));
HttpCookie cookie = cookies0.get(0);
assertThat(cookie.getName(), is("name"));
assertThat(cookie.getValue(), is("value"));
_fields.put(HttpHeader.COOKIE, "name=different");
List<HttpCookie> cookies1 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies1, hasSize(1));
assertThat(cookies1, not(sameInstance(cookies0)));
cookie = cookies1.get(0);
assertThat(cookie.getName(), is("name"));
assertThat(cookie.getValue(), is("different"));
}
@Test
public void testChangedFirstCookie() throws Exception
{
_fields.put("Some", "Header");
_fields.put("Cookie", "name=value");
_fields.put("Other", "header");
_fields.add(HttpHeader.COOKIE, "other=different");
List<HttpCookie> cookies0 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies0, hasSize(2));
HttpCookie cookie = cookies0.get(0);
assertThat(cookie.getName(), is("name"));
assertThat(cookie.getValue(), is("value"));
cookie = cookies0.get(1);
assertThat(cookie.getName(), is("other"));
assertThat(cookie.getValue(), is("different"));
ListIterator<HttpField> i = _fields.listIterator();
i.next();
i.next();
i.set(new HttpField("Cookie", "Name=Changed"));
List<HttpCookie> cookies1 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies1, hasSize(2));
assertThat(cookies1, not(sameInstance(cookies0)));
cookie = cookies1.get(0);
assertThat(cookie.getName(), is("Name"));
assertThat(cookie.getValue(), is("Changed"));
cookie = cookies0.get(1);
assertThat(cookie.getName(), is("other"));
assertThat(cookie.getValue(), is("different"));
}
@Test
public void testAddCookie() throws Exception
{
_fields.put("Some", "Header");
_fields.put("Cookie", "name=value");
_fields.put("Other", "header");
List<HttpCookie> cookies0 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies0, hasSize(1));
HttpCookie cookie = cookies0.get(0);
assertThat(cookie.getName(), is("name"));
assertThat(cookie.getValue(), is("value"));
_fields.add(HttpHeader.COOKIE, "other=different");
List<HttpCookie> cookies1 = _cache.getCookies(_fields.asImmutable());
assertThat(cookies1, hasSize(2));
assertThat(cookies1, not(sameInstance(cookies0)));
cookie = cookies0.get(0);
assertThat(cookie.getName(), is("name"));
assertThat(cookie.getValue(), is("value"));
cookie = cookies1.get(1);
assertThat(cookie.getName(), is("other"));
assertThat(cookie.getValue(), is("different"));
}
}

View File

@ -13,9 +13,17 @@
package org.eclipse.jetty.server;
import java.io.ByteArrayOutputStream;
import java.nio.ByteBuffer;
import java.util.List;
import org.eclipse.jetty.http.HttpCookie;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpTester;
import org.eclipse.jetty.server.LocalConnector.LocalEndPoint;
import org.eclipse.jetty.server.handler.DumpHandler;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.component.LifeCycle;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
@ -23,7 +31,9 @@ import org.junit.jupiter.api.Test;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.not;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
public class RequestTest
{
@ -77,4 +87,71 @@ public class RequestTest
assertThat(response.getContent(), containsString("httpURI.path=/fo%6f%2fbar"));
assertThat(response.getContent(), containsString("pathInContext=/foo%2Fbar"));
}
/**
* Test that multiple requests on the same connection with different cookies
* do not bleed cookies.
*
* @throws Exception if there is a problem
*/
@Test
public void testDifferentCookies() throws Exception
{
server.stop();
server.setHandler(new Handler.Processor()
{
@Override
public void process(org.eclipse.jetty.server.Request request, Response response, Callback callback) throws Exception
{
response.setStatus(200);
response.setContentType("text/plain");
ByteArrayOutputStream buff = new ByteArrayOutputStream();
request.getHeaders().getFields(HttpHeader.COOKIE).forEach(System.err::println);
List<HttpCookie> coreCookies = org.eclipse.jetty.server.Request.getCookies(request);
if (coreCookies != null)
{
for (HttpCookie c : coreCookies)
buff.writeBytes(("Core Cookie: " + c.getName() + "=" + c.getValue() + "\n").getBytes());
}
response.write(true, callback, ByteBuffer.wrap(buff.toByteArray()));
}
});
server.start();
String sessionId1 = "JSESSIONID=node0o250bm47otmz1qjqqor54fj6h0.node0";
String sessionId2 = "JSESSIONID=node0q4z00xb0pnyl1f312ec6e93lw1.node0";
String sessionId3 = "JSESSIONID=node0gqgmw5fbijm0f9cid04b4ssw2.node0";
String request1 = "GET /ctx HTTP/1.1\r\nHost: localhost\r\nCookie: " + sessionId1 + "\r\n\r\n";
String request2 = "GET /ctx HTTP/1.1\r\nHost: localhost\r\nCookie: " + sessionId2 + "\r\n\r\n";
String request3 = "GET /ctx HTTP/1.1\r\nHost: localhost\r\nCookie: " + sessionId3 + "\r\n\r\n";
try (LocalEndPoint lep = connector.connect())
{
lep.addInput(request1);
HttpTester.Response response = HttpTester.parseResponse(lep.getResponse());
checkCookieResult(sessionId1, new String[]{sessionId2, sessionId3}, response.getContent());
lep.addInput(request2);
response = HttpTester.parseResponse(lep.getResponse());
checkCookieResult(sessionId2, new String[]{sessionId1, sessionId3}, response.getContent());
lep.addInput(request3);
response = HttpTester.parseResponse(lep.getResponse());
checkCookieResult(sessionId3, new String[]{sessionId1, sessionId2}, response.getContent());
}
}
private static void checkCookieResult(String containedCookie, String[] notContainedCookies, String response)
{
assertNotNull(containedCookie);
assertNotNull(response);
assertThat(response, containsString("Core Cookie: " + containedCookie));
if (notContainedCookies != null)
{
for (String notContainsCookie : notContainedCookies)
{
assertThat(response, not(containsString(notContainsCookie)));
}
}
}
}

View File

@ -1630,7 +1630,54 @@ public class RequestTest
assertEquals("value", cookies[0]);
assertNull(cookies[1]);
}
/**
* Test that multiple requests on the same connection with different cookies
* do not bleed cookies.
*
* @throws Exception
*/
@Test
public void testDifferentCookies() throws Exception
{
_server.stop();
_context.setHandler(new AbstractHandler()
{
@Override
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException
{
baseRequest.setHandled(true);
response.setStatus(200);
response.setContentType("text/plain");
List<HttpCookie> coreCookies = org.eclipse.jetty.server.Request.getCookies(baseRequest.getCoreRequest());
if (coreCookies != null)
{
for (HttpCookie c : coreCookies)
response.getOutputStream().println("Core Cookie: " + c.getName() + "=" + c.getValue());
}
}
});
_server.start();
String sessionId1 = "JSESSIONID=node0o250bm47otmz1qjqqor54fj6h0.node0";
String sessionId2 = "JSESSIONID=node0q4z00xb0pnyl1f312ec6e93lw1.node0";
String sessionId3 = "JSESSIONID=node0gqgmw5fbijm0f9cid04b4ssw2.node0";
String request1 = "GET /ctx HTTP/1.1\r\nHost: localhost\r\nCookie: " + sessionId1 + "\r\n\r\n";
String request2 = "GET /ctx HTTP/1.1\r\nHost: localhost\r\nCookie: " + sessionId2 + "\r\n\r\n";
String request3 = "GET /ctx HTTP/1.1\r\nHost: localhost\r\nCookie: " + sessionId3 + "\r\n\r\n";
LocalEndPoint lep = _connector.connect();
lep.addInput(request1);
HttpTester.Response response = HttpTester.parseResponse(lep.getResponse());
checkCookieResult(sessionId1, new String[] {sessionId2, sessionId3}, response.getContent());
lep.addInput(request2);
response = HttpTester.parseResponse(lep.getResponse());
checkCookieResult(sessionId2, new String[] {sessionId1, sessionId3}, response.getContent());
lep.addInput(request3);
response = HttpTester.parseResponse(lep.getResponse());
checkCookieResult(sessionId3, new String[] {sessionId1, sessionId2}, response.getContent());
}
@Test
public void testHashDOSKeys() throws Exception
{
@ -2269,4 +2316,18 @@ public class RequestTest
}
}
private static void checkCookieResult(String containedCookie, String[] notContainedCookies, String response)
{
assertNotNull(containedCookie);
assertNotNull(response);
assertThat(response, containsString("Core Cookie: " + containedCookie));
if (notContainedCookies != null)
{
for (String notContainsCookie : notContainedCookies)
{
assertThat(response, not(containsString(notContainsCookie)));
}
}
}
}