diff --git a/src/review/visual/shot.ts b/src/review/visual/shot.ts index 602781a52f..858ea43c56 100644 --- a/src/review/visual/shot.ts +++ b/src/review/visual/shot.ts @@ -51,6 +51,7 @@ type ScreenshotRequest = { }; type ScreenshotPage = { evaluate(fn: () => T): Promise; + evaluate(fn: (...args: A) => T, ...args: A): Promise; screenshot(options: { type: "png"; fullPage: true }): Promise; }; // Viewport matrix (#4109): DELIBERATELY kept at 2 (desktop + mobile), not widened to metagraphed's 3-viewport @@ -71,6 +72,7 @@ export const MAX_SCREENSHOT_PIXELS = 14_400_000; // 1440 × 10000, matching the export const MAX_SCREENSHOT_BYTES = 5 * 1024 * 1024; const SCREENSHOT_TIMEOUT_MS = 10000; const SCREENSHOT_HEIGHT_PROBE_TIMEOUT_MS = 2_000; +const THEME_STORAGE_WRITE_TIMEOUT_MS = 2_000; // The reload triggered by a configured `themeStorageKey` (#4109) waits for the same network-idle signal as // the initial navigation, with the same bound -- a reload is not expected to be any slower than the first load. const THEME_STORAGE_RELOAD_TIMEOUT_MS = 20000; @@ -166,6 +168,30 @@ function readPngDimensions(png: Uint8Array): { width: number; height: number } | return { width: view.getUint32(16, false), height: view.getUint32(20, false) }; } +async function forceThemeStorage(page: ScreenshotPage, storageKey: string, storageValue: ShotTheme): Promise { + let timeoutId: ReturnType | undefined; + const write = page.evaluate( + (key: string, value: string) => { + try { + (globalThis as unknown as { localStorage: Storage }).localStorage.setItem(key, value); + } catch { + // Storage can be unavailable (privacy mode, disabled storage, a cross-origin frame, etc.) -- best-effort only. + } + }, + storageKey, + storageValue, + ); + const completed = await Promise.race([ + write.then(() => true, () => true), + new Promise((resolve) => { + timeoutId = setTimeout(() => resolve(false), THEME_STORAGE_WRITE_TIMEOUT_MS); + }), + ]); + clearTimeout(timeoutId as ReturnType); + if (!completed) console.log(JSON.stringify({ event: "render_theme_storage_write_timeout", timeoutMs: THEME_STORAGE_WRITE_TIMEOUT_MS })); + return completed; +} + 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 @@ -278,17 +304,7 @@ export async function captureShot(env: Env, url: string, viewport: Viewport = VI if (opts.theme && opts.themeStorageKey) { const storageKey = opts.themeStorageKey; const storageValue = opts.theme; - await page.evaluate( - (key: string, value: string) => { - try { - (globalThis as unknown as { localStorage: Storage }).localStorage.setItem(key, value); - } catch { - // Storage can be unavailable (privacy mode, disabled storage, a cross-origin frame, etc.) -- best-effort only. - } - }, - storageKey, - storageValue, - ); + if (!(await forceThemeStorage(page, storageKey, storageValue))) return { png: null, authWalled: false }; await page.reload({ waitUntil: "networkidle0", timeout: THEME_STORAGE_RELOAD_TIMEOUT_MS }); } // Full-page (not just the viewport), but bounded: before/after should include the same page position for @@ -393,17 +409,7 @@ export async function captureScrollFrames(env: Env, url: string, viewport: Viewp if (opts.theme && opts.themeStorageKey) { const storageKey = opts.themeStorageKey; const storageValue = opts.theme; - await page.evaluate( - (key: string, value: string) => { - try { - (globalThis as unknown as { localStorage: Storage }).localStorage.setItem(key, value); - } catch { - // Storage can be unavailable (privacy mode, disabled storage, a cross-origin frame, etc.) -- best-effort only. - } - }, - storageKey, - storageValue, - ); + if (!(await forceThemeStorage(page, storageKey, storageValue))) return { frames: [], authWalled: false }; await page.reload({ waitUntil: "networkidle0", timeout: THEME_STORAGE_RELOAD_TIMEOUT_MS }); } // `document`/`window` below run inside the real page (the callback is serialized and executed in the diff --git a/test/unit/visual-shot.test.ts b/test/unit/visual-shot.test.ts index 345cb8d796..e093fc5be6 100644 --- a/test/unit/visual-shot.test.ts +++ b/test/unit/visual-shot.test.ts @@ -299,6 +299,32 @@ describe("visual screenshot on-demand SSRF guard", () => { expect(mocks.reload).toHaveBeenCalledWith({ waitUntil: "networkidle0", timeout: 20000 }); }); + it("REGRESSION (security review): times out a hostile theme localStorage write before reload", async () => { + vi.useFakeTimers(); + try { + mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.evaluate.mockImplementation((fn: (...fnArgs: unknown[]) => unknown, ...fnArgs: unknown[]) => { + if (fnArgs.length > 0) return new Promise(() => undefined); + try { + fn(...fnArgs); + } catch { + // expected — see default mock comment above. + } + return Promise.resolve(mocks.scrollHeight); + }); + + const result = captureShot(env(), "https://preview.pages.dev/page", undefined, { theme: "dark", themeStorageKey: "theme" }); + await vi.advanceTimersByTimeAsync(2_000); + + await expect(result).resolves.toEqual({ png: null, authWalled: false }); + expect(mocks.reload).not.toHaveBeenCalled(); + expect(mocks.screenshot).not.toHaveBeenCalled(); + expect(mocks.close).toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + it("never forces a theme via localStorage/reload when no themeStorageKey is configured — byte-identical to pre-#4109", async () => { mocks.finalUrl = "https://preview.pages.dev/page"; await captureShot(env(), "https://preview.pages.dev/page", undefined, { theme: "dark" }); @@ -444,6 +470,31 @@ describe("captureScrollFrames (#3612 scroll-through GIF evidence)", () => { expect(mocks.reload).toHaveBeenCalledWith({ waitUntil: "networkidle0", timeout: 20000 }); }); + it("REGRESSION (security review): times out a hostile theme localStorage write before scroll capture", async () => { + vi.useFakeTimers(); + try { + mocks.evaluate.mockImplementation((fn: (...fnArgs: unknown[]) => unknown, ...fnArgs: unknown[]) => { + if (fnArgs.length > 0) return new Promise(() => undefined); + try { + fn(...fnArgs); + } catch { + // expected — see default mock comment above. + } + return Promise.resolve(mocks.scrollHeight); + }); + + const result = captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }, { theme: "dark", themeStorageKey: "theme" }); + await vi.advanceTimersByTimeAsync(2_000); + + await expect(result).resolves.toEqual({ frames: [], authWalled: false }); + expect(mocks.reload).not.toHaveBeenCalled(); + expect(mocks.screenshot).not.toHaveBeenCalled(); + expect(mocks.close).toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + } + }); + it("never forces a theme via localStorage/reload when no themeStorageKey is configured — byte-identical to pre-#4109", async () => { await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }, { theme: "dark" }); expect(mocks.reload).not.toHaveBeenCalled();