From a1624530ee776c47413047527990e79a3bbb51bb Mon Sep 17 00:00:00 2001 From: Dudu-0223 Date: Thu, 25 Jun 2026 16:32:39 +0800 Subject: [PATCH] fix: address codex review round 3 Resource-lifecycle and error-classification fixes in the local fetch provider: - Classify a timeout that fires DURING the body read as WEB_FETCH_TIMEOUT, not WEB_ABORTED: thread the controller signal into the body-read translate path and recover the timeout WebError from signal.reason, honoring the public WEB_FETCH_TIMEOUT contract for a stalled response body. - Cancel the response body before every blocked-redirect throw path (cross-origin, invalid target, missing Location), so a rejected redirect with a large or streaming body does not leak the socket after the tool returns WEB_REDIRECT_BLOCKED. - Cancel the body when charset validation fails, matching the unsupported-content-type and over-size paths (the round-1 charset check threw before readCapped owned the stream). --- packages/web/web-fetch-local/src/provider.ts | 73 +++++++++++++------ .../web-fetch-local/tests/fetch-local.spec.ts | 57 ++++++++++++++- 2 files changed, 108 insertions(+), 22 deletions(-) diff --git a/packages/web/web-fetch-local/src/provider.ts b/packages/web/web-fetch-local/src/provider.ts index c8b99d08b0..13ae7af708 100644 --- a/packages/web/web-fetch-local/src/provider.ts +++ b/packages/web/web-fetch-local/src/provider.ts @@ -71,7 +71,7 @@ export class LocalFetchProvider implements WebFetchProvider { const timer = setTimeout(() => { controller.abort(new WebError('web fetch timed out', 'WEB_FETCH_TIMEOUT')) }, timeoutMs) try { - return await this.followAndRead(request.url, controller, timeoutMs) + return await this.followAndRead(request.url, controller) } finally { clearTimeout(timer) if (exec?.signal !== undefined) exec.signal.removeEventListener('abort', onAbort) @@ -79,41 +79,50 @@ export class LocalFetchProvider implements WebFetchProvider { } /** Follow same-origin redirects up to the hop cap, then read the final response. */ - private async followAndRead(initialUrl: string, controller: AbortController, timeoutMs: number): Promise { + private async followAndRead(initialUrl: string, controller: AbortController): Promise { let currentUrl = validateFetchUrl(initialUrl, this.limits.maxUrlLength) for (let hop = 0; hop <= this.limits.maxRedirects; hop++) { - const response = await this.requestOnce(currentUrl, controller, timeoutMs) + const response = await this.requestOnce(currentUrl, controller) if (isRedirectStatus(response.status)) { const location = response.headers.get('location') if (location === null) { - // A redirect status with no Location is not a usable resource. + // A redirect status with no Location is not a usable resource. Cancel + // the (possibly streaming) body before throwing so no socket leaks. + await response.body?.cancel() throw new WebError(`redirect response (HTTP ${response.status}) without a Location header`, 'WEB_PROVIDER_ERROR') } const target = resolveRedirect(location, currentUrl) // Re-validate the target against the same transport hygiene a direct // request gets: a redirect must not be a back door to a credentialed, - // non-http(s), or over-long URL that validateFetchUrl would reject. - const validatedTarget = validateFetchUrl(target.toString(), this.limits.maxUrlLength) - if (!isSameOrigin(validatedTarget, currentUrl)) { - throw new WebError( - `cross-origin redirect to ${validatedTarget.origin} is not followed automatically; retry against that URL directly`, - 'WEB_REDIRECT_BLOCKED', - ) + // non-http(s), or over-long URL that validateFetchUrl would reject. A + // rejection here must still cancel the body first (see below). + let validatedTarget: URL + try { + validatedTarget = validateFetchUrl(target.toString(), this.limits.maxUrlLength) + if (!isSameOrigin(validatedTarget, currentUrl)) { + throw new WebError( + `cross-origin redirect to ${validatedTarget.origin} is not followed automatically; retry against that URL directly`, + 'WEB_REDIRECT_BLOCKED', + ) + } + } catch (error: unknown) { + await response.body?.cancel() + throw error } await response.body?.cancel() currentUrl = validatedTarget continue } - return await this.readBody(response, currentUrl) + return await this.readBody(response, currentUrl, controller.signal) } throw new WebError(`exceeded the maximum of ${this.limits.maxRedirects} redirects`, 'WEB_REDIRECT_BLOCKED') } - private async requestOnce(url: URL, controller: AbortController, _timeoutMs: number): Promise { + private async requestOnce(url: URL, controller: AbortController): Promise { try { return await fetch(url, { method: 'GET', @@ -122,12 +131,12 @@ export class LocalFetchProvider implements WebFetchProvider { signal: controller.signal, }) } catch (error: unknown) { - throw translateAbortOrNetwork(error) + throw translateAbortOrNetwork(error, controller.signal) } } /** Read, byte-cap, classify, and decode the final response body. */ - private async readBody(response: Response, finalUrl: URL): Promise { + private async readBody(response: Response, finalUrl: URL, signal: AbortSignal): Promise { const contentType = response.headers.get('content-type') const kind = classifyContentType(contentType) if (kind === undefined) { @@ -136,9 +145,16 @@ export class LocalFetchProvider implements WebFetchProvider { } // Resolve the decoder BEFORE reading the body so an unsupported charset - // fails without consuming the stream. - const decoder = decoderForCharset(parseCharset(contentType)) - const { bytes, truncatedByBytes } = await this.readCapped(response) + // fails without consuming the stream — but cancel the body on that failure + // so the socket does not leak (matching the unsupported-content-type path). + let decoder: TextDecoder + try { + decoder = decoderForCharset(parseCharset(contentType)) + } catch (error: unknown) { + await response.body?.cancel() + throw error + } + const { bytes, truncatedByBytes } = await this.readCapped(response, signal) const decoded = decoder.decode(bytes) const truncatedByChars = decoded.length > this.limits.maxBodyChars const content = truncatedByChars ? decoded.slice(0, this.limits.maxBodyChars) : decoded @@ -159,7 +175,7 @@ export class LocalFetchProvider implements WebFetchProvider { * past the cap is cut short (`truncatedByBytes`) rather than rejected, so a * server that under-reports still yields a bounded usable body. */ - private async readCapped(response: Response): Promise<{ bytes: Uint8Array; truncatedByBytes: boolean }> { + private async readCapped(response: Response, signal: AbortSignal): Promise<{ bytes: Uint8Array; truncatedByBytes: boolean }> { const declared = response.headers.get('content-length') if (declared !== null) { const length = Number(declared) @@ -195,7 +211,7 @@ export class LocalFetchProvider implements WebFetchProvider { } } catch (error: unknown) { /* v8 ignore next -- mid-stream read fault needs a network drop after headers; translate path covered by request-phase tests. */ - throw translateAbortOrNetwork(error) + throw translateAbortOrNetwork(error, signal) } finally { /* v8 ignore next 4 -- cancel() after a completed/broken read settles without rejecting; unobserved best-effort cleanup. */ await reader.cancel().catch(() => { @@ -235,9 +251,24 @@ function resolveRedirect(location: string, base: URL): URL { * already-typed `WebError` pass through; an `AbortError` becomes `WEB_ABORTED`; * anything else is a transport/network failure (`WEB_PROVIDER_ERROR`). */ -function translateAbortOrNetwork(error: unknown): WebError { +/** + * Translate a thrown fetch/stream error into a `WebError`. Our own + * `WEB_FETCH_TIMEOUT` (passed to `controller.abort(reason)`) and any other + * already-typed `WebError` pass through; an `AbortError` becomes `WEB_ABORTED`, + * UNLESS the abort was our timeout — the body-read reader surfaces a generic + * `AbortError` rather than the abort reason, so we recover the timeout's + * `WebError` from `signal.reason`; anything else is a transport/network failure + * (`WEB_PROVIDER_ERROR`). + */ +function translateAbortOrNetwork(error: unknown, signal?: AbortSignal): WebError { if (error instanceof WebError) return error if (error instanceof DOMException && error.name === 'AbortError') { + // A timeout abort carries its WebError as the signal reason; honor the + // WEB_FETCH_TIMEOUT contract instead of reporting a generic cancellation. + // (Node rejects WITH the reason — the WebError branch above — so this only + // fires on a runtime that surfaces a bare AbortError while reason is set.) + /* v8 ignore next */ + if (signal?.reason instanceof WebError) return signal.reason return new WebError('web fetch aborted', 'WEB_ABORTED', { cause: error }) } return new WebError(`web fetch failed: ${String(error)}`, 'WEB_PROVIDER_ERROR', { cause: error }) diff --git a/packages/web/web-fetch-local/tests/fetch-local.spec.ts b/packages/web/web-fetch-local/tests/fetch-local.spec.ts index 54e8de20c0..75a7cb1580 100644 --- a/packages/web/web-fetch-local/tests/fetch-local.spec.ts +++ b/packages/web/web-fetch-local/tests/fetch-local.spec.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { createServer, type IncomingMessage, type Server, type ServerResponse } from 'node:http' import { AddressInfo } from 'node:net' import { Context } from 'cordis' @@ -32,6 +32,7 @@ beforeEach(async () => { }) afterEach(async () => { + vi.unstubAllGlobals() await new Promise(resolve => server.close(() => { resolve() })) }) @@ -250,6 +251,20 @@ describe('LocalFetchProvider invalid URLs and abort', () => { .rejects.toThrow(expect.objectContaining({ code: 'WEB_FETCH_TIMEOUT' })) }) + it('classifies a timeout DURING the body read as WEB_FETCH_TIMEOUT, not WEB_ABORTED', async () => { + // Promise body that resolves headers (so fetch() returns) but a content-length + // that outlasts the bytes sent, so readCapped()'s reader awaits more and the + // timeout fires mid-read — the reader then surfaces a generic AbortError that + // must still be recovered as the timeout reason via signal.reason. + handler = (_req, res) => { + res.writeHead(200, { 'content-type': 'text/plain', 'content-length': '100' }) + res.write('partial') + // never send the remaining bytes nor end the response + } + await expect(provider({ timeoutMs: 80 }).fetch({ url: base })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_FETCH_TIMEOUT' })) + }) + it('maps a connection failure to WEB_PROVIDER_ERROR', async () => { // Port 1 on loopback is not listening: a real connection failure (not abort). await expect(provider().fetch({ url: 'http://127.0.0.1:1/' })) @@ -263,6 +278,46 @@ describe('LocalFetchProvider invalid URLs and abort', () => { }) }) +describe('LocalFetchProvider body cancellation on error paths', () => { + /** A fake Response whose body.cancel is observable. */ + type FakeInit = { status: number; headers: Record; location?: string } + function fakeResponse(init: FakeInit): { response: Response; cancelled: () => boolean } { + let cancelled = false + const headers = new Headers(init.headers) + if (init.location !== undefined) headers.set('location', init.location) + const response = { + status: init.status, + headers, + body: { cancel: () => { cancelled = true; return Promise.resolve() } }, + } as unknown as Response + return { response, cancelled: () => cancelled } + } + + it('cancels the body when a cross-origin redirect is blocked', async () => { + const { response, cancelled } = fakeResponse({ status: 302, headers: {}, location: 'https://elsewhere.test/' }) + vi.stubGlobal('fetch', vi.fn(async () => response)) + await expect(provider().fetch({ url: 'http://127.0.0.1:9/' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_REDIRECT_BLOCKED' })) + expect(cancelled()).toBe(true) + }) + + it('cancels the body when an unsupported charset is rejected', async () => { + const { response, cancelled } = fakeResponse({ status: 200, headers: { 'content-type': 'text/plain; charset=not-a-charset' } }) + vi.stubGlobal('fetch', vi.fn(async () => response)) + await expect(provider().fetch({ url: 'http://127.0.0.1:9/' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_UNSUPPORTED_CONTENT_TYPE' })) + expect(cancelled()).toBe(true) + }) + + it('cancels the body when a redirect has no Location header', async () => { + const { response, cancelled } = fakeResponse({ status: 302, headers: {} }) + vi.stubGlobal('fetch', vi.fn(async () => response)) + await expect(provider().fetch({ url: 'http://127.0.0.1:9/' })) + .rejects.toThrow(expect.objectContaining({ code: 'WEB_PROVIDER_ERROR' })) + expect(cancelled()).toBe(true) + }) +}) + describe('web-fetch-local plugin registration', () => { it('registers the provider into ctx.web (HMR-safe)', async () => { const ctx = new Context()