JSONbored · JSONbored · Jul 27, 2026 · Jul 27, 2026
@@ -4,8 +4,9 @@ import { z } from "zod";
 import { parsePositiveInt } from "../utils/json";
 import { analyzePRQueue, type AuthorRole, type ChecksStatus } from "../queue-intelligence";
 import { completeGitHubWebOAuth, createSessionFromGitHubToken, getLiveSessionGitHubToken, pollGitHubDeviceFlow, startGitHubDeviceFlow, startGitHubWebOAuth } from "../auth/github-oauth";
-import { enforceRateLimit, routeClassForPath } from "../auth/rate-limit";
+import { enforceRateLimit, enforceShotRenderGlobalCeiling, routeClassForPath } from "../auth/rate-limit";
 import { handleShot } from "../review/visual/shot";
+import { verifyShotRenderToken } from "../review/visual/shot-render-token";
 import { isScreenshotsEnabled } from "../review/visual-wire";
 import { buildFindingTaxonomyDocument } from "../review/finding-taxonomy";
 import { buildEnrichmentAnalyzersTaxonomyDocument } from "../review/enrichment-analyzers-taxonomy";
@@ -1377,12 +1378,35 @@ export function createApp() {
   // (*.workers.dev / *.pages.dev / PUBLIC_SITE_ORIGIN) AND the isSafeHttpUrl SSRF guard. Inert flag-OFF: with
   // LOOPOVER_REVIEW_SCREENSHOTS off nothing ever writes shots to R2, so ?key= 404s and ?url= still requires
   // an allowlisted public host. The route's own Cache-Control headers (per mode) are set inside handleShot;
-  // the rate-limit middleware classifies it as 'normal' (a sane public class) via routeClassForPath.
+  // the rate-limit middleware classifies it as 'expensive' (20 req/300s, the same class as every other
+  // headless-render/heavy-compute route) via routeClassForPath — it is the single most expensive
+  // unauthenticated operation this deployment exposes (a full headless-Chromium render per request), not a
+  // sane-default 'normal' route.
   // Flag-OFF = TRULY inert: when LOOPOVER_REVIEW_SCREENSHOTS is off nothing references this route (no comment
   // carries a /loopover/shot URL), so 404 it outright — that removes the on-demand `?url=` render surface
   // entirely until the feature is deliberately enabled, rather than relying on the host allowlist alone.
-  app.get("/loopover/shot", (c) => {
+  //
+  // TWO additional layers gate the render (`?url=`) mode specifically, on top of the per-identity rate limit
+  // above (#9044): (1) a signed, expiring token (shot-render-token.ts) that ORB itself mints when it embeds a
+  // `?url=` link in a review comment — a render only ever happens for a url ORB asked for, never an arbitrary
+  // caller-supplied one, checked BEFORE the render path so an invalid token never reaches handleShot's own
+  // host-allowlist/SSRF checks (which still apply underneath, defense-in-depth); and (2) a fixed-key GLOBAL
+  // render ceiling (enforceShotRenderGlobalCeiling), independent of caller identity, so rotating
+  // Cf-Connecting-Ip (the bypass this deployment's Node/tunnel topology made possible — see clientIp's own
+  // doc comment in auth/rate-limit.ts) can never manufacture more than the one shared bucket. Neither layer
+  // touches the `?key=`/placeholder modes, which never drive a render.
+  app.get("/loopover/shot", async (c) => {
     if (!isScreenshotsEnabled(c.env)) return c.notFound();
+    const params = new URL(c.req.url).searchParams;
+    const isRenderMode = !params.get("placeholder") && !params.get("key") && Boolean(params.get("url"));
+    if (isRenderMode) {
+      const target = params.get("url")!;
+      if (!(await verifyShotRenderToken(c.env, target, params))) {
+        return c.json({ error: "missing_or_invalid_shot_token" }, 403);
+      }
+      const limited = await enforceShotRenderGlobalCeiling(c);
+      if (limited) return limited;
+    }
     return handleShot(c.req.raw, c.env, {
       ...(c.env.PUBLIC_SITE_ORIGIN ? { productionUrl: c.env.PUBLIC_SITE_ORIGIN } : {}),
     });

@@ -57,9 +57,32 @@ export class RateLimiter extends DurableObject<Env> {
 }
 
 export async function enforceRateLimit(c: Context<{ Bindings: Env }>, routeClass: RateLimitClass): Promise<Response | null> {
-  if (!c.env.RATE_LIMITER) return null;
-  const config = CONFIG[routeClass];
   const key = await rateLimitKey(c, routeClass);
+  return checkRateLimitBucket(c, key, CONFIG[routeClass], routeClass);
+}
+
+// #9044: a SEPARATE, fixed-key (never identity-keyed) ceiling on /loopover/shot's on-demand render mode --
+// the one route on this deployment's single internet-facing path that drives a real headless-Chromium render
+// per request. The per-identity `expensive` bucket above is necessary but not sufficient: an attacker who can
+// get an arbitrary value into `Cf-Connecting-Ip` (or who simply has no header at all pre-clientIp-fix) gets a
+// FRESH per-identity bucket on every request by rotating it, bypassing the per-identity cap entirely. A fixed
+// key sidesteps that class of bypass completely -- there is only ever ONE bucket for this route's render mode,
+// no matter what identity a caller can manufacture. Deliberately its OWN (lower) budget rather than reusing
+// CONFIG.expensive: this caps the render primitive's TOTAL cost to the whole deployment, not any one caller's
+// share of it, so it is set independently of the per-identity number.
+const SHOT_RENDER_GLOBAL_CONFIG: RateLimitConfig = { limit: 30, windowSeconds: 60 };
+const SHOT_RENDER_GLOBAL_KEY = "global-ceiling:loopover-shot-render";
+
+/** Enforce the fixed-key global ceiling on `/loopover/shot`'s on-demand render mode (see
+ *  {@link SHOT_RENDER_GLOBAL_CONFIG}'s doc comment for why this exists alongside the per-identity `expensive`
+ *  class). Callers should invoke this ONLY for the render (`?url=`) mode -- the R2-serve (`?key=`) mode is
+ *  cheap and doesn't drive a render, so it stays covered by the ordinary per-identity check alone. */
+export async function enforceShotRenderGlobalCeiling(c: Context<{ Bindings: Env }>): Promise<Response | null> {
+  return checkRateLimitBucket(c, SHOT_RENDER_GLOBAL_KEY, SHOT_RENDER_GLOBAL_CONFIG, "expensive");
+}
+
+async function checkRateLimitBucket(c: Context<{ Bindings: Env }>, key: string, config: RateLimitConfig, routeClass: RateLimitClass): Promise<Response | null> {
+  if (!c.env.RATE_LIMITER) return null;
   let decisionResponse: Response;
   try {
     const id = c.env.RATE_LIMITER.idFromName(key);
@@ -243,9 +266,47 @@ async function validateBearerForRateLimit(c: Context<{ Bindings: Env }>, token:
   return Boolean((await authenticatePrivateToken(c.env, token)) ?? (await authenticateInternalToken(c.env, token)));
 }
 
+const LOOPBACK_PEER_IPS = new Set(["127.0.0.1", "::1"]);
+
+/** Rightmost hop of a (possibly multi-hop) `X-Forwarded-For` header -- the one nearest to THIS process, i.e.
+ *  the one a single trusted local proxy itself appended, as opposed to any earlier value a client could have
+ *  supplied. Only ever consulted from the loopback-trusted branch below. */
+function rightmostForwardedFor(header: string | undefined): string | undefined {
+  const hops = (header ?? "").split(",");
+  return normalizeIpAddress(hops[hops.length - 1]);
+}
+
+/**
+ * The client identity a rate-limit bucket keys on (#9044).
+ *
+ * On Cloudflare Workers, `cf-connecting-ip` is populated by the Workers runtime itself and cannot be
+ * supplied by the caller -- unconditionally trusting it there is correct and stays exactly as it always has
+ * (`env.LOOPOVER_PEER_IP` is never set on Workers, so that branch is unreachable there).
+ *
+ * Self-host runs the SAME Hono app behind a loopback-bound Node process (`server.ts`), fronted by a
+ * Cloudflare Tunnel (cloudflared). `env.LOOPOVER_PEER_IP` (set per-request by server.ts from the real TCP
+ * peer address) tells us who actually connected to this process -- the one thing that can never be spoofed
+ * by a request header. On THIS topology, a loopback peer IS the trust boundary: nothing other than the local
+ * cloudflared process can ever reach a port bound to 127.0.0.1 only, so a loopback peer legitimizes trusting
+ * the header it forwarded (falling back to the rightmost X-Forwarded-For hop, then the peer itself, if the
+ * header is absent). A NON-loopback peer means this Node process is reachable some other way (e.g. exposed
+ * directly with no tunnel in front of it, a foreseeable alternate self-host topology) -- an untrusted
+ * connection whose headers could be anything, so the header is ignored entirely and the bucket keys on the
+ * peer's own real address instead. Either way, once real peer info exists, "unknown-ip" (a bucket shared by
+ * every anonymous caller worldwide -- the DoS this closes) is never reachable again.
+ */
 function clientIp(c: Context<{ Bindings: Env }>): string {
-  // Only trust Cloudflare-populated client IPs. Proxy fallback headers can be supplied by clients in Workers.
-  return normalizeIpAddress(c.req.header("cf-connecting-ip")) ?? "unknown-ip";
+  const peerIp = normalizeIpAddress(c.env.LOOPOVER_PEER_IP);
+  if (!peerIp) {
+    // Workers, or any caller (tests, an unwired self-host build) that never threads a peer IP in --
+    // byte-identical to the pre-#9044 behavior.
+    return normalizeIpAddress(c.req.header("cf-connecting-ip")) ?? "unknown-ip";
+  }
+  if (LOOPBACK_PEER_IPS.has(peerIp)) {
+    return normalizeIpAddress(c.req.header("cf-connecting-ip")) ?? rightmostForwardedFor(c.req.header("x-forwarded-for")) ?? peerIp;
+  }
+  // Untrusted topology: never honor a caller-suppliable header here.
+  return peerIp;
 }
 
 function normalizeIpAddress(value: string | undefined): string | undefined {

@@ -75,6 +75,15 @@ declare global {
     };
     PUBLIC_API_ORIGIN?: string;
     PUBLIC_SITE_ORIGIN?: string;
+    /** #9044: the genuine TCP peer address of whoever connected directly to this self-host Node process --
+     *  threaded in by server.ts from `@hono/node-server`'s own `HttpBindings.incoming.socket.remoteAddress`
+     *  (the SECOND argument its `FetchCallback` type offers, which this repo's synthesized-env `serve()` call
+     *  previously ignored entirely). UNSPOOFABLE, unlike any header. Absent on Cloudflare Workers (no Node
+     *  socket exists there -- cf-connecting-ip is the only, and correct, signal) and absent in any test/harness
+     *  that doesn't wire it, which is exactly the "no peer info" case clientIp() (src/auth/rate-limit.ts) falls
+     *  back from -- see that function's own doc comment for the full trust-boundary reasoning. Never a wrangler
+     *  binding/var (no cf-typegen entry needed): a plain runtime-only field server.ts sets per request. */
+    LOOPOVER_PEER_IP?: string;
     /** Comma-separated extra origins (each `scheme://host[:port]`) allowed as a post-GitHub-OAuth `returnTo`
      *  redirect target, alongside PUBLIC_SITE_ORIGIN. Lets the web login flow work correctly from more than
      *  one live UI domain at once (e.g. during a brand/domain migration) without breaking PUBLIC_SITE_ORIGIN's

@@ -85,7 +85,11 @@ function ipv6IsPrivateOrLocal(host: string): boolean {
   return false;
 }
 
-function hostIsPrivateOrLocal(host: string): boolean {
+/** Exported (unlike this module's other internals) so a caller that has independently resolved a hostname to
+ *  a literal IP address (e.g. shot.ts's DNS-resolution pin, #9044) can run that resolved address through the
+ *  exact same disallowed-range checks a literal-IP URL already gets via isSafeHttpUrl -- without duplicating
+ *  the range tables here. Still pure: no I/O, just the same string-in/boolean-out check as always. */
+export function hostIsPrivateOrLocal(host: string): boolean {
   // Normalize once: lower-case, then strip the FQDN root dot(s) the parser keeps on named hosts
   // (`localhost.`) but not on IP literals — else `localhost.` / `foo.internal.` would read as public.
   const h = host.toLowerCase().replace(/\.+$/, "");

@@ -30,6 +30,7 @@ import {
   parseRepo,
 } from "./preview-url";
 import { captureInteractionFrames, captureScrollFrames, captureShot, DESKTOP_VIEWPORT, MOBILE_VIEWPORT, type InteractionAction, type ShotTheme, type Viewport } from "./shot";
+import { mintShotRenderToken } from "./shot-render-token";
 import { compareCapturedScreenshots, isVisualDiffAvailable, type VisualDiffOutcome } from "./pixel-diff";
 import { encodeScrollGif, isScrollGifAvailable } from "./scroll-gif";
 import { detectAutoHoverInteractions, type ChangedCssFile } from "./interaction-detection";
@@ -366,9 +367,12 @@ async function capturePage(
   // Carries the theme (#3678) and, when set, the storage key (#4109) so a LATER on-demand fetch of this
   // exact URL (e.g. a failed/never-persisted render retried by GitHub's image proxy) still requests the
   // matching prefers-color-scheme/localStorage forcing, not the default — handleShot's Mode B reads these
-  // same &theme=/&themeStorageKey= params. Omitted when unset, unchanged from today.
+  // same &theme=/&themeStorageKey= params. Omitted when unset, unchanged from today. The trailing
+  // &exp=&sig= (#9044) is a signed, expiring token ORB mints for exactly this url -- routes.ts requires it
+  // before ever invoking the render path, so a live Chromium render only ever happens for a page ORB itself
+  // decided to capture. See shot-render-token.ts's own doc comment for the full threat model.
   const onDemand = shotBase
-    ? `${shotBase}/${NAMESPACE}/shot?url=${encodeURIComponent(page)}&w=${viewport.width}&h=${viewport.height}${theme ? `&theme=${theme}` : ""}${theme && themeStorageKey ? `&themeStorageKey=${encodeURIComponent(themeStorageKey)}` : ""}`
+    ? `${shotBase}/${NAMESPACE}/shot?url=${encodeURIComponent(page)}&w=${viewport.width}&h=${viewport.height}${theme ? `&theme=${theme}` : ""}${theme && themeStorageKey ? `&themeStorageKey=${encodeURIComponent(themeStorageKey)}` : ""}&${await mintShotRenderToken(env, page)}`
     : page;
 
   if (env.REVIEW_AUDIT) {

@@ -0,0 +1,48 @@
+// Signed, expiring token gating /loopover/shot's on-demand `?url=` render mode (#9044).
+//
+// The route is public + unauthenticated by design (GitHub's camo image proxy fetches it with no bearer
+// token -- see routes.ts), so its only defense against an attacker requesting a render of an ARBITRARY
+// allowlisted-host URL (any path under *.workers.dev/*.pages.dev, which is free for anyone to register) was
+// the host allowlist itself. This token ties a render request back to a URL ORB ITSELF decided to capture:
+// capture.ts mints the token when it builds the on-demand fallback link for a review comment; routes.ts
+// validates it before ever calling handleShot's render path. The existing host allowlist stays in place
+// underneath this as defense-in-depth (belt and suspenders), not replaced by it.
+//
+// Reuses this repo's existing HMAC-SHA256 primitive (utils/crypto.ts hmacHex/timingSafeEqualHex -- the same
+// one verifyGitHubSignature uses) and the always-required INTERNAL_JOB_TOKEN as the signing secret, so no new
+// secret needs provisioning on any existing self-host install.
+import { hmacHex, timingSafeEqualHex } from "../../utils/crypto";
+
+// Generous enough that a normal reviewer opening a PR within about a day of the comment being posted still
+// gets a live render; short enough to bound this public, unauthenticated render primitive's real replay
+// window. A comment's embedded link is minted once and never re-signed, so this also bounds how long a
+// stale/never-viewed on-demand link stays renderable at all -- an explicit tradeoff, not an oversight (see
+// this repo's own PR discussion for #9044).
+export const SHOT_RENDER_TOKEN_TTL_MS = 24 * 60 * 60 * 1000;
+
+async function signShotRenderToken(env: Env, url: string, expiresAtMs: number): Promise<string> {
+  return hmacHex(env.INTERNAL_JOB_TOKEN, `${url}:${expiresAtMs}`);
+}
+
+/** Mint the `exp=<epoch-ms>&sig=<hmac-hex>` query fragment (no leading `&`/`?`) for `url`, to append to an
+ *  on-demand `/loopover/shot?url=...` link. `url` must be the EXACT string the request's own `?url=` param
+ *  will decode to (matching {@link verifyShotRenderToken}'s own `url` argument) -- any difference (a
+ *  different query-param ordering, an extra trailing slash) fails validation, since the signature covers the
+ *  url verbatim. */
+export async function mintShotRenderToken(env: Env, url: string): Promise<string> {
+  const expiresAt = Date.now() + SHOT_RENDER_TOKEN_TTL_MS;
+  const sig = await signShotRenderToken(env, url, expiresAt);
+  return `exp=${expiresAt}&sig=${sig}`;
+}
+
+/** Validate a render request's `exp`/`sig` params against the exact `url` it's requesting. False (fail
+ *  closed, matching every other guard on this public route) on a missing/malformed/expired/tampered token. */
+export async function verifyShotRenderToken(env: Env, url: string, params: URLSearchParams): Promise<boolean> {
+  const expParam = params.get("exp");
+  const sig = params.get("sig");
+  if (!expParam || !sig) return false;
+  const expiresAt = Number(expParam);
+  if (!Number.isFinite(expiresAt) || expiresAt <= Date.now()) return false;
+  const expected = await signShotRenderToken(env, url, expiresAt);
+  return timingSafeEqualHex(sig, expected);
+}