feat(ads): let a video unit measure itself, and harden the field that made that risky - #323
Merged
Merged
Conversation
… made that risky 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 `"` 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>
ThreatCrush Security Scan49 finding(s) HIGH/CRITICAL: 2 | MEDIUM: 32 | LOW: 15
Snippets are redacted; ThreatCrush never prints matched credential material. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Anthony asked for both halves of the open question from #320: harden
fontFamily, then grant the sandbox.The sandbox
ad.jsframed every creative with noallow-scripts, so an in-banner video served through the JSON tag could not report anything — roughly 9 video impressions a day, chosen and rendered and silent. That permission is now granted, for video and audio fills only, and the serve route injects the same beacon the frame path already uses.Per-medium rather than blanket, following the autoplay permission immediately below it in the same file: a static banner has nothing to report and still runs nothing at all.
allow-same-originis not granted and must never be. The two together let a framed document reachframeElementand delete its ownsandboxattribute, which is not a sandbox. Without it the creative keeps 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. There is a test asserting the string never appears in a sandbox value (comments stripped, since the tag explains the rule in prose).The hardening, which is the precondition
font_familywas the one advertiser-derived value reaching a CSS context, interpolated raw in four places.esc()is the wrong tool there — inside a<style>block"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.safeFontFamilyallow-lists the shape rather than escaping it: letters, digits, spaces, commas, hyphens, underscores. That covers both stacks in production across 2,978 creatives (system-ui, -apple-system, Segoe UI, Roboto, sans-serifandsystem-ui, sans-serif). Quotes are refused outright —Helvetica Neueis valid CSS unquoted, so the quoted form buys nothing and costs the character most useful for breaking out.Applied where the value is interpolated, not only where it is saved, so it is true for every row already stored rather than only for new ones. The save path is narrowed too.
Worth noting for scope:
font_familyis not settable through the campaign API today, and no stored value contains a bracket. This closes the hole before it matters, rather than after.Verification
npm run typecheckcleannpx vitest run— 2,696 passed, 2 skipped, 0 failed (10 new across two files)next build --webpackcompiles🤖 Generated with Claude Code