diff --git a/CHANGELOG.md b/CHANGELOG.md index 235981b8b07..87ac0076d1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ Docs: https://docs.openclaw.ai - Skills/Security: restrict `download` installer `targetDir` to the per-skill tools directory to prevent arbitrary file writes. Thanks @Adam55A-code. - Skills/Linux: harden go installer fallback on apt-based systems by handling root/no-sudo environments safely, doing best-effort apt index refresh, and returning actionable errors instead of failing with spawn errors. (#17687) Thanks @mcrolly. - Web Fetch/Security: cap downloaded response body size before HTML parsing to prevent memory exhaustion from oversized or deeply nested pages. Thanks @xuemian168. +- Infra/Fetch: ensure foreign abort-signal listener cleanup never masks original fetch successes/failures, while still preventing detached-finally unhandled rejection noise in `wrapFetchWithAbortSignal`. Thanks @Jackten. - Config/Gateway: make sensitive-key whitelist suffix matching case-insensitive while preserving `passwordFile` path exemptions, preventing accidental redaction of non-secret config values like `maxTokens` and IRC password-file paths. (#16042) Thanks @akramcodez. - Gateway/Config: prevent `config.patch` from falling back to full-array replacement when an object-array patch includes entries without `id`, so partial `agents.list` updates no longer risk deleting unrelated agents. (#17989) Thanks @stakeswky. - Dev tooling: harden git `pre-commit` hook against option injection from malicious filenames (for example `--force`), preventing accidental staging of ignored files. Thanks @mrthankyou. diff --git a/src/infra/fetch.test.ts b/src/infra/fetch.test.ts index 6fb471106d4..783a2d78473 100644 --- a/src/infra/fetch.test.ts +++ b/src/infra/fetch.test.ts @@ -50,4 +50,136 @@ describe("wrapFetchWithAbortSignal", () => { await promise; }); + + it("does not emit an extra unhandled rejection when wrapped fetch rejects", async () => { + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { + unhandled.push(reason); + }; + process.on("unhandledRejection", onUnhandled); + + const fetchError = new TypeError("fetch failed"); + const fetchImpl = vi.fn((_input: RequestInfo | URL, _init?: RequestInit) => + Promise.reject(fetchError), + ); + const wrapped = wrapFetchWithAbortSignal(fetchImpl); + + let abortHandler: (() => void) | null = null; + const removeEventListener = vi.fn((event: string, handler: () => void) => { + if (event === "abort" && abortHandler === handler) { + abortHandler = null; + } + }); + + const fakeSignal = { + aborted: false, + addEventListener: (event: string, handler: () => void) => { + if (event === "abort") { + abortHandler = handler; + } + }, + removeEventListener, + } as AbortSignal; + + try { + await expect(wrapped("https://example.com", { signal: fakeSignal })).rejects.toBe(fetchError); + await Promise.resolve(); + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(unhandled).toEqual([]); + expect(removeEventListener).toHaveBeenCalledOnce(); + } finally { + process.off("unhandledRejection", onUnhandled); + } + }); + + it("cleans up listener and rethrows when fetch throws synchronously", () => { + const syncError = new TypeError("sync fetch failure"); + const fetchImpl = vi.fn(() => { + throw syncError; + }); + const wrapped = wrapFetchWithAbortSignal(fetchImpl); + + let abortHandler: (() => void) | null = null; + const removeEventListener = vi.fn((event: string, handler: () => void) => { + if (event === "abort" && abortHandler === handler) { + abortHandler = null; + } + }); + + const fakeSignal = { + aborted: false, + addEventListener: (event: string, handler: () => void) => { + if (event === "abort") { + abortHandler = handler; + } + }, + removeEventListener, + } as AbortSignal; + + expect(() => wrapped("https://example.com", { signal: fakeSignal })).toThrow(syncError); + expect(removeEventListener).toHaveBeenCalledOnce(); + }); + + it("preserves original rejection when listener cleanup throws", async () => { + const fetchError = new TypeError("fetch failed"); + const cleanupError = new TypeError("cleanup failed"); + const fetchImpl = vi.fn((_input: RequestInfo | URL, _init?: RequestInit) => + Promise.reject(fetchError), + ); + const wrapped = wrapFetchWithAbortSignal(fetchImpl); + + const removeEventListener = vi.fn(() => { + throw cleanupError; + }); + + const fakeSignal = { + aborted: false, + addEventListener: (_event: string, _handler: () => void) => {}, + removeEventListener, + } as AbortSignal; + + await expect(wrapped("https://example.com", { signal: fakeSignal })).rejects.toBe(fetchError); + expect(removeEventListener).toHaveBeenCalledOnce(); + }); + + it("preserves original sync throw when listener cleanup throws", () => { + const syncError = new TypeError("sync fetch failure"); + const cleanupError = new TypeError("cleanup failed"); + const fetchImpl = vi.fn(() => { + throw syncError; + }); + const wrapped = wrapFetchWithAbortSignal(fetchImpl); + + const removeEventListener = vi.fn(() => { + throw cleanupError; + }); + + const fakeSignal = { + aborted: false, + addEventListener: (_event: string, _handler: () => void) => {}, + removeEventListener, + } as AbortSignal; + + expect(() => wrapped("https://example.com", { signal: fakeSignal })).toThrow(syncError); + expect(removeEventListener).toHaveBeenCalledOnce(); + }); + + it("skips listener cleanup when foreign signal is already aborted", async () => { + const addEventListener = vi.fn(); + const removeEventListener = vi.fn(); + const fetchImpl = vi.fn(async () => ({ ok: true }) as Response); + const wrapped = wrapFetchWithAbortSignal(fetchImpl); + + const fakeSignal = { + aborted: true, + addEventListener, + removeEventListener, + } as AbortSignal; + + await wrapped("https://example.com", { signal: fakeSignal }); + + expect(addEventListener).not.toHaveBeenCalled(); + expect(removeEventListener).not.toHaveBeenCalled(); + }); }); diff --git a/src/infra/fetch.ts b/src/infra/fetch.ts index fe4c7c351ab..88f5ec0e531 100644 --- a/src/infra/fetch.ts +++ b/src/infra/fetch.ts @@ -45,18 +45,32 @@ export function wrapFetchWithAbortSignal(fetchImpl: typeof fetch): typeof fetch } const controller = new AbortController(); const onAbort = bindAbortRelay(controller); + let listenerAttached = false; if (signal.aborted) { controller.abort(); } else { signal.addEventListener("abort", onAbort, { once: true }); + listenerAttached = true; } - const response = fetchImpl(input, { ...patchedInit, signal: controller.signal }); - if (typeof signal.removeEventListener === "function") { - void response.finally(() => { + const cleanup = () => { + if (!listenerAttached || typeof signal.removeEventListener !== "function") { + return; + } + listenerAttached = false; + try { signal.removeEventListener("abort", onAbort); - }); + } catch { + // Foreign/custom AbortSignal implementations may throw here. + // Never let cleanup mask the original fetch result/error. + } + }; + try { + const response = fetchImpl(input, { ...patchedInit, signal: controller.signal }); + return response.finally(cleanup); + } catch (error) { + cleanup(); + throw error; } - return response; }) as FetchWithPreconnect; const fetchWithPreconnect = fetchImpl as FetchWithPreconnect;