Repository: salesforce/tough-cookie
Pull request #424 (2024-06-23 19:57:41)

Fix validation errors not calling callbacks
I noticed this issue while upgrading `tough-cookie` in `jsdom` which was causing their test suite to hang. There is a sneaky little bug around our internal validations where, in several methods that accept callbacks, our validations error out directly and never invoke the given callback with the error. This can lead to code that waits for a callback that will never complete. This also affects functions wrapped in `Sync` which leverage callbacks to capture their return values.

The affected methods were:
- `getCookiesSync`
- `getCookieString`
- `getSetCookieStrings`
- `serializeSync`
- `serialize`
- `setCookieSync`
- `setCookie`
- `getCookiesSync`
- `getCookies`

There were also several validation errors hiding in support methods. These appear to be redundant since the type signatures present already do much of the validation. These have been removed from:
- `checkSameSiteContext`
- `isHostPrefixConditionMet`
- `isSecurePrefixConditionMet`
- `cookieCompare`
- `formatDate`
- `permutePath`


Patch:
diff --git a/api/docs/tough-cookie.cookiejar.getcookies.md b/api/docs/tough-cookie.cookiejar.getcookies.md
index 512f80a..b3530cf 100644
--- a/api/docs/tough-cookie.cookiejar.getcookies.md
+++ b/api/docs/tough-cookie.cookiejar.getcookies.md
@@ -9,7 +9,7 @@ Retrieve the list of cookies that can be sent in a Cookie header for the current
 **Signature:**
 
 ```typescript
-getCookies(url: string, callback: Callback<Cookie[]>): void;
+getCookies(url: string): Promise<Cookie[]>;
 ```
 
 ## Parameters
@@ -45,27 +45,11 @@ string
 The domain to store the cookie with.
 
 
-</td></tr>
-<tr><td>
-
-callback
-
-
-</td><td>
-
-[Callback](./tough-cookie.callback.md)<!-- -->&lt;[Cookie](./tough-cookie.cookie.md)<!-- -->\[\]&gt;
-
-
-</td><td>
-
-A function to call after a cookie has been successfully retrieved.
-
-
 </td></tr>
 </tbody></table>
 **Returns:**
 
-void
+Promise&lt;[Cookie](./tough-cookie.cookie.md)<!-- -->\[\]&gt;
 
 ## Remarks
 
diff --git a/api/docs/tough-cookie.cookiejar.getcookies_1.md b/api/docs/tough-cookie.cookiejar.getcookies_1.md
index f842189..5e6fbf8 100644
--- a/api/docs/tough-cookie.cookiejar.getcookies_1.md
+++ b/api/docs/tough-cookie.cookiejar.getcookies_1.md
@@ -9,7 +9,7 @@ Retrieve the list of cookies that can be sent in a Cookie header for the current
 **Signature:**
 
 ```typescript
-getCookies(url: string | URL, options: GetCookiesOptions | undefined, callback: Callback<Cookie[]>): void;
+getCookies(url: string, callback: Callback<Cookie[]>): void;
 ```
 
 ## Parameters
@@ -37,7 +37,7 @@ url
 
 </td><td>
 
-string \| URL
+string
 
 
 </td><td>
@@ -45,22 +45,6 @@ string \| URL
 The domain to store the cookie with.
 
 
-</td></tr>
-<tr><td>
-
-options
-
-
-</td><td>
-
-[GetCookiesOptions](./tough-cookie.getcookiesoptions.md) \| undefined
-
-
-</td><td>
-
-Configuration settings to use when retrieving the cookies.
-
-
 </td></tr>
 <tr><td>
 
diff --git a/api/docs/tough-cookie.cookiejar.getcookies_2.md b/api/docs/tough-cookie.cookiejar.getcookies_2.md
index 760ce4d..5e7d236 100644
--- a/api/docs/tough-cookie.cookiejar.getcookies_2.md
+++ b/api/docs/tough-cookie.cookiejar.getcookies_2.md
@@ -9,7 +9,7 @@ Retrieve the list of cookies that can be sent in a Cookie header for the current
 **Signature:**
 
 ```typescript
-getCookies(url: string | URL, options?: GetCookiesOptions | undefined): Promise<Cookie[]>;
+getCookies(url: string | URL, options: GetCookiesOptions | undefined, callback: Callback<Cookie[]>): void;
 ```
 
 ## Parameters
@@ -58,14 +58,30 @@ options
 
 </td><td>
 
-_(Optional)_ Configuration settings to use when retrieving the cookies.
+Configuration settings to use when retrieving the cookies.
+
+
+</td></tr>
+<tr><td>
+
+callback
+
+
+</td><td>
+
+[Callback](./tough-cookie.callback.md)<!-- -->&lt;[Cookie](./tough-cookie.cookie.md)<!-- -->\[\]&gt;
+
+
+</td><td>
+
+A function to call after a cookie has been successfully retrieved.
 
 
 </td></tr>
 </tbody></table>
 **Returns:**
 
-Promise&lt;[Cookie](./tough-cookie.cookie.md)<!-- -->\[\]&gt;
+void
 
 ## Remarks
 
diff --git a/api/docs/tough-cookie.cookiejar.getcookies_3.md b/api/docs/tough-cookie.cookiejar.getcookies_3.md
new file mode 100644
index 0000000..1aa8007
--- /dev/null
+++ b/api/docs/tough-cookie.cookiejar.getcookies_3.md
@@ -0,0 +1,75 @@
+<!-- Do not edit this file. It is automatically generated by API Documenter. -->
+
+[Home](./index.md) &gt; [tough-cookie](./tough-cookie.md) &gt; [CookieJar](./tough-cookie.cookiejar.md) &gt; [getCookies](./tough-cookie.cookiejar.getcookies_3.md)
+
+## CookieJar.getCookies() method
+
+Retrieve the list of cookies that can be sent in a Cookie header for the current URL.
+
+**Signature:**
+
+```typescript
+getCookies(url: string | URL, options?: GetCookiesOptions | undefined): Promise<Cookie[]>;
+```
+
+## Parameters
+
+<table><thead><tr><th>
+
+Parameter
+
+
+</th><th>
+
+Type
+
+
+</th><th>
+
+Description
+
+
+</th></tr></thead>
+<tbody><tr><td>
+
+url
+
+
+</td><td>
+
+string \| URL
+
+
+</td><td>
+
+The domain to store the cookie with.
+
+
+</td></tr>
+<tr><td>
+
+options
+
+
+</td><td>
+
+[GetCookiesOptions](./tough-cookie.getcookiesoptions.md) \| undefined
+
+
+</td><td>
+
+_(Optional)_ Configuration settings to use when retrieving the cookies.
+
+
+</td></tr>
+</tbody></table>
+**Returns:**
+
+Promise&lt;[Cookie](./tough-cookie.cookie.md)<!-- -->\[\]&gt;
+
+## Remarks
+
+- The array of cookies returned will be sorted according to [cookieCompare()](./tough-cookie.cookiecompare.md)<!-- -->.
+
+- The [Cookie.lastAccessed](./tough-cookie.cookie.lastaccessed.md) property will be updated on all returned cookies.
+
diff --git a/api/docs/tough-cookie.cookiejar.md b/api/docs/tough-cookie.cookiejar.md
index 57a132d..a155372 100644
--- a/api/docs/tough-cookie.cookiejar.md
+++ b/api/docs/tough-cookie.cookiejar.md
@@ -275,7 +275,7 @@ Alias of [CookieJar.deserializeSync()](./tough-cookie.cookiejar.deserializesync.
 </td></tr>
 <tr><td>
 
-[getCookies(url, callback)](./tough-cookie.cookiejar.getcookies.md)
+[getCookies(url)](./tough-cookie.cookiejar.getcookies.md)
 
 
 </td><td>
@@ -289,7 +289,7 @@ Retrieve the list of cookies that can be sent in a Cookie header for the current
 </td></tr>
 <tr><td>
 
-[getCookies(url, options, callback)](./tough-cookie.cookiejar.getcookies_1.md)
+[getCookies(url, callback)](./tough-cookie.cookiejar.getcookies_1.md)
 
 
 </td><td>
@@ -303,7 +303,21 @@ Retrieve the list of cookies that can be sent in a Cookie header for the current
 </td></tr>
 <tr><td>
 
-[getCookies(url, options)](./tough-cookie.cookiejar.getcookies_2.md)
+[getCookies(url, options, callback)](./tough-cookie.cookiejar.getcookies_2.md)
+
+
+</td><td>
+
+
+</td><td>
+
+Retrieve the list of cookies that can be sent in a Cookie header for the current URL.
+
+
+</td></tr>
+<tr><td>
+
+[getCookies(url, options)](./tough-cookie.cookiejar.getcookies_3.md)
 
 
 </td><td>
diff --git a/api/tough-cookie.api.md b/api/tough-cookie.api.md
index 8553381..672106f 100644
--- a/api/tough-cookie.api.md
+++ b/api/tough-cookie.api.md
@@ -81,6 +81,7 @@ export class CookieJar {
     static deserialize(strOrObj: string | object, store?: Store | Callback<CookieJar>, callback?: Callback<CookieJar>): unknown;
     static deserializeSync(strOrObj: string | SerializedCookieJar, store?: Store): CookieJar;
     static fromJSON(jsonString: string | SerializedCookieJar, store?: Store): CookieJar;
+    getCookies(url: string): Promise<Cookie[]>;
     getCookies(url: string, callback: Callback<Cookie[]>): void;
     getCookies(url: string | URL, options: GetCookiesOptions | undefined, callback: Callback<Cookie[]>): void;
     getCookies(url: string | URL, options?: GetCookiesOptions | undefined): Promise<Cookie[]>;
diff --git a/lib/cookie/cookie.ts b/lib/cookie/cookie.ts
index 30733fd..5c33ea6 100644
--- a/lib/cookie/cookie.ts
+++ b/lib/cookie/cookie.ts
@@ -71,7 +71,6 @@ function parseCookiePair(
   looseMode: boolean,
 ): Cookie | undefined {
   cookiePair = trimTerminator(cookiePair)
-  validators.validate(validators.isString(cookiePair), cookiePair)
 
   let firstEq = cookiePair.indexOf('=')
   if (looseMode) {
diff --git a/lib/cookie/cookieCompare.ts b/lib/cookie/cookieCompare.ts
index 97a0394..2a11095 100644
--- a/lib/cookie/cookieCompare.ts
+++ b/lib/cookie/cookieCompare.ts
@@ -1,5 +1,3 @@
-import { safeToString } from '../utils'
-import * as validators from '../validators'
 import type { Cookie } from './cookie'
 
 /**
@@ -65,8 +63,6 @@ const MAX_TIME = 2147483647000
  * @public
  */
 export function cookieCompare(a: Cookie, b: Cookie): number {
-  validators.validate(validators.isObject(a), safeToString(a))
-  validators.validate(validators.isObject(b), safeToString(b))
   let cmp: number
 
   // descending for length: b CMP a
diff --git a/lib/cookie/cookieJar.ts b/lib/cookie/cookieJar.ts
index 07cae3b..8dddf41 100644
--- a/lib/cookie/cookieJar.ts
+++ b/lib/cookie/cookieJar.ts
@@ -206,7 +206,6 @@ function getCookieContext(url: unknown): URL | urlParse<string> {
 
 type SameSiteLevel = keyof (typeof Cookie)['sameSiteLevel']
 function checkSameSiteContext(value: string): SameSiteLevel | undefined {
-  validators.validate(validators.isNonEmptyString(value), value)
   const context = String(value).toLowerCase()
   if (context === 'none' || context === 'lax' || context === 'strict') {
     return context
@@ -223,7 +222,6 @@ function checkSameSiteContext(value: string): SameSiteLevel | undefined {
  * @returns boolean
  */
 function isSecurePrefixConditionMet(cookie: Cookie): boolean {
-  validators.validate(validators.isObject(cookie), safeToString(cookie))
   const startsWithSecurePrefix =
     typeof cookie.key === 'string' && cookie.key.startsWith('__Secure-')
   return !startsWithSecurePrefix || cookie.secure
@@ -241,7 +239,6 @@ function isSecurePrefixConditionMet(cookie: Cookie): boolean {
  * @returns boolean
  */
 function isHostPrefixConditionMet(cookie: Cookie): boolean {
-  validators.validate(validators.isObject(cookie))
   const startsWithHostPrefix =
     typeof cookie.key === 'string' && cookie.key.startsWith('__Host-')
   return (
@@ -330,12 +327,16 @@ export class CookieJar {
     }
     let syncErr: Error | null = null
     let syncResult: T | undefined = undefined
-    fn.call(this, (error: Error | null, result?: T | undefined) => {
-      syncErr = error
-      syncResult = result
-    })
-    // These seem to be false positives; it can't detect that the value may be changed in the callback
-    // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition, @typescript-eslint/only-throw-error
+
+    try {
+      fn.call(this, (error: Error | null, result?: T | undefined) => {
+        syncErr = error
+        syncResult = result
+      })
+    } catch (err) {
+      syncErr = err as Error
+    }
+
     if (syncErr) throw syncErr
 
     return syncResult
@@ -436,41 +437,45 @@ export class CookieJar {
     }
     const promiseCallback = createPromiseCallback(callback)
     const cb = promiseCallback.callback
+    let context
 
-    if (typeof url === 'string') {
-      validators.validate(
-        validators.isNonEmptyString(url),
-        callback,
-        safeToString(options),
-      )
-    }
-
-    const context = getCookieContext(url)
+    try {
+      if (typeof url === 'string') {
+        validators.validate(
+          validators.isNonEmptyString(url),
+          callback,
+          safeToString(options),
+        )
+      }
 
-    let err
+      context = getCookieContext(url)
 
-    if (typeof url === 'function') {
-      return promiseCallback.reject(new Error('No URL was specified'))
-    }
+      if (typeof url === 'function') {
+        return promiseCallback.reject(new Error('No URL was specified'))
+      }
 
-    if (typeof options === 'function') {
-      options = defaultSetCookieOptions
-    }
+      if (typeof options === 'function') {
+        options = defaultSetCookieOptions
+      }
 
-    validators.validate(typeof cb === 'function', cb)
+      validators.validate(typeof cb === 'function', cb)
 
-    if (
-      !validators.isNonEmptyString(cookie) &&
-      !validators.isObject(cookie) &&
-      cookie instanceof String &&
-      cookie.length == 0
-    ) {
-      return promiseCallback.resolve(undefined)
+      if (
+        !validators.isNonEmptyString(cookie) &&
+        !validators.isObject(cookie) &&
+        cookie instanceof String &&
+        cookie.length == 0
+      ) {
+        return promiseCallback.resolve(undefined)
+      }
+    } catch (err) {
+      return promiseCallback.reject(err as Error)
     }
 
     const host = canonicalDomain(context.hostname) ?? null
     const loose = options?.loose || this.enableLooseMode
 
+    let err
     let sameSiteContext = null
     if (options?.sameSiteContext) {
       sameSiteContext = checkSameSiteContext(options.sameSiteContext)
@@ -731,6 +736,18 @@ export class CookieJar {
     return this.callSync(setCookieFn)
   }
 
+  /**
+   * Retrieve the list of cookies that can be sent in a Cookie header for the
+   * current URL.
+   *
+   * @remarks
+   * - The array of cookies returned will be sorted according to {@link cookieCompare}.
+   *
+   * - The {@link Cookie.lastAccessed} property will be updated on all returned cookies.
+   *
+   * @param url - The domain to store the cookie with.
+   */
+  getCookies(url: string): Promise<Cookie[]>
   /**
    * Retrieve the list of cookies that can be sent in a Cookie header for the
    * current URL.
@@ -803,13 +820,25 @@ export class CookieJar {
     }
     const promiseCallback = createPromiseCallback(callback)
     const cb = promiseCallback.callback
+    let context
+
+    try {
+      if (typeof url === 'string') {
+        validators.validate(validators.isNonEmptyString(url), cb, url)
+      }
+
+      context = getCookieContext(url)
 
-    if (typeof url === 'string') {
-      validators.validate(validators.isNonEmptyString(url), cb, url)
+      validators.validate(
+        validators.isObject(options),
+        cb,
+        safeToString(options),
+      )
+
+      validators.validate(typeof cb === 'function', cb)
+    } catch (parameterError) {
+      return promiseCallback.reject(parameterError as Error)
     }
-    const context = getCookieContext(url)
-    validators.validate(validators.isObject(options), cb, safeToString(options))
-    validators.validate(typeof cb === 'function', cb)
 
     const host = canonicalDomain(context.hostname)
     const path = context.pathname || '/'
@@ -1147,9 +1176,7 @@ export class CookieJar {
    */
   serialize(callback?: Callback<SerializedCookieJar>): unknown {
     const promiseCallback = createPromiseCallback<SerializedCookieJar>(callback)
-    const cb = promiseCallback.callback
 
-    validators.validate(typeof cb === 'function', cb)
     let type: string | null = this.store.constructor.name
     if (validators.isObject(type)) {
       type = null
diff --git a/lib/cookie/formatDate.ts b/lib/cookie/formatDate.ts
index 4c5ecfc..c865062 100644
--- a/lib/cookie/formatDate.ts
+++ b/lib/cookie/formatDate.ts
@@ -1,6 +1,3 @@
-import * as validators from '../validators'
-import { safeToString } from '../utils'
-
 /**
  * Format a {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date | Date} into
  * the {@link https://www.rfc-editor.org/rfc/rfc2616#section-3.3.1 | preferred Internet standard format}
@@ -16,6 +13,5 @@ import { safeToString } from '../utils'
  * @public
  */
 export function formatDate(date: Date): string {
-  validators.validate(validators.isDate(date), safeToString(date))
   return date.toUTCString()
 }
diff --git a/lib/cookie/permutePath.ts b/lib/cookie/permutePath.ts
index 4574b9e..d56b1a1 100644
--- a/lib/cookie/permutePath.ts
+++ b/lib/cookie/permutePath.ts
@@ -1,5 +1,3 @@
-import * as validators from '../validators'
-
 /**
  * Generates the permutation of all possible values that {@link pathMatch} the `path` parameter.
  * The array is in longest-to-shortest order.  Useful when building custom {@link Store} implementations.
@@ -14,7 +12,6 @@ import * as validators from '../validators'
  * @public
  */
 export function permutePath(path: string): string[] {
-  validators.validate(validators.isString(path))
   if (path === '/') {
     return ['/']
   }


Test patch:
diff --git a/lib/__tests__/cookieJar.spec.ts b/lib/__tests__/cookieJar.spec.ts
index cc1d52e..ebc8233 100644
--- a/lib/__tests__/cookieJar.spec.ts
+++ b/lib/__tests__/cookieJar.spec.ts
@@ -1169,12 +1169,12 @@ it('should fix issue #144', async () => {
   ])
 })
 
-it('should fix issue #145 - missing 2nd url parameter', () => {
+it('should fix issue #145 - missing 2nd url parameter', async () => {
   const cookieJar = new CookieJar()
-  expect(
+  await expect(
     // @ts-expect-error test case explicitly violates the expected function signature
-    () => cookieJar.setCookie('x=y; Domain=example.com; Path=/'),
-  ).toThrowError('`url` argument is not a string or URL.')
+    cookieJar.setCookie('x=y; Domain=example.com; Path=/', undefined),
+  ).rejects.toThrow('`url` argument is not a string or URL.')
 })
 
 it('should fix issue #197 - CookieJar().setCookie throws an error when empty cookie is passed', async () => {
@@ -1488,6 +1488,30 @@ describe('Synchronous API on async CookieJar', () => {
   })
 })
 
+describe('validation errors invoke callbacks', () => {
+  it('getCookies', () => {
+    const invalidUrl = {}
+    const cookieJar = new CookieJar()
+    // @ts-expect-error deliberately trigger validation error
+    void cookieJar.getCookies(invalidUrl, (err) => {
+      expect(err).toMatchObject({
+        message: '`url` argument is not a string or URL.',
+      })
+    })
+  })
+
+  it('setCookie', () => {
+    const invalidUrl = {}
+    const cookieJar = new CookieJar()
+    // @ts-expect-error deliberately trigger validation error
+    void cookieJar.setCookie('a=b', invalidUrl, (err) => {
+      expect(err).toMatchObject({
+        message: '`url` argument is not a string or URL.',
+      })
+    })
+  })
+})
+
 function createCookie(
   cookieString: string,
   options: {
diff --git a/test/cookie_jar_test.js b/test/cookie_jar_test.js
index a4191d2..7899eb6 100644
--- a/test/cookie_jar_test.js
+++ b/test/cookie_jar_test.js
@@ -780,7 +780,8 @@ vows
         topic: function() {
           const jar = new tough.CookieJar();
           jar.setCookie(
-            new String("x=y; Domain=example.com; Path=/"),
+            undefined,
+            undefined,
             this.callback
           );
         },