Clarify Cookie attribute behavior for empty and null values

[bclozel]

#175 introduced the support for Cookie attributes in order to generally address the need for attributes such as "SameSite".

Right now, third party cookie restrictions are being rolled out in Google Chrome and many application will need to add the "Partitioned" attribute to keep things working. I have tried to use the attributes API for this purpose and failed:

Cookie cookie = new Cookie("test", "value");
// set attribute value
cookie.setAttribute("attribute", "attrvalue");
// set empty attribute
cookie.setAttribute("Partitioned", "");
// set empty attribute and then null it out
cookie.setAttribute("nullable", "value");
cookie.setAttribute("nullable", null);
response.addCookie(cookie);

Container	Cookie header
Tomcat 10.1.17	Set-Cookie: test=value; attribute=attrvalue; Partitioned=
Jetty 12.0.5	Set-Cookie: test=value; attribute=attrvalue
Undertow 2.3.10.Final	Set-Cookie: test=value

Note, I have removed the "nullable" attribute in my tests with Tomcat 10.1.17 since it currently fails for null attribute values with the following:

java.lang.NullPointerException: Cannot invoke "String.toCharArray()" because "value" is null
	at org.apache.tomcat.util.http.Rfc6265CookieProcessor.validateAttribute(Rfc6265CookieProcessor.java:270) ~[tomcat-embed-core-10.1.17.jar:10.1.17]
	at org.apache.tomcat.util.http.Rfc6265CookieProcessor.generateHeader(Rfc6265CookieProcessor.java:193) ~[tomcat-embed-core-10.1.17.jar:10.1.17]

Could you clarify the cookie attributes behavior for the following cases:

1. Add a cookie attribute with a null value - should this remove the attribute from the map or should it be rejected?
2. Add a cookie attribute with an empty value - ideally, we should support the Partitioned case (so no additional = sign)

[markt-asf]

RFC 6265 allows cookie values to be empty. Given that, I think we should take the following approach:

• null - remove attribute
• empty - attr=

The above should be what is currently implemented.

Implementations will need to add special handling for attributes like Partioned and ignore the value when writing the cookie header.
If we want consistency with how we implemented HttpOnly then the special handling for Partioned would be "include the attribute name only in the Set-Cookie header iff Boolan.parseBoolean(attrValue) returns true.
Personally, I favour consistency with HttpOnly.

[volosied]

With the Partition cookies feature being added, I encountered similar problems when I tried to added the Partitioned attribute via cookie.setAttribute("Partitioned", ""). My server only rendered it as partitioned=.

With the propose change, I would need to usecookie.setAttribute("Partitioned", "true") in order to have Partitioned; rendered on a cookie?

Just to throw some other ideas out: Is setAttribute trying to do too much here? Should we other method such as setAttributeOnly() and removeAttribute();

Lastly, could a solution for this be included in Servlet 6.1 rather than waiting for another whole release cycle? I'll send out an email in the servlet dev mailing list, too. Thanks!

[joakime]

Per the parsing steps in https://datatracker.ietf.org/doc/html/rfc6265#section-5.2

These are all equivalent Set-Cookie strings.

Set-Cookie: test=value; Secure; HttpOnly; Partitioned
Set-Cookie: test=value; Secure=; HttpOnly=; Partitioned=
Set-Cookie: test=value; Partitioned=; Secure; HttpOnly=;

Why do we need special handling for Partitioned? (or HttpOnly and Secure for that matter)

Alternatively, knowing the parsing rules, an empty string value used in cookie.setAttribtue("Name", "") could always produce the attribute without an equals sign on the Set-Cookie line too.

[volosied]

Thanks for pointing that out @joakime! I didn't realize that.

I saw Mark made a PR: https://github.com/jakartaee/servlet/pull/572/files

The changes look good to me. Could you also take a look? Perhaps it could be in the M2?