Closed janbartel closed 5 months ago
Everything cookie related is now (should now be) RFC 6265 compliant. Note RFC 6265 prefers Max-Age
to Expires
(RFC 6265, 4.1.2.2). Ideally servers should be sending both in case there are still user agents that don't honour Max-Age
.
The listed problematic tests all look correct to me.
If there are tests that are inconsistent with RFC 6265 then I'd expect challenges against those to succeed.
Can you provide, ideally with references to RFC 6265, a little more detail on why you think these tests are invalid?
@markt-asf i think the tests should not fail a server that chooses to only send max-age, as allowed by the RFC. Sure having the option to also send expire is a good configuration for a server to have, but the tck should be testing for the preferred variant. Maybe if max-age is not present, the test can also check for expire.
Those tests aren't the easiest to read and I'm only going from the source but I don't see them requiring Expires
to be present. What am I missing? Or is the Expires
problem elsewhere (I'm wondering if we have hit another HttpClient cookie issue)?
@markt-asf RFC6265
prefers Max-Age
only in the case where the Set-Cookie
header contains both Max-Age
and Expires
. The Max-Age
field is from earlier versions of the cookie spec, eg RFC2965
. The Expires
field is introduced in RFC6265
, and all of the given examples of Set-Cookie
headers in the spec contain only an Expires
header, see Section 3.1:
Set-Cookie: SID=31d4d96e407aad42; Path=/; Secure; HttpOnly Set-Cookie: lang=en-US; Path=/; Domain=example.com
== User Agent -> Server ==
Cookie: SID=31d4d96e407aad42; lang=en-US
Notice that the Cookie header above contains two cookies, one named SID and one named lang. If the server wishes the user agent to persist the cookie over multiple "sessions" (e.g., user agent restarts), the server can specify an expiration date in the Expires attribute. Note that the user agent might delete the cookie before the expiration date if the user agent's cookie store exceeds its quota or if the user manually deletes the server's cookie.
== Server -> User Agent ==
Set-Cookie: lang=en-US; Expires=Wed, 09 Jun 2021 10:18:14 GMT
== User Agent -> Server ==
Cookie: SID=31d4d96e407aad42; lang=en-US
Finally, to remove a cookie, the server returns a Set-Cookie header with an expiration date in the past. The server will be successful in removing the cookie only if the Path and the Domain attribute in the Set-Cookie header match the values used when the cookie was created.
== Server -> User Agent ==
Set-Cookie: lang=; Expires=Sun, 06 Nov 1994 08:49:37 GMT
== User Agent -> Server ==
Cookie: SID=31d4d96e407aad42
Moreover, the spec instructs user agents to process the presence of a Max-Age
field into an expiry date (and has deleted all the instructions for how to handle -ve and 0 valued Max-Age
), see sections 5.2.1 and 5.2.2:
5.2.2. The Max-Age Attribute
If the attribute-name case-insensitively matches the string "Max- Age", the user agent MUST process the cookie-av as follows.
If the first character of the attribute-value is not a DIGIT or a "-" character, ignore the cookie-av.
If the remainder of attribute-value contains a non-DIGIT character, ignore the cookie-av.
Let delta-seconds be the attribute-value converted to an integer.
If delta-seconds is less than or equal to zero (0), let expiry-time be the earliest representable date and time. Otherwise, let the expiry-time be the current date and time plus delta-seconds seconds.
Append an attribute to the cookie-attribute-list with an attribute- name of Max-Age and an attribute-value of expiry-time.
It seems clear that Max-Age
is for backward compatibility, and I think the tck should mostly be testing for RFC6265
compatibility. Note also that many of the tck cookie tests explicitly call setVersion(0)
which is pointless because the Cookie
class hardcodes a value of 0
, which further makes it look like the tests just haven't been fully updated to RFC6265
.
@janbartel should you (or @olamy) prepare a PR indicating what you think the tests should be checking for?
I think the tests should pass so long as the server indicates that it has used a RFC6265 compliant Set-Cookie
. It should not test for a specific variant.
@janbartel A couple of those statements aren't consistent with my recollection of the various cookie specs, so I went back and did a more thorough read (last time I just searched RFC 6265 for Max-Age
and read those paragraphs).
First of all, what is hopefully all of the relevant facts:
Expires
was not introduced in RFC 6265. RFC 6265 explicitly states it does not introduce any new syntax. The Expires
attribute is from the original Netscape "Version 0" cookie specification. RFC 2109 replaced Expires
with Max-Age
.
RFC 6265 uses Expires
rather than Max-Age
both in the text and the example when discussing cookie expiration and deletion.
RFC 6265 no longer allows well-behaved servers to use Max-Age=0
to delete a cookie.
RFC 6265 notes that some user agents don't support Max-Age
.
RFC 6265 states that Max-Age
takes priority over Expires
where both are present and supported by the user agent.
RFC 6265 does not state whether well-behaved servers should set Expires
, Max-Age
or both.
The latest draft of the update to RFC 6265 adds a recommended limit to the Max-Age
/Expires
upper bound but doesn't propose any other changes that I can see in this area.
In the Servlet API Cookie
has [g|s]etMaxAge()
methods. It does not have equivalent methods for Expires
. The Cookie
Javadoc for setMaxAge()
discusses the effect of the value passed but does not explicitly state that the Max-Age
attribute is set. The implementation stores this attribute as Max-Age
. The code comment for the MAX_AGE
constant implies that the value will be sent in the Max-Age
cookie attribute.
There is plenty of advice on the web telling developers to use Max-Age=0
to delete a cookie. While an RFC 6265 compliant user agent should process this correctly, an RFC 6265 well-behaved server should never send it.
The TCK tests were updated for RFC 6265 but only to the minimum extent necessary for them to pass when cookies were parsed and generated as per RFC 6265. The choice to keep things like the RFC 2965 cookies tests and just adjust them to ensure a cookie that looked like an RFC 2965 cookie was treated as required by RFC 6265 was deliberate.
And now for the opinion/analysis:
It isn't clear if Max-Age
is a failed replacement for Expires
that has been effectively deprecated or if the continued support for Expires
is to account for user agents that haven't (fully) implemented the newer cookie specifications.
The Servlet API for cookies implies that Max-Age
will be used.
The Servlet API Javadoc for Cookie implication that Max-Age=0
can be used to delete a cookie goes against the RFC 6265 requirements for well-behaved servers.
I think that Servlet containers should send both Expires
and Max-Age
unless that would be inconsistent with the requirements of a well-behaved server in which case it is likely only Expires
would be sent. Whether the previous sentence should be turned into a MUST requirement is TBD and something to debate for Servlet 6.1.
What that means for the TCK is that I think the TCK needs to use the algorithm described in sections 5.2.1 and 5.2.2 of RFC 6265 to parse both Expires
and Max-Age
and confirm that the resulting expiry time is consistent with the expected value. That is going to add some complications as the TCK will need to allow some margin to allow for clock skew, processing time, etc.
In terms of the challenge, I think the only test impacted is setMaxAgeZeroTest
as that test is the only one that requires a Max-Age
attribute to be present in the Set-Cookie
header. I think it would be reasonable to accept a challenge for that test on the grounds either that RFC 6265 allows Expires
or Max-Age
or that a well-behaved server should not be sending Max-Age=0
in this case. The other cookie tests don't appear to be affected (but again I am only going by looking at the source code and might have missed something).
@markt-asf thanks for the thorough analysis.
I also think the tests should remove calls to setVersion
.
I should have a PR for the Cookie TCK tests to address this shortly. I've done quite a lot of clean-up including the removal of the calls to setVersion
apart from where we want to call it to test it truly is a NO-OP.
My approach for the setMaxAgeZeroTest
was:
MaxAge
attribute and there was an Expires
attributesetMaxAgePositiveTest
) that checks the actual date returned is in the past.Fixed via #583
Challenged Tests:
TCK Version: Jakarta EE Platform TCK 10.0.0
Tested Implementation: Jetty
Description: Some of the cookie tests are testing
RFC6265
behaviour (eg$Domain
attribute being parsed as a separate cookie) so it seems the tck wants to conform to that spec, yet other tests are still testing older cookie spec behaviour (eg requiring the presence ofMax-Age
instead ofExpires
attribute). The tck should be consistent, and should preferably move toRFC6265
.