From 2ac160b4a2b7cd7872a517dbac149476c431e999 Mon Sep 17 00:00:00 2001
From: Anthony Ettinger <anthony@chovy.com>
Date: Fri, 25 Sep 2026 06:06:32 +0000
Subject: [PATCH] feat(ads): let a video unit measure itself, and harden the
 field that made that risky
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Adds allow-scripts to the ad.js srcdoc iframe for video and audio fills
only, so an in-banner video served through the JSON tag can report
playback the way the frame path already does. Roughly 9 video
impressions a day were invisible: chosen, rendered, and silent.

allow-same-origin is NOT granted and must never be. The two together let
a framed document reach frameElement and delete its own sandbox
attribute, which is not a sandbox. Without it the creative keeps an
opaque origin: it cannot read the publisher's DOM, cookies or storage,
and a compromised creative gets its own inert box and nothing else.
There is a test asserting the string never appears in a sandbox value.

Granting it per-medium rather than to every fill follows the autoplay
permission immediately below it: a static banner has nothing to report
and still runs nothing at all.

The hardening is the precondition. `font_family` was the one
advertiser-derived value reaching a CSS context, interpolated raw in
four places. esc() is the wrong tool there — inside a <style> block
`&quot;` is not a quote and `}` is still a closing brace — so an
unfiltered value could close the rule and open its own. Harmless while
nothing executes; not something to leave standing while granting
scripts.

safeFontFamily allow-lists the shape instead of escaping: letters,
digits, spaces, commas, hyphens, underscores, which covers both stacks
in production across 2,978 creatives. Applied where the value is
interpolated, not only where it is saved, so it is true for every row
already stored.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 app/ad.js/route.ts                   | 20 +++++++++++--
 app/api/ads/serve/route.ts           | 28 +++++++++++++++++-
 lib/ads/campaigns.ts                 |  3 +-
 lib/ads/creative.ts                  | 39 +++++++++++++++++++++---
 tests/ads-font-family-safety.test.ts | 44 ++++++++++++++++++++++++++++
 tests/ads-sandbox-scripts.test.ts    | 40 +++++++++++++++++++++++++
 6 files changed, 166 insertions(+), 8 deletions(-)
 create mode 100644 tests/ads-font-family-safety.test.ts
 create mode 100644 tests/ads-sandbox-scripts.test.ts

diff --git a/app/ad.js/route.ts b/app/ad.js/route.ts
index 0629fef2..617c8173 100644
--- a/app/ad.js/route.ts
+++ b/app/ad.js/route.ts
@@ -100,8 +100,24 @@ ${VISITOR_SNIPPET}
           iframe.setAttribute('scrolling', 'no');
           iframe.setAttribute('frameborder', '0');
           iframe.setAttribute('loading', 'lazy');
-          // Sandbox: allow the ad's click link to open a new tab, nothing else.
-          iframe.setAttribute('sandbox', 'allow-popups allow-popups-to-escape-sandbox allow-top-navigation-by-user-activation');
+          // Sandbox: allow the ad's click link to open a new tab, and nothing
+          // else — except on a fill that has something to measure.
+          //
+          // allow-scripts is granted ONLY to video and audio, the two media
+          // that can report playback, and for the same reason the autoplay
+          // permission below is: a tag that hands out a capability on every
+          // fill is handing out more than it uses. A static banner still runs
+          // nothing at all.
+          //
+          // allow-same-origin is NEVER granted, and the two together are why.
+          // A frame with both can reach its own frameElement and delete the
+          // sandbox attribute, which is not a sandbox at all. Without it this
+          // document has an opaque origin: it cannot read the publisher's DOM,
+          // cookies or storage, and the worst a compromised creative gets is
+          // its own inert box. Keep them apart.
+          var sandbox = 'allow-popups allow-popups-to-escape-sandbox allow-top-navigation-by-user-activation';
+          if (res.media === 'video' || res.media === 'audio') sandbox += ' allow-scripts';
+          iframe.setAttribute('sandbox', sandbox);
           // A muted <video> still needs the frame to hold the autoplay
           // permission, or Chrome refuses to start it and the unit is a poster
           // that never moves. Granted only on the fill that actually has a video
diff --git a/app/api/ads/serve/route.ts b/app/api/ads/serve/route.ts
index e94d8413..edc46e0e 100644
--- a/app/api/ads/serve/route.ts
+++ b/app/api/ads/serve/route.ts
@@ -5,6 +5,10 @@
 
 import { NextRequest, NextResponse } from "next/server";
 import { serveAd, isAdFormat } from "@/lib/ads/serve";
+import { serviceClient } from "@/lib/supabase/service";
+import { recordDecision } from "@/lib/ads/video/decisions";
+import { bannerBeaconScript, injectBannerBeacon } from "@/lib/ads/video/bannerBeacon";
+import { env } from "@/lib/env";
 import { clientIpFromHeaders, lookupGeo } from "@/lib/tracker/geo";
 import { parseDevice } from "@/lib/tracker/device";
 
@@ -64,11 +68,33 @@ export async function GET(request: NextRequest) {
 
     if (!fill) return NextResponse.json({ ok: false }, { status: 200, headers });
 
+    // Playback measurement, on the two media that have any. The tag grants this
+    // frame allow-scripts for exactly these, so the beacon has somewhere to run;
+    // every other medium gets the markup untouched and a script-free sandbox.
+    let html = fill.html;
+    if (fill.media === "video" || fill.media === "audio") {
+      const decisionId = await recordDecision(serviceClient(), {
+        slotId,
+        // A display unit is drawn once and a reload is a genuinely new
+        // impression, so the fill is its own session.
+        sessionId: crypto.randomUUID(),
+        placement: "in_banner",
+        kind: fill.media === "audio" ? "audio" : "video",
+        surface: "web",
+        fill,
+        assetRevision: null,
+      });
+      // Unmeasurable is not unservable: no decision, no script, same ad.
+      if (decisionId) {
+        html = injectBannerBeacon(html, bannerBeaconScript(decisionId, env.siteUrl));
+      }
+    }
+
     return NextResponse.json(
       {
         ok: true,
         impressionId: fill.impressionId,
-        html: fill.html,
+        html,
         clickUrl: fill.clickUrl,
         // What was actually served, which is not always what was asked for —
         // the tag sizes its iframe from this.
diff --git a/lib/ads/campaigns.ts b/lib/ads/campaigns.ts
index 2d5bb378..6afab715 100644
--- a/lib/ads/campaigns.ts
+++ b/lib/ads/campaigns.ts
@@ -1,3 +1,4 @@
+import { safeFontFamily } from "./creative";
 // Campaigns created from outside the dashboard.
 //
 // The dashboard's saveCampaign (app/actions/ads.ts) takes creatives the person
@@ -70,7 +71,7 @@ function creativeRow(campaignId: string, ownerId: string, c: AdCreative) {
     light_bg_color: c.lightBgColor ?? null,
     light_fg_color: c.lightFgColor ?? null,
     light_accent_color: c.lightAccentColor ?? null,
-    font_family: (c.fontFamily ?? "system-ui, sans-serif").slice(0, 200),
+    font_family: safeFontFamily(c.fontFamily),
   };
 }
 
diff --git a/lib/ads/creative.ts b/lib/ads/creative.ts
index fe9d6f9e..d31566d9 100644
--- a/lib/ads/creative.ts
+++ b/lib/ads/creative.ts
@@ -540,6 +540,37 @@ function esc(s: string): string {
     .replace(/"/g, "&quot;");
 }
 
+/** What a creative gets when its stored font is not something we will emit. */
+export const DEFAULT_FONT_STACK = "system-ui, sans-serif";
+
+/**
+ * A font stack, or the default.
+ *
+ * `font_family` is the one advertiser-derived value that lands in a CSS
+ * context rather than a text node, and `esc()` is the wrong tool for that: it
+ * escapes for HTML, and inside a `<style>` block `&quot;` is not a quote and
+ * `}` is still a closing brace. An unfiltered value could close the rule and
+ * open its own — a defacement today, and worse once the unit is allowed to run
+ * scripts.
+ *
+ * So this is an allow-list of shapes rather than an escape: letters, digits,
+ * spaces, commas, hyphens and underscores, which is every stack we actually
+ * ship (`system-ui, -apple-system, Segoe UI, Roboto, sans-serif` and
+ * `system-ui, sans-serif` are the only two in production). Quotes are refused
+ * outright — `Helvetica Neue` is valid CSS unquoted, so the quoted form buys
+ * nothing and costs the one character most useful for breaking out.
+ *
+ * Applied where the value is interpolated, not only where it is saved: that is
+ * what makes it true for the 2,978 creatives already stored.
+ */
+const SAFE_FONT_STACK = /^[A-Za-z0-9 _,-]+$/;
+
+export function safeFontFamily(v: string | null | undefined): string {
+  const raw = (v ?? "").trim();
+  if (!raw || raw.length > 120) return DEFAULT_FONT_STACK;
+  return SAFE_FONT_STACK.test(raw) ? raw : DEFAULT_FONT_STACK;
+}
+
 // The brand mark: a real <img> logo when we have one, otherwise an accent-tinted
 // monogram tile. Never renders empty. Sandboxed served ads can't run JS, so we
 // only show the <img> when the URL was verified at generation time.
@@ -628,7 +659,7 @@ export function renderCreativeHtml(
   if (creative.format === "feed_item") {
     return `<!doctype html><html><head><meta charset="utf-8"><style>${vars}
       body{margin:0;padding:12px;background:${cssVar("bg")};color:${cssVar("fg")};
-        font-family:${creative.fontFamily};font-size:14px;line-height:1.45}
+        font-family:${safeFontFamily(creative.fontFamily)};font-size:14px;line-height:1.45}
       a{color:${cssVar("accent")}}
       pre{overflow:auto;font:12px/1.35 ui-monospace,SFMono-Regular,Menlo,Consolas,monospace}
       hr{border:0;border-top:1px solid ${cssVar("edge")}}
@@ -655,7 +686,7 @@ export function renderCreativeHtml(
       *{box-sizing:border-box;margin:0}
       a{text-decoration:none;display:block}
       .cp-ad{display:flex;align-items:center;gap:8px;width:100%;height:${h}px;
-        background:${cssVar("bg")};font-family:${creative.fontFamily};font-size:13px;
+        background:${cssVar("bg")};font-family:${safeFontFamily(creative.fontFamily)};font-size:13px;
         padding:0 12px;overflow:hidden;border-radius:0;
         border:1px solid ${cssVar("edge")};border-left:3px solid ${cssVar("accent")}}
       .cp-head{color:${cssVar("fg")};flex:0 1 auto;min-width:0;white-space:nowrap;
@@ -730,7 +761,7 @@ export function renderCreativeHtml(
     *{box-sizing:border-box;margin:0}
     a{text-decoration:none;display:block}
     .cp-ad{position:relative;width:${w}px;height:${h}px;background:${cssVar("bg")};
-      font-family:${creative.fontFamily};overflow:hidden;border-radius:0;
+      font-family:${safeFontFamily(creative.fontFamily)};overflow:hidden;border-radius:0;
       border:1px solid ${cssVar("edge")};display:flex;flex-direction:column}
     .cp-stage{width:100%;height:${stage}px;flex:0 0 auto;background:${cssVar("solidBg")};
       display:block;object-fit:cover}
@@ -847,7 +878,7 @@ export function renderCreativeHtml(
     *{box-sizing:border-box;margin:0}
     a{text-decoration:none;display:block}
     .cp-wrap{position:relative;width:${w}px;height:${h}px;overflow:hidden}
-    .cp-ad{position:relative;width:${w}px;height:${h}px;background:${bg};font-family:${creative.fontFamily};
+    .cp-ad{position:relative;width:${w}px;height:${h}px;background:${bg};font-family:${safeFontFamily(creative.fontFamily)};
       border-radius:0;padding:${isMobile ? "8px 10px" : "14px"};overflow:hidden;
       border:1px solid ${cssVar("edge")}}
   </style></head><body>
diff --git a/tests/ads-font-family-safety.test.ts b/tests/ads-font-family-safety.test.ts
new file mode 100644
index 00000000..a0d8db26
--- /dev/null
+++ b/tests/ads-font-family-safety.test.ts
@@ -0,0 +1,44 @@
+import { describe, expect, it } from "vitest";
+import { DEFAULT_FONT_STACK, safeFontFamily } from "@/lib/ads/creative";
+
+describe("safeFontFamily", () => {
+  it("keeps both stacks that actually ship", () => {
+    // The only two values in production, across 2,978 creatives.
+    expect(safeFontFamily("system-ui, -apple-system, Segoe UI, Roboto, sans-serif")).toBe(
+      "system-ui, -apple-system, Segoe UI, Roboto, sans-serif",
+    );
+    expect(safeFontFamily("system-ui, sans-serif")).toBe("system-ui, sans-serif");
+  });
+
+  it("refuses a value that could close the CSS rule it sits in", () => {
+    // The whole point: this lands inside a <style> block, where esc() does
+    // nothing useful — `}` is still a closing brace after HTML-escaping.
+    expect(safeFontFamily("x} .cp-ad{background:red} y{")).toBe(DEFAULT_FONT_STACK);
+    expect(safeFontFamily("serif;color:red")).toBe(DEFAULT_FONT_STACK);
+  });
+
+  it("refuses anything that could pull in a remote resource", () => {
+    // CSS can exfiltrate through a URL even with scripts disabled.
+    expect(safeFontFamily("serif} *{background:url(https://evil.test/x)}")).toBe(DEFAULT_FONT_STACK);
+    expect(safeFontFamily("url(https://evil.test/x)")).toBe(DEFAULT_FONT_STACK);
+  });
+
+  it("refuses quotes, angle brackets and backslashes outright", () => {
+    for (const bad of ['"Helvetica"', "'Helvetica'", "a<b", "a>b", "a\\b", "a/*x*/b"]) {
+      expect(safeFontFamily(bad)).toBe(DEFAULT_FONT_STACK);
+    }
+  });
+
+  it("falls back on empty, missing and over-long values", () => {
+    expect(safeFontFamily("")).toBe(DEFAULT_FONT_STACK);
+    expect(safeFontFamily("   ")).toBe(DEFAULT_FONT_STACK);
+    expect(safeFontFamily(null)).toBe(DEFAULT_FONT_STACK);
+    expect(safeFontFamily(undefined)).toBe(DEFAULT_FONT_STACK);
+    // Long but otherwise legal: still refused, because nothing we ship is.
+    expect(safeFontFamily("a".repeat(121))).toBe(DEFAULT_FONT_STACK);
+  });
+
+  it("trims rather than rejecting incidental whitespace", () => {
+    expect(safeFontFamily("  Roboto, sans-serif  ")).toBe("Roboto, sans-serif");
+  });
+});
diff --git a/tests/ads-sandbox-scripts.test.ts b/tests/ads-sandbox-scripts.test.ts
new file mode 100644
index 00000000..0acb4802
--- /dev/null
+++ b/tests/ads-sandbox-scripts.test.ts
@@ -0,0 +1,40 @@
+import { describe, expect, it } from "vitest";
+import { GET } from "@/app/ad.js/route";
+
+async function tag(): Promise<string> {
+  const res = await GET();
+  return await res.text();
+}
+
+describe("ad.js sandbox", () => {
+  it("never grants allow-same-origin", async () => {
+    // With allow-scripts AND allow-same-origin together, the framed document
+    // can reach frameElement and delete its own sandbox attribute — which is
+    // not a sandbox. This assertion is the standing rule, in executable form.
+    //
+    // Comments are stripped first: the tag explains in prose why the pair is
+    // never granted, and the rule is about the value, not the file.
+    const js = (await tag()).replace(/^\s*\/\/.*$/gm, "");
+    expect(js).not.toContain("allow-same-origin");
+  });
+
+  it("grants scripts only to the media that can report playback", async () => {
+    const js = await tag();
+    expect(js).toContain("if (res.media === 'video' || res.media === 'audio') sandbox += ' allow-scripts';");
+    // The base sandbox, used for every other fill, stays script-free.
+    expect(js).toContain(
+      "var sandbox = 'allow-popups allow-popups-to-escape-sandbox allow-top-navigation-by-user-activation';",
+    );
+  });
+
+  it("still keeps the click-out permissions it always had", async () => {
+    const js = await tag();
+    expect(js).toContain("allow-popups");
+    expect(js).toContain("allow-top-navigation-by-user-activation");
+  });
+
+  it("grants autoplay only to video, as before", async () => {
+    const js = await tag();
+    expect(js).toContain("if (res.media === 'video') iframe.setAttribute('allow', 'autoplay');");
+  });
+});