Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 20 additions & 7 deletions src/review/visual/shot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ 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;
const SCREENSHOT_HEIGHT_PROBE_TIMEOUT_MS = 2_000;

/** 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. */
Expand Down Expand Up @@ -141,13 +142,25 @@ async function captureBoundedFullPageShot(page: ScreenshotPage, viewport: Viewpo
// 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));
});
// the post-capture dimension re-check below, against Chromium's actual rasterized output. Keep this probe
// time-bounded too: hostile getters/globals can hang before the screenshot timeout is even armed.
let heightProbeTimeoutId: ReturnType<typeof setTimeout> | undefined;
const height = await Promise.race([
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));
}),
new Promise<null>((resolve) => {
heightProbeTimeoutId = setTimeout(() => resolve(null), SCREENSHOT_HEIGHT_PROBE_TIMEOUT_MS);
}),
]);
clearTimeout(heightProbeTimeoutId as ReturnType<typeof setTimeout>);
if (height === null) {
console.log(JSON.stringify({ ev: "render_screenshot_height_probe_timeout", timeoutMs: SCREENSHOT_HEIGHT_PROBE_TIMEOUT_MS }));
return null;
}
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 }));
Expand Down
17 changes: 17 additions & 0 deletions test/unit/visual-shot.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,23 @@ describe("visual screenshot on-demand SSRF guard", () => {
}
});

it("REGRESSION (security review): times out a hostile page-realm height probe before rasterization", async () => {
vi.useFakeTimers();
try {
mocks.finalUrl = "https://preview.pages.dev/page";
mocks.evaluate.mockReturnValue(new Promise(() => undefined));

const result = captureShot(env(), "https://preview.pages.dev/page");
await vi.advanceTimersByTimeAsync(2_000);

await expect(result).resolves.toEqual({ png: null, authWalled: false });
expect(mocks.screenshot).not.toHaveBeenCalled();
expect(mocks.close).toHaveBeenCalled();
} 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");
Expand Down