From fd815df75f31afbd6080d5c64a6d8c0c895ca3e1 Mon Sep 17 00:00:00 2001 From: Bret Comnes Date: Tue, 19 May 2026 15:24:01 -0700 Subject: [PATCH 1/3] fix(api): always use node:https.request in apiFetch to avoid undici body timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Node's built-in fetch uses undici which has a default bodyTimeout of 300s. For large streaming ND-JSON responses (e.g. full scan results) this timeout fires before the body has fully transferred, producing a BodyTimeoutError. Route all apiFetch calls through the existing _httpsRequestFetch path, which uses node:https.request and has no body timeout — consistent with the SDK and all other HTTP clients used across socket-cli, socket-sdk-js, and coana. When SSL_CERT_FILE is not set, agent is passed as undefined and https.request uses its default agent. Behaviour is otherwise identical to before. Update the apiFetch test to assert https.request is always used and that no custom HttpsAgent is created when CA certs are not configured. --- src/utils/api.mts | 17 +++++++-------- src/utils/api.test.mts | 49 +++++++++++++++++++++++++++++++++--------- 2 files changed, 47 insertions(+), 19 deletions(-) diff --git a/src/utils/api.mts b/src/utils/api.mts index 26f1085ca..826890d7a 100644 --- a/src/utils/api.mts +++ b/src/utils/api.mts @@ -72,9 +72,12 @@ function getHttpsAgent(): HttpsAgent | undefined { return _httpsAgent } -// Wrapper around fetch that supports extra CA certificates via SSL_CERT_FILE. -// Uses node:https.request with a custom agent when extra CA certs are needed, -// falling back to regular fetch() otherwise. Follows redirects like fetch(). +// Wrapper around fetch using node:https.request for all calls. +// This avoids undici's default 300 s bodyTimeout, which would otherwise fire +// on large streaming responses before the body has fully transferred. Behaviour +// matches the SDK, which also uses node:https.request with no body timeout. +// Passes the custom HTTPS agent when extra CA certificates are configured via +// SSL_CERT_FILE, otherwise uses Node's default agent. Follows redirects like fetch(). export type ApiFetchInit = { body?: string | undefined headers?: Record | undefined @@ -85,7 +88,7 @@ export type ApiFetchInit = { function _httpsRequestFetch( url: string, init: ApiFetchInit, - agent: HttpsAgent, + agent: HttpsAgent | undefined, redirectCount: number, ): Promise { return new Promise((resolve, reject) => { @@ -186,11 +189,7 @@ export async function apiFetch( url: string, init: ApiFetchInit = {}, ): Promise { - const agent = getHttpsAgent() - if (!agent) { - return await fetch(url, init as globalThis.RequestInit) - } - return await _httpsRequestFetch(url, init, agent, 0) + return await _httpsRequestFetch(url, init, getHttpsAgent(), 0) } export type CommandRequirements = { diff --git a/src/utils/api.test.mts b/src/utils/api.test.mts index 22146a2df..222960940 100644 --- a/src/utils/api.test.mts +++ b/src/utils/api.test.mts @@ -6,8 +6,9 @@ * direct API calls when NODE_EXTRA_CA_CERTS is not set at process startup. * * Test Coverage: - * - apiFetch falls back to regular fetch when no extra CA certs are needed. - * - apiFetch uses node:https.request with custom agent when CA certs are set. + * - apiFetch always uses node:https.request (no undici body timeout). + * - apiFetch passes a custom HttpsAgent when CA certs are set via SSL_CERT_FILE. + * - apiFetch passes no agent (undefined) when no CA certs are configured. * - Response object construction from https.request output. * - POST requests with JSON body through https.request path. * - Error propagation from https.request failures. @@ -113,18 +114,46 @@ describe('apiFetch with extra CA certificates', () => { globalThis.fetch = originalFetch }) - it('should use regular fetch when no extra CA certs are needed', async () => { - const mockResponse = new Response(JSON.stringify({ ok: true }), { - status: 200, - statusText: 'OK', - }) - globalThis.fetch = vi.fn().mockResolvedValue(mockResponse) + it('should use https.request with no agent when no extra CA certs are needed', async () => { + const mockReq = { + end: vi.fn(), + on: vi.fn(), + write: vi.fn(), + } + + mockHttpsRequest.mockImplementation( + (_url: string, _opts: unknown, callback: RequestCallback) => { + setTimeout(() => { + const mockRes = { + headers: { 'content-type': 'text/plain' }, + on: vi.fn(), + statusCode: 200, + statusMessage: 'OK', + } + const handlers: Record = {} + mockRes.on.mockImplementation((event: string, handler: Function) => { + handlers[event] = handler + return mockRes + }) + callback(mockRes) + handlers['data']?.(Buffer.from('response body')) + handlers['end']?.() + }, 0) + return mockReq + }, + ) const { queryApiSafeText } = await import('./api.mts') const result = await queryApiSafeText('test/path', 'test request') - expect(globalThis.fetch).toHaveBeenCalled() - expect(mockHttpsRequest).not.toHaveBeenCalled() + // Always uses https.request — no undici body timeout. + expect(mockHttpsRequest).toHaveBeenCalled() + // No custom HttpsAgent created when CA certs are not configured. + expect(MockHttpsAgent).not.toHaveBeenCalled() + // agent is undefined when no CA certs are configured. + const callArgs = mockHttpsRequest.mock.calls[0] + expect(callArgs[1]).toEqual(expect.objectContaining({ agent: undefined })) + expect(result.ok).toBe(true) }) it('should use https.request when extra CA certs are available', async () => { From ee86ba4bd33265847c4801b3dda3cb6d32dd6e69 Mon Sep 17 00:00:00 2001 From: Bret Comnes Date: Tue, 19 May 2026 15:25:54 -0700 Subject: [PATCH 2/3] fix(api): update apiFetch comment to describe permanent behaviour --- src/utils/api.mts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/utils/api.mts b/src/utils/api.mts index 826890d7a..0711f3582 100644 --- a/src/utils/api.mts +++ b/src/utils/api.mts @@ -72,12 +72,11 @@ function getHttpsAgent(): HttpsAgent | undefined { return _httpsAgent } -// Wrapper around fetch using node:https.request for all calls. -// This avoids undici's default 300 s bodyTimeout, which would otherwise fire -// on large streaming responses before the body has fully transferred. Behaviour -// matches the SDK, which also uses node:https.request with no body timeout. -// Passes the custom HTTPS agent when extra CA certificates are configured via -// SSL_CERT_FILE, otherwise uses Node's default agent. Follows redirects like fetch(). +// All outbound API requests use node:https.request rather than global fetch. +// This ensures no body timeout is applied — large streaming ND-JSON responses +// (e.g. full scan results) can transfer without a hard deadline. When +// SSL_CERT_FILE is configured, a custom HttpsAgent carrying the extra CA +// certificates is passed; otherwise the default agent is used. export type ApiFetchInit = { body?: string | undefined headers?: Record | undefined From 837cb4576915a1821291650de1dce580dce7f5b2 Mon Sep 17 00:00:00 2001 From: Bret Comnes Date: Tue, 19 May 2026 16:58:41 -0700 Subject: [PATCH 3/3] fix(api): stream response body in _httpsRequestFetch rather than buffering Resolving the promise as soon as headers arrive and constructing the Response with a ReadableStream body, matching fetch() semantics. Previously the response body was fully buffered before the promise resolved, which broke streaming call sites such as streamDownloadWithFetch that pipe response.body to disk expecting a live stream. --- src/utils/api.mts | 56 ++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/src/utils/api.mts b/src/utils/api.mts index 0711f3582..7582b54a0 100644 --- a/src/utils/api.mts +++ b/src/utils/api.mts @@ -20,6 +20,7 @@ */ import { Agent as HttpsAgent, request as httpsRequest } from 'node:https' +import { ReadableStream } from 'node:stream/web' import { messageWithCauses } from 'pony-cause' @@ -151,29 +152,44 @@ function _httpsRequestFetch( ) return } - const chunks: Buffer[] = [] - res.on('data', (chunk: Buffer) => chunks.push(chunk)) - res.on('end', () => { - const body = Buffer.concat(chunks) - const responseHeaders = new Headers() - for (const [key, value] of Object.entries(res.headers)) { - if (typeof value === 'string') { - responseHeaders.set(key, value) - } else if (Array.isArray(value)) { - for (const v of value) { - responseHeaders.append(key, v) - } + // Build response headers immediately on receipt. + const responseHeaders = new Headers() + for (const [key, value] of Object.entries(res.headers)) { + if (typeof value === 'string') { + responseHeaders.set(key, value) + } else if (Array.isArray(value)) { + for (const v of value) { + responseHeaders.append(key, v) } } - resolve( - new Response(body, { - status: statusCode ?? 0, - statusText: res.statusMessage ?? '', - headers: responseHeaders, - }), - ) + } + // Resolve with a streaming body as soon as headers are available, + // matching fetch() semantics. Callers that pipe response.body (e.g. + // streamDownloadWithFetch) receive a live ReadableStream rather than + // a fully-buffered Buffer. + const body = new ReadableStream({ + start(controller) { + res.on('data', (chunk: Buffer) => { + controller.enqueue(chunk) + }) + res.on('end', () => { + controller.close() + }) + res.on('error', (err: Error) => { + controller.error(err) + }) + }, + cancel() { + res.destroy() + }, }) - res.on('error', reject) + resolve( + new Response(body, { + status: statusCode ?? 0, + statusText: res.statusMessage ?? '', + headers: responseHeaders, + }), + ) }, ) if (init.body) {