-
Notifications
You must be signed in to change notification settings - Fork 8
Harden inline creative rendering #459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| 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', | ||
| ]); | ||
| const DANGEROUS_URI_VALUE_PATTERN = /^\s*(?:javascript:|vbscript:|data\s*:\s*text\/html\b)/i; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 wrench — Only blocks
Ad creatives have no legitimate need for 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', | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ refactor — Sandbox policy is internally inconsistent.
Suggestion: Remove |
||
| '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)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 wrench — Any removed attribute in Compare with 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 { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ refactor — This re-parses DOMPurify's output for the same patterns DOMPurify should have already handled. It's a valid safety net — especially for CSS 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); | ||
|
|
@@ -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 = {} | ||
|
|
@@ -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); | ||
|
|
@@ -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( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 praise — Good catch on Using function callbacks prevents |
||
| '%CREATIVE_HTML%', | ||
| creativeHtml | ||
| () => creativeHtml | ||
| ); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏ nitpick —
srcdocis HTML content, not a URI.srcdocdoesn't belong inURI_ATTRIBUTE_NAMES. DOMPurify already strips it (only valid on<iframe>, which is inDANGEROUS_TAG_NAMES). Having it here is semantically wrong and could cause false positives ifisDangerousRemovalis refined.