Repository navigation
mcp apps: browser e2e for the view's real URL - #4667
chelojimenez wants to merge 1 commit into
Conversation
The properties that made views mountable by document.write are all browser-level, and jsdom models none of them — its document.open() only clears child nodes, adopting no URL and enforcing no policy. Until now they were verified by a scratch Playwright script; this makes them a checked-in gate. Six cases, against a dev server (the harness route only exists under import.meta.env.DEV): - the view's own location.href is the sandbox proxy's URL, on the sandbox origin (127.0.0.1) and not the app's (localhost), with no srcdoc attribute; - a third-party fixture records what the browser actually sent it. Referrer-Policy: strict-origin-when-cross-origin trims the path, so what arrives is the view's ORIGIN — which is the granularity a referrer allowlist matches, and the change from carrying nothing at all. Origin-based allowlists (WebKit strips Referer cross-origin) get the same value; - the injected CSP still binds after the write: an undeclared host is refused while the declared fixture is not; - a widget calling location.reload() restarts instead of blanking; - the srcdoc mount path renders and reports it has no URL, so the origin chip never offers a value no provider accepts; - doctype-less HTML renders in quirks mode, with a doctype control — the one behaviour change from the srcdoc era, pinned as the deliberate claude.ai-fidelity choice it is. The harness mounts SandboxedIframe directly rather than driving the chat renderer: every property above lives in the sandbox layer, and the renderer's lifecycle/applied plumbing and the origin chip are already covered by component tests that can drive them directly. `test:e2e:mcp-apps` runs `predev` first — `dev:app:default` assumes the generated server bundles exist, so in a fresh clone the dev server dies on a missing codex-appserver bundle. The default (production-server) suite ignores this spec, since /__e2e/* is not registered there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_96e70ced-4859-4a12-8740-62adc595ab59) |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review. WalkthroughAdds a DEV-only MCP App view harness with configurable mount mode and doctype handling. Adds a fixture HTTP server and Playwright tests for sandbox URLs, request headers, CSP enforcement, reload behavior, srcdoc fallback, and quirks mode. Adds a dedicated Playwright configuration and npm script for running the suite. Merge Risk: ⚪ Minimal · up to This adds browser coverage for MCP App sandbox mounting behavior without changing production runtime behavior. The dedicated development test path and reported passing checks leave no actionable merge-blocking risk. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Internal previewPreview URL: https://mcp-inspector-pr-4667.up.railway.app |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/playwright.mcp-apps.config.ts">
<violation number="1" location="mcpjam-inspector/playwright.mcp-apps.config.ts:41">
P2: The webServer readiness check only polls the Vite client, which returns index.html for any path as soon as it is listening. The harness actually depends on the Hono server at 6475 (the /api proxy that serves /api/apps/mcp-apps/sandbox-proxy), started concurrently by dev:app:default, so tests can begin before the server is ready and fail on proxied requests. Poll the server or the proxied proxy path for readiness, e.g. a second webServer entry keyed on http://localhost:6475 or a probe of the sandbox-proxy route, so startup is confirmed before tests run.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| webServer: [ | ||
| { | ||
| command: "npm run dev:app:default", | ||
| url: "http://localhost:5375/__e2e/mcp-app-view", |
There was a problem hiding this comment.
P2: The webServer readiness check only polls the Vite client, which returns index.html for any path as soon as it is listening. The harness actually depends on the Hono server at 6475 (the /api proxy that serves /api/apps/mcp-apps/sandbox-proxy), started concurrently by dev:app:default, so tests can begin before the server is ready and fail on proxied requests. Poll the server or the proxied proxy path for readiness, e.g. a second webServer entry keyed on http://localhost:6475 or a probe of the sandbox-proxy route, so startup is confirmed before tests run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/playwright.mcp-apps.config.ts, line 41:
<comment>The webServer readiness check only polls the Vite client, which returns index.html for any path as soon as it is listening. The harness actually depends on the Hono server at 6475 (the /api proxy that serves /api/apps/mcp-apps/sandbox-proxy), started concurrently by dev:app:default, so tests can begin before the server is ready and fail on proxied requests. Poll the server or the proxied proxy path for readiness, e.g. a second webServer entry keyed on http://localhost:6475 or a probe of the sandbox-proxy route, so startup is confirmed before tests run.</comment>
<file context>
@@ -0,0 +1,57 @@
+ webServer: [
+ {
+ command: "npm run dev:app:default",
+ url: "http://localhost:5375/__e2e/mcp-app-view",
+ cwd: packageRoot,
+ reuseExistingServer: !process.env.CI,
</file context>
This branch was successfully deployed
1 participant
Why
The properties that justify mounting views by
document.write(#4661) are all browser-level, and jsdom models none of them — itsdocument.open()only clears child nodes, adopting no URL and enforcing no policy. Up to now they were verified by a throwaway Playwright script I ran by hand. This makes them a checked-in gate.Completes Phase 1. Remaining in the plan: host-origin pinning (1B), then the
_meta.ui.domainwork.The six cases
location.hrefis the proxy URL, on the sandbox origin (127.0.0.1) and not the app's (localhost), with nosrcdocattributelocation.reload()inside the view restarts it instead of blankingBackCompat, with a doctype control →CSS1CompatOne finding worth flagging. My first version of the referrer test asserted the fixture would see the view's full URL. It doesn't: the app sends
Referrer-Policy: strict-origin-when-cross-origin, so a cross-origin request carries the origin only —http://127.0.0.1:5375/. That's correct and privacy-preserving, it's the granularity a referrer allowlist matches anyway (http://127.0.0.1:*/*), and the thing that changed is that the value is a real origin rather than absent. The test now pins that, plus the two regressions that would matter: it must not be empty, and must not be the app's origin — otherwise the origin we tell developers to allowlist would be the wrong one.Scope choice
The harness mounts
SandboxedIframedirectly rather than driving the chat renderer against a live MCP connection. Every property above lives in the sandbox layer, and the renderer's lifecycle/appliedplumbing and the Sandbox Stack origin chip already have component tests that drive them directly. Building a fixture MCP server plus a connection flow to re-assert them through the UI would be a lot of machinery for coverage that exists — the harness header says so explicitly.Infrastructure notes for the reviewer
test:e2e:mcp-appsrunspredevfirst.dev:app:defaultassumes the generated server bundles already exist; in this fresh worktree the dev server died on a missingcodex-appserver-bridge.bundled.jsand every test timed out on a page that rendered but never mounted.playwright.oauth-debugger.config.tshas the same latent dependency — it survives because a dev has usually runnpm run devfirst. Worth considering the same change there separately.playwright.config.tstestIgnore), because it boots a production server where/__e2e/*is not registered. Verified:--liston the default config matches 0 of these, and the oauth-debugger config still matches only its own.mountModeis a harness query parameter, not a second dev server withVITE_MCPJAM_VIEW_MOUNT=srcdoc— one build, one browser, both mount paths.viewFrame()deliberately does not match on URL. Once mounted, the view reports the same URL as its parent proxy — the property under test — so a URL match would return the proxy and pass even if the view never mounted. It walks the frame tree instead. (My first attempt reached throughouter.contentDocumentfrom the page, which is null cross-origin; that's what the first red run was.)client/src/router.tsx: mylazyblock matches the formatting of the identicalOAuthDebuggerE2EHarnessblock directly above it. Prettier v3 would reformat both; I left the neighbour consistency rather than making two adjacent identical imports look different. Say the word if you'd rather I match prettier.Verification
npm run test:e2e:mcp-apps— 6 passed (~17s).npm run typecheck:clientclean;route-elements-coveragestill green with the new route; new files are prettier-clean.🤖 Generated with Claude Code
Note
Low Risk
Adds dev-only routes, e2e fixtures, and npm/Playwright config only; no production sandbox or proxy behavior changes in this diff.
Overview
Adds a checked-in browser e2e suite for MCP App view mounting behaviors that jsdom cannot model (proxy URL adoption, cross-origin
Referer/Origin, CSP binding afterdocument.write, widgetlocation.reload(), srcdoc fallback, and quirks mode without a doctype).A dev-only
/__e2e/mcp-app-viewharness mountsSandboxedIframedirectly (query params formount,doctype, fixture origin) and recordsmcpjam:view-mode/ CSP violation messages onwindow.__mcpAppViewE2E. Playwright drives six cases against a local fixture HTTP server that logs incomingRefererandOrigin.CI wiring: new
test:e2e:mcp-apps(runspredevthenplaywright.mcp-apps.config.tson a dedicated dev port), and the defaultplaywright.config.tsnow ignoresmcp-app-view-origin.spec.tslike the oauth-debugger suite.Reviewed by Cursor Bugbot for commit 2878816. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds browser e2e coverage for MCP App views mounted by
document.write, pinning the real-URL behavior that jsdom can't model. Six cases verify the view runs at the sandbox proxy's URL on127.0.0.1(never the app'slocalhost), a third-party fixture sees the sandbox origin as referrer (trimmed bystrict-origin-when-cross-origin, which is the granularity a referrer allowlist matches anyway), the injected CSP still binds after the write,location.reload()restarts a widget instead of blanking it, thesrcdocfallback still renders and reports no URL, and doctype-less HTML lands in quirks mode (matching claude.ai).Infrastructure
playwright.mcp-apps.config.tsruns a dev server, since the/__e2e/mcp-app-viewharness route exists only behindimport.meta.env.DEV; the default production suite ignores this spec.mountModeis a harness query parameter, so one browser build covers both mount paths against one server.SandboxedIframedirectly rather than driving the chat renderer; rendererappliedplumbing and the origin chip already have component tests.test:e2e:mcp-appsrunspredevfirst, because the dev server dies in a fresh worktree on missing generated server bundles.Written for commit 2878816. Summary will update on new commits.