Skip to content

fix(review): bound theme storage writes #4177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JSONbored merged 1 commit into from
Jul 8, 2026
Merged

fix(review): bound theme storage writes #4177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 28 additions & 22 deletions src/review/visual/shot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ type ScreenshotRequest = {
};
type ScreenshotPage = {
evaluate<T>(fn: () => T): Promise<T>;
evaluate<T, A extends unknown[]>(fn: (...args: A) => T, ...args: A): Promise<T>;
screenshot(options: { type: "png"; fullPage: true }): Promise<Uint8Array>;
};
// Viewport matrix (#4109): DELIBERATELY kept at 2 (desktop + mobile), not widened to metagraphed's 3-viewport
Expand All @@ -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;
Expand Down Expand Up @@ -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<boolean> {
let timeoutId: ReturnType<typeof setTimeout> | 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<false>((resolve) => {
timeoutId = setTimeout(() => resolve(false), THEME_STORAGE_WRITE_TIMEOUT_MS);
}),
]);
clearTimeout(timeoutId as ReturnType<typeof setTimeout>);
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<Uint8Array | null> {
// 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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
51 changes: 51 additions & 0 deletions test/unit/visual-shot.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" });
Expand Down Expand Up @@ -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();
Expand Down