Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions crates/js/lib/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/js/lib/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"format:write": "prettier --write \"**/*.{ts,tsx,js,json,css,md}\""
},
"dependencies": {
"dompurify": "3.3.2",
"prebid.js": "^10.26.0"
},
"devDependencies": {
Expand Down
227 changes: 215 additions & 12 deletions crates/js/lib/src/core/render.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,221 @@
// Rendering utilities for Trusted Server demo placements: find slots, seed placeholders,
// and inject creatives into sandboxed iframes.
import createDOMPurify, {
type DOMPurify as DOMPurifyInstance,
type RemovedAttribute,
type RemovedElement,
} from 'dompurify';

import { log } from './log';
import type { AdUnit } from './types';
import { getUnit, getAllUnits, firstSize } from './registry';
import NORMALIZE_CSS from './styles/normalize.css?inline';
import IFRAME_TEMPLATE from './templates/iframe.html?raw';

const DANGEROUS_TAG_NAMES = new Set([
'base',
'embed',
'form',
'iframe',
'link',
'meta',
'object',
'script',
]);
const URI_ATTRIBUTE_NAMES = new Set([
'action',
'background',
'formaction',
'href',
'poster',
'src',
'srcdoc',
'xlink:href',
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpicksrcdoc is HTML content, not a URI.

srcdoc doesn't belong in URI_ATTRIBUTE_NAMES. DOMPurify already strips it (only valid on <iframe>, which is in DANGEROUS_TAG_NAMES). Having it here is semantically wrong and could cause false positives if isDangerousRemoval is refined.

]);
const DANGEROUS_URI_VALUE_PATTERN = /^\s*(?:javascript:|vbscript:|data\s*:\s*text\/html\b)/i;
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔧 wrenchDANGEROUS_URI_VALUE_PATTERN is too narrow for data: URIs.

Only blocks data:text/html. Misses dangerous MIME types that can execute scripts:

  • data:text/xml
  • data:application/xhtml+xml
  • data:image/svg+xml (SVG can embed <script> elements)

Ad creatives have no legitimate need for data: URIs.

Fix:

const DANGEROUS_URI_VALUE_PATTERN = /^\s*(?:javascript:|vbscript:|data\s*:)/i;

const DANGEROUS_STYLE_PATTERN = /\bexpression\s*\(|\burl\s*\(\s*['"]?\s*javascript:/i;
const CREATIVE_SANDBOX_TOKENS = [
'allow-forms',
Copy link
Collaborator

Choose a reason for hiding this comment

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

♻️ refactor — Sandbox policy is internally inconsistent.

form is in DANGEROUS_TAG_NAMES (creatives with <form> are rejected), but allow-forms is in CREATIVE_SANDBOX_TOKENS (form submission is permitted in the sandbox). These contradict each other.

Suggestion: Remove allow-forms from the sandbox tokens to match the rejection policy. Or, if form creatives should be supported, remove form from DANGEROUS_TAG_NAMES.

'allow-popups',
'allow-popups-to-escape-sandbox',
'allow-top-navigation-by-user-activation',
] as const;

export type CreativeSanitizationRejectionReason =
| 'empty-after-sanitize'
| 'invalid-creative-html'
| 'removed-dangerous-content'
| 'sanitizer-unavailable';

export type AcceptedCreativeHtml = {
kind: 'accepted';
originalLength: number;
sanitizedHtml: string;
sanitizedLength: number;
removedCount: number;
};

export type RejectedCreativeHtml = {
kind: 'rejected';
originalLength: number;
sanitizedLength: number;
removedCount: number;
rejectionReason: CreativeSanitizationRejectionReason;
};

export type SanitizeCreativeHtmlResult = AcceptedCreativeHtml | RejectedCreativeHtml;

let creativeSanitizer: DOMPurifyInstance | null | undefined;

function normalizeId(raw: string): string {
const s = String(raw ?? '').trim();
return s.startsWith('#') ? s.slice(1) : s;
}

function getCreativeSanitizer(): DOMPurifyInstance | null {
if (creativeSanitizer !== undefined) {
return creativeSanitizer;
}

if (typeof window === 'undefined') {
creativeSanitizer = null;
return creativeSanitizer;
}

try {
creativeSanitizer = createDOMPurify(window);
} catch (err) {
log.warn('sanitizeCreativeHtml: failed to initialize DOMPurify', err);
creativeSanitizer = null;
}

return creativeSanitizer;
}

function isDangerousRemoval(removedItem: RemovedAttribute | RemovedElement): boolean {
if ('element' in removedItem) {
const tagName = removedItem.element.nodeName.toLowerCase();
return DANGEROUS_TAG_NAMES.has(tagName);
}

const attrName = removedItem.attribute?.name.toLowerCase() ?? '';
const attrValue = removedItem.attribute?.value ?? '';

if (attrName.startsWith('on')) {
return true;
}

if (URI_ATTRIBUTE_NAMES.has(attrName)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔧 wrenchisDangerousRemoval over-flags all removed URI attributes regardless of value.

Any removed attribute in URI_ATTRIBUTE_NAMES is treated as dangerous. DOMPurify may strip a src or href for benign reasons (malformed URL, data:image/png). This creates false rejections for legitimate creatives.

Compare with hasDangerousMarkup (line 142) which correctly checks the attribute value against DANGEROUS_URI_VALUE_PATTERN. The two checks are inconsistent.

Fix: Check the removed value, not just the attribute name:

if (URI_ATTRIBUTE_NAMES.has(attrName) && DANGEROUS_URI_VALUE_PATTERN.test(attrValue)) {
  return true;
}

return true;
}

if (attrName === 'style' && DANGEROUS_STYLE_PATTERN.test(attrValue)) {
return true;
}

return false;
}

function hasDangerousMarkup(candidateHtml: string): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

♻️ refactorhasDangerousMarkup post-sanitization scan lacks intent documentation.

This re-parses DOMPurify's output for the same patterns DOMPurify should have already handled. It's a valid safety net — especially for CSS expression() in <style> elements that DOMPurify's default config allows through — but the comment doesn't explain why the output of a sanitizer is being re-scanned.

Suggestion: Add a comment clarifying the defense-in-depth intent, e.g.: "Safety net: re-scan after sanitization to catch patterns DOMPurify may allow through (e.g., CSS expressions) or sanitizer bugs."

const fragment = document.createElement('template');
// The HTML parser normalizes entity-encoded attribute values before we inspect them.
fragment.innerHTML = candidateHtml;

for (const element of fragment.content.querySelectorAll('*')) {
const tagName = element.nodeName.toLowerCase();
if (DANGEROUS_TAG_NAMES.has(tagName)) {
return true;
}

if (tagName === 'style' && DANGEROUS_STYLE_PATTERN.test(element.textContent ?? '')) {
return true;
}

for (const attrName of element.getAttributeNames()) {
const normalizedAttrName = attrName.toLowerCase();
const attrValue = element.getAttribute(attrName) ?? '';

if (normalizedAttrName.startsWith('on')) {
return true;
}

if (
URI_ATTRIBUTE_NAMES.has(normalizedAttrName) &&
DANGEROUS_URI_VALUE_PATTERN.test(attrValue)
) {
return true;
}

if (normalizedAttrName === 'style' && DANGEROUS_STYLE_PATTERN.test(attrValue)) {
return true;
}
}
}

return false;
}

// Sanitize the untrusted creative fragment before it is embedded into the trusted iframe shell.
export function sanitizeCreativeHtml(creativeHtml: unknown): SanitizeCreativeHtmlResult {
if (typeof creativeHtml !== 'string') {
return {
kind: 'rejected',
originalLength: 0,
sanitizedLength: 0,
removedCount: 0,
rejectionReason: 'invalid-creative-html',
};
}

const originalLength = creativeHtml.length;
const sanitizer = getCreativeSanitizer();

if (!sanitizer || !sanitizer.isSupported) {
return {
kind: 'rejected',
originalLength,
sanitizedLength: 0,
removedCount: 0,
rejectionReason: 'sanitizer-unavailable',
};
}

const sanitizedHtml = sanitizer.sanitize(creativeHtml, {
// Keep the result as a plain string because iframe.srcdoc expects string HTML.
RETURN_TRUSTED_TYPE: false,
});
const removedItems = [...sanitizer.removed];
const sanitizedLength = sanitizedHtml.length;

if (removedItems.some(isDangerousRemoval) || hasDangerousMarkup(sanitizedHtml)) {
return {
kind: 'rejected',
originalLength,
sanitizedLength,
removedCount: removedItems.length,
rejectionReason: 'removed-dangerous-content',
};
}

if (sanitizedHtml.trim().length === 0) {
return {
kind: 'rejected',
originalLength,
sanitizedLength,
removedCount: removedItems.length,
rejectionReason: 'empty-after-sanitize',
};
}

return {
kind: 'accepted',
originalLength,
sanitizedHtml,
sanitizedLength,
removedCount: removedItems.length,
};
}

// Locate an ad slot element by id, tolerating funky selectors provided by tag managers.
export function findSlot(id: string): HTMLElement | null {
const nid = normalizeId(id);
Expand Down Expand Up @@ -85,7 +290,7 @@ export function renderAllAdUnits(): void {

type IframeOptions = { name?: string; title?: string; width?: number; height?: number };

// Construct a sandboxed iframe sized for the ad so we can render arbitrary HTML.
// Construct a sandboxed iframe sized for sanitized, non-executable creative HTML.
export function createAdIframe(
container: HTMLElement,
opts: IframeOptions = {}
Expand All @@ -101,16 +306,14 @@ export function createAdIframe(
iframe.setAttribute('aria-label', 'Advertisement');
// Sandbox permissions for creatives
try {
iframe.sandbox.add(
'allow-forms',
'allow-popups',
'allow-popups-to-escape-sandbox',
'allow-same-origin',
'allow-scripts',
'allow-top-navigation-by-user-activation'
);
if (iframe.sandbox && typeof iframe.sandbox.add === 'function') {
iframe.sandbox.add(...CREATIVE_SANDBOX_TOKENS);
} else {
iframe.setAttribute('sandbox', CREATIVE_SANDBOX_TOKENS.join(' '));
}
} catch (err) {
log.debug('createAdIframe: sandbox add failed', err);
iframe.setAttribute('sandbox', CREATIVE_SANDBOX_TOKENS.join(' '));
}
// Sizing + style
const w = Math.max(0, Number(opts.width ?? 0) | 0);
Expand All @@ -129,10 +332,10 @@ export function createAdIframe(
return iframe;
}

// Build a complete HTML document for a creative, suitable for use with iframe.srcdoc
// Build a complete HTML document for a sanitized creative fragment, suitable for iframe.srcdoc.
export function buildCreativeDocument(creativeHtml: string): string {
return IFRAME_TEMPLATE.replace('%NORMALIZE_CSS%', NORMALIZE_CSS).replace(
return IFRAME_TEMPLATE.replace('%NORMALIZE_CSS%', () => NORMALIZE_CSS).replace(
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 praise — Good catch on String.replace $-sequence injection.

Using function callbacks prevents $&, $$, $', etc. from being interpreted as replacement patterns. Well-tested with the dollar sequences test case.

'%CREATIVE_HTML%',
creativeHtml
() => creativeHtml
);
}
Loading