diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c6b20ca4e..ae8422471 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -437,7 +437,7 @@ export { // Nothing in this file calls processJob itself, so this is a pure re-export (no separate import needed). export { processJob } from "./job-dispatch"; import { isVisualPath } from "../review/visual/paths"; -import { buildCapture, fetchShotContentBlock, hasSuccessfulBotCapture, resolveVisualRoutes, type CaptureRoute } from "../review/visual/capture"; +import { buildCapture, fetchExternalScreenshotContentBlock, fetchShotContentBlock, hasSuccessfulBotCapture, resolveVisualRoutes, type CaptureRoute } from "../review/visual/capture"; import { clearFallbackDispatchMarker, fallbackShotFileName, @@ -7078,8 +7078,8 @@ export async function runScreenshotTableVisionForAdvisory( /* v8 ignore next -- defensive: rawPairs only contains rows with >=2 urls, so both slots exist here. */ if (!beforeUrl || !afterUrl) continue; const [beforeBlock, afterBlock] = await Promise.all([ - fetchShotContentBlock(beforeUrl), - fetchShotContentBlock(afterUrl), + fetchExternalScreenshotContentBlock(beforeUrl), + fetchExternalScreenshotContentBlock(afterUrl), ]); if (!beforeBlock || !afterBlock) continue; /* v8 ignore next -- defensive: fetchShotContentBlock's only success return shape is {type:"image",...}. */ diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index a998dca49..bc5831bca 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -16,6 +16,7 @@ // default TanStack route convention; those hooks can return if a per-repo visual config is added. import { base64Encode, sha256Hex } from "../../utils/crypto"; import type { AiContentBlock } from "../../types"; +import { isSafeHttpUrl } from "../content-lane/safe-url"; import { downscaleForVision } from "./image-downscale"; import type { GitHubRateLimitAdmissionKey } from "../../github/client"; import { dispatchVisualCaptureFallback, fallbackShotR2Key, isFallbackDispatchInFlight, markFallbackDispatched } from "./actions-fallback"; @@ -41,6 +42,10 @@ const DEFAULT_ROUTE_FILE = /apps\/[^/]+\/src\/routes\/(.+?)\.(?:tsx|jsx)$/i; // wall-clock — Browser Rendering is the costliest binding. const MAX_ROUTES = 2; const MAX_CONFIGURED_ROUTES = 5; +const MAX_EXTERNAL_SCREENSHOT_REDIRECTS = 2; +const MAX_EXTERNAL_SCREENSHOT_BYTES = 4 * 1024 * 1024; +const EXTERNAL_SCREENSHOT_FETCH_TIMEOUT_MS = 8_000; +const ALLOWED_EXTERNAL_SCREENSHOT_MIME_TYPES = new Set(["image/png", "image/jpeg", "image/webp", "image/gif"]); /** A single captured route's before/after shot URLs (desktop + mobile), plus an optional pixel-diff overlay * per viewport (#3674) — self-host only (isVisualDiffAvailable), and only when the diff clears the visual- @@ -123,6 +128,80 @@ export async function fetchShotContentBlock(url: string): Promise { + const contentLength = Number(response.headers.get("content-length") ?? ""); + if (Number.isFinite(contentLength) && contentLength > maxBytes) return undefined; + /* v8 ignore next -- Fetch Response bodies are present in Workers/Node; keep the fallback for nonstandard test doubles. */ + if (!response.body) { + const bytes = new Uint8Array(await response.arrayBuffer()); + return bytes.byteLength > maxBytes ? undefined : bytes; + } + + const reader = response.body.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + try { + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + total += value.byteLength; + if (total > maxBytes) return undefined; + chunks.push(value); + } + } finally { + reader.releaseLock(); + } + const bytes = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + bytes.set(chunk, offset); + offset += chunk.byteLength; + } + return bytes; +} + +/** + * Fetch a contributor-supplied screenshot-table image with SSRF/resource guards before it can be compared + * locally or forwarded to a vision provider. Unlike bot-produced shot URLs, these URLs come from PR markdown: + * re-check every redirect hop, cap bytes before buffering, require a real image content type, and time out. + */ +export async function fetchExternalScreenshotContentBlock(url: string): Promise { + try { + let currentUrl = url; + for (let redirects = 0; redirects <= MAX_EXTERNAL_SCREENSHOT_REDIRECTS; redirects += 1) { + if (!isSafeHttpUrl(currentUrl)) return undefined; + const response = await fetch(currentUrl, { redirect: "manual", signal: AbortSignal.timeout(EXTERNAL_SCREENSHOT_FETCH_TIMEOUT_MS) }); + if (response.status >= 300 && response.status < 400) { + const nextUrl = redirectLocation(response, currentUrl); + if (!nextUrl) return undefined; + currentUrl = nextUrl; + continue; + } + if (!response.ok) return undefined; + const mimeType = response.headers.get("content-type")?.split(";", 1)[0]?.trim().toLowerCase() ?? ""; + if (!ALLOWED_EXTERNAL_SCREENSHOT_MIME_TYPES.has(mimeType)) return undefined; + const bytes = await readBoundedResponseBytes(response, MAX_EXTERNAL_SCREENSHOT_BYTES); + if (!bytes) return undefined; + const imageBytes = mimeType === "image/png" ? await downscaleForVision(bytes) : bytes; + return { type: "image", data: base64Encode(imageBytes), mimeType }; + } + return undefined; + } catch { + return undefined; + } +} + /** Inputs the capture pipeline needs about the PR under review (resolved by the caller from gittensory data). */ export interface CaptureTarget { repoFullName: string; diff --git a/test/unit/screenshot-table-vision-wiring.test.ts b/test/unit/screenshot-table-vision-wiring.test.ts index 797a6befb..3a27ab86f 100644 --- a/test/unit/screenshot-table-vision-wiring.test.ts +++ b/test/unit/screenshot-table-vision-wiring.test.ts @@ -51,8 +51,8 @@ function stubShotsAndProvider(providerResponseText: string | null, bytes: { befo if (url === "https://api.anthropic.com/v1/messages") { return providerResponseText === null ? new Response("upstream error", { status: 500 }) : anthropicOk(providerResponseText); } - if (url === BEFORE_URL) return new Response(new Uint8Array(bytes.before), { status: 200 }); - if (url === AFTER_URL) return new Response(new Uint8Array(bytes.after), { status: 200 }); + if (url === BEFORE_URL) return new Response(new Uint8Array(bytes.before), { status: 200, headers: { "content-type": "image/png" } }); + if (url === AFTER_URL) return new Response(new Uint8Array(bytes.after), { status: 200, headers: { "content-type": "image/png" } }); return new Response("not found", { status: 404 }); })); } @@ -380,8 +380,8 @@ describe("runScreenshotTableVisionForAdvisory (#4366)", () => { const fetchMock = vi.fn(async (input: RequestInfo | URL) => { const url = input.toString(); if (url === "https://api.anthropic.com/v1/messages") return anthropicOk(findingsResponse([])); - if (url === BEFORE_URL) return new Response(new Uint8Array([1]), { status: 200 }); - if (url === AFTER_URL) return new Response(new Uint8Array([2]), { status: 200 }); + if (url === BEFORE_URL) return new Response(new Uint8Array([1]), { status: 200, headers: { "content-type": "image/png" } }); + if (url === AFTER_URL) return new Response(new Uint8Array([2]), { status: 200, headers: { "content-type": "image/png" } }); return new Response("not found", { status: 404 }); }); vi.stubGlobal("fetch", fetchMock); @@ -406,7 +406,7 @@ describe("runScreenshotTableVisionForAdvisory (#4366)", () => { const env = byokEnv(); const fetchMock = vi.fn(async (input: RequestInfo | URL) => { const url = input.toString(); - if (url === BEFORE_URL) return new Response(new Uint8Array([1, 2, 3]), { status: 200 }); + if (url === BEFORE_URL) return new Response(new Uint8Array([1, 2, 3]), { status: 200, headers: { "content-type": "image/png" } }); return new Response("not found", { status: 404 }); }); vi.stubGlobal("fetch", fetchMock); @@ -472,8 +472,8 @@ describe("runScreenshotTableVisionForAdvisory (#4366)", () => { await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); const fetchMock = vi.fn(async (input: RequestInfo | URL) => { const url = input.toString(); - if (url === BEFORE_URL) return new Response(new Uint8Array([1, 2, 3]), { status: 200 }); - if (url === AFTER_URL) return new Response(new Uint8Array([4, 5, 6]), { status: 200 }); + if (url === BEFORE_URL) return new Response(new Uint8Array([1, 2, 3]), { status: 200, headers: { "content-type": "image/png" } }); + if (url === AFTER_URL) return new Response(new Uint8Array([4, 5, 6]), { status: 200, headers: { "content-type": "image/png" } }); return new Response("not found", { status: 404 }); }); vi.stubGlobal("fetch", fetchMock); @@ -527,8 +527,8 @@ describe("runScreenshotTableVisionForAdvisory (#4366)", () => { ].join("\n"); const fetchMock = vi.fn(async (input: RequestInfo | URL) => { const url = input.toString(); - if (url === BEFORE_URL || url === AFTER_URL) return new Response(new Uint8Array([1, 1, 1]), { status: 200 }); - if (url === secondBefore || url === secondAfter) return new Response(new Uint8Array([2, 2, 2]), { status: 200 }); + if (url === BEFORE_URL || url === AFTER_URL) return new Response(new Uint8Array([1, 1, 1]), { status: 200, headers: { "content-type": "image/png" } }); + if (url === secondBefore || url === secondAfter) return new Response(new Uint8Array([2, 2, 2]), { status: 200, headers: { "content-type": "image/png" } }); return new Response("not found", { status: 404 }); }); vi.stubGlobal("fetch", fetchMock); diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index fdec0f6b2..a00b6ef27 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -5,7 +5,7 @@ import { latestGitHubRestRateLimitObservation, } from "../../src/github/client"; import { fallbackShotR2Key, markFallbackDispatched } from "../../src/review/visual/actions-fallback"; -import { buildCapture, fetchShotContentBlock, hasSuccessfulBotCapture, mapFilesToRoutes, resolvePreviewUrlTemplate, resolveVisualRoutes } from "../../src/review/visual/capture"; +import { buildCapture, fetchExternalScreenshotContentBlock, fetchShotContentBlock, hasSuccessfulBotCapture, mapFilesToRoutes, resolvePreviewUrlTemplate, resolveVisualRoutes } from "../../src/review/visual/capture"; import type { CaptureRoute } from "../../src/review/visual/capture"; import * as pixelDiffModule from "../../src/review/visual/pixel-diff"; import * as previewUrlModule from "../../src/review/visual/preview-url"; @@ -1847,3 +1847,117 @@ describe("fetchShotContentBlock (#4111)", () => { await expect(fetchShotContentBlock("https://x/gittensory/shot?key=broken")).resolves.toBeUndefined(); }); }); + + +describe("fetchExternalScreenshotContentBlock", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("returns an image block for safe public image responses", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => new Response(new Uint8Array([1, 2, 3]), { status: 200, headers: { "content-type": "image/png" } })), + ); + + await expect(fetchExternalScreenshotContentBlock("https://example.com/before.png")).resolves.toEqual({ + type: "image", + data: Buffer.from([1, 2, 3]).toString("base64"), + mimeType: "image/png", + }); + }); + + it("revalidates redirect targets before fetching contributor screenshots", async () => { + const fetchMock = vi.fn(async () => + new Response(null, { status: 302, headers: { location: "http://127.0.0.1/metadata" } }), + ); + vi.stubGlobal("fetch", fetchMock); + + await expect(fetchExternalScreenshotContentBlock("https://example.com/before.png")).resolves.toBeUndefined(); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock).toHaveBeenCalledWith( + "https://example.com/before.png", + expect.objectContaining({ redirect: "manual" }), + ); + }); + + it("rejects oversized contributor screenshots before buffering the body", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async () => + new Response(new Uint8Array([1]), { + status: 200, + headers: { "content-type": "image/png", "content-length": String(4 * 1024 * 1024 + 1) }, + }), + ), + ); + + await expect(fetchExternalScreenshotContentBlock("https://example.com/huge.png")).resolves.toBeUndefined(); + }); + + it("rejects non-image contributor screenshot responses", async () => { + vi.stubGlobal("fetch", vi.fn(async () => new Response("secret", { status: 200, headers: { "content-type": "text/plain" } }))); + + await expect(fetchExternalScreenshotContentBlock("https://example.com/not-image.txt")).resolves.toBeUndefined(); + }); + + it("rejects a response with no content-type header at all (falls through the ?? \"\" fallback, not just a wrong MIME type)", async () => { + // A string body auto-gets a "text/plain" content-type from the Fetch API itself, which would only + // re-exercise the existing wrong-MIME-type test -- a binary body has no such auto-assignment, so + // headers.get("content-type") genuinely returns null here. + vi.stubGlobal("fetch", vi.fn(async () => new Response(new Uint8Array([1, 2, 3]), { status: 200 }))); + + await expect(fetchExternalScreenshotContentBlock("https://example.com/no-content-type")).resolves.toBeUndefined(); + }); + + it("follows safe redirects and preserves non-png image MIME types", async () => { + const fetchMock = vi + .fn() + .mockResolvedValueOnce(new Response(null, { status: 302, headers: { location: "/after.jpg" } })) + .mockResolvedValueOnce(new Response(new Uint8Array([4, 5]), { status: 200, headers: { "content-type": "image/jpeg; charset=binary" } })); + vi.stubGlobal("fetch", fetchMock); + + await expect(fetchExternalScreenshotContentBlock("https://example.com/before.png")).resolves.toEqual({ + type: "image", + data: Buffer.from([4, 5]).toString("base64"), + mimeType: "image/jpeg", + }); + expect(fetchMock).toHaveBeenNthCalledWith(2, "https://example.com/after.jpg", expect.objectContaining({ redirect: "manual" })); + }); + + it("rejects invalid inputs, broken redirects, non-ok responses, overlong streams, and thrown fetches", async () => { + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + + await expect(fetchExternalScreenshotContentBlock("http://example.com/before.png")).resolves.toBeUndefined(); + expect(fetchMock).not.toHaveBeenCalled(); + + fetchMock.mockResolvedValueOnce(new Response(null, { status: 302 })); + await expect(fetchExternalScreenshotContentBlock("https://example.com/no-location.png")).resolves.toBeUndefined(); + + // A Location header the URL parser itself rejects (not merely absent) -- exercises redirectLocation's own + // catch, distinct from the no-header branch above. + fetchMock.mockResolvedValueOnce(new Response(null, { status: 302, headers: { location: "http://[not-valid-ipv6" } })); + await expect(fetchExternalScreenshotContentBlock("https://example.com/malformed-location.png")).resolves.toBeUndefined(); + + fetchMock.mockResolvedValueOnce(new Response("missing", { status: 404 })); + await expect(fetchExternalScreenshotContentBlock("https://example.com/missing.png")).resolves.toBeUndefined(); + + fetchMock.mockResolvedValueOnce( + new Response(new Uint8Array(4 * 1024 * 1024 + 1), { status: 200, headers: { "content-type": "image/png" } }), + ); + await expect(fetchExternalScreenshotContentBlock("https://example.com/stream-too-large.png")).resolves.toBeUndefined(); + + fetchMock.mockRejectedValueOnce(new Error("network down")); + await expect(fetchExternalScreenshotContentBlock("https://example.com/broken.png")).resolves.toBeUndefined(); + }); + + it("stops after the bounded redirect budget", async () => { + vi.stubGlobal( + "fetch", + vi.fn(async (_input: RequestInfo | URL) => new Response(null, { status: 302, headers: { location: "https://example.com/next.png" } })), + ); + + await expect(fetchExternalScreenshotContentBlock("https://example.com/first.png")).resolves.toBeUndefined(); + }); +});