Trac oEmbed: Apply the iframe's sandbox flags to the embed response - #876
Trac oEmbed: Apply the iframe's sandbox flags to the embed response#876obenland wants to merge 3 commits into
Conversation
The oEmbed payload wraps the embed document in an iframe carrying `sandbox="allow-scripts allow-top-navigation-by-user-activation"`. Those flags sit on the element rather than on the response, so they only take effect when the document is loaded through that particular iframe. Consumers that rebuild the iframe from the payload's `src`, and requests made to the endpoint directly, get the document without them. Send the same flags as a `Content-Security-Policy: sandbox` header so they travel with the response however it is loaded. They are identical to the iframe's, so the framed path is unchanged. Also build the injected height reporter with a text node. Passing the script to `DOMDocument::createElement()` as a value parses it for entity references, which mangles or drops a script containing `&`. The current script is unaffected; this keeps a later edit to it from breaking silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe iframe response now sends a sandbox Content Security Policy, skips reporting when opened directly, and creates the reporting script by inserting JavaScript as a text node. ChangesIframe response security
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The response adds sandbox confinement and adjusts iframe reporting behavior without any identified remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security headers and browser sandboxing behavior on a production-facing endpoint and should be validated by a human in a real browser/embed consumer flow.
Pull request overview
This PR updates the Trac oEmbed embed document response so the iframe sandbox restrictions also apply when the document is loaded outside the original oEmbed-provided <iframe> (e.g., consumers reconstruct the iframe from the src, or the embed endpoint is fetched directly).
Changes:
- Adds a
Content-Security-Policy: sandbox ...header matching the existing iframesandboxattribute flags. - Adjusts DOM injection of the height-reporter script to use a text node instead of
DOMDocument::createElement( 'script', $js ), avoiding entity parsing of the script content.
File summaries
| File | Description |
|---|---|
| api.wordpress.org/public_html/dotorg/trac/oembed/index.php | Adds CSP sandbox header to the embed HTML response and changes DOMDocument script injection to preserve script text as-authored. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The reporter posts to `window.parent`, which is the window itself when the embed document is loaded on its own. That delivers the message straight back to its own `message` listener, which posts again, so the script spins for as long as the page is open. Return early when there is no parent frame; there is nothing to report to. Framed embeds are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The endpoint re-serves markup it does not control, so the confinement has to sit on the response rather than on what the markup happens to contain. Record that alongside the header, since the reasoning is not visible from the flags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The oEmbed payload wraps the embed document in an iframe carrying
sandbox="allow-scripts allow-top-navigation-by-user-activation". Those flags sit on the element rather than on the response, so they only take effect when the document is loaded through that particular iframe. Consumers that rebuild the iframe from the payload'ssrc, and requests made to the endpoint directly, get the document without them.This sends the same flags as a
Content-Security-Policy: sandboxheader so they travel with the response however it is loaded. They are identical to the iframe's, so the framed path is unchanged.Also builds the injected height reporter with a text node. Passing the script to
DOMDocument::createElement()as a value parses it for entity references, which mangles or drops a script containing&. The current script is unaffected; this keeps a later edit to it from breaking silently.Finally, the reporter now returns early when the document is not framed. It posts to
window.parent, which is the window itself on a direct load, so the message comes straight back to its ownmessagelistener and the script spins for as long as the page is open. Measured in Chromium: an unframed load reached the probe's 25-message cap in under 1.5s, and reports 0 with the guard in place. A framed load posts twice —DOMContentLoadedandload— both before and after.Testing
phpcsagainstphpcs.xml.distreports nothing new.The sandbox behaviour itself is from the spec and was not exercised in a browser; the reporter change was.
🤖 Generated with Claude Code
Summary by CodeRabbit