Skip to content

fix(review): bound visual screenshot rendering #3712

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 2 commits into from
Jul 6, 2026
Merged

fix(review): bound visual screenshot rendering #3712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
043709c
fix(review): bound visual screenshot rendering
JSONbored Jul 6, 2026
ab12cdb
fix(review): re-validate rasterized screenshot dimensions, not just t…
JSONbored Jul 6, 2026
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
1 change: 1 addition & 0 deletions src/auth/rate-limit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/") ||
Expand Down
73 changes: 67 additions & 6 deletions src/review/visual/shot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,17 @@ type ScreenshotRequest = {
abort(): Promise<unknown>;
continue(): Promise<unknown>;
};
type ScreenshotPage = {
evaluate<T>(fn: () => T): Promise<T>;
screenshot(options: { type: "png"; fullPage: true }): Promise<Uint8Array>;
};
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. */
Expand Down Expand Up @@ -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<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
// 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(() => {

@superagent-security superagent-security Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2

Screenshot bounds check runs in attacker-controlled page context and can be bypassed

page.evaluate() reads DOM height properties that a malicious page can spoof to bypass raster limits.

Query page metrics via a browser API the page cannot tamper with instead of page.evaluate().

AI prompt 
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="src/review/visual/shot.ts">
<violation number="1" location="src/review/visual/shot.ts:126">
<priority>P2</priority>
<title>Screenshot bounds check runs in attacker-controlled page context and can be bypassed</title>
<evidence>The captureBoundedFullPageShot function uses page.evaluate(() =&gt; { ... body.scrollHeight ... }) to measure document height before screenshot rasterization. Because this JavaScript executes inside the potentially attacker-controlled page context, a malicious page can override document.body.scrollHeight, documentElement.scrollHeight, and related getters (for example via Object.defineProperty) to report a small height, pass the MAX_SCREENSHOT_HEIGHT and MAX_SCREENSHOT_PIXELS checks, and still cause unbounded Chromium raster work when page.screenshot({ fullPage: true }) runs.</evidence>
<recommendation>Use a browser/CDP metric or binding API that the page cannot tamper with to obtain the true rendered document size before rasterizing. If only page.evaluate is available, consider adding a post-screenshot dimension validation (e.g., check actual PNG width/height or browser viewport metrics after capture) and reject if they exceed the claimed bounds.</recommendation>
</violation>
</file>

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<null>((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
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions test/unit/auth.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
97 changes: 95 additions & 2 deletions test/unit/visual-shot.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}));
Expand Down Expand Up @@ -45,6 +47,18 @@ function r2Env(objects: Record<string, Uint8Array>): 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<ArrayBuffer> {
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,
Expand All @@ -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
Expand Down Expand Up @@ -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");
Expand Down