Breaking API change in HttpCookieUtils in Jetty 12.0.2

[wilkinsona]

Jetty version(s) 12.0.2

Jetty Environment ee10

Java version/vendor (use: java -version) All

OS type/version All

Description The changes made in https://github.com/eclipse/jetty.project/commit/530ed33611edd8f327f57a631c04ac32c9c8e15a have removed the getSetCookie(HttpField) method from HttpCookieUtils that was added in https://github.com/eclipse/jetty.project/commit/a44a99ad9be97e613121dfb5b0866577dab4c646 in support of https://github.com/eclipse/jetty.project/issues/9173.

Looking at the changes made to HttpCookieUtils, I'm not sure how best to adapt to the removal of getSetCookie(HttpField). Would it be possible for the method to be reinstated or, failing that, could some guidance be offered for an alternative? Unfortunately, as things stand, Spring Boot 3.2 is stuck on Jetty 12.0.1.

How to reproduce? N/A

[sbordet]

@wilkinsona can you point us at how you use HttpCookieUtils (which BTW is a class that should have been internal and not for public usage)?

The replacement would be as follows:

HttpCookie cookie = SetCookieParser.newInstance().parse(field.getValue());

The SetCookieParser instance is currently stateless, so it can be saved in a static field, but I would recommend to save it in a once object (e.g. as a Spring bean) in case in the future some parser configuration will be required.

Let us know if this work for you.

[wilkinsona]

Thanks, @sbordet.

We're only using it in one place, thankfully: https://github.com/spring-projects/spring-boot/blob/7b1059a4b510e9fd58e6a3816e919ea52088546d/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyServletWebServerFactory.java#L810.

From what you've said, it sounds like we can do this instead:

diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyServletWebServerFactory.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyServletWebServerFactory.java
index 43be8f8766..e416943f5b 100644
--- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyServletWebServerFactory.java
+++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/web/embedded/jetty/JettyServletWebServerFactory.java
@@ -55,8 +55,10 @@ import org.eclipse.jetty.ee10.webapp.WebInfConfiguration;
 import org.eclipse.jetty.http.HttpCookie;
 import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields.Mutable;
+import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.MimeTypes;
 import org.eclipse.jetty.http.MimeTypes.Wrapper;
+import org.eclipse.jetty.http.SetCookieParser;
 import org.eclipse.jetty.http2.server.HTTP2CServerConnectionFactory;
 import org.eclipse.jetty.server.AbstractConnector;
 import org.eclipse.jetty.server.ConnectionFactory;
@@ -787,6 +789,8 @@ public class JettyServletWebServerFactory extends AbstractServletWebServerFactor

                private final class SameSiteCookieHttpStreamWrapper extends HttpStream.Wrapper {

+                       private static final SetCookieParser setCookieParser = SetCookieParser.newInstance();
+
                        private final Request request;

                        private SameSiteCookieHttpStreamWrapper(HttpStream wrapped, Request request) {
@@ -807,7 +811,10 @@ public class JettyServletWebServerFactory extends AbstractServletWebServerFactor
                        }

                        private HttpCookieUtils.SetCookieHttpField applySameSiteIfNecessary(HttpField headerField) {
-                               HttpCookie cookie = HttpCookieUtils.getSetCookie(headerField);
+                               if (headerField.getHeader() != HttpHeader.SET_COOKIE) {
+                                       return null;
+                               }
+                               HttpCookie cookie = setCookieParser.parse(headerField.getValue());
                                if (cookie == null) {
                                        return null;
                                }

After some initial testing, it appears that works. Thank you. I'll run a full build to be more certain.

which BTW is a class that should have been internal and not for public usage

As you can see in the diff above, we're still making use of HttpCookieUtils.SetCookieHttpField. Is that OK, or should we be trying to move away from HttpCookieUtils and its inner-classes entirely?

[sbordet]

@wilkinsona I would change the return type to just be HttpField.

What does the caller to with the returned HttpField? If the caller can stick to the HttpField API that would be great.

Also, it's a bit weird that this code is in a HttpStream.Wrapper? Any particular reason to be there and not in a simpler Response.Wrapper?

[sbordet]

Okay so I looked at the original code, and the caller of applySameSiteIfNecessary() only needs an HttpField, so I'd stick with that as the return type of applySameSiteIfNecessary().

Regarding wrapping the response vs wrapping the HttpStream, it depends on your logic around applying SameSite.

Done in the HttpStream.Wrapper (the current code) means that no Handler (or Filter/Servlet) will see the modified Set-Cookie header, as prepareResponse() is called only at the time of committing the response (typically by the last Handler in the chain).

On the contrary, if the cookies are applied by some Handler, which then forwards the request processing to its child Handler, and you want the child Handler to see the Set-Cookie with the SameSite attribute, you must do it in a Response.Wrapper. Look at HttpFields.Wrapper.onAddField(), where you can intercept the Set-Cookie field apply your modification, and return the modified field.

They are equivalent, in the end. What's different is only who will be able to see your modifications: with the current code pretty much only the client, with a Response.Wrapper also other Handlers along the chain.

Makes sense?

[wilkinsona]

Also, it's a bit weird that this code is in a HttpStream.Wrapper? Any particular reason to be there and not in a simpler Response.Wrapper?

IIRC, the approach was heavily inspired by suggestions from @joakime and/or @gregw, but we were aiming at a moving target so I may well have misinterpreted something or seized on something that you later decided wasn't the best way to do it.

From what you've said a Handler-based approach sounds better. I've opened https://github.com/spring-projects/spring-boot/issues/37809 so that we can explore that.

[sbordet]

Closing as answered.