Skip to content

Commit

Permalink
Fixes #10219 - Review HTTP Cookie parsing (#10464)
Browse files Browse the repository at this point in the history 
* Added SetCookieParser interface and RFC6265SetCookieParser implementation to properly parse Set-Cookie values.
* Removed hacky implementation in HttpClient.
* Removed unused methods in HttpCookieUtils.
* Using SetCookieParser for the implementation of newPushBuilder in ee9,ee10.
* Reworked HttpCookieStore.Default implementation.
* Implemented properly cookie path resolution.
* Using URI.getRawPath() to resolve cookie paths.
* Removed secure vs. non-secure scheme distinction when storing cookies.
* Refactored common code in HttpCookieStore.Default to avoid duplications.

Signed-off-by: Simone Bordet <[email protected]>
  • Loading branch information
@sbordet
sbordet authored Sep 14, 2023
1 parent 388d3e3 commit 530ed33
Show file tree
Hide file tree
Showing 15 changed files with 885 additions and 401 deletions.
90 changes: 5 additions & 85 deletions jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,6 @@

package org.eclipse.jetty.client;

import java.io.IOException;
import java.net.CookieManager;
import java.net.CookiePolicy;
import java.net.CookieStore;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.net.SocketAddress;
Expand All @@ -25,7 +21,6 @@
import java.nio.channels.SocketChannel;
import java.time.Duration;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand All @@ -50,6 +45,7 @@
import org.eclipse.jetty.http.HttpMethod;
import org.eclipse.jetty.http.HttpParser;
import org.eclipse.jetty.http.HttpScheme;
import org.eclipse.jetty.http.SetCookieParser;
import org.eclipse.jetty.io.ArrayByteBufferPool;
import org.eclipse.jetty.io.ByteBufferPool;
import org.eclipse.jetty.io.ClientConnectionFactory;
Expand Down Expand Up @@ -113,6 +109,7 @@ public class HttpClient extends ContainerLifeCycle
{
public static final String USER_AGENT = "Jetty/" + Jetty.VERSION;
private static final Logger LOG = LoggerFactory.getLogger(HttpClient.class);
private static final SetCookieParser COOKIE_PARSER = SetCookieParser.newInstance();

private final ConcurrentMap<Origin, HttpDestination> destinations = new ConcurrentHashMap<>();
private final ProtocolHandlers handlers = new ProtocolHandlers();
Expand All @@ -123,7 +120,6 @@ public class HttpClient extends ContainerLifeCycle
private final ClientConnector connector;
private AuthenticationStore authenticationStore = new HttpAuthenticationStore();
private HttpCookieStore cookieStore;
private HttpCookieParser cookieParser;
private SocketAddressResolver resolver;
private HttpField agentField = new HttpField(HttpHeader.USER_AGENT, USER_AGENT);
private boolean followRedirects = true;
Expand Down Expand Up @@ -221,7 +217,6 @@ protected void doStart() throws Exception

if (cookieStore == null)
cookieStore = new HttpCookieStore.Default();
cookieParser = new HttpCookieParser();

transport.setHttpClient(this);

Expand Down Expand Up @@ -284,17 +279,9 @@ public void setHttpCookieStore(HttpCookieStore cookieStore)

public void putCookie(URI uri, HttpField field)
{
try
{
HttpCookie cookie = cookieParser.parse(uri, field);
if (cookie != null)
cookieStore.add(uri, cookie);
}
catch (IOException x)
{
if (LOG.isDebugEnabled())
LOG.debug("Unable to store cookies {} from {}", field, uri, x);
}
HttpCookie cookie = COOKIE_PARSER.parse(field.getValue());
if (cookie != null)
cookieStore.add(uri, cookie);
}

/**
Expand Down Expand Up @@ -1102,71 +1089,4 @@ public ClientConnectionFactory newSslClientConnectionFactory(SslContextFactory.C
sslContextFactory = getSslContextFactory();
return new SslClientConnectionFactory(sslContextFactory, getByteBufferPool(), getExecutor(), connectionFactory);
}

private static class HttpCookieParser extends CookieManager
{
public HttpCookieParser()
{
super(new Store(), CookiePolicy.ACCEPT_ALL);
}

public HttpCookie parse(URI uri, HttpField field) throws IOException
{
// TODO: hacky implementation waiting for a real HttpCookie parser.
String value = field.getValue();
if (value == null)
return null;
Map<String, List<String>> header = new HashMap<>(1);
header.put(field.getHeader().asString(), List.of(value));
put(uri, header);
Store store = (Store)getCookieStore();
HttpCookie cookie = store.cookie;
store.cookie = null;
return cookie;
}

private static class Store implements CookieStore
{
private HttpCookie cookie;

@Override
public void add(URI uri, java.net.HttpCookie cookie)
{
String domain = cookie.getDomain();
if ("localhost.local".equals(domain))
cookie.setDomain("localhost");
this.cookie = HttpCookie.from(cookie);
}

@Override
public List<java.net.HttpCookie> get(URI uri)
{
throw new UnsupportedOperationException();
}

@Override
public List<java.net.HttpCookie> getCookies()
{
throw new UnsupportedOperationException();
}

@Override
public List<URI> getURIs()
{
throw new UnsupportedOperationException();
}

@Override
public boolean remove(URI uri, java.net.HttpCookie cookie)
{
throw new UnsupportedOperationException();
}

@Override
public boolean removeAll()
{
throw new UnsupportedOperationException();
}
}
}
}
62 changes: 59 additions & 3 deletions jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/HttpCookieTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,8 @@ protected void service(Request request, org.eclipse.jetty.server.Response respon
List<HttpCookie> cookies = Request.getCookies(request);
switch (target)
{
case "/", "/foo", "/foobar" -> assertEquals(0, cookies.size(), target);
case "/foo/", "/foo/bar", "/foo/bar/baz" ->
case "/", "/foobar" -> assertEquals(0, cookies.size(), target);
case "/foo", "/foo/", "/foo/bar", "/foo/bar/", "/foo/bar/baz" ->
{
assertEquals(1, cookies.size(), target);
HttpCookie cookie = cookies.get(0);
Expand All @@ -263,7 +263,63 @@ protected void service(Request request, org.eclipse.jetty.server.Response respon
.timeout(5, TimeUnit.SECONDS));
assertEquals(HttpStatus.OK_200, response.getStatus());

Arrays.asList("/", "/foo", "/foo/", "/foobar", "/foo/bar", "/foo/bar/baz").forEach(path ->
Arrays.asList("/", "/foo", "/foo/", "/foobar", "/foo/bar", "/foo/bar/", "/foo/bar/baz").forEach(path ->
{
ContentResponse r = send(client.newRequest("localhost", connector.getLocalPort())
.scheme(scenario.getScheme())
.path(path)
.headers(headers -> headers.put(headerName, "1"))
.timeout(5, TimeUnit.SECONDS));
assertEquals(HttpStatus.OK_200, r.getStatus());
});
}

@ParameterizedTest
@ArgumentsSource(ScenarioProvider.class)
public void testSetCookieWithoutPathRequestURIWithTwoSegmentsEndingWithSlash(Scenario scenario) throws Exception
{
String headerName = "X-Request";
String cookieName = "a";
String cookieValue = "1";
start(scenario, new EmptyServerHandler()
{
@Override
protected void service(Request request, org.eclipse.jetty.server.Response response)
{
String target = Request.getPathInContext(request);
int r = (int)request.getHeaders().getLongField(headerName);
if ("/foo/bar/".equals(target) && r == 0)
{
HttpCookie cookie = HttpCookie.from(cookieName, cookieValue);
org.eclipse.jetty.server.Response.addCookie(response, cookie);
}
else
{
List<HttpCookie> cookies = Request.getCookies(request);
switch (target)
{
case "/", "/foo", "/foo/", "/foobar" -> assertEquals(0, cookies.size(), target);
case "/foo/bar", "/foo/bar/", "/foo/bar/baz" ->
{
assertEquals(1, cookies.size(), target);
HttpCookie cookie = cookies.get(0);
assertEquals(cookieName, cookie.getName(), target);
assertEquals(cookieValue, cookie.getValue(), target);
}
default -> fail("Unrecognized Target: " + target);
}
}
}
});

ContentResponse response = send(client.newRequest("localhost", connector.getLocalPort())
.scheme(scenario.getScheme())
.path("/foo/bar/")
.headers(headers -> headers.put(headerName, "0"))
.timeout(5, TimeUnit.SECONDS));
assertEquals(HttpStatus.OK_200, response.getStatus());

Arrays.asList("/", "/foo", "/foo/", "/foobar", "/foo/bar", "/foo/bar/", "/foo/bar/baz").forEach(path ->
{
ContentResponse r = send(client.newRequest("localhost", connector.getLocalPort())
.scheme(scenario.getScheme())
Expand Down
9 changes: 6 additions & 3 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,12 @@ public void addCookie(String cookieName, String cookieValue, int cookieVersion,
else
{
Map<String, String> attributes = new HashMap<>();
attributes.put(HttpCookie.DOMAIN_ATTRIBUTE, cookieDomain);
attributes.put(HttpCookie.PATH_ATTRIBUTE, cookiePath);
attributes.put(HttpCookie.COMMENT_ATTRIBUTE, cookieComment);
if (!StringUtil.isEmpty(cookieDomain))
attributes.put(HttpCookie.DOMAIN_ATTRIBUTE, cookieDomain);
if (!StringUtil.isEmpty(cookiePath))
attributes.put(HttpCookie.PATH_ATTRIBUTE, cookiePath);
if (!StringUtil.isEmpty(cookieComment))
attributes.put(HttpCookie.COMMENT_ATTRIBUTE, cookieComment);
_cookieList.add(HttpCookie.from(cookieName, cookieValue, cookieVersion, attributes));
}
}
Expand Down
61 changes: 43 additions & 18 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ class Wrapper implements HttpCookie

public Wrapper(HttpCookie wrapped)
{
this.wrapped = wrapped;
this.wrapped = Objects.requireNonNull(wrapped);
}

public HttpCookie getWrapped()
Expand Down Expand Up @@ -534,10 +534,42 @@ private Builder(String name, String value, int version)

public Builder attribute(String name, String value)
{
_attributes = lazyAttributePut(_attributes, name, value);
if (name == null)
return this;
// Sanity checks on the values, expensive but necessary to avoid to store garbage.
switch (name.toLowerCase(Locale.ENGLISH))
{
case "expires" -> expires(parseExpires(value));
case "httponly" ->
{
if (!isTruthy(value))
throw new IllegalArgumentException("Invalid HttpOnly attribute");
httpOnly(true);
}
case "max-age" -> maxAge(Long.parseLong(value));
case "samesite" ->
{
SameSite sameSite = SameSite.from(value);
if (sameSite == null)
throw new IllegalArgumentException("Invalid SameSite attribute");
sameSite(sameSite);
}
case "secure" ->
{
if (!isTruthy(value))
throw new IllegalArgumentException("Invalid Secure attribute");
secure(true);
}
default -> _attributes = lazyAttributePut(_attributes, name, value);
}
return this;
}

private boolean isTruthy(String value)
{
return value != null && (value.isEmpty() || "true".equalsIgnoreCase(value));
}

public Builder comment(String comment)
{
_attributes = lazyAttributePut(_attributes, COMMENT_ATTRIBUTE, comment);
Expand Down Expand Up @@ -818,26 +850,19 @@ private static boolean equalsIgnoreCase(String obj1, String obj2)
return obj1.equalsIgnoreCase(obj2);
}

/**
* <p>Formats this cookie into a string suitable to be used
* in {@code Cookie} or {@code Set-Cookie} headers.</p>
*
* @param httpCookie the cookie to format
* @return a header string representation of the cookie
*/
private static String asString(HttpCookie httpCookie)
{
StringBuilder builder = new StringBuilder();
builder.append(httpCookie.getName()).append("=").append(httpCookie.getValue());
int version = httpCookie.getVersion();
if (version > 0)
builder.append(";Version=").append(version);
String domain = httpCookie.getDomain();
if (domain != null)
builder.append(";Domain=").append(domain);
String path = httpCookie.getPath();
if (path != null)
builder.append(";Path=").append(path);
Map<String, String> attributes = httpCookie.getAttributes();
if (!attributes.isEmpty())
{
for (Map.Entry<String, String> entry : attributes.entrySet())
{
builder.append("; ");
builder.append(entry.getKey()).append("=").append(entry.getValue());
}
}
return builder.toString();
}

Expand Down
Loading

0 comments on commit 530ed33

Please sign in to comment.