Fix validation errors not calling callbacks (#424)

* 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` * Fix lint error * Fix incorrect test * Addressing code review comments
salesforce · Jul 5, 2024 · 120058c · 120058c
1 parent 972efeb
commit 120058c
Show file tree

Hide file tree

Showing 13 changed files with 213 additions and 99 deletions.
diff --git 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
@@ -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,30 +37,14 @@ url
 
 </td><td>
 
-string \| URL
+string
 
 
 </td><td>
 
 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
@@ -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
@@ -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
@@ -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
@@ -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/__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/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
@@ -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