From 0e4f3ccbdfc5907ae2249bf03ebe4e4c53989839 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 22 Feb 2026 18:36:54 +0000 Subject: [PATCH] refactor: dedupe media and request-body test scaffolding --- src/infra/http-body.test.ts | 18 +++++++++-- src/infra/http-body.ts | 29 +++++++++++------- src/slack/monitor/media.test.ts | 11 ++----- src/test-helpers/ssrf.ts | 14 +++++++++ src/web/media.test.ts | 24 ++++++++------- src/web/media.ts | 53 ++++++++++++++++++--------------- 6 files changed, 93 insertions(+), 56 deletions(-) create mode 100644 src/test-helpers/ssrf.ts diff --git a/src/infra/http-body.test.ts b/src/infra/http-body.test.ts index cfe693486..bfb14b92d 100644 --- a/src/infra/http-body.test.ts +++ b/src/infra/http-body.test.ts @@ -113,15 +113,29 @@ describe("http body limits", () => { expect(req.__unhandledDestroyError).toBeUndefined(); }); - it("timeout surfaces typed error", async () => { + it("timeout surfaces typed error when timeoutMs is clamped", async () => { const req = createMockRequest({ emitEnd: false }); - const promise = readRequestBodyWithLimit(req, { maxBytes: 128, timeoutMs: 10 }); + const promise = readRequestBodyWithLimit(req, { maxBytes: 128, timeoutMs: 0 }); await expect(promise).rejects.toSatisfy((error: unknown) => isRequestBodyLimitError(error, "REQUEST_BODY_TIMEOUT"), ); expect(req.__unhandledDestroyError).toBeUndefined(); }); + it("guard clamps invalid maxBytes to one byte", async () => { + const req = createMockRequest({ chunks: ["ab"], emitEnd: false }); + const res = createMockServerResponse(); + const guard = installRequestBodyLimitGuard(req, res, { + maxBytes: Number.NaN, + responseFormat: "text", + }); + await waitForMicrotaskTurn(); + expect(guard.isTripped()).toBe(true); + expect(guard.code()).toBe("PAYLOAD_TOO_LARGE"); + expect(res.statusCode).toBe(413); + expect(req.__unhandledDestroyError).toBeUndefined(); + }); + it("declared oversized content-length does not emit unhandled error", async () => { const req = createMockRequest({ headers: { "content-length": "9999" }, diff --git a/src/infra/http-body.ts b/src/infra/http-body.ts index 3f7fc9c3d..cfa655600 100644 --- a/src/infra/http-body.ts +++ b/src/infra/http-body.ts @@ -79,10 +79,15 @@ export type ReadRequestBodyOptions = { encoding?: BufferEncoding; }; -export async function readRequestBodyWithLimit( - req: IncomingMessage, - options: ReadRequestBodyOptions, -): Promise { +type RequestBodyLimitValues = { + maxBytes: number; + timeoutMs: number; +}; + +function resolveRequestBodyLimitValues(options: { + maxBytes: number; + timeoutMs?: number; +}): RequestBodyLimitValues { const maxBytes = Number.isFinite(options.maxBytes) ? Math.max(1, Math.floor(options.maxBytes)) : 1; @@ -90,6 +95,14 @@ export async function readRequestBodyWithLimit( typeof options.timeoutMs === "number" && Number.isFinite(options.timeoutMs) ? Math.max(1, Math.floor(options.timeoutMs)) : DEFAULT_WEBHOOK_BODY_TIMEOUT_MS; + return { maxBytes, timeoutMs }; +} + +export async function readRequestBodyWithLimit( + req: IncomingMessage, + options: ReadRequestBodyOptions, +): Promise { + const { maxBytes, timeoutMs } = resolveRequestBodyLimitValues(options); const encoding = options.encoding ?? "utf-8"; const declaredLength = parseContentLengthHeader(req); @@ -241,13 +254,7 @@ export function installRequestBodyLimitGuard( res: ServerResponse, options: RequestBodyLimitGuardOptions, ): RequestBodyLimitGuard { - const maxBytes = Number.isFinite(options.maxBytes) - ? Math.max(1, Math.floor(options.maxBytes)) - : 1; - const timeoutMs = - typeof options.timeoutMs === "number" && Number.isFinite(options.timeoutMs) - ? Math.max(1, Math.floor(options.timeoutMs)) - : DEFAULT_WEBHOOK_BODY_TIMEOUT_MS; + const { maxBytes, timeoutMs } = resolveRequestBodyLimitValues(options); const responseFormat = options.responseFormat ?? "json"; const customText = options.responseText ?? {}; diff --git a/src/slack/monitor/media.test.ts b/src/slack/monitor/media.test.ts index b3d0bcb51..eeeb5c88d 100644 --- a/src/slack/monitor/media.test.ts +++ b/src/slack/monitor/media.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as ssrf from "../../infra/net/ssrf.js"; import type { SavedMedia } from "../../media/store.js"; import * as mediaStore from "../../media/store.js"; +import { mockPinnedHostnameResolution } from "../../test-helpers/ssrf.js"; import { type FetchMock, withFetchPreconnect } from "../../test-utils/fetch-mock.js"; import { fetchWithSlackAuth, @@ -173,15 +174,7 @@ describe("resolveSlackMedia", () => { beforeEach(() => { mockFetch = vi.fn(); globalThis.fetch = withFetchPreconnect(mockFetch); - vi.spyOn(ssrf, "resolvePinnedHostname").mockImplementation(async (hostname) => { - const normalized = hostname.trim().toLowerCase().replace(/\.$/, ""); - const addresses = ["93.184.216.34"]; - return { - hostname: normalized, - addresses, - lookup: ssrf.createPinnedLookup({ hostname: normalized, addresses }), - }; - }); + mockPinnedHostnameResolution(); }); afterEach(() => { diff --git a/src/test-helpers/ssrf.ts b/src/test-helpers/ssrf.ts new file mode 100644 index 000000000..1669f45bc --- /dev/null +++ b/src/test-helpers/ssrf.ts @@ -0,0 +1,14 @@ +import { vi } from "vitest"; +import * as ssrf from "../infra/net/ssrf.js"; + +export function mockPinnedHostnameResolution(addresses: string[] = ["93.184.216.34"]) { + return vi.spyOn(ssrf, "resolvePinnedHostname").mockImplementation(async (hostname) => { + const normalized = hostname.trim().toLowerCase().replace(/\.$/, ""); + const pinnedAddresses = [...addresses]; + return { + hostname: normalized, + addresses: pinnedAddresses, + lookup: ssrf.createPinnedLookup({ hostname: normalized, addresses: pinnedAddresses }), + }; + }); +} diff --git a/src/web/media.test.ts b/src/web/media.test.ts index 605f0dad5..4bfcc7fdd 100644 --- a/src/web/media.test.ts +++ b/src/web/media.test.ts @@ -5,9 +5,9 @@ import sharp from "sharp"; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import { resolveStateDir } from "../config/paths.js"; import { sendVoiceMessageDiscord } from "../discord/send.js"; -import * as ssrf from "../infra/net/ssrf.js"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; import { optimizeImageToPng } from "../media/image-ops.js"; +import { mockPinnedHostnameResolution } from "../test-helpers/ssrf.js"; import { captureEnv } from "../test-utils/env.js"; import { LocalMediaAccessError, @@ -126,15 +126,7 @@ describe("web media loading", () => { }); beforeAll(() => { - vi.spyOn(ssrf, "resolvePinnedHostname").mockImplementation(async (hostname) => { - const normalized = hostname.trim().toLowerCase().replace(/\.$/, ""); - const addresses = ["93.184.216.34"]; - return { - hostname: normalized, - addresses, - lookup: ssrf.createPinnedLookup({ hostname: normalized, addresses }), - }; - }); + mockPinnedHostnameResolution(); }); it("strips MEDIA: prefix before reading local file (including whitespace variants)", async () => { @@ -240,6 +232,18 @@ describe("web media loading", () => { fetchMock.mockRestore(); }); + it("keeps raw mode when options object sets optimizeImages true", async () => { + const { buffer, file } = await createLargeTestJpeg(); + const cap = Math.max(1, Math.floor(buffer.length * 0.8)); + + await expect( + loadWebMediaRaw(file, { + maxBytes: cap, + optimizeImages: true, + }), + ).rejects.toThrow(/Media exceeds/i); + }); + it("uses content-disposition filename when available", async () => { const fetchMock = vi.spyOn(globalThis, "fetch").mockResolvedValueOnce({ ok: true, diff --git a/src/web/media.ts b/src/web/media.ts index d07e52690..cccd88e71 100644 --- a/src/web/media.ts +++ b/src/web/media.ts @@ -34,6 +34,27 @@ type WebMediaOptions = { readFile?: (filePath: string) => Promise; }; +function resolveWebMediaOptions(params: { + maxBytesOrOptions?: number | WebMediaOptions; + options?: { ssrfPolicy?: SsrFPolicy; localRoots?: readonly string[] | "any" }; + optimizeImages: boolean; +}): WebMediaOptions { + if (typeof params.maxBytesOrOptions === "number" || params.maxBytesOrOptions === undefined) { + return { + maxBytes: params.maxBytesOrOptions, + optimizeImages: params.optimizeImages, + ssrfPolicy: params.options?.ssrfPolicy, + localRoots: params.options?.localRoots, + }; + } + return { + ...params.maxBytesOrOptions, + optimizeImages: params.optimizeImages + ? (params.maxBytesOrOptions.optimizeImages ?? true) + : false, + }; +} + export type LocalMediaAccessErrorCode = | "path-not-allowed" | "invalid-root" @@ -385,18 +406,10 @@ export async function loadWebMedia( maxBytesOrOptions?: number | WebMediaOptions, options?: { ssrfPolicy?: SsrFPolicy; localRoots?: readonly string[] | "any" }, ): Promise { - if (typeof maxBytesOrOptions === "number" || maxBytesOrOptions === undefined) { - return await loadWebMediaInternal(mediaUrl, { - maxBytes: maxBytesOrOptions, - optimizeImages: true, - ssrfPolicy: options?.ssrfPolicy, - localRoots: options?.localRoots, - }); - } - return await loadWebMediaInternal(mediaUrl, { - ...maxBytesOrOptions, - optimizeImages: maxBytesOrOptions.optimizeImages ?? true, - }); + return await loadWebMediaInternal( + mediaUrl, + resolveWebMediaOptions({ maxBytesOrOptions, options, optimizeImages: true }), + ); } export async function loadWebMediaRaw( @@ -404,18 +417,10 @@ export async function loadWebMediaRaw( maxBytesOrOptions?: number | WebMediaOptions, options?: { ssrfPolicy?: SsrFPolicy; localRoots?: readonly string[] | "any" }, ): Promise { - if (typeof maxBytesOrOptions === "number" || maxBytesOrOptions === undefined) { - return await loadWebMediaInternal(mediaUrl, { - maxBytes: maxBytesOrOptions, - optimizeImages: false, - ssrfPolicy: options?.ssrfPolicy, - localRoots: options?.localRoots, - }); - } - return await loadWebMediaInternal(mediaUrl, { - ...maxBytesOrOptions, - optimizeImages: false, - }); + return await loadWebMediaInternal( + mediaUrl, + resolveWebMediaOptions({ maxBytesOrOptions, options, optimizeImages: false }), + ); } export async function optimizeImageToJpeg(