Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CookieJar.setCookie does not slide expiration for set-cookie headers with max-age #343

Closed
VelynM opened this issue Dec 11, 2023 · 1 comment · Fixed by #345
Closed

CookieJar.setCookie does not slide expiration for set-cookie headers with max-age #343

VelynM opened this issue Dec 11, 2023 · 1 comment · Fixed by #345

Comments

@VelynM
Copy link

VelynM commented Dec 11, 2023

My reading of RFC6265 is that expiry-time and creation-time are two distinct fields (Section 5.3), and creation-time is not used for expiration at all. Steps 5.3.2 and 5.3.11.3 cover creation-time and expiry-time is covered by 5.3.3, and sections 5.2.1 and 5.2.2.

As far as I can tell, this issue still exists, assuming my past interpretation of RFC6265 was correct.

Originally posted by @VelynM in #154 (comment)

colincasey added a commit that referenced this issue Dec 13, 2023
The `cookie.expiryTime()` method was calculating an `expiry-time` value using `creation-time + max-age` if the `max-age` value was set. This is not correct as RFC6265 does not say anything about `creation-time` having any effect on `max-age`.

The main issue is that `max-age` should be stored as a date (see text below), not an offset number as it currently is implemented.

> 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.

Because `max-age` is an offset, the `expiry-time` now needs to be calculated from "some" point in time and `creation-time` was chosen. In many cases this works fine but, if you have an existing cookie stored with a `max-age` attribute and then update it, the `expiry-time` will be incorrectly report the previous value's `expiry-time`. This is due to the following rule regarding `creation-time` from the spec:

> 5.3.  Storage Model
>
>     11.  If the cookie store contains a cookie with the same name,
>          domain, and path as the newly created cookie:
>
>          1.  Let old-cookie be the existing cookie with the same name,
>              domain, and path as the newly created cookie.  (Notice that
>              this algorithm maintains the invariant that there is at most
>              one such cookie.)
>
>          2.  If the newly created cookie was received from a "non-HTTP"
>              API and the old-cookie's http-only-flag is set, abort these
>              steps and ignore the newly created cookie entirely.
>
>          3.  Update the creation-time of the newly created cookie to
>              match the creation-time of the old-cookie.
>
>          4.  Remove the old-cookie from the cookie store.

This should make it clear that `creation-time` should not be used to calculate the `expiry-time`. Given that we still need to calculate the `expiry-time` due to how `max-age` is stored, the better value to use in this circumstance is the `last-access-time` which is always updated when `setCookie(...)` is called.

Fixes #343
Fixes #154
@colincasey
Copy link
Contributor

@VelynM Thanks for raising this issue (again 😄 )

I agree with your interpretation and confirmed that Chrome, Firefox, and Safari also follow this interpretation. This should be fixed by #345.

wjhsf pushed a commit that referenced this issue Feb 8, 2024
* chore: update LWC to v5.0.2

* chore: update dependencies

* chore: upgrade eslint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants