-
Notifications
You must be signed in to change notification settings - Fork 60
fix: proper CSP handling in basic-host sandboxing (HTTP headers, worker-src, document.write) #234
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
Conversation
…Domains, permissions Security improvements: - CSP is now set via HTTP headers in serve.ts instead of meta tags (meta tag CSP can be tampered with by same-origin content) - CSP passed as query param to sandbox.html for header-based enforcement New CSP/permissions features (borrowed from PR #158): - frameDomains: control frame-src directive for nested iframes - baseUriDomains: control base-uri directive - permissions: camera, microphone, geolocation via iframe allow attribute WebGL fix: - Use document.write() instead of srcdoc for inner iframe content (srcdoc creates opaque origin that breaks WebGL canvas updates) - Add worker-src directive with blob: support (critical for WebGL apps like CesiumJS/Three.js that use workers for tile decoding, terrain processing, image processing) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@modelcontextprotocol/ext-apps
@modelcontextprotocol/server-basic-react
@modelcontextprotocol/server-basic-vanillajs
@modelcontextprotocol/server-budget-allocator
@modelcontextprotocol/server-cohort-heatmap
@modelcontextprotocol/server-customer-segmentation
@modelcontextprotocol/server-scenario-modeler
@modelcontextprotocol/server-system-monitor
@modelcontextprotocol/server-threejs
@modelcontextprotocol/server-wiki-explorer
commit: |
Keep this PR focused on CSP security fixes only. Permissions (camera, microphone, geolocation) will be handled in #158. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
55750c5 to
73b69e7
Compare
Add support for passing CSP configuration via URL query parameter (?csp=<json>) to the sandbox proxy. This enables proxy servers to set Content-Security-Policy via HTTP headers (tamper-proof) rather than relying on meta tags or postMessage. Changes: - AppFrame.tsx: Build sandbox URL with CSP query param before loading iframe - SandboxConfig.csp: Updated docs explaining query-param + postMessage fallback - using-a-proxy.md: Added CSP Query Parameter section with server-side example - Updated architecture diagram to show CSP flow through server The CSP is still sent via postMessage as a fallback for proxies that don't support the query parameter approach. See: modelcontextprotocol/ext-apps#234 Claude-Generated-By: Claude Code (cli/claude-opus-4-5=100%) Claude-Steers: 2 Claude-Permission-Prompts: 0 Claude-Escapes: 0
| // Images: same-origin + data/blob URIs + specified domains | ||
| `img-src 'self' data: blob: ${resourceDomains}`.trim(), | ||
| // Fonts: same-origin + data/blob URIs + specified domains | ||
| `font-src 'self' data: blob: ${resourceDomains}`.trim(), |
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.
@claude do I understand correctly that we have a risk of csp injection here?
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.
Pushed a fix, ptal @antonpk1
Validate CSP domain entries to reject characters that could: - Break out of CSP directives (semicolons, newlines) - Inject CSP keywords like 'unsafe-eval' (quotes) - Inject multiple sources in one entry (spaces) This prevents injection attacks where malicious domains could override the security policy.
| modifiedHtml = cspMetaTag + modifiedHtml; | ||
| } | ||
| // Use document.write instead of srcdoc for WebGL compatibility. | ||
| // srcdoc creates an opaque origin which prevents WebGL canvas updates |
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.
I'm confused by this bit. srcdoc iframes are not all opaque-origined. For example, navigate to https://example.com and run this in DevTools:
const iframe = document.createElement('iframe');
iframe.srcdoc = `<p id=log></p><script>log.textContent = self.origin</script>`;
document.body.append(iframe);By default, srcdoc iframes inherit their navigation initiator's origin (i.e., the origin of the document that set the srcdoc attribute), unless their sandbox attribute overrides this.
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.
Oops sorry good catch, will send update
Real reason: was to work around browser canvas tainting checks that seemed to treat about:srcdoc URLs as tainted, even when the iframe is same-origin with its parent. Will confirm and update the comment. in a followup
| severity: "high", | ||
| }, | ||
| { | ||
| target: "Element.prototype", |
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.
What dictates what's in this list? Is there documentation somewhere? APIs like setHTMLUnsafe() come to mind (maybe it's fine since it's already unsafe), but I was just curious how these were chosen.
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.
Oh this wasn't meant to be submitted, sorry :-(
Follow-up to PR #234. This file was accidentally included.
Update comment to accurately explain the canvas tainting issue rather than incorrectly claiming srcdoc creates an opaque origin. Follow-up to PR #234 per domfarolino's review feedback.
Follow-up to PR #234 per domfarolino's review feedback.
This PR tightens CSP handling in basic-host (which was setting very loose permissions so far).
Changes
frameDomains,baseUriDomains(borrowed from feat: enhance sandbox capability negotiation #158, cc/ @idosal @domfarolino) - Extend CSP configuration optionsworker-srcdirective - Needed for WebGL apps (CesiumJS, Three.js) that use Web Workers for tile decoding, terrain processing, etc.document.write()instead ofsrcdoc- Fixes WebGL rendering issues (srcdoc creates opaque origin that breaks canvas updates)(last two points fix the upcoming Map App example #235 )
Implementation
Note: basic-host is still not to be considered as a hardened production host.
🤖 Generated with Claude Code