diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java index 506a5128da1..7f3ca6a6a47 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java @@ -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; @@ -25,7 +21,6 @@ import java.nio.channels.SelectionKey; 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; @@ -50,6 +45,7 @@ import org.eclipse.jetty.http.HttpHeader; 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; @@ -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 destinations = new ConcurrentHashMap<>(); private final ProtocolHandlers handlers = new ProtocolHandlers(); @@ -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; @@ -221,7 +217,6 @@ public class HttpClient extends ContainerLifeCycle if (cookieStore == null) cookieStore = new HttpCookieStore.Default(); - cookieParser = new HttpCookieParser(); transport.setHttpClient(this); @@ -284,17 +279,9 @@ public class HttpClient extends ContainerLifeCycle 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); } /** @@ -1102,71 +1089,4 @@ public class HttpClient extends ContainerLifeCycle 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> 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 get(URI uri) - { - throw new UnsupportedOperationException(); - } - - @Override - public List getCookies() - { - throw new UnsupportedOperationException(); - } - - @Override - public List getURIs() - { - throw new UnsupportedOperationException(); - } - - @Override - public boolean remove(URI uri, java.net.HttpCookie cookie) - { - throw new UnsupportedOperationException(); - } - - @Override - public boolean removeAll() - { - throw new UnsupportedOperationException(); - } - } - } } diff --git a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/HttpCookieTest.java b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/HttpCookieTest.java index b1ef104d007..9b4df45be56 100644 --- a/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/HttpCookieTest.java +++ b/jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/HttpCookieTest.java @@ -242,8 +242,8 @@ public class HttpCookieTest extends AbstractHttpClientServerTest List 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); @@ -263,7 +263,63 @@ public class HttpCookieTest extends AbstractHttpClientServerTest .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 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()) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCache.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCache.java index ec04f976b23..f6c37d860af 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCache.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCache.java @@ -56,9 +56,12 @@ public class CookieCache implements CookieParser.Handler else { Map 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)); } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java index e892ddc0d9b..763a4f2da6d 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java @@ -174,7 +174,7 @@ public interface HttpCookie public Wrapper(HttpCookie wrapped) { - this.wrapped = wrapped; + this.wrapped = Objects.requireNonNull(wrapped); } public HttpCookie getWrapped() @@ -534,10 +534,42 @@ public interface HttpCookie 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); @@ -818,26 +850,19 @@ public interface HttpCookie return obj1.equalsIgnoreCase(obj2); } - /** - *

Formats this cookie into a string suitable to be used - * in {@code Cookie} or {@code Set-Cookie} headers.

- * - * @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 attributes = httpCookie.getAttributes(); + if (!attributes.isEmpty()) + { + for (Map.Entry entry : attributes.entrySet()) + { + builder.append("; "); + builder.append(entry.getKey()).append("=").append(entry.getValue()); + } + } return builder.toString(); } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookieStore.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookieStore.java index f54ec29926b..1e27eb3efa2 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookieStore.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookieStore.java @@ -23,8 +23,11 @@ import java.util.Iterator; import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; +import java.util.function.Predicate; import org.eclipse.jetty.util.NanoTime; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.thread.AutoLock; /** @@ -121,85 +124,106 @@ public interface HttpCookieStore public static class Default implements HttpCookieStore { private final AutoLock lock = new AutoLock(); - private final Map> cookies = new HashMap<>(); + private final Map> cookies = new HashMap<>(); @Override public boolean add(URI uri, HttpCookie cookie) { // TODO: reject if cookie size is too big? - boolean secure = HttpScheme.isSecure(uri.getScheme()); - // Do not accept a secure cookie sent over an insecure channel. - if (cookie.isSecure() && !secure) + String resolvedDomain = resolveDomain(uri, cookie); + if (resolvedDomain == null) return false; - String cookieDomain = cookie.getDomain(); - if (cookieDomain != null) - { - cookieDomain = cookieDomain.toLowerCase(Locale.ENGLISH); - if (cookieDomain.startsWith(".")) - cookieDomain = cookieDomain.substring(1); - // RFC 6265 section 4.1.2.3, ignore Domain if ends with ".". - if (cookieDomain.endsWith(".")) - cookieDomain = uri.getHost(); - // Reject top-level domains. - // TODO: should also reject "top" domain such as co.uk, gov.au, etc. - if (!cookieDomain.contains(".")) - { - if (!cookieDomain.equals("localhost")) - return false; - } + String resolvedPath = resolvePath(uri, cookie); - String domain = uri.getHost(); - if (domain != null) - { - domain = domain.toLowerCase(Locale.ENGLISH); - // If uri.host==foo.example.com, only accept - // cookie.domain==(foo.example.com|example.com). - if (!domain.endsWith(cookieDomain)) - return false; - int beforeMatch = domain.length() - cookieDomain.length() - 1; - if (beforeMatch >= 0 && domain.charAt(beforeMatch) != '.') - return false; - } - } - else - { - // No explicit cookie domain, use the origin domain. - cookieDomain = uri.getHost(); - } - - // Cookies are stored under their domain, so that: - // - add(sub.example.com, cookie[Domain]=null) => Key[domain=sub.example.com] - // - add(sub.example.com, cookie[Domain]=example.com) => Key[domain=example.com] + // Cookies are stored under their resolved domain, so that: + // - add(sub.example.com, cookie[Domain]=null) => key=sub.example.com + // - add(sub.example.com, cookie[Domain]=example.com) => key=example.com // This facilitates the matching algorithm. - Key key = new Key(secure, cookieDomain); - boolean[] result = new boolean[]{true}; + boolean[] added = new boolean[1]; + StoredHttpCookie storedCookie = new StoredHttpCookie(cookie, uri, resolvedDomain, resolvedPath); try (AutoLock ignored = lock.lock()) { + String key = resolvedDomain.toLowerCase(Locale.ENGLISH); cookies.compute(key, (k, v) -> { // RFC 6265, section 4.1.2. // Evict an existing cookie with // same name, domain and path. if (v != null) - v.remove(cookie); + v.remove(storedCookie); // Add only non-expired cookies. if (cookie.isExpired()) - { - result[0] = false; return v == null || v.isEmpty() ? null : v; - } + added[0] = true; if (v == null) v = new ArrayList<>(); - v.add(new Cookie(cookie)); + v.add(storedCookie); return v; }); } - return result[0]; + return added[0]; + } + + private String resolveDomain(URI uri, HttpCookie cookie) + { + String uriDomain = uri.getHost(); + if (uriDomain == null) + return null; + + String cookieDomain = cookie.getDomain(); + // No explicit cookie domain, use the origin domain. + if (cookieDomain == null) + return uriDomain; + + String resolvedDomain = cookieDomain; + if (resolvedDomain.startsWith(".")) + resolvedDomain = cookieDomain.substring(1); + // RFC 6265 section 4.1.2.3, ignore Domain if ends with ".". + if (resolvedDomain.endsWith(".")) + resolvedDomain = uriDomain; + // Reject top-level domains. + // TODO: should also reject "top" domain such as co.uk, gov.au, etc. + if (!resolvedDomain.contains(".")) + { + if (!resolvedDomain.equalsIgnoreCase("localhost")) + return null; + } + + // Reject if the resolved domain is not either + // the same or a parent domain of the URI domain. + if (!isSameOrSubDomain(uriDomain, resolvedDomain)) + return null; + + return resolvedDomain; + } + + private String resolvePath(URI uri, HttpCookie cookie) + { + // RFC 6265, section 5.1.4 and 5.2.4. + // Note that cookies with the Path attribute different from the + // URI path are accepted, as specified in sections 8.5 and 8.6. + String resolvedPath = cookie.getPath(); + if (resolvedPath == null || !resolvedPath.startsWith("/")) + { + String uriPath = uri.getRawPath(); + if (StringUtil.isBlank(uriPath) || !uriPath.startsWith("/")) + { + resolvedPath = "/"; + } + else + { + int lastSlash = uriPath.lastIndexOf('/'); + resolvedPath = uriPath.substring(0, lastSlash); + if (resolvedPath.isEmpty()) + resolvedPath = "/"; + } + } + return resolvedPath; } @Override @@ -209,6 +233,8 @@ public interface HttpCookieStore { return cookies.values().stream() .flatMap(Collection::stream) + .filter(Predicate.not(StoredHttpCookie::isExpired)) + .map(HttpCookie.class::cast) .toList(); } } @@ -216,13 +242,17 @@ public interface HttpCookieStore @Override public List match(URI uri) { - List result = new ArrayList<>(); - boolean secure = HttpScheme.isSecure(uri.getScheme()); String uriDomain = uri.getHost(); - String path = uri.getPath(); - if (path == null || path.trim().isEmpty()) + if (uriDomain == null) + return List.of(); + + String path = uri.getRawPath(); + if (path == null || path.isBlank()) path = "/"; + boolean secure = HttpScheme.isSecure(uri.getScheme()); + + List result = new ArrayList<>(); try (AutoLock ignored = lock.lock()) { // Given the way cookies are stored in the Map, the matching @@ -235,15 +265,14 @@ public interface HttpCookieStore // - Key[domain=example.com] // - chop domain to com // invalid domain, exit iteration. - String domain = uriDomain; - while (true) + String domain = uriDomain.toLowerCase(Locale.ENGLISH); + while (domain != null) { - Key key = new Key(secure, domain); - List stored = cookies.get(key); - Iterator iterator = stored == null ? Collections.emptyIterator() : stored.iterator(); + List stored = cookies.get(domain); + Iterator iterator = stored == null ? Collections.emptyIterator() : stored.iterator(); while (iterator.hasNext()) { - HttpCookie cookie = iterator.next(); + StoredHttpCookie cookie = iterator.next(); // Check and remove expired cookies. if (cookie.isExpired()) @@ -257,24 +286,16 @@ public interface HttpCookieStore continue; // Match the domain. - if (!domainMatches(uriDomain, key.domain, cookie.getDomain())) + if (!domainMatches(uriDomain, cookie.domain, cookie.getWrapped().getDomain())) continue; // Match the path. - if (!pathMatches(path, cookie.getPath())) + if (!pathMatches(path, cookie.path)) continue; result.add(cookie); } - - int dot = domain.indexOf('.'); - if (dot < 0) - break; - // Remove one subdomain. - domain = domain.substring(dot + 1); - // Exit if the top-level domain was reached. - if (domain.indexOf('.') < 0) - break; + domain = parentDomain(domain); } } @@ -283,64 +304,97 @@ public interface HttpCookieStore private static boolean domainMatches(String uriDomain, String domain, String cookieDomain) { - if (uriDomain == null) - return true; - if (domain != null) - domain = domain.toLowerCase(Locale.ENGLISH); - uriDomain = uriDomain.toLowerCase(Locale.ENGLISH); - if (cookieDomain != null) - cookieDomain = cookieDomain.toLowerCase(Locale.ENGLISH); + // If the cookie has no domain, or ends with ".", it must only be sent to the origin domain. if (cookieDomain == null || cookieDomain.endsWith(".")) - { - // RFC 6265, section 4.1.2.3. - // No cookie domain -> cookie sent only to origin server. - return uriDomain.equals(domain); - } - if (cookieDomain.startsWith(".")) - cookieDomain = cookieDomain.substring(1); - if (uriDomain.endsWith(cookieDomain)) - { - // The domain is the same as, or a subdomain of, the cookie domain. - int beforeMatch = uriDomain.length() - cookieDomain.length() - 1; - // Domains are the same. - if (beforeMatch == -1) - return true; - // Verify it is a proper subdomain such as bar.foo.com, - // not just a suffix of a domain such as bazfoo.com. - return uriDomain.charAt(beforeMatch) == '.'; - } - return false; + return uriDomain.equalsIgnoreCase(domain); + return isSameOrSubDomain(uriDomain, cookieDomain); } - private static boolean pathMatches(String path, String cookiePath) + private static boolean isSameOrSubDomain(String subDomain, String domain) + { + int subDomainLength = subDomain.length(); + int domainLength = domain.length(); + // Case-insensitive version of subDomain.endsWith(domain). + if (!subDomain.regionMatches(true, subDomainLength - domainLength, domain, 0, domainLength)) + return false; + // Make sure it is a subdomain. + int beforeMatch = subDomainLength - domainLength - 1; + // Domains are the same. + if (beforeMatch < 0) + return true; + // Verify it is a proper subdomain such as bar.foo.com, + // not just a suffix of a domain such as bazfoo.com. + return subDomain.charAt(beforeMatch) == '.'; + } + + private static boolean pathMatches(String uriPath, String cookiePath) { if (cookiePath == null) return true; // RFC 6265, section 5.1.4, path matching algorithm. - if (path.equals(cookiePath)) + if (uriPath.equals(cookiePath)) return true; - if (path.startsWith(cookiePath)) - return cookiePath.endsWith("/") || path.charAt(cookiePath.length()) == '/'; + if (uriPath.startsWith(cookiePath)) + return cookiePath.endsWith("/") || uriPath.charAt(cookiePath.length()) == '/'; return false; } @Override public boolean remove(URI uri, HttpCookie cookie) { - Key key = new Key(HttpScheme.isSecure(uri.getScheme()), uri.getHost()); + String uriDomain = uri.getHost(); + if (uriDomain == null) + return false; + + String resolvedPath = resolvePath(uri, cookie); + + boolean[] removed = new boolean[1]; try (AutoLock ignored = lock.lock()) { - boolean[] result = new boolean[1]; - cookies.compute(key, (k, v) -> + String domain = uriDomain.toLowerCase(Locale.ENGLISH); + while (domain != null) { - if (v == null) - return null; - boolean removed = v.remove(cookie); - result[0] = removed; - return v.isEmpty() ? null : v; - }); - return result[0]; + cookies.compute(domain, (k, v) -> + { + if (v == null) + return null; + + Iterator iterator = v.iterator(); + while (iterator.hasNext()) + { + StoredHttpCookie storedCookie = iterator.next(); + if (uriDomain.equalsIgnoreCase(storedCookie.uri.getHost())) + { + if (storedCookie.path.equals(resolvedPath)) + { + if (storedCookie.getWrapped().getName().equals(cookie.getName())) + { + iterator.remove(); + removed[0] = true; + } + } + } + } + + return v.isEmpty() ? null : v; + }); + domain = parentDomain(domain); + } } + return removed[0]; + } + + private String parentDomain(String domain) + { + int dot = domain.indexOf('.'); + if (dot < 0) + return null; + // Remove one subdomain. + domain = domain.substring(dot + 1); + // Exit if the top-level domain was reached. + if (domain.indexOf('.') < 0) + return null; + return domain; } @Override @@ -355,22 +409,19 @@ public interface HttpCookieStore } } - private record Key(boolean secure, String domain) - { - private Key(boolean secure, String domain) - { - this.secure = secure; - this.domain = domain.toLowerCase(Locale.ENGLISH); - } - } - - private static class Cookie extends HttpCookie.Wrapper + private static class StoredHttpCookie extends HttpCookie.Wrapper { private final long creationNanoTime = NanoTime.now(); + private final URI uri; + private final String domain; + private final String path; - public Cookie(HttpCookie wrapped) + private StoredHttpCookie(HttpCookie wrapped, URI uri, String domain, String path) { super(wrapped); + this.uri = Objects.requireNonNull(uri); + this.domain = Objects.requireNonNull(domain); + this.path = Objects.requireNonNull(path); } @Override @@ -382,6 +433,24 @@ public interface HttpCookieStore Instant expires = getExpires(); return expires != null && Instant.now().isAfter(expires); } + + @Override + public int hashCode() + { + return Objects.hash(getWrapped().getName(), domain.toLowerCase(Locale.ENGLISH), path); + } + + @Override + public boolean equals(Object obj) + { + if (this == obj) + return true; + if (!(obj instanceof StoredHttpCookie that)) + return false; + return getName().equals(that.getName()) && + domain.equalsIgnoreCase(that.domain) && + path.equals(that.path); + } } } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265SetCookieParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265SetCookieParser.java new file mode 100644 index 00000000000..c3418607b3b --- /dev/null +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265SetCookieParser.java @@ -0,0 +1,215 @@ +// +// ======================================================================== +// Copyright (c) 1995 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.http; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + *

A parser for {@code Set-Cookie} header values following + * RFC 6265.

+ *

White spaces around cookie name and value, and around attribute + * name and value, are permitted but stripped. + * Cookie values and attribute values may be quoted with double-quotes.

+ */ +public class RFC6265SetCookieParser implements SetCookieParser +{ + private static final Logger LOG = LoggerFactory.getLogger(RFC6265SetCookieParser.class); + + @Override + public HttpCookie parse(String setCookieValue) + { + // Implementation of the algorithm from RFC 6265, section 5.2. + + // Parser state. + State state = State.NAME; + String name = null; + boolean quoted = false; + HttpCookie.Builder cookie = null; + int offset = 0; + int length = setCookieValue.length(); + + // Parse. + for (int i = 0; i < length; ++i) + { + char ch = setCookieValue.charAt(i); + switch (state) + { + case NAME -> + { + HttpTokens.Token token = HttpTokens.getToken(ch); + if (token == null) + { + if (LOG.isDebugEnabled()) + LOG.debug("invalid character {} at index {} of {}", ch, i, setCookieValue); + return null; + } + if (ch == '=') + { + name = setCookieValue.substring(offset, i).trim(); + if (name.isEmpty()) + { + if (LOG.isDebugEnabled()) + LOG.debug("invalid empty cookie name at index {} of {}", i, setCookieValue); + return null; + } + offset = i + 1; + state = State.VALUE_START; + } + } + case VALUE_START -> + { + if (isWhitespace(ch)) + continue; + if (ch == '"') + quoted = true; + else + --i; + offset = i + 1; + state = State.VALUE; + } + case VALUE -> + { + if (quoted && ch == '"') + { + quoted = false; + String value = setCookieValue.substring(offset, i).trim(); + cookie = HttpCookie.build(name, value); + offset = i + 1; + state = State.ATTRIBUTE; + } + else + { + if (ch == ';') + { + String value = setCookieValue.substring(offset, i).trim(); + cookie = HttpCookie.build(name, value); + offset = i + 1; + state = State.ATTRIBUTE_NAME; + } + } + } + case ATTRIBUTE -> + { + if (isWhitespace(ch)) + continue; + if (ch != ';') + { + if (LOG.isDebugEnabled()) + LOG.debug("invalid character {} at index {} of {}", ch, i, setCookieValue); + return null; + } + offset = i + 1; + state = State.ATTRIBUTE_NAME; + } + case ATTRIBUTE_NAME -> + { + HttpTokens.Token token = HttpTokens.getToken(ch); + if (token == null || token.getType() == HttpTokens.Type.CNTL) + { + if (LOG.isDebugEnabled()) + LOG.debug("invalid character {} at index {} of {}", ch, i, setCookieValue); + return null; + } + if (ch == '=') + { + name = setCookieValue.substring(offset, i).trim(); + offset = i + 1; + state = State.ATTRIBUTE_VALUE_START; + } + else if (ch == ';') + { + name = setCookieValue.substring(offset, i).trim(); + if (!setAttribute(cookie, name, "")) + return null; + offset = i + 1; + // Stay in the ATTRIBUTE_NAME state. + } + } + case ATTRIBUTE_VALUE_START -> + { + if (isWhitespace(ch)) + continue; + if (ch == '"') + quoted = true; + else + --i; + offset = i + 1; + state = State.ATTRIBUTE_VALUE; + } + case ATTRIBUTE_VALUE -> + { + if (quoted && ch == '"') + { + quoted = false; + String value = setCookieValue.substring(offset, i).trim(); + if (!setAttribute(cookie, name, value)) + return null; + offset = i + 1; + state = State.ATTRIBUTE; + } + else + { + if (ch == ';') + { + String value = setCookieValue.substring(offset, i).trim(); + if (!setAttribute(cookie, name, value)) + return null; + offset = i + 1; + state = State.ATTRIBUTE_NAME; + } + } + } + default -> throw new IllegalStateException("invalid state " + state); + } + } + + return switch (state) + { + case NAME -> null; + case VALUE_START -> HttpCookie.from(name, ""); + case VALUE -> HttpCookie.from(name, setCookieValue.substring(offset, length).trim()); + case ATTRIBUTE -> cookie.build(); + case ATTRIBUTE_NAME -> setAttribute(cookie, setCookieValue.substring(offset, length).trim(), "") ? cookie.build() : null; + case ATTRIBUTE_VALUE_START -> setAttribute(cookie, name, "") ? cookie.build() : null; + case ATTRIBUTE_VALUE -> setAttribute(cookie, name, setCookieValue.substring(offset, length).trim()) ? cookie.build() : null; + }; + } + + private static boolean isWhitespace(char ch) + { + // A little more forgiving than RFC 6265. + return ch == ' ' || ch == '\t'; + } + + private boolean setAttribute(HttpCookie.Builder cookie, String name, String value) + { + try + { + cookie.attribute(name, value); + return true; + } + catch (Throwable x) + { + if (LOG.isDebugEnabled()) + LOG.debug("could not set attribute {}={}", name, value, x); + return false; + } + } + + private enum State + { + NAME, VALUE_START, VALUE, ATTRIBUTE, ATTRIBUTE_NAME, ATTRIBUTE_VALUE_START, ATTRIBUTE_VALUE + } +} diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/SetCookieParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/SetCookieParser.java new file mode 100644 index 00000000000..0103957d776 --- /dev/null +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/SetCookieParser.java @@ -0,0 +1,42 @@ +// +// ======================================================================== +// Copyright (c) 1995 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.http; + +/** + *

A parser for {@code Set-Cookie} header values.

+ *

Differently from other HTTP headers, {@code Set-Cookie} cannot be multi-valued + * with values separated by commas, because cookies supports the {@code Expires} + * attribute whose value is an RFC 1123 date that contains a comma.

+ */ +public interface SetCookieParser +{ + /** + *

Returns an {@link HttpCookie} obtained by parsing the given + * {@code Set-Cookie} value.

+ *

Returns {@code null} if the {@code Set-Cookie} value cannot + * be parsed due to syntax errors.

+ * + * @param setCookieValue the {@code Set-Cookie} value to parse + * @return the parse {@link HttpCookie} or {@code null} + */ + HttpCookie parse(String setCookieValue); + + /** + * @return a new instance of the default {@link SetCookieParser} + */ + static SetCookieParser newInstance() + { + return new RFC6265SetCookieParser(); + } +} diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieStoreTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieStoreTest.java index 7072c6e6383..ccbc0d09126 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieStoreTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieStoreTest.java @@ -19,6 +19,8 @@ import java.util.List; import java.util.concurrent.TimeUnit; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -26,6 +28,14 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public class HttpCookieStoreTest { + @Test + public void testRejectCookieForNoDomain() + { + HttpCookieStore store = new HttpCookieStore.Default(); + URI uri = URI.create("/path"); + assertFalse(store.add(uri, HttpCookie.from("n", "v"))); + } + @Test public void testRejectCookieForTopDomain() { @@ -60,6 +70,7 @@ public class HttpCookieStoreTest URI uri = URI.create("http://sub.example.com"); assertTrue(store.add(uri, HttpCookie.build("n", "v").domain("sub.example.com").build())); assertTrue(store.add(uri, HttpCookie.build("n", "v").domain("example.com").build())); + assertEquals(2, store.all().size()); } @Test @@ -78,6 +89,7 @@ public class HttpCookieStoreTest assertTrue(store.add(uri, HttpCookie.from("n", "v1"))); // Replace the cookie with another that has a different value. assertTrue(store.add(uri, HttpCookie.from("n", "v2"))); + assertEquals(1, store.all().size()); List matches = store.match(uri); assertEquals(1, matches.size()); assertEquals("v2", matches.get(0).getValue()); @@ -92,6 +104,7 @@ public class HttpCookieStoreTest // Replace the cookie with another that has a different value. // Domain comparison must be case-insensitive. assertTrue(store.add(uri, HttpCookie.build("n", "v2").domain("EXAMPLE.COM").build())); + assertEquals(1, store.all().size()); List matches = store.match(uri); assertEquals(1, matches.size()); assertEquals("v2", matches.get(0).getValue()); @@ -106,13 +119,26 @@ public class HttpCookieStoreTest // Replace the cookie with another that has a different value. // Path comparison must be case-sensitive. assertTrue(store.add(uri, HttpCookie.build("n", "v2").path("/path").build())); + assertEquals(1, store.all().size()); List matches = store.match(uri); assertEquals(1, matches.size()); assertEquals("v2", matches.get(0).getValue()); // Same path but different case should generate another cookie. assertTrue(store.add(uri, HttpCookie.build("n", "v3").path("/PATH").build())); - matches = store.all(); - assertEquals(2, matches.size()); + assertEquals(2, store.all().size()); + } + + @Test + public void testMatchNoDomain() + { + HttpCookieStore store = new HttpCookieStore.Default(); + URI cookieURI = URI.create("http://example.com"); + assertTrue(store.add(cookieURI, HttpCookie.from("n", "v1"))); + + // No domain, no match. + URI uri = URI.create("/path"); + List matches = store.match(uri); + assertEquals(0, matches.size()); } @Test @@ -220,6 +246,46 @@ public class HttpCookieStoreTest assertEquals(2, matches.size()); } + @Test + public void testMatchWithEscapedURIPath() + { + HttpCookieStore store = new HttpCookieStore.Default(); + URI cookieURI = URI.create("http://example.com/foo%2Fbar/baz"); + assertTrue(store.add(cookieURI, HttpCookie.build("n1", "v1").build())); + + URI uri = URI.create("http://example.com/foo"); + List matches = store.match(uri); + assertEquals(0, matches.size()); + + uri = URI.create("http://example.com/foo/"); + matches = store.match(uri); + assertEquals(0, matches.size()); + + uri = URI.create("http://example.com/foo/bar"); + matches = store.match(uri); + assertEquals(0, matches.size()); + + uri = URI.create("http://example.com/foo/bar/"); + matches = store.match(uri); + assertEquals(0, matches.size()); + + uri = URI.create("http://example.com/foo/bar/baz"); + matches = store.match(uri); + assertEquals(0, matches.size()); + + uri = URI.create("http://example.com/foo%2Fbar"); + matches = store.match(uri); + assertEquals(1, matches.size()); + + uri = URI.create("http://example.com/foo%2Fbar/"); + matches = store.match(uri); + assertEquals(1, matches.size()); + + uri = URI.create("http://example.com/foo%2Fbar/qux"); + matches = store.match(uri); + assertEquals(1, matches.size()); + } + @Test public void testExpiredCookieDoesNotMatch() throws Exception { @@ -237,14 +303,48 @@ public class HttpCookieStoreTest @Test public void testRemove() { + // Note that the URI path is "/path/", but the cookie path "/remove" is used. HttpCookieStore store = new HttpCookieStore.Default(); - URI cookieURI = URI.create("http://example.com/path"); - assertTrue(store.add(cookieURI, HttpCookie.from("n1", "v1"))); + URI cookieURI = URI.create("http://example.com/path/"); + assertTrue(store.add(cookieURI, HttpCookie.build("n1", "v1").path("/remove").build())); + + // Path does not match. + assertFalse(store.remove(URI.create("http://example.com"), HttpCookie.from("n1", "v2"))); + assertFalse(store.remove(URI.create("http://example.com/path/"), HttpCookie.from("n1", "v2"))); + assertFalse(store.remove(URI.create("http://example.com"), HttpCookie.build("n1", "v2").path("/path").build())); + assertFalse(store.remove(URI.create("http://example.com/remove/"), HttpCookie.build("n1", "v2").path("/path").build())); + + // Domain does not match. + assertFalse(store.remove(URI.create("http://domain.com/remove/"), HttpCookie.build("n1", "v2").build())); - URI removeURI = URI.create("http://example.com"); // Cookie value should not matter. - assertTrue(store.remove(removeURI, HttpCookie.from("n1", "n2"))); - assertFalse(store.remove(removeURI, HttpCookie.from("n1", "n2"))); + // The URI path must be "/remove/" (end with slash) because URI paths + // are chopped to the parent directory while cookie paths are not chopped. + URI removeURI = URI.create("http://example.com/remove/"); + assertTrue(store.remove(removeURI, HttpCookie.from("n1", "v2"))); + // Try again, should not be there. + assertFalse(store.remove(removeURI, HttpCookie.from("n1", "v2"))); + } + + @Test + public void testRemoveWithSubDomains() + { + // Subdomains can set cookies on the parent domain. + HttpCookieStore store = new HttpCookieStore.Default(); + URI cookieURI = URI.create("http://sub.example.com/path"); + assertTrue(store.add(cookieURI, HttpCookie.build("n1", "v1").domain("example.com").build())); + + // Cannot remove the cookie from the parent domain. + assertFalse(store.remove(URI.create("http://example.com/path"), HttpCookie.from("n1", "v2"))); + assertFalse(store.remove(URI.create("http://example.com/path"), HttpCookie.build("n1", "v2").domain("example.com").build())); + assertFalse(store.remove(URI.create("http://example.com/path"), HttpCookie.build("n1", "v2").domain("sub.example.com").build())); + + // Cannot remove the cookie from a sibling domain. + assertFalse(store.remove(URI.create("http://foo.example.com/path"), HttpCookie.from("n1", "v2"))); + assertFalse(store.remove(URI.create("http://foo.example.com/path"), HttpCookie.build("n1", "v2").domain("sub.example.com").build())); + + // Remove the cookie. + assertTrue(store.remove(cookieURI, HttpCookie.from("n1", "v2"))); } @Test @@ -252,17 +352,19 @@ public class HttpCookieStoreTest { HttpCookieStore store = new HttpCookieStore.Default(); URI uri = URI.create("http://example.com"); - // Dumb server sending a secure cookie on clear-text scheme. - assertFalse(store.add(uri, HttpCookie.build("n1", "v1").secure(true).build())); + // A secure cookie on clear-text scheme. + assertTrue(store.add(uri, HttpCookie.build("n1", "v1").secure(true).build())); + URI secureURI = URI.create("https://example.com"); assertTrue(store.add(secureURI, HttpCookie.build("n2", "v2").secure(true).build())); assertTrue(store.add(secureURI, HttpCookie.from("n3", "v3"))); List matches = store.match(uri); - assertEquals(0, matches.size()); + assertEquals(1, matches.size()); + assertEquals("n3", matches.get(0).getName()); matches = store.match(secureURI); - assertEquals(2, matches.size()); + assertEquals(3, matches.size()); } @Test @@ -290,9 +392,51 @@ public class HttpCookieStoreTest cookieURI = URI.create("wss://example.com"); assertTrue(store.add(cookieURI, HttpCookie.from("n2", "v2"))); + // Cookie matching does not depend on the scheme, + // not even with regard to non-secure vs secure. matchURI = URI.create("https://example.com"); matches = store.match(matchURI); + assertEquals(2, matches.size()); + } + + @ParameterizedTest + @ValueSource(strings = {"localhost.", "domain.com."}) + public void testCookieDomainEndingWithDotIsIgnored(String cookieDomain) + { + HttpCookieStore store = new HttpCookieStore.Default(); + URI cookieURI = URI.create("http://example.com"); + assertTrue(store.add(cookieURI, HttpCookie.build("n1", "v1").domain(cookieDomain).build())); + + List matches = store.match(cookieURI); assertEquals(1, matches.size()); - assertEquals("n2", matches.get(0).getName()); + } + + @ParameterizedTest + @ValueSource(strings = {"localhost.", "domain.com."}) + public void testAddWithURIDomainEndingWithDot(String uriDomain) + { + HttpCookieStore store = new HttpCookieStore.Default(); + URI cookieURI = URI.create("http://" + uriDomain); + assertTrue(store.add(cookieURI, HttpCookie.from("n1", "v1"))); + + List matches = store.match(cookieURI); + assertEquals(1, matches.size()); + + cookieURI = URI.create("http://" + uriDomain.substring(0, uriDomain.length() - 1)); + matches = store.match(cookieURI); + assertEquals(0, matches.size()); + } + + @ParameterizedTest + @ValueSource(strings = {"localhost.", "domain.com."}) + public void testMatchWithURIDomainEndingWithDot(String uriDomain) + { + HttpCookieStore store = new HttpCookieStore.Default(); + URI cookieURI = URI.create("http://" + uriDomain.substring(0, uriDomain.length() - 1)); + assertTrue(store.add(cookieURI, HttpCookie.from("n1", "v1"))); + + cookieURI = URI.create("http://" + uriDomain); + List matches = store.match(cookieURI); + assertEquals(0, matches.size()); } } diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java new file mode 100644 index 00000000000..bb2b481ab3f --- /dev/null +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java @@ -0,0 +1,108 @@ +// +// ======================================================================== +// Copyright (c) 1995 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.http; + +import java.time.Instant; +import java.time.format.DateTimeParseException; +import java.util.List; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class HttpCookieTest +{ + public static List cookies() + { + return List.of( + Arguments.of("=", null), + Arguments.of("=B", null), + Arguments.of("=B; a", null), + Arguments.of("=B; a=", null), + Arguments.of("=B; a=v", null), + Arguments.of("A", null), + Arguments.of("A=", HttpCookie.build("A", "").build()), + Arguments.of("A=; HttpOnly", HttpCookie.build("A", "").httpOnly(true).build()), + Arguments.of("A=; a", HttpCookie.build("A", "").attribute("a", "").build()), + Arguments.of("A=; a=", HttpCookie.build("A", "").attribute("a", "").build()), + Arguments.of("A=; a=v", HttpCookie.build("A", "").attribute("a", "v").build()), + Arguments.of("A=B", HttpCookie.build("A", "B").build()), + Arguments.of("A= B", HttpCookie.build("A", "B").build()), + Arguments.of("A =B", HttpCookie.build("A", "B").build()), + Arguments.of(" A=B", HttpCookie.build("A", "B").build()), + Arguments.of(" A= B", HttpCookie.build("A", "B").build()), + Arguments.of("A=B; Secure", HttpCookie.build("A", "B").secure(true).build()), + Arguments.of("A=B; Expires=Thu, 01 Jan 1970 00:00:00 GMT", HttpCookie.build("A", "B").expires(Instant.EPOCH).build()), + Arguments.of("A=B; a", HttpCookie.build("A", "B").attribute("a", "").build()), + Arguments.of("A=B; a=", HttpCookie.build("A", "B").attribute("a", "").build()), + Arguments.of("A=B; a=v", HttpCookie.build("A", "B").attribute("a", "v").build()), + Arguments.of("A=B; Secure; Path=/", HttpCookie.build("A", "B").secure(true).path("/").build()), + // Quoted cookie. + Arguments.of("A=\"1\"", HttpCookie.build("A", "1").build()), + Arguments.of("A=\"1\"; HttpOnly", HttpCookie.build("A", "1").httpOnly(true).build()), + Arguments.of(" A = \"1\" ; a = v", HttpCookie.build("A", "1").attribute("a", "v").build()), + Arguments.of(" A = \"1\" ; a = \"v\"; Secure", HttpCookie.build("A", "1").attribute("a", "v").secure(true).build()), + Arguments.of(" A = \"1\" ; Path= \"/\"", HttpCookie.build("A", "1").path("/").build()), + Arguments.of(" A = \"1\" ; Expires= \"Thu, 01 Jan 1970 00:00:00 GMT\"", HttpCookie.build("A", "1").expires(Instant.EPOCH).build()), + // Invalid cookie. + Arguments.of("A=\"1\" Bad", null), + Arguments.of("A=1; Expires=blah", null), + Arguments.of("A=1; Expires=blah; HttpOnly", null), + Arguments.of("A=1; HttpOnly=blah", null), + Arguments.of("A=1; Max-Age=blah", null), + Arguments.of("A=1; SameSite=blah", null), + Arguments.of("A=1; SameSite=blah; Secure", null), + Arguments.of("A=1; Secure=blah", null), + Arguments.of("A=1; Max-Age=\"blah\"", null), + // Weird cookie. + Arguments.of("A=1; Domain=example.org; Domain=domain.com", HttpCookie.build("A", "1").domain("domain.com").build()), + Arguments.of("A=1; Path=/; Path=/ctx", HttpCookie.build("A", "1").path("/ctx").build()) + ); + } + + @ParameterizedTest + @MethodSource("cookies") + public void testParseCookies(String setCookieValue, HttpCookie expectedCookie) + { + SetCookieParser parser = SetCookieParser.newInstance(); + HttpCookie parsed = parser.parse(setCookieValue); + assertEquals(expectedCookie, parsed); + if (expectedCookie != null) + assertThat(expectedCookie.getAttributes(), is(parsed.getAttributes())); + } + + public static List invalidAttributes() + { + return List.of( + Arguments.of("Expires", "blah", DateTimeParseException.class), + Arguments.of("HttpOnly", "blah", IllegalArgumentException.class), + Arguments.of("Max-Age", "blah", NumberFormatException.class), + Arguments.of("SameSite", "blah", IllegalArgumentException.class), + Arguments.of("Secure", "blah", IllegalArgumentException.class) + ); + } + + @ParameterizedTest + @MethodSource("invalidAttributes") + public void testParseInvalidAttributes(String name, String value, Class failure) + { + assertThrows(failure, () -> HttpCookie.build("A", "1") + .attribute(name, value)); + } +} diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpCookieUtils.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpCookieUtils.java index 40869b0a5cf..9016f874a98 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpCookieUtils.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpCookieUtils.java @@ -14,18 +14,14 @@ package org.eclipse.jetty.server; import java.time.Instant; -import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; -import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; -import org.eclipse.jetty.http.QuotedCSVParser; import org.eclipse.jetty.http.Syntax; import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.Index; @@ -75,35 +71,6 @@ public final class HttpCookieUtils return HttpCookie.from(cookie, HttpCookie.SAME_SITE_ATTRIBUTE, contextDefault.getAttributeValue()); } - /** - * Extract the bare minimum of info from a Set-Cookie header string. - * - *

- * Ideally this method should not be necessary, however as java.net.HttpCookie - * does not yet support generic attributes, we have to use it in a minimal - * fashion. When it supports attributes, we could look at reverting to a - * constructor on o.e.j.h.HttpCookie to take the set-cookie header string. - *

- * - * @param setCookieHeader the header as a string - * @return a map containing the name, value, domain, path. max-age of the set cookie header - */ - public static Map extractBasics(String setCookieHeader) - { - //Parse the bare minimum - List cookies = java.net.HttpCookie.parse(setCookieHeader); - if (cookies.size() != 1) - return Collections.emptyMap(); - java.net.HttpCookie cookie = cookies.get(0); - Map fields = new HashMap<>(); - fields.put("name", cookie.getName()); - fields.put("value", cookie.getValue()); - fields.put("domain", cookie.getDomain()); - fields.put("path", cookie.getPath()); - fields.put("max-age", Long.toString(cookie.getMaxAge())); - return fields; - } - /** * Get the default value for SameSite cookie attribute, if one * has been set for the given context. @@ -406,52 +373,6 @@ public final class HttpCookieUtils return oldPath.equals(newPath); } - /** - * Get a {@link HttpHeader#SET_COOKIE} field as a {@link HttpCookie}, either - * by optimally checking for a {@link SetCookieHttpField} or by parsing - * the value with {@link #parseSetCookie(String)}. - * @param field The field - * @return The field value as a {@link HttpCookie} or null if the field - * is not a {@link HttpHeader#SET_COOKIE} or cannot be parsed. - */ - public static HttpCookie getSetCookie(HttpField field) - { - if (field == null || field.getHeader() != HttpHeader.SET_COOKIE) - return null; - if (field instanceof SetCookieHttpField setCookieHttpField) - return setCookieHttpField.getHttpCookie(); - return parseSetCookie(field.getValue()); - } - - public static HttpCookie parseSetCookie(String value) - { - AtomicReference builder = new AtomicReference<>(); - new QuotedCSVParser(false) - { - @Override - protected void parsedParam(StringBuffer buffer, int valueLength, int paramName, int paramValue) - { - String name = buffer.substring(paramName, paramValue - 1); - String value = buffer.substring(paramValue); - HttpCookie.Builder b = builder.get(); - if (b == null) - { - b = HttpCookie.build(name, value); - builder.set(b); - } - else - { - b.attribute(name, value); - } - } - }.addValue(value); - - HttpCookie.Builder b = builder.get(); - if (b == null) - return null; - return b.build(); - } - private static void quoteIfNeededAndAppend(String text, StringBuilder builder) { if (isQuoteNeeded(text)) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java index 2bfe8e945dc..0e6fb0bed42 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java @@ -23,6 +23,7 @@ import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; +import org.eclipse.jetty.http.SetCookieParser; import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.component.LifeCycle; @@ -396,9 +397,7 @@ public class ResponseTest if (field.getHeader() != HttpHeader.SET_COOKIE) continue; - HttpCookie cookie = HttpCookieUtils.getSetCookie(field); - if (cookie == null) - continue; + HttpCookie cookie = SetCookieParser.newInstance().parse(field.getValue()); i.set(new HttpCookieUtils.SetCookieHttpField( HttpCookie.build(cookie) @@ -411,7 +410,7 @@ public class ResponseTest }); response.setStatus(200); Response.addCookie(response, HttpCookie.from("name", "test1")); - response.getHeaders().add(HttpHeader.SET_COOKIE, "other=test2; Domain=wrong; SameSite=wrong; Attr=x"); + response.getHeaders().add(HttpHeader.SET_COOKIE, "other=test2; Domain=original; SameSite=None; Attr=x"); Content.Sink.write(response, true, "OK", callback); return true; } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java index d702561dda9..a8575d5dbcd 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java @@ -96,10 +96,10 @@ public class ServerHttpCookieTest Arguments.of(RFC6265, "Cookie: $version=1; name=value", 200, "Version=", List.of("[$version=1]", "[name=value]").toArray(new String[0])), Arguments.of(RFC6265, "Cookie: name=value;$path=/path", 200, "Path=", List.of("[name=value]", "[$path=/path]").toArray(new String[0])), Arguments.of(from("RFC6265,ATTRIBUTES"), "Cookie: name=value;$path=/path", 200, "/path", List.of("name=value").toArray(new String[0])), - Arguments.of(from("RFC6265_STRICT,ATTRIBUTE_VALUES"), "Cookie: name=value;$path=/path", 200, null, List.of("name=value;Path=/path").toArray(new String[0])), - Arguments.of(RFC2965, "Cookie: name=value;$path=/path", 200, null, List.of("name=value;Path=/path").toArray(new String[0])), - Arguments.of(RFC2965, "Cookie: $Version=1;name=value;$path=/path", 200, null, List.of("name=value;Version=1;Path=/path").toArray(new String[0])), - Arguments.of(RFC2965, "Cookie: $Version=1;name=value;$path=/path;$Domain=host", 200, null, List.of("name=value;Version=1;Domain=host;Path=/path").toArray(new String[0])), + Arguments.of(from("RFC6265_STRICT,ATTRIBUTE_VALUES"), "Cookie: name=value;$path=/path", 200, null, List.of("name=value; Path=/path").toArray(new String[0])), + Arguments.of(RFC2965, "Cookie: name=value;$path=/path", 200, null, List.of("name=value; Path=/path").toArray(new String[0])), + Arguments.of(RFC2965, "Cookie: $Version=1;name=value;$path=/path", 200, null, List.of("name=value; Path=/path").toArray(new String[0])), + Arguments.of(RFC2965, "Cookie: $Version=1;name=value;$path=/path;$Domain=host", 200, null, List.of("name=value; Domain=host; Path=/path").toArray(new String[0])), // multiple cookie tests Arguments.of(RFC6265_STRICT, "Cookie: name=value; other=extra", 200, "Version=", List.of("[name=value]", "[other=extra]").toArray(new String[0])), diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java index 87786fb5688..0ca45502162 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java @@ -22,7 +22,6 @@ import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.security.Principal; import java.util.AbstractList; -import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.EnumSet; @@ -69,6 +68,7 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MimeTypes; +import org.eclipse.jetty.http.SetCookieParser; import org.eclipse.jetty.io.QuietException; import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.security.AuthenticationState; @@ -98,6 +98,8 @@ import org.slf4j.LoggerFactory; public class ServletApiRequest implements HttpServletRequest { private static final Logger LOG = LoggerFactory.getLogger(ServletApiRequest.class); + private static final SetCookieParser SET_COOKIE_PARSER = SetCookieParser.newInstance(); + private final ServletContextRequest _servletContextRequest; private final ServletChannel _servletChannel; private AsyncContextState _async; @@ -615,32 +617,37 @@ public class ServletApiRequest implements HttpServletRequest referrer += "?" + query; pushHeaders.put(HttpHeader.REFERER, referrer); - // Any Set-Cookie in the response should be present in the push. - HttpFields.Mutable responseHeaders = _servletChannel.getResponse().getHeaders(); - List setCookies = new ArrayList<>(responseHeaders.getValuesList(HttpHeader.SET_COOKIE)); - setCookies.addAll(responseHeaders.getValuesList(HttpHeader.SET_COOKIE2)); - String cookies = pushHeaders.get(HttpHeader.COOKIE); - if (!setCookies.isEmpty()) + StringBuilder cookieBuilder = new StringBuilder(); + Cookie[] cookies = getCookies(); + if (cookies != null) { - StringBuilder pushCookies = new StringBuilder(); - if (cookies != null) - pushCookies.append(cookies); - for (String setCookie : setCookies) + for (Cookie cookie : cookies) { - Map cookieFields = HttpCookieUtils.extractBasics(setCookie); - String cookieName = cookieFields.get("name"); - String cookieValue = cookieFields.get("value"); - String cookieMaxAge = cookieFields.get("max-age"); - long maxAge = cookieMaxAge != null ? Long.parseLong(cookieMaxAge) : -1; - if (maxAge > 0) - { - if (pushCookies.length() > 0) - pushCookies.append("; "); - pushCookies.append(cookieName).append("=").append(cookieValue); - } + if (!cookieBuilder.isEmpty()) + cookieBuilder.append("; "); + cookieBuilder.append(cookie.getName()).append("=").append(cookie.getValue()); } - pushHeaders.put(HttpHeader.COOKIE, pushCookies.toString()); } + // Any Set-Cookie in the response should be present in the push. + for (HttpField field : _servletContextRequest.getServletContextResponse().getHeaders()) + { + HttpHeader header = field.getHeader(); + if (header == HttpHeader.SET_COOKIE || header == HttpHeader.SET_COOKIE2) + { + HttpCookie httpCookie; + if (field instanceof HttpCookieUtils.SetCookieHttpField set) + httpCookie = set.getHttpCookie(); + else + httpCookie = SET_COOKIE_PARSER.parse(field.getValue()); + if (httpCookie == null || httpCookie.isExpired()) + continue; + if (!cookieBuilder.isEmpty()) + cookieBuilder.append("; "); + cookieBuilder.append(httpCookie.getName()).append("=").append(httpCookie.getValue()); + } + } + if (!cookieBuilder.isEmpty()) + pushHeaders.put(HttpHeader.COOKIE, cookieBuilder.toString()); String sessionId; HttpSession httpSession = getSession(false); diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/SessionHandlerTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/SessionHandlerTest.java index 133845af7b2..eda011a8584 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/SessionHandlerTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/SessionHandlerTest.java @@ -25,7 +25,6 @@ import java.nio.file.Path; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; -import java.util.Map; import java.util.function.Consumer; import java.util.function.Function; @@ -60,7 +59,6 @@ import org.eclipse.jetty.session.SessionDataStoreFactory; import org.eclipse.jetty.toolchain.test.IO; import org.eclipse.jetty.toolchain.test.jupiter.WorkDir; import org.eclipse.jetty.toolchain.test.jupiter.WorkDirExtension; -import org.eclipse.jetty.util.URIUtil; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -413,7 +411,7 @@ public class SessionHandlerTest assertThat(response.getContentAsString(), containsString("valid=false")); //test with a cookie for non-existant session - URI uri = URIUtil.toURI(URIUtil.newURI("http", "localhost", port, path, "")); + URI uri = URI.create(url); HttpCookie cookie = HttpCookie.build(SessionHandler.__DefaultSessionCookie, "123456789").path("/").domain("localhost").build(); client.getHttpCookieStore().add(uri, cookie); response = client.GET(url); diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index fe4ef9b81fd..c09a8c4291a 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -79,11 +79,11 @@ import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.http.MimeTypes; +import org.eclipse.jetty.http.SetCookieParser; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.security.UserIdentity; import org.eclipse.jetty.server.HttpCookieUtils; -import org.eclipse.jetty.server.HttpCookieUtils.SetCookieHttpField; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.Session; import org.eclipse.jetty.session.AbstractSessionManager; @@ -107,11 +107,11 @@ public class Request implements HttpServletRequest public static final String __MULTIPART_CONFIG_ELEMENT = "org.eclipse.jetty.multipartConfig"; private static final Logger LOG = LoggerFactory.getLogger(Request.class); + private static final SetCookieParser SET_COOKIE_PARSER = SetCookieParser.newInstance(); private static final Collection __defaultLocale = Collections.singleton(Locale.getDefault()); private static final int INPUT_NONE = 0; private static final int INPUT_STREAM = 1; private static final int INPUT_READER = 2; - private static final MultiMap NO_PARAMS = new MultiMap<>(); private static final MultiMap BAD_PARAMS = new MultiMap<>(); @@ -289,60 +289,37 @@ public class Request implements HttpServletRequest id = getRequestedSessionId(); } - Map cookies = new HashMap<>(); - Cookie[] existingCookies = getCookies(); - if (existingCookies != null) + StringBuilder cookieBuilder = new StringBuilder(); + Cookie[] cookies = getCookies(); + if (cookies != null) { - for (Cookie c : getCookies()) + for (Cookie cookie : cookies) { - cookies.put(c.getName(), c.getValue()); + if (!cookieBuilder.isEmpty()) + cookieBuilder.append("; "); + cookieBuilder.append(cookie.getName()).append("=").append(cookie.getValue()); } } - - //Any Set-Cookies that were set on the response must be set as Cookies on the - //PushBuilder, unless the max-age of the cookie is <= 0 - HttpFields responseFields = getResponse().getHttpFields(); - for (HttpField field : responseFields) + // Any Set-Cookie in the response should be present in the push. + for (HttpField field : getResponse().getHttpFields()) { HttpHeader header = field.getHeader(); - if (header == HttpHeader.SET_COOKIE) + if (header == HttpHeader.SET_COOKIE || header == HttpHeader.SET_COOKIE2) { - String cookieName; - String cookieValue; - long cookieMaxAge; - if (field instanceof SetCookieHttpField) - { - HttpCookie cookie = ((SetCookieHttpField)field).getHttpCookie(); - cookieName = cookie.getName(); - cookieValue = cookie.getValue(); - cookieMaxAge = cookie.getMaxAge(); - } + HttpCookie httpCookie; + if (field instanceof HttpCookieUtils.SetCookieHttpField set) + httpCookie = set.getHttpCookie(); else - { - Map cookieFields = HttpCookieUtils.extractBasics(field.getValue()); - cookieName = cookieFields.get("name"); - cookieValue = cookieFields.get("value"); - cookieMaxAge = cookieFields.get("max-age") != null ? Long.parseLong(cookieFields.get("max-age")) : -1; - } - - if (cookieMaxAge > 0) - cookies.put(cookieName, cookieValue); - else - cookies.remove(cookieName); + httpCookie = SET_COOKIE_PARSER.parse(field.getValue()); + if (httpCookie == null || httpCookie.isExpired()) + continue; + if (!cookieBuilder.isEmpty()) + cookieBuilder.append("; "); + cookieBuilder.append(httpCookie.getName()).append("=").append(httpCookie.getValue()); } } - - if (!cookies.isEmpty()) - { - StringBuilder buff = new StringBuilder(); - for (Map.Entry entry : cookies.entrySet()) - { - if (buff.length() > 0) - buff.append("; "); - buff.append(entry.getKey()).append('=').append(entry.getValue()); - } - fields.add(new HttpField(HttpHeader.COOKIE, buff.toString())); - } + if (!cookieBuilder.isEmpty()) + fields.put(HttpHeader.COOKIE, cookieBuilder.toString()); String query = getQueryString(); PushBuilder builder = new PushBuilderImpl(this, fields, getMethod(), query, id);