From a44a99ad9be97e613121dfb5b0866577dab4c646 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 7 Jun 2023 13:50:19 +0200 Subject: [PATCH] SetCookieHttpField #9173 (#9789) Added utility methods and classes to allow for efficient interception of a Set-Cookie header in a HttpStream wrapper. --- .../org/eclipse/jetty/http/HttpCookie.java | 8 +- .../org/eclipse/jetty/http/HttpField.java | 15 +- .../eclipse/jetty/server/HttpCookieUtils.java | 63 +++++++- .../server/internal/ResponseHttpFields.java | 150 ++---------------- .../eclipse/jetty/server/ResponseTest.java | 69 +++++++- 5 files changed, 153 insertions(+), 152 deletions(-) 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 95526fcf9d0..e892ddc0d9b 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 @@ -592,6 +592,12 @@ public interface HttpCookie return this; } + public Builder sameSite(SameSite sameSite) + { + _attributes = lazyAttributePut(_attributes, SAME_SITE_ATTRIBUTE, sameSite.attributeValue); + return this; + } + /** * @return an immutable {@link HttpCookie} instance. */ @@ -878,7 +884,7 @@ public interface HttpCookie private static Map lazyAttributePut(Map attributes, String key, String value) { if (value == null) - return attributes; + return lazyAttributeRemove(attributes, key); if (attributes == null) attributes = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); attributes.put(key, value); diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpField.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpField.java index 5d1c6921233..d2f8625a7e2 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpField.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpField.java @@ -152,7 +152,7 @@ public class HttpField */ public boolean contains(String search) { - return contains(_value, search); + return contains(getValue(), search); } /** @@ -337,7 +337,7 @@ public class HttpField return false; if (!_name.equalsIgnoreCase(field.getName())) return false; - return Objects.equals(_value, field.getValue()); + return Objects.equals(getValue(), field.getValue()); } public HttpHeader getHeader() @@ -347,12 +347,12 @@ public class HttpField public int getIntValue() { - return Integer.parseInt(_value); + return Integer.parseInt(getValue()); } public long getLongValue() { - return Long.parseLong(_value); + return Long.parseLong(getValue()); } public String getLowerCaseName() @@ -380,16 +380,17 @@ public class HttpField public List getValueList() { - if (_value == null) + String value = getValue(); + if (value == null) return null; - QuotedCSV list = new QuotedCSV(false, _value); + QuotedCSV list = new QuotedCSV(false, value); return list.getValues(); } @Override public int hashCode() { - int vhc = Objects.hashCode(_value); + int vhc = Objects.hashCode(getValue()); if (_header == null) return vhc ^ nameHashCode(); return vhc ^ _header.hashCode(); 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 6981ac5cc11..90546c075db 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 @@ -19,11 +19,13 @@ 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; @@ -404,6 +406,52 @@ 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)) @@ -416,19 +464,32 @@ public final class HttpCookieUtils { } + /** + * A {@link HttpField} that holds an {@link HttpHeader#SET_COOKIE} as a + * {@link HttpCookie} instance, delaying any value generation until + * {@link #getValue()} is called. + */ public static class SetCookieHttpField extends HttpField { private final HttpCookie _cookie; + private final CookieCompliance _compliance; public SetCookieHttpField(HttpCookie cookie, CookieCompliance compliance) { - super(HttpHeader.SET_COOKIE, getSetCookie(cookie, compliance)); + super(HttpHeader.SET_COOKIE, HttpHeader.SET_COOKIE.asString(), null); this._cookie = cookie; + _compliance = compliance; } public HttpCookie getHttpCookie() { return _cookie; } + + @Override + public String getValue() + { + return getSetCookie(_cookie, _compliance); + } } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java index f1dc0172d4c..cf95c3fb552 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/ResponseHttpFields.java @@ -13,22 +13,16 @@ package org.eclipse.jetty.server.internal; -import java.util.EnumSet; import java.util.Iterator; -import java.util.List; import java.util.ListIterator; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.BiFunction; import java.util.stream.Stream; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; -import org.eclipse.jetty.http.HttpHeader; -import org.eclipse.jetty.http.HttpHeaderValue; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -// TODO: review whether it needs to override these many methods, as it may be enough to override iterator(). public class ResponseHttpFields implements HttpFields.Mutable { private static final Logger LOG = LoggerFactory.getLogger(ResponseHttpFields.class); @@ -85,52 +79,12 @@ public class ResponseHttpFields implements HttpFields.Mutable return _fields.asImmutable(); } - @Override - public Mutable add(String name, String value) - { - return _committed.get() ? this : _fields.add(name, value); - } - - @Override - public Mutable add(HttpHeader header, HttpHeaderValue value) - { - return _fields.add(header, value); - } - - @Override - public Mutable add(HttpHeader header, String value) - { - return _committed.get() ? this : _fields.add(header, value); - } - @Override public Mutable add(HttpField field) { - return _committed.get() ? this : _fields.add(field); - } - - @Override - public Mutable add(HttpFields fields) - { - return _committed.get() ? this : _fields.add(fields); - } - - @Override - public Mutable addCSV(HttpHeader header, String... values) - { - return _committed.get() ? this : _fields.addCSV(header, values); - } - - @Override - public Mutable addCSV(String name, String... values) - { - return _committed.get() ? this : _fields.addCSV(name, values); - } - - @Override - public Mutable addDateField(String name, long date) - { - return _committed.get() ? this : _fields.addDateField(name, date); + if (field != null && !_committed.get()) + _fields.add(field); + return this; } @Override @@ -231,109 +185,27 @@ public class ResponseHttpFields implements HttpFields.Mutable } @Override - public void set(HttpField httpField) + public void set(HttpField field) { if (_committed.get()) throw new UnsupportedOperationException("Read Only"); - i.set(httpField); + if (field == null) + i.remove(); + else + i.set(field); } @Override - public void add(HttpField httpField) + public void add(HttpField field) { if (_committed.get()) throw new UnsupportedOperationException("Read Only"); - i.add(httpField); + if (field != null) + i.add(field); } }; } - @Override - public Mutable put(HttpField field) - { - return _committed.get() ? this : _fields.put(field); - } - - @Override - public Mutable put(String name, String value) - { - return _committed.get() ? this : _fields.put(name, value); - } - - @Override - public Mutable put(HttpHeader header, HttpHeaderValue value) - { - return _committed.get() ? this : _fields.put(header, value); - } - - @Override - public Mutable put(HttpHeader header, String value) - { - return _committed.get() ? this : _fields.put(header, value); - } - - @Override - public Mutable put(String name, List list) - { - return _committed.get() ? this : _fields.put(name, list); - } - - @Override - public Mutable putDate(HttpHeader name, long date) - { - return _committed.get() ? this : _fields.putDate(name, date); - } - - @Override - public Mutable putDate(String name, long date) - { - return _committed.get() ? this : _fields.putDate(name, date); - } - - @Override - public Mutable put(HttpHeader header, long value) - { - return _committed.get() ? this : _fields.put(header, value); - } - - @Override - public Mutable put(String name, long value) - { - return _committed.get() ? this : _fields.put(name, value); - } - - @Override - public void computeField(HttpHeader header, BiFunction, HttpField> computeFn) - { - if (!_committed.get()) - _fields.computeField(header, computeFn); - } - - @Override - public void computeField(String name, BiFunction, HttpField> computeFn) - { - if (!_committed.get()) - _fields.computeField(name, computeFn); - } - - @Override - public Mutable remove(HttpHeader name) - { - return _committed.get() ? this : _fields.remove(name); - } - - @Override - public Mutable remove(EnumSet fields) - { - return _committed.get() ? this : _fields.remove(fields); - } - - @Override - public Mutable remove(String name) - { - return _committed.get() ? this : _fields.remove(name); - } - @Override public String toString() { 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 bb3546a6dcd..14d9ddca76c 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 @@ -13,9 +13,15 @@ package org.eclipse.jetty.server; +import java.util.ListIterator; + +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.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; +import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.component.LifeCycle; import org.junit.jupiter.api.AfterEach; @@ -23,6 +29,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -53,7 +60,7 @@ public class ResponseTest server.setHandler(new Handler.Abstract() { @Override - public boolean handle(Request request, Response response, Callback callback) throws Exception + public boolean handle(Request request, Response response, Callback callback) { Response.sendRedirect(request, response, callback, "/somewhere/else"); return true; @@ -89,7 +96,7 @@ public class ResponseTest server.setHandler(new Handler.Abstract() { @Override - public boolean handle(Request request, Response response, Callback callback) throws Exception + public boolean handle(Request request, Response response, Callback callback) { Response.sendRedirect(request, response, callback, "/somewhere/else"); return true; @@ -123,7 +130,7 @@ public class ResponseTest server.setHandler(new Handler.Abstract() { @Override - public boolean handle(Request request, Response response, Callback callback) throws Exception + public boolean handle(Request request, Response response, Callback callback) { Response.sendRedirect(request, response, callback, "/somewhere/else"); return true; @@ -159,7 +166,7 @@ public class ResponseTest server.setHandler(new Handler.Abstract() { @Override - public boolean handle(Request request, Response response, Callback callback) throws Exception + public boolean handle(Request request, Response response, Callback callback) { Response.sendRedirect(request, response, callback, "/somewhere/else"); return true; @@ -186,4 +193,58 @@ public class ResponseTest assertEquals(HttpStatus.SEE_OTHER_303, response.getStatus()); assertThat(response.get(HttpHeader.LOCATION), is("/somewhere/else")); } + + @Test + public void testHttpCookieProcessing() throws Exception + { + server.setHandler(new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) + { + request.addHttpStreamWrapper(httpStream -> new HttpStream.Wrapper(httpStream) + { + @Override + public void prepareResponse(HttpFields.Mutable headers) + { + super.prepareResponse(headers); + for (ListIterator i = headers.listIterator(); i.hasNext();) + { + HttpField field = i.next(); + if (field.getHeader() != HttpHeader.SET_COOKIE) + continue; + + HttpCookie cookie = HttpCookieUtils.getSetCookie(field); + if (cookie == null) + continue; + + i.set(new HttpCookieUtils.SetCookieHttpField( + HttpCookie.build(cookie) + .domain("customized") + .sameSite(HttpCookie.SameSite.LAX) + .build(), + request.getConnectionMetaData().getHttpConfiguration().getResponseCookieCompliance())); + } + } + }); + response.setStatus(200); + Response.addCookie(response, HttpCookie.from("name", "test1")); + response.getHeaders().add(HttpHeader.SET_COOKIE, "other=test2; Domain=wrong; SameSite=wrong; Attr=x"); + Content.Sink.write(response, true, "OK", callback); + return true; + } + }); + server.start(); + + String request = """ + POST /path HTTP/1.0\r + Host: hostname\r + \r + """; + HttpTester.Response response = HttpTester.parseResponse(connector.getResponse(request)); + assertEquals(HttpStatus.OK_200, response.getStatus()); + assertThat(response.getValuesList(HttpHeader.SET_COOKIE), containsInAnyOrder( + "name=test1; Domain=customized; SameSite=Lax", + "other=test2; Domain=customized; SameSite=Lax; Attr=x")); + } }