fix(review): guard contributor screenshot fetches

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",...}. */

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<AiContentBlock
   }
 }
 
+
+function redirectLocation(response: Response, currentUrl: string): string {
+  const location = response.headers.get("location");
+  if (!location) return "";
+  try {
+    return new URL(location, currentUrl).toString();
+  } catch {
+    return "";
+  }
+}
+
+async function readBoundedResponseBytes(response: Response, maxBytes: number): Promise<Uint8Array | undefined> {
+  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<AiContentBlock | undefined> {
+  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;

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);

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();
+  });
+});