Remove special cookie attributes with null value (#13540)

* Remove special cookie attributes with null value

Fix #13539 by allowing null to be used to remove cookie special attributes.

* Fix handling of bad values

* updates from review

Author: gregw

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCookie.java

@@ -410,9 +410,12 @@ public String getAttributeValue()
          */
         public static SameSite from(String sameSite)
         {
-            if (sameSite == null)
+            if (StringUtil.isBlank(sameSite))
                 return null;
-            return CACHE.get(sameSite);
+            SameSite ss = CACHE.get(sameSite);
+            if (ss == null)
+                throw new IllegalArgumentException("Unknown same site");
+            return ss;
         }
     }
 
@@ -560,41 +563,26 @@ public Builder attribute(String name, String value)
             // 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);
-                }
-                case "partitioned" ->
-                {
-                    if (!isTruthy(value))
-                        throw new IllegalArgumentException("Invalid Partitioned attribute");
-                    partitioned(true);
-                }
+                case "expires" -> expires(StringUtil.isBlank(value) ? null : parseExpires(value));
+                case "httponly" -> httpOnly(asBoolean("httponly", value));
+                case "max-age" -> maxAge(StringUtil.isBlank(value) ? -1 : Long.parseLong(value));
+                case "samesite" -> sameSite(SameSite.from(value));
+                case "secure" -> secure(asBoolean("secure", value));
+                case "partitioned" -> partitioned(asBoolean("partitioned", value));
                 default -> _attributes = lazyAttributePut(_attributes, name, value);
             }
             return this;
         }
 
-        private boolean isTruthy(String value)
+        private boolean asBoolean(String attribute, String value)
         {
-            return value != null && (value.isEmpty() || "true".equalsIgnoreCase(value));
+            if (value == null)
+                return false;
+            if (value.isEmpty() || "true".equalsIgnoreCase(value))
+                return true;
+            if ("false".equalsIgnoreCase(value))
+                return false;
+            throw new IllegalArgumentException("Invalid value for " + attribute);
         }
 
         public Builder comment(String comment)
@@ -653,7 +641,7 @@ public Builder secure(boolean secure)
 
         public Builder sameSite(SameSite sameSite)
         {
-            _attributes = lazyAttributePut(_attributes, SAME_SITE_ATTRIBUTE, sameSite.attributeValue);
+            _attributes = lazyAttributePut(_attributes, SAME_SITE_ATTRIBUTE, sameSite == null ? null : sameSite.attributeValue);
             return this;
         }
 

jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpCookieTest.java

@@ -16,12 +16,15 @@
 import java.time.Instant;
 import java.util.List;
 
+import org.junit.jupiter.api.Test;
 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.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
@@ -105,4 +108,53 @@ public void testParseInvalidAttributes(String name, String value, Class<? extend
             .attribute(name, value));
     }
 
+    @Test
+    public void testRemoveSpecialAttributes()
+    {
+        HttpCookie cookie = HttpCookie.build("name", "value")
+            .expires(Instant.now())
+            .httpOnly(true)
+            .maxAge(1000)
+            .sameSite(HttpCookie.SameSite.STRICT)
+            .secure(true)
+            .partitioned(true)
+            .build();
+
+        assertThat(cookie.getExpires(), notNullValue());
+        assertThat(cookie.isHttpOnly(), is(true));
+        assertThat(cookie.getMaxAge(), is(1000L));
+        assertThat(cookie.getSameSite(), is(HttpCookie.SameSite.STRICT));
+        assertThat(cookie.isSecure(), is(true));
+        assertThat(cookie.isPartitioned(), is(true));
+
+        HttpCookie noAttributes = HttpCookie.build(cookie)
+            .expires(null)
+            .httpOnly(false)
+            .maxAge(-1)
+            .sameSite(null)
+            .secure(false)
+            .partitioned(false)
+            .build();
+        assertThat(noAttributes.getExpires(), nullValue());
+        assertThat(noAttributes.isHttpOnly(), is(false));
+        assertThat(noAttributes.getMaxAge(), is(-1L));
+        assertThat(noAttributes.getSameSite(), nullValue());
+        assertThat(noAttributes.isSecure(), is(false));
+        assertThat(noAttributes.isPartitioned(), is(false));
+
+        noAttributes = HttpCookie.build(cookie)
+            .attribute(HttpCookie.EXPIRES_ATTRIBUTE, null)
+            .attribute(HttpCookie.HTTP_ONLY_ATTRIBUTE, null)
+            .attribute(HttpCookie.MAX_AGE_ATTRIBUTE, null)
+            .attribute(HttpCookie.SAME_SITE_ATTRIBUTE, null)
+            .attribute(HttpCookie.SECURE_ATTRIBUTE, null)
+            .attribute(HttpCookie.PARTITIONED_ATTRIBUTE, null)
+            .build();
+        assertThat(noAttributes.getExpires(), nullValue());
+        assertThat(noAttributes.isHttpOnly(), is(false));
+        assertThat(noAttributes.getMaxAge(), is(-1L));
+        assertThat(noAttributes.getSameSite(), nullValue());
+        assertThat(noAttributes.isSecure(), is(false));
+        assertThat(noAttributes.isPartitioned(), is(false));
+    }
 }