fix(review): bound visual screenshot rendering

[JSONbored]

Motivation

• Full-page screenshots (fullPage: true) let attacker-controlled document height drive unbounded Chromium raster work and unbounded R2 storage from the public /gittensory/shot?url= surface.
• The change restores safety by bounding document height, pixel area, output size, and raster timeout while preserving full-page captures for normal review pages.

Description

• Add explicit caps and a bounded helper: MAX_SCREENSHOT_HEIGHT, MAX_SCREENSHOT_PIXELS, MAX_SCREENSHOT_BYTES, and SCREENSHOT_TIMEOUT_MS plus captureBoundedFullPageShot(page, viewport) which measures document height, enforces the caps, times out the raster, and checks returned PNG bytes.
• Replace the direct page.screenshot({ type: "png", fullPage: true }) call with the bounded helper so oversized or slow renders degrade to png: null.
• Classify the public screenshot route as expensive in routeClassForPath so the endpoint is rate-limited more strictly.
• Add regression/unit tests in test/unit/visual-shot.test.ts and update test/unit/auth.test.ts to cover bounded full-page captures, over-height pages, excessive pixel-area, oversized PNG output, a screenshot timeout, and the route classification.

Testing

• Ran npx vitest run test/unit/visual-shot.test.ts test/unit/auth.test.ts and all targeted unit tests passed (47 tests passed).
• Ran npm run typecheck and git diff --check, both succeeded locally.
• Attempted to run the full gate with npm run test:ci, but the environment hit unrelated long-running/unit timeouts during unrelated queue/backfill tests so the full suite could not be completed here.
• Attempted npm audit --audit-level=moderate, but the registry audit endpoint returned 403 in this environment so that check could not be completed here.

---

Codex Task

[loopover-orb]

Important

🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪

🔍 Gittensory is reviewing…

AI analysis is in progress. This comment will update when the review is complete.

_{🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟪 Reviewing}

[codecov]

Codecov Report

❌ Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.33%. Comparing base (f15b832) to head (ab12cdb).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/review/visual/shot.ts	94.11%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3712   +/-   ##
=======================================
  Coverage   93.33%   93.33%           
=======================================
  Files         317      317           
  Lines       32433    32467   +34     
  Branches    11892    11901    +9     
=======================================
+ Hits        30270    30303   +33     
  Misses       1530     1530           
- Partials      633      634    +1     

Files with missing lines	Coverage Δ
src/auth/rate-limit.ts	98.83% <100.00%> (+0.01%)	⬆️
src/review/visual/shot.ts	91.52% <94.11%> (+1.24%)	⬆️

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JSONbored]

Rebased onto current `main` to clear the `codecov/patch` failure (patch coverage was 83.33%, 4 lines missing in `src/review/visual/shot.ts` — the `page.evaluate()` height-computation callback inside `captureBoundedFullPageShot` was never actually invoked by the test's mock, just short-circuited).

The rebase conflicted with `test/unit/visual-shot.test.ts` against the since-merged scroll-through GIF capture feature (#3612/#3688), which had already solved the identical problem (a mocked `evaluate()` that needs to really invoke its callback for coverage) with a cleaner technique than my first attempt — it just calls the callback and swallows the expected "no `document` in this Node test env" throw, rather than stubbing a fake DOM object. Verified that resolution already covers the previously-missing lines (confirmed via local coverage), so I dropped my own redundant fix and kept the merged result. 45/45 tests pass, typecheck/actionlint/audit clean.

[superagent-security]

Superagent found 1 security concern(s).

[superagent-security]

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>

[JSONbored]

Addressed Superagent's P2 finding (`src/review/visual/shot.ts:126`): the pre-capture height check runs `page.evaluate()` inside the target page's own JS context, so a malicious page can override `document.body.scrollHeight`/`offsetHeight` (e.g. via `Object.defineProperty`) to report a small height, pass the bound, and still get rasterized to its real, oversized size.

Added a tamper-proof recheck: decode the actual width/height Chromium wrote into the output PNG's own IHDR chunk (a cheap ~24-byte header read, not a re-render) and reject if those real dimensions exceed the same bounds — independent of anything the page's own JS could influence. 2 new tests (spoofed-height rejection + normal-dimensions acceptance), 47/47 pass, typecheck/actionlint clean.

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.