Repository: salesforce/tough-cookie (ts)

Issue:
CookieJar.setCookie does not slide expiration for set-cookie headers with max-age
The creation time from old-cookie is carried forward per the spec, but this causes the expiryTime() to be unaffected by subsequent set-cookie headers, so the cookie will expire when its creation + max-age > now, even if subsequent set-cookie headers specified max-age with the intent of making the expiration now + max-age.

I'm currently working around this by just having a Store implementation that passes `null` to the callback, so that the creation is not carried forward.
CookieJar.setCookie does not slide expiration for set-cookie headers with max-age
              > 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 https://github.com/salesforce/tough-cookie/issues/154#issuecomment-1839869846_
            

Pull request description:
Fix expiry time not updating when cookie is updated
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

Patch:
diff --git a/jest.config.ts b/jest.config.ts
index 85cdcf0..8a2e150 100644
--- a/jest.config.ts
+++ b/jest.config.ts
@@ -5,6 +5,9 @@ const config: JestConfigWithTsJest = {
   testEnvironment: 'node',
   rootDir: './lib/',
   testPathIgnorePatterns: ['./lib/__tests__/data/'],
+  fakeTimers: {
+    enableGlobally: true
+  }
 }
 
 export default config
diff --git a/lib/cookie/cookie.ts b/lib/cookie/cookie.ts
index 545d0a0..c3dd7da 100644
--- a/lib/cookie/cookie.ts
+++ b/lib/cookie/cookie.ts
@@ -671,7 +671,7 @@ export class Cookie {
   // elsewhere)
   expiryTime(now?: Date): number | undefined {
     if (this.maxAge != null) {
-      const relativeTo = now || this.creation || new Date()
+      const relativeTo = now || this.lastAccessed || new Date()
       const maxAge = typeof this.maxAge === 'number' ? this.maxAge : -Infinity
       const age = maxAge <= 0 ? -Infinity : maxAge * 1000
       if (relativeTo === 'Infinity') {


Test patch:
diff --git a/lib/__tests__/cookie.spec.ts b/lib/__tests__/cookie.spec.ts
index 5f497bf..439c5c9 100644
--- a/lib/__tests__/cookie.spec.ts
+++ b/lib/__tests__/cookie.spec.ts
@@ -31,8 +31,6 @@
 
 import { Cookie } from '../cookie/cookie'
 
-jest.useFakeTimers()
-
 // ported from test/api_test.js (cookie tests)
 describe('Cookie', () => {
   let cookie: Cookie
diff --git a/lib/__tests__/cookieJar.spec.ts b/lib/__tests__/cookieJar.spec.ts
index 5b3b760..d3f591d 100644
--- a/lib/__tests__/cookieJar.spec.ts
+++ b/lib/__tests__/cookieJar.spec.ts
@@ -36,8 +36,6 @@ import { MemoryCookieStore } from '../memstore'
 import { Store } from '../store'
 import { ParameterError } from '../validators'
 
-jest.useFakeTimers()
-
 // ported from:
 // - test/api_test.js (cookie jar tests)
 // - test/cookie_jar_test.js
@@ -1203,6 +1201,46 @@ it('should fix issue #282 - Prototype pollution when setting a cookie with the d
   expect('/notauth' in pollutedObject).toBe(false)
 })
 
+it('should fix issue #154 - Expiry should not be affected by creation date', async () => {
+  const now = Date.now()
+  const jar = new CookieJar()
+
+  await jar.setCookie('foo=bar; Max-Age=60;', 'https://example.com')
+
+  const initialCookies = await jar.getCookies('https://example.com')
+  expect(initialCookies).toEqual([
+    expect.objectContaining({
+      key: 'foo',
+      value: 'bar',
+      path: '/',
+      domain: 'example.com',
+      maxAge: 60,
+    }),
+  ])
+  // the expiry time should be 60s from now (0)
+  expect(initialCookies[0]?.expiryTime()).toBe(now + 60 * 1000)
+
+  // advance the time by 1s, so now = 1000
+  jest.advanceTimersByTime(1000)
+
+  await jar.setCookie('foo=bar; Max-Age=60;', 'https://example.com')
+
+  const updatedCookies = await jar.getCookies('https://example.com')
+  expect(updatedCookies).toEqual([
+    expect.objectContaining({
+      key: 'foo',
+      value: 'bar',
+      path: '/',
+      domain: 'example.com',
+      maxAge: 60,
+      // the creation time should be unchanged as per the spec
+      creation: initialCookies[0]?.creation,
+    }),
+  ])
+  // the expiry time should be 60s from now (1000)
+  expect(updatedCookies[0]?.expiryTime()).toBe(now + 60 * 1000 + 1000)
+})
+
 // special use domains under a sub-domain
 describe.each(['local', 'example', 'invalid', 'localhost', 'test'])(
   'when special use domain is dev.%s',
diff --git a/lib/__tests__/cookieSorting.spec.ts b/lib/__tests__/cookieSorting.spec.ts
index 99cecdc..784457c 100644
--- a/lib/__tests__/cookieSorting.spec.ts
+++ b/lib/__tests__/cookieSorting.spec.ts
@@ -2,8 +2,6 @@ import { Cookie } from '../cookie/cookie'
 import { cookieCompare } from '../cookie/cookieCompare'
 import { CookieJar } from '../cookie/cookieJar'
 
-jest.useFakeTimers()
-
 describe('Cookie sorting', () => {
   describe('assumptions', () => {
     it('should set the creation index during construction', () => {
diff --git a/lib/__tests__/cookieToAndFromJson.spec.ts b/lib/__tests__/cookieToAndFromJson.spec.ts
index 373a53e..aba7477 100644
--- a/lib/__tests__/cookieToAndFromJson.spec.ts
+++ b/lib/__tests__/cookieToAndFromJson.spec.ts
@@ -1,7 +1,5 @@
 import { Cookie } from '../cookie/cookie'
 
-jest.useFakeTimers()
-
 describe('Cookie.toJSON()', () => {
   it('should serialize a cookie to JSON', () => {
     const cookie = Cookie.parse(
diff --git a/lib/__tests__/jarSerialization.spec.ts b/lib/__tests__/jarSerialization.spec.ts
index ff2b3dd..3c6e819 100644
--- a/lib/__tests__/jarSerialization.spec.ts
+++ b/lib/__tests__/jarSerialization.spec.ts
@@ -35,8 +35,6 @@ import { MemoryCookieStore } from '../memstore'
 import { Store } from '../store'
 import { version } from '../version'
 
-jest.useFakeTimers()
-
 describe('cookieJar serialization', () => {
   it('should use the expected version', () => {
     expect(version).toBe('5.0.0-rc.0')
diff --git a/lib/__tests__/lifetime.spec.ts b/lib/__tests__/lifetime.spec.ts
index 025ad26..6a59e90 100644
--- a/lib/__tests__/lifetime.spec.ts
+++ b/lib/__tests__/lifetime.spec.ts
@@ -1,7 +1,5 @@
 import { Cookie } from '../cookie/cookie'
 
-jest.useFakeTimers()
-
 describe('Lifetime', () => {
   it('should be able to set a TTL using max-age', () => {
     const cookie = new Cookie()
diff --git a/lib/__tests__/nodeUtilFallback.spec.ts b/lib/__tests__/nodeUtilFallback.spec.ts
index 5a977fd..9d3c536 100644
--- a/lib/__tests__/nodeUtilFallback.spec.ts
+++ b/lib/__tests__/nodeUtilFallback.spec.ts
@@ -1,11 +1,9 @@
 import util from 'util'
 import { Cookie } from '../cookie/cookie'
 import { CookieJar } from '../cookie/cookieJar'
-import { MemoryCookieStore, inspectFallback } from '../memstore'
+import { inspectFallback, MemoryCookieStore } from '../memstore'
 import { getCustomInspectSymbol, getUtilInspect } from '../utilHelper'
 
-jest.useFakeTimers()
-
 describe('Node util module fallback for non-node environments', () => {
   describe('getCustomInspectSymbol', () => {
     it('should not be null in a node environment', () => {