diff --git a/src/auth/rate-limit.ts b/src/auth/rate-limit.ts index fa5f99b28a..7402f3d70b 100644 --- a/src/auth/rate-limit.ts +++ b/src/auth/rate-limit.ts @@ -108,6 +108,7 @@ export function routeClassForPath(path: string): RateLimitClass { if (path === "/v1/orb/ingest") return "strict"; if (path === "/v1/auth/session" || path === "/v1/auth/logout") return "normal"; if (path.startsWith("/v1/auth/")) return "strict"; + if (path === "/gittensory/shot") return "expensive"; if ( path.includes("/branch-analysis") || path.includes("/v1/agent/") || diff --git a/src/review/visual/shot.ts b/src/review/visual/shot.ts index 5123fcb1e2..da360a04d0 100644 --- a/src/review/visual/shot.ts +++ b/src/review/visual/shot.ts @@ -34,9 +34,17 @@ type ScreenshotRequest = { abort(): Promise; continue(): Promise; }; +type ScreenshotPage = { + evaluate(fn: () => T): Promise; + screenshot(options: { type: "png"; fullPage: true }): Promise; +}; export const DESKTOP_VIEWPORT: Viewport = { width: 1440, height: 900 }; export const MOBILE_VIEWPORT: Viewport = { width: 390, height: 844 }; // iPhone-class portrait const VIEWPORT = DESKTOP_VIEWPORT; +export const MAX_SCREENSHOT_HEIGHT = 10000; +export const MAX_SCREENSHOT_PIXELS = 14_400_000; // 1440 × 10000, matching the full-page cap. +export const MAX_SCREENSHOT_BYTES = 5 * 1024 * 1024; +const SCREENSHOT_TIMEOUT_MS = 10000; /** Per-call shot-route options: the R2 namespace (key prefix) + the production host for the on-demand render * allowlist. Defaults to gittensory so the /gittensory/shot route works with no options. */ @@ -114,6 +122,61 @@ function isAllowedHost(targetUrl: string, env: Env, productionUrl?: string): boo return false; } +const PNG_SIGNATURE = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]; + +/** Reads a PNG's real width/height straight from its IHDR chunk -- Chromium's own rasterized output, not a + * value the screenshotted page's JavaScript can influence. Returns null (fail-closed) for anything that + * isn't a well-formed PNG IHDR header, which the caller must treat as "reject", not "skip the check". */ +function readPngDimensions(png: Uint8Array): { width: number; height: number } | null { + if (png.byteLength < 24) return null; + for (let i = 0; i < PNG_SIGNATURE.length; i++) { + if (png[i] !== PNG_SIGNATURE[i]) return null; + } + if (String.fromCharCode(png[12]!, png[13]!, png[14]!, png[15]!) !== "IHDR") return null; + const view = new DataView(png.buffer, png.byteOffset, png.byteLength); + return { width: view.getUint32(16, false), height: view.getUint32(20, false) }; +} + +async function captureBoundedFullPageShot(page: ScreenshotPage, viewport: Viewport): Promise { + // Fast-path only: this executes inside the screenshotted PAGE's own JS realm, so a hostile page can override + // scrollHeight/offsetHeight getters (e.g. via Object.defineProperty) to under-report its height and sail + // through this check -- it does not by itself guard anything (#3712 security review). Real enforcement is + // the post-capture dimension re-check below, against Chromium's actual rasterized output. + const height = await page.evaluate(() => { + const doc = (globalThis as unknown as { document: { body: { scrollHeight: number; offsetHeight: number }; documentElement: { clientHeight: number; scrollHeight: number; offsetHeight: number } } }).document; + const body = doc.body; + const element = doc.documentElement; + return Math.ceil(Math.max(body.scrollHeight, body.offsetHeight, element.clientHeight, element.scrollHeight, element.offsetHeight)); + }); + const pixelArea = viewport.width * height; + if (height > MAX_SCREENSHOT_HEIGHT || pixelArea > MAX_SCREENSHOT_PIXELS) { + console.log(JSON.stringify({ ev: "render_screenshot_too_large", width: viewport.width, height, maxHeight: MAX_SCREENSHOT_HEIGHT, maxPixels: MAX_SCREENSHOT_PIXELS })); + return null; + } + + const shot = await Promise.race([ + page.screenshot({ type: "png", fullPage: true }), + new Promise((resolve) => setTimeout(() => resolve(null), SCREENSHOT_TIMEOUT_MS)), + ]); + if (!shot) { + console.log(JSON.stringify({ ev: "render_screenshot_timeout", timeoutMs: SCREENSHOT_TIMEOUT_MS })); + return null; + } + if (shot.byteLength > MAX_SCREENSHOT_BYTES) { + console.log(JSON.stringify({ ev: "render_screenshot_bytes_too_large", bytes: shot.byteLength, maxBytes: MAX_SCREENSHOT_BYTES })); + return null; + } + // Re-validate against the ACTUAL rendered PNG dimensions -- these come from Chromium's rasterizer, not page + // script, so the height spoof above cannot reach them. Anything that isn't a readable PNG header is rejected + // rather than let through, since that's precisely what a successful spoof would look like from here. + const dims = readPngDimensions(shot); + if (!dims || dims.height > MAX_SCREENSHOT_HEIGHT || dims.width * dims.height > MAX_SCREENSHOT_PIXELS) { + console.log(JSON.stringify({ ev: "render_screenshot_dimensions_too_large", width: dims?.width ?? null, height: dims?.height ?? null, maxHeight: MAX_SCREENSHOT_HEIGHT, maxPixels: MAX_SCREENSHOT_PIXELS })); + return null; + } + return shot; +} + /** * Render a page to a PNG via the Browser Rendering binding, also reporting whether the route redirected to a * sign-in wall. `authWalled` is true when the FINAL url looks like a login page that the REQUESTED url was @@ -167,12 +230,10 @@ export async function captureShot(env: Env, url: string, viewport: Viewport = VI console.log(JSON.stringify({ ev: "render_screenshot_auth_walled", url, final: page.url().slice(0, 200) })); return { png: null, authWalled: true }; } - // Full-page (not just the viewport): before/after must show the SAME position on the page for any - // change, however far down it is. A viewport-only shot only captures whatever happens to be in frame at - // load time, which for a change midway down a long page would silently miss it in both cells. Capturing - // the whole scrollable height means the changed region is always present in both images at the same - // relative offset, with no need to locate/scroll to it first. - const shot = (await page.screenshot({ type: "png", fullPage: true })) as Uint8Array; + // Full-page (not just the viewport), but bounded: before/after should include the same page position for + // normal review pages without letting attacker-controlled document height or PNG size drive unbounded + // Chromium raster work on the public screenshot route. + const shot = await captureBoundedFullPageShot(page, viewport); return { png: shot, authWalled: false }; } catch (error) { // Log before degrading to null — otherwise a networkidle0 timeout, a binding quota error, or a render diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index 50886c2686..11aef8ec8a 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -105,6 +105,7 @@ describe("private-beta auth and rate limiting", () => { expect(routeClassForPath("/v1/orb/ingest")).toBe("strict"); // open telemetry ingest — abuse-capped per IP expect(routeClassForPath("/v1/auth/github/device/start")).toBe("strict"); expect(routeClassForPath("/v1/local/branch-analysis")).toBe("expensive"); + expect(routeClassForPath("/gittensory/shot")).toBe("expensive"); expect(routeClassForPath("/v1/scoring/preview")).toBe("expensive"); expect(routeClassForPath("/v1/upstream/status")).toBe("expensive"); expect(routeClassForPath("/v1/contributors/jsonbored/decision-pack")).toBe("expensive"); diff --git a/test/unit/visual-shot.test.ts b/test/unit/visual-shot.test.ts index 7f93edaf2b..5343f5919a 100644 --- a/test/unit/visual-shot.test.ts +++ b/test/unit/visual-shot.test.ts @@ -12,7 +12,9 @@ const mocks = vi.hoisted(() => ({ evaluate: vi.fn(), // captureScrollFrames' FIRST page.evaluate() call queries scrollHeight; every later call (scrollTo, the // settle delay) discards its return value — so only the first call's resolved value matters to the code - // under test, regardless of exactly how many scroll/settle evaluate() calls happen after it. + // under test, regardless of exactly how many scroll/settle evaluate() calls happen after it. captureShot's + // own bounded-full-page-screenshot height check is likewise a single evaluate() call, so it reuses this + // same mock rather than introducing a second, redundant height property. scrollHeight: 900, evaluateCallCount: 0, })); @@ -45,6 +47,18 @@ function r2Env(objects: Record): Env { } as unknown as Env; } +// A minimal (not fully spec-complete) PNG buffer whose IHDR chunk reports the given width/height -- enough +// for readPngDimensions() to parse, which is all captureBoundedFullPageShot's post-capture check reads. +function fakePng(width: number, height: number): Uint8Array { + const buf = new Uint8Array(24); + buf.set([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a], 0); + buf.set([0x00, 0x00, 0x00, 0x0d], 8); + buf.set([0x49, 0x48, 0x44, 0x52], 12); + new DataView(buf.buffer).setUint32(16, width, false); + new DataView(buf.buffer).setUint32(20, height, false); + return buf; +} + function makeRequest(url: string, navigation = true) { return { url: () => url, @@ -60,6 +74,7 @@ describe("visual screenshot on-demand SSRF guard", () => { mocks.finalUrl = "https://preview.pages.dev/page"; mocks.scrollHeight = 900; mocks.evaluateCallCount = 0; + mocks.screenshot.mockResolvedValue(fakePng(1440, 900)); mocks.evaluate.mockImplementation(async (fn: (...fnArgs: unknown[]) => unknown, ...fnArgs: unknown[]) => { mocks.evaluateCallCount++; // The real callback runs inside the browser's own realm (document/window), which this Node test @@ -135,14 +150,92 @@ describe("visual screenshot on-demand SSRF guard", () => { expect(mocks.screenshot).toHaveBeenCalled(); }); - it("captures the FULL page, not just the viewport — before/after must show the same page position for a change however far down it is", async () => { + it("captures the full page for bounded review pages", async () => { mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.scrollHeight = 10_000; await handleShot(request("https://preview.pages.dev/page"), env()); expect(mocks.screenshot).toHaveBeenCalledWith({ type: "png", fullPage: true }); }); + it("rejects attacker-controlled pages taller than the full-page screenshot cap before rasterizing", async () => { + mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.scrollHeight = 10_001; + + const response = await handleShot(request("https://preview.pages.dev/page"), env()); + + expect(response.status).toBe(502); + expect(mocks.screenshot).not.toHaveBeenCalled(); + }); + + it("rejects full-page screenshots whose pixel area exceeds the cap", async () => { + mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.scrollHeight = 10_000; + + const response = await handleShot(shotRequest(`url=${encodeURIComponent("https://preview.pages.dev/page")}&w=2560&h=900`), env()); + + expect(response.status).toBe(502); + expect(mocks.screenshot).not.toHaveBeenCalled(); + }); + + it("rejects oversized PNG output before returning it from the public shot route", async () => { + mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.screenshot.mockResolvedValue(new Uint8Array(5 * 1024 * 1024 + 1)); + + const response = await handleShot(request("https://preview.pages.dev/page"), env()); + + expect(response.status).toBe(502); + }); + + it("REGRESSION (security review, #3712): rejects an oversized screenshot even when the page's own evaluate() height lies", async () => { + // A hostile page can override document.body/documentElement scrollHeight/offsetHeight getters to + // under-report its own height and sail through the pre-capture fast-path check. + mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.scrollHeight = 100; + mocks.screenshot.mockResolvedValue(fakePng(1440, 10_001)); + + const response = await handleShot(request("https://preview.pages.dev/page"), env()); + + expect(response.status).toBe(502); + }); + + it("REGRESSION (security review, #3712): rejects a spoofed page whose real PNG area (not height alone) exceeds the cap", async () => { + // Height (6000) is under MAX_SCREENSHOT_HEIGHT on its own -- only the width*height area check should + // catch this one, isolating that OR-branch from the height branch exercised by the test above. + mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.scrollHeight = 100; + mocks.screenshot.mockResolvedValue(fakePng(2560, 6000)); + + const response = await handleShot(shotRequest(`url=${encodeURIComponent("https://preview.pages.dev/page")}&w=2560&h=100`), env()); + + expect(response.status).toBe(502); + }); + + it("REGRESSION (security review, #3712): fails closed when the rasterized output is not a well-formed PNG", async () => { + mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.screenshot.mockResolvedValue(new Uint8Array([1, 2, 3])); + + const response = await handleShot(request("https://preview.pages.dev/page"), env()); + + expect(response.status).toBe(502); + }); + + it("times out screenshot rasterization that does not finish", async () => { + vi.useFakeTimers(); + try { + mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.screenshot.mockReturnValue(new Promise(() => undefined)); + + const result = captureShot(env(), "https://preview.pages.dev/page"); + await vi.advanceTimersByTimeAsync(10_000); + + await expect(result).resolves.toEqual({ png: null, authWalled: false }); + } finally { + vi.useRealTimers(); + } + }); + it("never emulates a color scheme when no theme is requested — every existing caller, byte-identical to today", async () => { mocks.finalUrl = "https://preview.pages.dev/page"; await captureShot(env(), "https://preview.pages.dev/page");