From ad4f2a4dc35ff3f68a0bf0e3e26952b48dd3c4ed Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Sun, 23 Jun 2024 16:55:31 -0300 Subject: [PATCH 1/4] 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` --- api/docs/tough-cookie.cookiejar.getcookies.md | 20 +--- .../tough-cookie.cookiejar.getcookies_1.md | 20 +--- .../tough-cookie.cookiejar.getcookies_2.md | 22 +++- .../tough-cookie.cookiejar.getcookies_3.md | 75 ++++++++++++ api/docs/tough-cookie.cookiejar.md | 20 +++- api/tough-cookie.api.md | 1 + lib/__tests__/cookieJar.spec.ts | 32 ++++- lib/cookie/cookie.ts | 1 - lib/cookie/cookieCompare.ts | 4 - lib/cookie/cookieJar.ts | 111 ++++++++++++------ lib/cookie/formatDate.ts | 4 - lib/cookie/permutePath.ts | 3 - 12 files changed, 216 insertions(+), 97 deletions(-) create mode 100644 api/docs/tough-cookie.cookiejar.getcookies_3.md diff --git a/api/docs/tough-cookie.cookiejar.getcookies.md b/api/docs/tough-cookie.cookiejar.getcookies.md index 512f80a4..b3530cfe 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): void; +getCookies(url: string): Promise; ``` ## Parameters @@ -45,27 +45,11 @@ string The domain to store the cookie with. - - - -callback - - - - -[Callback](./tough-cookie.callback.md)<[Cookie](./tough-cookie.cookie.md)\[\]> - - - - -A function to call after a cookie has been successfully retrieved. - - **Returns:** -void +Promise<[Cookie](./tough-cookie.cookie.md)\[\]> ## Remarks diff --git a/api/docs/tough-cookie.cookiejar.getcookies_1.md b/api/docs/tough-cookie.cookiejar.getcookies_1.md index f842189f..5e6fbf8b 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): void; +getCookies(url: string, callback: Callback): void; ``` ## Parameters @@ -37,7 +37,7 @@ url -string \| URL +string @@ -45,22 +45,6 @@ string \| URL The domain to store the cookie with. - - - -options - - - - -[GetCookiesOptions](./tough-cookie.getcookiesoptions.md) \| undefined - - - - -Configuration settings to use when retrieving the cookies. - - diff --git a/api/docs/tough-cookie.cookiejar.getcookies_2.md b/api/docs/tough-cookie.cookiejar.getcookies_2.md index 760ce4d2..5e7d236f 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; +getCookies(url: string | URL, options: GetCookiesOptions | undefined, callback: Callback): void; ``` ## Parameters @@ -58,14 +58,30 @@ options -_(Optional)_ Configuration settings to use when retrieving the cookies. +Configuration settings to use when retrieving the cookies. + + + + + +callback + + + + +[Callback](./tough-cookie.callback.md)<[Cookie](./tough-cookie.cookie.md)\[\]> + + + + +A function to call after a cookie has been successfully retrieved. **Returns:** -Promise<[Cookie](./tough-cookie.cookie.md)\[\]> +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 00000000..1aa8007a --- /dev/null +++ b/api/docs/tough-cookie.cookiejar.getcookies_3.md @@ -0,0 +1,75 @@ + + +[Home](./index.md) > [tough-cookie](./tough-cookie.md) > [CookieJar](./tough-cookie.cookiejar.md) > [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; +``` + +## Parameters + + + + +
+ +Parameter + + + + +Type + + + + +Description + + +
+ +url + + + + +string \| URL + + + + +The domain to store the cookie with. + + +
+ +options + + + + +[GetCookiesOptions](./tough-cookie.getcookiesoptions.md) \| undefined + + + + +_(Optional)_ Configuration settings to use when retrieving the cookies. + + +
+**Returns:** + +Promise<[Cookie](./tough-cookie.cookie.md)\[\]> + +## 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 57a132d0..a1553724 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. -[getCookies(url, callback)](./tough-cookie.cookiejar.getcookies.md) +[getCookies(url)](./tough-cookie.cookiejar.getcookies.md) @@ -289,7 +289,7 @@ Retrieve the list of cookies that can be sent in a Cookie header for the current -[getCookies(url, options, callback)](./tough-cookie.cookiejar.getcookies_1.md) +[getCookies(url, callback)](./tough-cookie.cookiejar.getcookies_1.md) @@ -303,7 +303,21 @@ Retrieve the list of cookies that can be sent in a Cookie header for the current -[getCookies(url, options)](./tough-cookie.cookiejar.getcookies_2.md) +[getCookies(url, options, callback)](./tough-cookie.cookiejar.getcookies_2.md) + + + + + + + +Retrieve the list of cookies that can be sent in a Cookie header for the current URL. + + + + + +[getCookies(url, options)](./tough-cookie.cookiejar.getcookies_3.md) diff --git a/api/tough-cookie.api.md b/api/tough-cookie.api.md index 4d2dcd6c..6bdc6b27 100644 --- a/api/tough-cookie.api.md +++ b/api/tough-cookie.api.md @@ -83,6 +83,7 @@ export class CookieJar { static deserialize(strOrObj: string | object, store?: Store | Callback, callback?: Callback): unknown; static deserializeSync(strOrObj: string | SerializedCookieJar, store?: Store): CookieJar; static fromJSON(jsonString: string | SerializedCookieJar, store?: Store): CookieJar; + getCookies(url: string): Promise; getCookies(url: string, callback: Callback): void; getCookies(url: string | URL, options: GetCookiesOptions | undefined, callback: Callback): void; getCookies(url: string | URL, options?: GetCookiesOptions | undefined): Promise; diff --git a/lib/__tests__/cookieJar.spec.ts b/lib/__tests__/cookieJar.spec.ts index cc1d52e4..41dbff79 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', async () => { + 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 index 30733fdd..5c33ea6c 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 97a03941..2a110950 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 4c094705..88ffa636 100644 --- a/lib/cookie/cookieJar.ts +++ b/lib/cookie/cookieJar.ts @@ -206,7 +206,6 @@ function getCookieContext(url: unknown): URL | urlParse { 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,22 @@ 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 - }) + + try { + fn.call(this, (error: Error | null, result?: T | undefined) => { + syncErr = error + syncResult = result + }) + } catch (err) { + if (err instanceof Error) { + syncErr = err + } else { + throw err + } + } + // 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 + if (syncErr) throw syncErr return syncResult @@ -436,41 +443,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 +742,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 /** * Retrieve the list of cookies that can be sent in a Cookie header for the * current URL. @@ -803,13 +826,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) + } - if (typeof url === 'string') { - validators.validate(validators.isNonEmptyString(url), cb, url) + context = getCookieContext(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 +1182,7 @@ export class CookieJar { */ serialize(callback?: Callback): unknown { const promiseCallback = createPromiseCallback(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 4c5ecfc0..c8650628 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 4574b9ec..d56b1a1a 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 ['/'] } From d1f8100fe1602ac1d16c73e5f4c58d9ed15dd078 Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Sun, 23 Jun 2024 16:59:07 -0300 Subject: [PATCH 2/4] Fix lint error --- lib/__tests__/cookieJar.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/__tests__/cookieJar.spec.ts b/lib/__tests__/cookieJar.spec.ts index 41dbff79..ebc8233f 100644 --- a/lib/__tests__/cookieJar.spec.ts +++ b/lib/__tests__/cookieJar.spec.ts @@ -1489,7 +1489,7 @@ describe('Synchronous API on async CookieJar', () => { }) describe('validation errors invoke callbacks', () => { - it('getCookies', async () => { + it('getCookies', () => { const invalidUrl = {} const cookieJar = new CookieJar() // @ts-expect-error deliberately trigger validation error From ff124e62262ecfa36d0d81012eddf188c80f9b09 Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Sun, 23 Jun 2024 17:05:59 -0300 Subject: [PATCH 3/4] Fix incorrect test --- test/cookie_jar_test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/cookie_jar_test.js b/test/cookie_jar_test.js index a4191d22..7899eb6d 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 ); }, From b127e4b0b73de4148751cad4f5e08ec9ae1d1ff4 Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Thu, 4 Jul 2024 11:45:00 -0300 Subject: [PATCH 4/4] Addressing code review comments --- lib/cookie/cookieJar.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/cookie/cookieJar.ts b/lib/cookie/cookieJar.ts index 88ffa636..0c03b8ce 100644 --- a/lib/cookie/cookieJar.ts +++ b/lib/cookie/cookieJar.ts @@ -334,15 +334,9 @@ export class CookieJar { syncResult = result }) } catch (err) { - if (err instanceof Error) { - syncErr = err - } else { - throw err - } + syncErr = err as Error } - // These seem to be false positives; it can't detect that the value may be changed in the callback - if (syncErr) throw syncErr return syncResult