Skip to content

Commit 2df9b7e

Browse files
committed
Fix hook state leaks and fetch option forwarding
Clone nested retry options before init hooks run so hook mutations do not leak across requests. Avoid canceling shared response bodies when an `afterResponse` hook wraps an existing body in a new `Response`. Forward all non-standard, non-Ky options directly to `fetch()` instead of filtering against `Request`, which removes the obsolete vendor allowlist and preserves fetch-only extensions like dispatcher and Next.js next.
1 parent 7f2eade commit 2df9b7e

7 files changed

Lines changed: 249 additions & 24 deletions

File tree

source/core/Ky.ts

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
mergeHooks,
2222
deletedParametersSymbol,
2323
} from '../utils/merge.js';
24+
import type {RetryOptions} from '../types/retry.js';
2425
import {normalizeRequestMethod, normalizeRetryOptions} from '../utils/normalize.js';
2526
import timeout from '../utils/timeout.js';
2627
import delay from '../utils/delay.js';
@@ -58,16 +59,35 @@ const createTextDecoder = (contentType: string): TextDecoder => {
5859

5960
const invalidSchemaMessage = 'The `schema` argument must follow the Standard Schema specification';
6061

62+
const cloneRetryOptions = (retry: RetryOptions | number): RetryOptions | number => {
63+
if (typeof retry !== 'object') {
64+
return retry;
65+
}
66+
67+
// Clone nested arrays too so init hooks can mutate retry config without leaking state across requests.
68+
return {
69+
...retry,
70+
...(retry.methods && {methods: [...retry.methods]}),
71+
...(retry.statusCodes && {statusCodes: [...retry.statusCodes]}),
72+
...(retry.afterStatusCodes && {afterStatusCodes: [...retry.afterStatusCodes]}),
73+
};
74+
};
75+
6176
// Shallow-clone mutable option properties so init hook mutations don't leak across requests.
6277
function cloneInitHookOptions(options: Options): Options {
63-
return {
78+
const clonedOptions: Options = {
6479
...options,
6580
json: cloneShallow(options.json),
66-
retry: cloneShallow(options.retry)!,
6781
context: cloneShallow(options.context)!,
6882
headers: cloneShallow(options.headers)!,
6983
searchParams: cloneShallow(options.searchParams) as SearchParamsOption | undefined,
7084
};
85+
86+
if (options.retry !== undefined) {
87+
clonedOptions.retry = cloneRetryOptions(options.retry);
88+
}
89+
90+
return clonedOptions;
7191
}
7292

7393
const validateJsonWithSchema = async (jsonValue: unknown, schema: StandardSchemaV1): Promise<unknown> => {
@@ -771,11 +791,12 @@ export class Ky {
771791

772792
// Cancel any response bodies we won't use to prevent memory leaks.
773793
// Uses fire-and-forget since hooks may have cloned the response, creating tee branches that block cancellation.
774-
if (clonedResponse !== nextResponse) {
794+
// If the hook wrapped an existing body into a new Response, both Response objects can still point at the same stream.
795+
if (clonedResponse !== nextResponse && clonedResponse.body !== nextResponse.body) {
775796
this.#cancelResponseBody(clonedResponse);
776797
}
777798

778-
if (response !== nextResponse) {
799+
if (response !== nextResponse && response.body !== nextResponse.body) {
779800
this.#cancelResponseBody(response);
780801
}
781802

@@ -881,7 +902,7 @@ export class Ky {
881902
this.request = new globalThis.Request(this.request, {signal: this.#options.signal});
882903
}
883904

884-
const nonRequestOptions = findUnknownOptions(this.request, this.#options);
905+
const nonRequestOptions = findUnknownOptions(this.#options);
885906
const retryRequest = this.#options.retry.limit > 0 ? this.request.clone() : undefined;
886907
const request = this.#wrapRequestWithUploadProgress(this.request, this.#options.body ?? undefined);
887908

source/core/constants.ts

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -259,13 +259,6 @@ export const kyOptionKeys: KyOptionsRegistry = {
259259
context: true,
260260
};
261261

262-
// Vendor-specific fetch options that should always be passed to fetch()
263-
// even if they appear on the Request object due to vendor patching.
264-
// See: https://github.com/sindresorhus/ky/issues/541
265-
export const vendorSpecificOptions = {
266-
next: true, // Next.js cache revalidation (revalidate, tags)
267-
} as const;
268-
269262
// Standard RequestInit options that should NOT be passed separately to fetch()
270263
// because they're already applied to the Request object.
271264
// Note: `dispatcher` and `priority` are NOT included here - they're fetch-only

source/utils/normalize.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,14 @@ export const normalizeRetryOptions = (retry: number | RetryOptions = {}): Intern
3737
throw new Error('retry.methods must be an array');
3838
}
3939

40-
retry.methods &&= retry.methods.map(method => method.toLowerCase());
41-
4240
if (retry.statusCodes && !Array.isArray(retry.statusCodes)) {
4341
throw new Error('retry.statusCodes must be an array');
4442
}
4543

46-
const normalizedRetry = Object.fromEntries(Object.entries(retry).filter(([, value]) => value !== undefined)) as RetryOptions;
44+
const normalizedRetry = Object.fromEntries(Object.entries({
45+
...retry,
46+
methods: retry.methods?.map(method => method.toLowerCase()),
47+
}).filter(([, value]) => value !== undefined)) as RetryOptions;
4748

4849
return {
4950
...defaultRetryOptions,

source/utils/options.ts

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
1-
import {kyOptionKeys, requestOptionsRegistry, vendorSpecificOptions} from '../core/constants.js';
1+
import {kyOptionKeys, requestOptionsRegistry} from '../core/constants.js';
22
import type {SearchParamsOption} from '../types/options.js';
33
import {deletedParametersSymbol} from './merge.js';
44

55
export const findUnknownOptions = (
6-
request: Request,
76
options: Record<string, unknown>,
87
): Record<string, unknown> => {
98
const unknownOptions: Record<string, unknown> = {};
@@ -14,13 +13,12 @@ export const findUnknownOptions = (
1413
continue;
1514
}
1615

17-
// An option is passed to fetch() if:
18-
// 1. It's not a standard RequestInit option (not in requestOptionsRegistry)
19-
// 2. It's not a ky-specific option (not in kyOptionKeys)
20-
// 3. Either:
21-
// a. It's not on the Request object, OR
22-
// b. It's a vendor-specific option that should always be passed (in vendorSpecificOptions)
23-
if (!(key in requestOptionsRegistry) && !(key in kyOptionKeys) && (!(key in request) || key in vendorSpecificOptions)) {
16+
// Forward every non-standard, non-Ky option to fetch().
17+
// We intentionally do not check whether the key also exists on `Request`, because some runtimes
18+
// patch `Request.prototype` with fetch-only extensions. For example, Next.js adds `next`, and the
19+
// old `key in request` heuristic dropped it unless Ky kept a special-case allowlist.
20+
// Passing all non-standard keys makes that allowlist unnecessary and preserves future fetch extensions too.
21+
if (!(key in requestOptionsRegistry) && !(key in kyOptionKeys)) {
2422
unknownOptions[key] = options[key];
2523
}
2624
}

test/fetch.ts

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,34 @@ test('unknown options are passed to fetch', async t => {
135135
await ky(fixture, {...options, fetch: customFetch}).text();
136136
});
137137

138+
test('unknown options with falsy values are passed to fetch', async t => {
139+
t.plan(3);
140+
141+
const customFetch: typeof fetch = async (request, init) => {
142+
t.is(init.customNull, null);
143+
t.is(init.customFalse, false);
144+
t.is(init.customZero, 0);
145+
return new Response(request.url);
146+
};
147+
148+
await ky(fixture, {
149+
customNull: null, customFalse: false, customZero: 0, fetch: customFetch,
150+
}).text();
151+
});
152+
153+
test('ky-specific options are not passed to fetch', async t => {
154+
const customFetch: typeof fetch = async (request, init) => {
155+
t.is(init.retry, undefined);
156+
t.is(init.timeout, undefined);
157+
t.is(init.hooks, undefined);
158+
t.is(init.throwHttpErrors, undefined);
159+
t.is(init.json, undefined);
160+
return new Response(request.url);
161+
};
162+
163+
await ky(fixture, {retry: 3, timeout: 5000, fetch: customFetch}).text();
164+
});
165+
138166
test('fetch-only options like dispatcher are passed to fetch', async t => {
139167
t.plan(1);
140168

@@ -148,6 +176,35 @@ test('fetch-only options like dispatcher are passed to fetch', async t => {
148176
await ky(fixture, {dispatcher: mockDispatcher, fetch: customFetch}).text();
149177
});
150178

179+
test.serial('fetch-only options like dispatcher are passed to fetch even when Request is patched', async t => {
180+
t.plan(1);
181+
182+
const mockDispatcher = {name: 'custom-agent'};
183+
const originalDescriptor = Object.getOwnPropertyDescriptor(Request.prototype, 'dispatcher');
184+
185+
try {
186+
Object.defineProperty(Request.prototype, 'dispatcher', {
187+
value: undefined,
188+
writable: true,
189+
enumerable: true,
190+
configurable: true,
191+
});
192+
193+
const customFetch: typeof fetch = async (request, init) => {
194+
t.is(init.dispatcher, mockDispatcher);
195+
return new Response(request.url);
196+
};
197+
198+
await ky(fixture, {dispatcher: mockDispatcher, fetch: customFetch}).text();
199+
} finally {
200+
if (originalDescriptor) {
201+
Object.defineProperty(Request.prototype, 'dispatcher', originalDescriptor);
202+
} else {
203+
delete (Request.prototype as any).dispatcher;
204+
}
205+
}
206+
});
207+
151208
test('priority option is passed to fetch', async t => {
152209
t.plan(1);
153210

test/hooks.ts

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,71 @@ test('afterResponse hook can return the provided response', async t => {
214214
t.true(originalResponse?.bodyUsed);
215215
});
216216

217+
test('afterResponse hook can wrap the provided body in a new response', async t => {
218+
const responseText = await ky('https://example.com', {
219+
fetch: async () => new Response('ok', {
220+
headers: {
221+
'content-type': 'text/plain',
222+
},
223+
}),
224+
hooks: {
225+
afterResponse: [
226+
({response}) => new Response(response.body, {
227+
headers: response.headers,
228+
}),
229+
],
230+
},
231+
}).text();
232+
233+
t.is(responseText, 'ok');
234+
});
235+
236+
test('afterResponse hook can wrap a streaming body in a new response', async t => {
237+
const customFetch = createStreamFetch({text: 'streamed'});
238+
239+
const responseText = await ky('https://example.com', {
240+
fetch: customFetch,
241+
hooks: {
242+
afterResponse: [
243+
({response}) => new Response(response.body, {
244+
headers: response.headers,
245+
}),
246+
],
247+
},
248+
}).text();
249+
250+
t.is(responseText, 'streamed');
251+
});
252+
253+
test('afterResponse hook chain can forward the same body through multiple hooks', async t => {
254+
let originalResponse: Response | undefined;
255+
256+
const customFetch = createStreamFetch({
257+
text: 'chained',
258+
onResponse(response) {
259+
originalResponse = response;
260+
},
261+
});
262+
263+
const responseText = await ky('https://example.com', {
264+
fetch: customFetch,
265+
hooks: {
266+
afterResponse: [
267+
({response}) => new Response(response.body, {
268+
headers: response.headers,
269+
}),
270+
({response}) => new Response(response.body, {
271+
headers: response.headers,
272+
}),
273+
],
274+
},
275+
}).text();
276+
277+
t.is(responseText, 'chained');
278+
// The original fetch response body should be cancelled since hooks created new wrappers
279+
t.true(originalResponse?.bodyUsed);
280+
});
281+
217282
test('afterResponse hook with multiple hooks cancels all unused clones', async t => {
218283
let originalResponse: Response | undefined;
219284
const clones: Response[] = [];
@@ -4725,6 +4790,81 @@ test('init hook in-place retry mutations do not leak across requests', async t =
47254790
t.deepEqual(seenLimits, [2, 2]);
47264791
});
47274792

4793+
test('init hook nested retry mutations do not leak across requests', async t => {
4794+
const seenMethods: string[][] = [];
4795+
4796+
const api = ky.extend({
4797+
retry: {
4798+
methods: ['get'],
4799+
},
4800+
hooks: {
4801+
init: [
4802+
options => {
4803+
seenMethods.push([...options.retry.methods]);
4804+
options.retry.methods.push('post');
4805+
},
4806+
],
4807+
},
4808+
});
4809+
4810+
const fetch: typeof globalThis.fetch = async () => new Response('ok');
4811+
4812+
await api.get('https://example.com', {fetch});
4813+
await api.get('https://example.com', {fetch});
4814+
4815+
t.deepEqual(seenMethods, [['get'], ['get']]);
4816+
});
4817+
4818+
test('init hook nested retry statusCodes mutations do not leak across requests', async t => {
4819+
const seenStatusCodes: number[][] = [];
4820+
4821+
const api = ky.extend({
4822+
retry: {
4823+
statusCodes: [500],
4824+
},
4825+
hooks: {
4826+
init: [
4827+
options => {
4828+
seenStatusCodes.push([...options.retry.statusCodes]);
4829+
options.retry.statusCodes.push(502);
4830+
},
4831+
],
4832+
},
4833+
});
4834+
4835+
const fetch: typeof globalThis.fetch = async () => new Response('ok');
4836+
4837+
await api.get('https://example.com', {fetch});
4838+
await api.get('https://example.com', {fetch});
4839+
4840+
t.deepEqual(seenStatusCodes, [[500], [500]]);
4841+
});
4842+
4843+
test('init hook nested retry afterStatusCodes mutations do not leak across requests', async t => {
4844+
const seenAfterStatusCodes: number[][] = [];
4845+
4846+
const api = ky.extend({
4847+
retry: {
4848+
afterStatusCodes: [429],
4849+
},
4850+
hooks: {
4851+
init: [
4852+
options => {
4853+
seenAfterStatusCodes.push([...options.retry.afterStatusCodes]);
4854+
options.retry.afterStatusCodes.push(503);
4855+
},
4856+
],
4857+
},
4858+
});
4859+
4860+
const fetch: typeof globalThis.fetch = async () => new Response('ok');
4861+
4862+
await api.get('https://example.com', {fetch});
4863+
await api.get('https://example.com', {fetch});
4864+
4865+
t.deepEqual(seenAfterStatusCodes, [[429], [429]]);
4866+
});
4867+
47284868
test('init hook in-place json mutations do not leak across requests', async t => {
47294869
let requestIdentifier = 0;
47304870
const seenRequestIdentifiers: string[] = [];

test/main.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -778,6 +778,21 @@ test('timeout option is cancelled when the promise is resolved', async t => {
778778
t.true(duration < 10);
779779
});
780780

781+
test('normalizing retry options does not mutate the caller retry object', async t => {
782+
const retry = {
783+
methods: ['GET'],
784+
};
785+
786+
await ky('https://example.com', {
787+
fetch: async () => new Response('ok'),
788+
retry,
789+
}).text();
790+
791+
t.deepEqual(retry, {
792+
methods: ['GET'],
793+
});
794+
});
795+
781796
test('searchParams option', async t => {
782797
const server = await createHttpTestServer(t);
783798

0 commit comments

Comments
 (0)