Uh oh!
There was an error while loading. Please reload this page.
fix(web): honor _meta.ui.domain with a dedicated app origin - #2100
Conversation
a661794 to
6568dffCompareThere was a problem hiding this comment.
Pull request overview
Adds dedicated loopback origins for MCP Apps declaring _meta.ui.domain, while retaining opaque-origin fallback behavior.
Changes:
- Adds app-origin hosting, authenticated publishing, and sandbox navigation.
- Wires both App render paths and adds integration/smoke coverage.
- Adds showcase configuration and documentation.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
test-servers/src/test-server-fixtures.ts | Adds optional domain metadata to the demo resource. |
test-servers/src/preset-registry.ts | Passes domain preset parameters. |
test-servers/configs/mcp-app-domain-http.json | Adds a dedicated-origin showcase server. |
scripts/smoke-web-app.mjs | Tests dedicated-origin rendering end to end. |
README.md | Documents the showcase and port forwarding. |
core/mcp/remote/node/server.ts | Adds the authenticated document-publishing endpoint. |
clients/web/static/sandbox_proxy.html | Supports dedicated-origin iframe navigation. |
clients/web/src/test/integration/server/web-server-config.test.ts | Tests app-origin port configuration. |
clients/web/src/test/integration/server/server-token-injection.test.ts | Updates server test configuration. |
clients/web/src/test/integration/server/server-auto-open.test.ts | Updates auto-open test configuration. |
clients/web/src/test/integration/server/app-origin-controller.test.ts | Tests listener behavior and failure handling. |
clients/web/src/test/integration/mcp/remote/app-document-route.test.ts | Tests the publishing API route. |
clients/web/src/test/core/react/useServers.test.tsx | Corrects metadata fixture typing. |
clients/web/src/lib/publishAppDocument.ts | Adds the browser publishing client. |
clients/web/src/lib/publishAppDocument.test.ts | Tests publishing and fallback behavior. |
clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts | Selects dedicated or opaque rendering. |
clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts | Tests domain metadata behavior. |
clients/web/src/App.tsx | Wires publishing into both bridge factories. |
clients/web/server/web-server-config.ts | Adds app-origin port configuration. |
clients/web/server/vite-hono-plugin.ts | Starts the listener in development. |
clients/web/server/server.ts | Starts the listener in production. |
clients/web/server/sandbox-controller.ts | Shares frame-ancestor policy construction. |
clients/web/server/app-origin-controller.ts | Implements dedicated document hosting. |
clients/web/README.md | Documents the dedicated-origin contract. |
AGENTS.md | Records the new server architecture. |
Suppressed comments (1)
clients/web/src/lib/publishAppDocument.test.ts:97
- Avoid the second unjustified double cast here as well. A real response with invalid JSON provides the intended rejection without bypassing
Response's type.
const fetchFn = vi.fn<typeof fetch>().mockResolvedValue({
ok: true,
status: 200,
json: async () => {
throw new Error("not json");
},
} as unknown as Response);
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Copilot review round 1 on #2100. - POST /api/app-document: `null` and `[1,2]` are valid JSON, so the destructure that followed threw OUTSIDE the parse try/catch and turned a malformed body into a 500 instead of the documented 400. - app-origin controller: the error handler cleared `server` but not `origin`, and `publish` gates on `origin` — so an error after a successful listen kept minting URLs on a dead port and the browser never took the srcdoc fallback. Clear both, and drop the now-unfetchable documents. - sandbox proxy: drop a stale `srcdoc` before navigating to a dedicated origin (srcdoc wins over src), and restore the opaque-origin posture — strip `allow-same-origin`, reset `innerOrigin` — before assigning `srcdoc` on the fallback path. Not reachable today (each render remounts the iframe), but the cost of being wrong about that later is an isolation boundary. - publishAppDocument tests: replace two `as unknown as Response` casts with real `Response`s. - docs: `frame-ancestors` admits the proxy AND the Inspector page (every ancestor is checked); the shared helper falls back to loopback, not `'none'`; the showcase origin is `127.0.0.1`, the default bind address. - tests: null/array body, post-listen error, and both factories receiving a working `publishAppDocument`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall
commented
Aug 24, 2026
Copilot review round 1 — all 9 comments addressed (+ the suppressed one)Pushed as af3910f. Mirrored here because inline replies get hidden once the threads go outdated. Real defects — fixed
Both have new tests. The controller one mocks Correct-but-unreachable — taken as hardeningComments 1 and 2 ( Taken anyway, with comments stating the reachability so nobody later reads them as evidence the path is live. The Documentation — three claims that were wrong
Test coverageComment 9 is fair: the smoke only drives the Apps-screen factory, so the elicitation publisher could be deleted with everything still green. Suppressed commentAlso addressed — |
Copilot review round 1 on #2100. - POST /api/app-document: `null` and `[1,2]` are valid JSON, so the destructure that followed threw OUTSIDE the parse try/catch and turned a malformed body into a 500 instead of the documented 400. - app-origin controller: the error handler cleared `server` but not `origin`, and `publish` gates on `origin` — so an error after a successful listen kept minting URLs on a dead port and the browser never took the srcdoc fallback. Clear both, and drop the now-unfetchable documents. - sandbox proxy: drop a stale `srcdoc` before navigating to a dedicated origin (srcdoc wins over src), and restore the opaque-origin posture — strip `allow-same-origin`, reset `innerOrigin` — before assigning `srcdoc` on the fallback path. Not reachable today (each render remounts the iframe), but the cost of being wrong about that later is an isolation boundary. - publishAppDocument tests: replace two `as unknown as Response` casts with real `Response`s. - docs: `frame-ancestors` admits the proxy AND the Inspector page (every ancestor is checked); the shared helper falls back to loopback, not `'none'`; the showcase origin is `127.0.0.1`, the default bind address. - tests: null/array body, post-listen error, and both factories receiving a working `publishAppDocument`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
af3910f to
4af54e7CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
clients/web/static/sandbox_proxy.html:266
- Assigning
srchas no browser-side failure fallback. If the listener bound on the backend but this URL is unreachable from the browser (for example, port 6276 was not forwarded, or the fixed port fell back to an unforwarded dynamic port), the publish POST succeeds, this branch is selected, and the iframe stays on an error page instead of rendering the suppliedhtmlviasrcdoc. Add a reachability/readiness failure path that revokesallow-same-origin, resetsinnerOrigin, and loads the preserved HTML so the documented fallback contract is actually met.
inner.removeAttribute("srcdoc");
innerOrigin = srcUrl.origin;
inner.src = srcUrl.href;
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Copilot review round 2 on #2100, plus a collision the review did not find. The default app-origin port was 6276 — which is already RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, the CLI/TUI loopback OAuth callback. That port is fixed precisely so an OAuth app can pre-register http://127.0.0.1:6276/oauth/callback, which makes it the one listener here that cannot move when something takes it first. A running --web would hold it and break a later --cli/--tui OAuth flow at its registered redirect URI, and docs/mcp-app-review.md has those two running side by side. This listener has a documented dynamic fallback, so this listener yields: the default is now 6278 (6277 skipped — v1's retired proxy port, which the migration guide tells people to stop forwarding). A test asserts the two defaults stay distinct. Review comments: - POST /api/app-document now applies hono's bodyLimit BEFORE parsing. The 8MiB check ran after c.req.json() had buffered and parsed the whole request, and bounded only `html` — so bulk in `csp`, or in keys the route never reads, was fully materialized before the 413. - `csp` is validated as an HTTP header value and length-bounded. The controller hands it to res.writeHead() verbatim, where Node throws ERR_INVALID_CHAR synchronously — inside the handler for a later, unrelated request, outside this route's error handling. - The app-origin port now propagates everywhere the other two do: the CLI --print-handoff port-forward command (and its test), the Dockerfile's ENV and EXPOSE, and the remote-review SSH recipe in docs/mcp-app-review.md. Without it an app declaring _meta.ui.domain is unreachable through every documented remote workflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall
commented
Aug 24, 2026
Copilot review round 2 — all 3 addressed, plus a port collision the review surfaced indirectlyPushed as f3d20ce (
|
| # | Fix |
|---|---|
| 1 | bodyLimitbefore the parse (24MiB — headroom for JSON escaping so it can't reject a document the 8M-char check accepts). The old check ran after c.req.json() had buffered and parsed everything, and bounded only html. csp separately bounded at 8KiB, since that is what is retained per document. |
| 3 | csp validated as an HTTP header value. Worse than it reads: res.writeHead() throws ERR_INVALID_CHARsynchronously, on a later unrelated GET of the document URL — a 200 publish becomes a thrown listener, nowhere near this route. Rejected with 400, not sanitized. |
| 2 | Port propagated to the CLI --print-handoff command + its test, the DockerfileENV/EXPOSE, and the SSH recipe in docs/mcp-app-review.md. The root README already covered Docker publish and the remap caveat. |
Suppressed comment — declined, with the residual documented
The suggestion is a browser-side reachability fallback: if the publish succeeds but the URL is unreachable (an unforwarded port), revoke allow-same-origin, reset innerOrigin, and render the preserved html instead.
I looked for a way to implement it honestly and there isn't one at this layer. The proxy cannot observe the outcome of a cross-origin navigation — iframe.onerror does not fire for HTTP error responses, onload fires for an error page too, and the document is cross-origin so nothing about it is readable. The only detectors available are a no-cors probe (proves TCP reachability, not that this fetch will succeed, and costs a round trip on every domain-declaring app) or a load timeout (racing an app that is merely slow, and silently downgrading it to an opaque origin — the exact outcome the feature exists to avoid, arrived at by guessing).
What I did instead is remove the realistic cause and make the residual legible: comment 2's propagation means every documented remote workflow now forwards the port, and the docs/mcp-app-review.md bullet states plainly that the backend cannot detect a missing forward, because publishing succeeded on its side. Happy to revisit if a reliable signal turns up.
Unrelated: a pre-existing flake I hit
AppRenderer.test.tsx > pushes a live theme flip … failed twice consecutively in npm run ci (~1021ms, 0 calls) while passing every time that file runs alone. Pre-existing — the branch does not touch that file, and the test already carries a comment about failing this way once. That signature is a too-tight budget, not a broken observer, so its waitFor now gets 5s. Fixed here rather than left behind, since the tests this PR adds are part of the load that tips it.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Copilot review round 3 on #2100. - MCP_APP_ORIGIN_PORT could equal CLIENT_PORT or MCP_SANDBOX_PORT. The app-origin listener starts BEFORE the web server binds, so that is not a race it loses — it wins the port and the Inspector dies with EADDRINUSE. `CLIENT_PORT=6278` was a valid config before this feature existed. It now resolves to a dynamic port with a warning: of the three listeners this is the only one that can move, and refusing to boot would cost the user the whole Inspector over a feature they may not be using. - The proxy's same-origin refusal covered only its own origin. An app served from the INSPECTOR's origin is no safer: same-origin with the top document means direct access to window.top, its DOM and its storage, and the cross-origin frame in between buys nothing. Both trusted origins are now refused before allow-same-origin is granted. - Replaced the `as unknown as number` on `server.address()` with a real narrowing of the string/null members of its return type. - MCP_APP_ORIGIN_PORT added to the v1→v2 migration guide's env table, its Docker recipe, and a troubleshooting entry for the symptom this actually produces (the app renders, but its own backend rejects `Origin: null`). - clients/web/README.md now states which failure is outside the fallback guarantee — a published document the browser cannot reach — and why there is no honest signal to fall back on. Docs and code no longer disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall
commented
Aug 24, 2026
Copilot review round 3 — 5 fixed, 1 declined with the contract correctedPushed as 1b65a96. Fixed
The migration-guide entry is worth calling out: the symptom is not "the Apps tab is blank". The app renders fine and its own backend rejects it, because the requests carry Declined — the unreachable-document fallback (re-raised from round 2)Re-raising it was right: the code and the documented contract genuinely disagreed. I fixed the disagreement, but by correcting the contract, not by adding the fallback — so this is a decline, stated as one. There is no honest signal at this layer. The navigation is cross-origin:
So the cause is removed and the residual made legible: every documented remote workflow now forwards the port (round 2 + the migration guide this round), and Happy to revisit if a reliable signal turns up. A guess dressed as a fallback is worse than a documented limit. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
README.md:480
- This fallback claim is too broad. If the backend publishes successfully but port 6278 is not exposed or forwarded, the proxy has already selected
src; the cross-origin navigation failure is not observable, so the frame stays blank rather than reverting tosrcdocor emitting this warning. This contradicts the limitation documented inclients/web/README.md:192-205. Distinguish publication failures (which fall back) from browser reachability failures (which do not).
**And `6278` if your app declares `_meta.ui.domain`.** That is the spec field a server uses to ask its host for a stable, dedicated origin — without one the app runs at an opaque origin and its requests carry `Origin: null`, which no CORS / OAuth-callback / API-key allowlist can admit. The Inspector answers the request with a real loopback origin on a third listener, `MCP_APP_ORIGIN_PORT` (default `6278`); apps that declare no `domain` never touch it. An app that declares one and can't reach it still renders — at an opaque origin, with a console warning. See [MCP App dedicated origins](./clients/web/README.md#mcp-app-dedicated-origins-metauidomain) for the host-specific contract and its isolation trade-offs.
docs/v1-to-v2-migration.md:371
- A missing Docker/SSH forward does not cause the opaque-origin fallback or the
_meta.ui.domainwarning: publication succeeds on the backend, but the browser cannot load the returned third-port URL, leaving this app's frame blank. Update the troubleshooting symptom and outcome so users can diagnose the actual failure.
**"One particular App renders, but its requests are rejected by its own backend."** That App declares `_meta.ui.domain` and is being served from the opaque-origin fallback, so its requests carry `Origin: null`. It needs the **third** port (`MCP_APP_ORIGIN_PORT`, `6278` by default) published or forwarded as well; the console carries a warning naming the field. See [MCP App dedicated origins](../clients/web/README.md#mcp-app-dedicated-origins-metauidomain).
Uh oh!
There was an error while loading. Please reload this page.
Copilot review round 4 on #2100. All three findings are the same error: clients/web/README.md documents the limitation correctly, and every port-forwarding blurb then said the opposite. An unpublished/unforwarded port is NOT the opaque-origin fallback case. The backend's listener binds fine inside the container, so publication succeeds and the browser is handed a URL it cannot reach — the frame stays blank, with no console warning, because a cross-origin navigation failure is not observable from the page. The fallback and its warning cover the failures the BACKEND can see: no listener, a port that never bound, an older backend, a network error. Corrected in the root README's Docker blurb and the migration guide's third-port recipe. The migration guide's troubleshooting entry had the wrong symptom for the same reason — a missing forward produces a blank frame, not an app whose requests are rejected — so it now leads with that and keeps the rejected-requests case beside it as the neighbouring symptom, since the two have opposite causes and opposite console output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall
commented
Aug 24, 2026
Copilot review round 4 — one finding, in three places, all fixedPushed as 4797223. All three (one inline + two suppressed, including one flagged "previously missed") are the same error, and it is mine: I wrote the limitation correctly into The distinction, now stated the same way everywhere:
Corrected in the root README's Docker blurb and the migration guide's third-port recipe. The troubleshooting entry needed more than rewording — it had the wrong symptom. "Renders, but its requests are rejected by its own backend" describes the fallback case, which is precisely the case a missing forward does not produce. It now leads with the blank frame, says there will be no console warning and why, and keeps the rejected-requests case beside it as the neighbouring symptom: opposite causes, opposite console output, and otherwise easy to confuse. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
clients/web/src/lib/publishAppDocument.ts:60
- This fetch has no deadline. If the backend or an intermediary accepts the request but never completes the response,
createAppBridgeFactoryremains blocked awaiting publication and never sendssandbox-resource-ready, so the promisedsrcdocfallback does not occur and the app stays loading indefinitely. Add a bounded abort/timeout and let the existing catch returnnull.
const res = await doFetch(`${base}/api/app-document`, {
method: "POST",
headers,
body: JSON.stringify(doc),
});
clients/web/server/app-origin-controller.ts:128
- This interface comment says the embedder list is “in practice” only the sandbox proxy, but
frame-ancestorsis checked against every ancestor andappDocumentEmbeddersdeliberately includes both the proxy and the Inspector page. Update the comment to preserve that security-critical requirement for future callers.
/**
* The origin(s) allowed to frame a published document — in practice the
* sandbox proxy's own origin, since the proxy is what embeds the inner
* iframe. Malformed entries are dropped and an empty result falls back to
* loopback, exactly as the sandbox proxy's own `frame-ancestors` does.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Copilot review round 5 on #2100. - The store bounded entry COUNT only, and the route admits 8Mi code units per document with a one-hour TTL — ~256Mi retainable on documents a server under test chooses the size of. Added a 64Mi retained-size budget evicted oldest-first alongside the count bound, with a running total kept in step with the map through a single `drop` seam so it cannot drift. - The isolation claim was wrong about cookies. A separate port makes a separate ORIGIN, so DOM access, localStorage, sessionStorage and IndexedDB are isolated — but cookies are keyed by host and path, ignoring port, so an app served here shares the 127.0.0.1 jar with the Inspector and every other loopback service. Not a hole in the Inspector's own auth (the API token is a header plus a window global and sessionStorage, none of them cookies) and not closable without a distinct host this listener cannot mint, so it is stated plainly in both the module header and clients/web/README.md rather than left as a claim that is false for one of the three surfaces it names. - Removed a shell-escaping artifact I left in the migration guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
cliffhall
commented
Aug 24, 2026
Copilot review round 5 — all 3 addressedPushed as b8dc39d. Retained-memory budgetRight that the count bound alone was not one: The running total goes through a single The cookie claim was wrongI named three surfaces as isolated by the separate port and cookies is not one of them — they are keyed by host and path, ignoring port. I did not take the suggested fix, because this listener cannot mint a distinct host: the Inspector owns no DNS, and a second loopback address is not portable and would contradict the bind-host policy in Worth bounding the impact, since "not isolated" reads worse than it is here: this is not a hole in the Inspector's own auth. The API token travels in the Escaping artifactFixed — two occurrences on that line, from a shell escape sequence that ended up inside a Python string. Swept the diff; that line was the only one. |
Copilot review round 1 on #2100. - POST /api/app-document: `null` and `[1,2]` are valid JSON, so the destructure that followed threw OUTSIDE the parse try/catch and turned a malformed body into a 500 instead of the documented 400. - app-origin controller: the error handler cleared `server` but not `origin`, and `publish` gates on `origin` — so an error after a successful listen kept minting URLs on a dead port and the browser never took the srcdoc fallback. Clear both, and drop the now-unfetchable documents. - sandbox proxy: drop a stale `srcdoc` before navigating to a dedicated origin (srcdoc wins over src), and restore the opaque-origin posture — strip `allow-same-origin`, reset `innerOrigin` — before assigning `srcdoc` on the fallback path. Not reachable today (each render remounts the iframe), but the cost of being wrong about that later is an isolation boundary. - publishAppDocument tests: replace two `as unknown as Response` casts with real `Response`s. - docs: `frame-ancestors` admits the proxy AND the Inspector page (every ancestor is checked); the shared helper falls back to loopback, not `'none'`; the showcase origin is `127.0.0.1`, the default bind address. - tests: null/array body, post-listen error, and both factories receiving a working `publishAppDocument`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2 on #2100, plus a collision the review did not find. The default app-origin port was 6276 — which is already RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, the CLI/TUI loopback OAuth callback. That port is fixed precisely so an OAuth app can pre-register http://127.0.0.1:6276/oauth/callback, which makes it the one listener here that cannot move when something takes it first. A running --web would hold it and break a later --cli/--tui OAuth flow at its registered redirect URI, and docs/mcp-app-review.md has those two running side by side. This listener has a documented dynamic fallback, so this listener yields: the default is now 6278 (6277 skipped — v1's retired proxy port, which the migration guide tells people to stop forwarding). A test asserts the two defaults stay distinct. Review comments: - POST /api/app-document now applies hono's bodyLimit BEFORE parsing. The 8MiB check ran after c.req.json() had buffered and parsed the whole request, and bounded only `html` — so bulk in `csp`, or in keys the route never reads, was fully materialized before the 413. - `csp` is validated as an HTTP header value and length-bounded. The controller hands it to res.writeHead() verbatim, where Node throws ERR_INVALID_CHAR synchronously — inside the handler for a later, unrelated request, outside this route's error handling. - The app-origin port now propagates everywhere the other two do: the CLI --print-handoff port-forward command (and its test), the Dockerfile's ENV and EXPOSE, and the remote-review SSH recipe in docs/mcp-app-review.md. Without it an app declaring _meta.ui.domain is unreachable through every documented remote workflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3 on #2100. - MCP_APP_ORIGIN_PORT could equal CLIENT_PORT or MCP_SANDBOX_PORT. The app-origin listener starts BEFORE the web server binds, so that is not a race it loses — it wins the port and the Inspector dies with EADDRINUSE. `CLIENT_PORT=6278` was a valid config before this feature existed. It now resolves to a dynamic port with a warning: of the three listeners this is the only one that can move, and refusing to boot would cost the user the whole Inspector over a feature they may not be using. - The proxy's same-origin refusal covered only its own origin. An app served from the INSPECTOR's origin is no safer: same-origin with the top document means direct access to window.top, its DOM and its storage, and the cross-origin frame in between buys nothing. Both trusted origins are now refused before allow-same-origin is granted. - Replaced the `as unknown as number` on `server.address()` with a real narrowing of the string/null members of its return type. - MCP_APP_ORIGIN_PORT added to the v1→v2 migration guide's env table, its Docker recipe, and a troubleshooting entry for the symptom this actually produces (the app renders, but its own backend rejects `Origin: null`). - clients/web/README.md now states which failure is outside the fallback guarantee — a published document the browser cannot reach — and why there is no honest signal to fall back on. Docs and code no longer disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4 on #2100. All three findings are the same error: clients/web/README.md documents the limitation correctly, and every port-forwarding blurb then said the opposite. An unpublished/unforwarded port is NOT the opaque-origin fallback case. The backend's listener binds fine inside the container, so publication succeeds and the browser is handed a URL it cannot reach — the frame stays blank, with no console warning, because a cross-origin navigation failure is not observable from the page. The fallback and its warning cover the failures the BACKEND can see: no listener, a port that never bound, an older backend, a network error. Corrected in the root README's Docker blurb and the migration guide's third-port recipe. The migration guide's troubleshooting entry had the wrong symptom for the same reason — a missing forward produces a blank frame, not an app whose requests are rejected — so it now leads with that and keeps the rejected-requests case beside it as the neighbouring symptom, since the two have opposite causes and opposite console output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5 on #2100. - The store bounded entry COUNT only, and the route admits 8Mi code units per document with a one-hour TTL — ~256Mi retainable on documents a server under test chooses the size of. Added a 64Mi retained-size budget evicted oldest-first alongside the count bound, with a running total kept in step with the map through a single `drop` seam so it cannot drift. - The isolation claim was wrong about cookies. A separate port makes a separate ORIGIN, so DOM access, localStorage, sessionStorage and IndexedDB are isolated — but cookies are keyed by host and path, ignoring port, so an app served here shares the 127.0.0.1 jar with the Inspector and every other loopback service. Not a hole in the Inspector's own auth (the API token is a header plus a window global and sessionStorage, none of them cookies) and not closable without a distinct host this listener cannot mint, so it is stated plainly in both the module header and clients/web/README.md rather than left as a claim that is false for one of the three surfaces it names. - Removed a shell-escaping artifact I left in the migration guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
b8dc39d to
d040c5fCompareAn MCP App is rendered by handing its HTML to the sandbox proxy as `srcdoc`, in a frame sandboxed without `allow-same-origin` — the #1565 isolation model. That gives the app document an opaque origin, so every request it makes carries `Origin: null` and no CORS / OAuth-callback / API-key allowlist can admit it. `_meta.ui.domain` is the spec field a server uses to ask its host for a stable, dedicated origin, and it was being dropped on the floor. The field is explicitly host-dependent and the Inspector owns no domain infrastructure, so it treats any non-empty value as a request rather than an address, and answers with a real loopback origin on its own listener (MCP_APP_ORIGIN_PORT, default 6276). One shared origin, path-keyed per document: that delivers what the field is for without minting a port or DNS name per app, and is therefore not a per-app isolation boundary. The inner frame is granted `allow-same-origin` on this path only, which is what makes the origin real. It is not a #1565 regression: the listener is on its own port, so the app stays cross-origin to both the proxy and the Inspector. The proxy refuses the grant if the URL's origin equals its own, and the grant is never reachable from the server-supplied `sandbox` string, which is still stripped. The per-app CSP is delivered as a real response header there rather than only as a <meta>. Every failure path falls back to the default srcdoc render with a console warning naming the field, rather than blanking the app. Also fixes a pre-existing `tsc -b` break on v2/main (useServers.test.tsx passed `metadata: []` where `RequestMetadata` is expected), which has had CI red since #2087 and blocks the pre-push gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
`frame-ancestors` is checked against every ancestor, not just the parent. A published app document is framed by the sandbox proxy, which is framed by the Inspector page — so admitting only the proxy's origin blocked the frame outright (it rendered as a chrome-error frame that never reached the bridge). `appDocumentEmbedders` now admits both. That failure was invisible to every unit test, so `smoke:web:app` grows a second phase: the same demo app declaring `_meta.ui.domain`, driven through the same connect → open → ready chain, then asserting the two facts that only hold on the dedicated path — the inner frame was navigated to a published document, and its own `location.origin` is a real http origin rather than "null". Both phases end at `data-app-status="ready"`, so nothing short of reading the frame's origin can tell them apart. Adds `test-servers/configs/mcp-app-domain-http.json` (and a `domain` param on the `mcp_app_demo_widget` preset) as the showcase server for both the smoke and hand testing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 1 on #2100. - POST /api/app-document: `null` and `[1,2]` are valid JSON, so the destructure that followed threw OUTSIDE the parse try/catch and turned a malformed body into a 500 instead of the documented 400. - app-origin controller: the error handler cleared `server` but not `origin`, and `publish` gates on `origin` — so an error after a successful listen kept minting URLs on a dead port and the browser never took the srcdoc fallback. Clear both, and drop the now-unfetchable documents. - sandbox proxy: drop a stale `srcdoc` before navigating to a dedicated origin (srcdoc wins over src), and restore the opaque-origin posture — strip `allow-same-origin`, reset `innerOrigin` — before assigning `srcdoc` on the fallback path. Not reachable today (each render remounts the iframe), but the cost of being wrong about that later is an isolation boundary. - publishAppDocument tests: replace two `as unknown as Response` casts with real `Response`s. - docs: `frame-ancestors` admits the proxy AND the Inspector page (every ancestor is checked); the shared helper falls back to loopback, not `'none'`; the showcase origin is `127.0.0.1`, the default bind address. - tests: null/array body, post-listen error, and both factories receiving a working `publishAppDocument`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2 on #2100, plus a collision the review did not find. The default app-origin port was 6276 — which is already RUNNER_OAUTH_CALLBACK_DEFAULT_PORT, the CLI/TUI loopback OAuth callback. That port is fixed precisely so an OAuth app can pre-register http://127.0.0.1:6276/oauth/callback, which makes it the one listener here that cannot move when something takes it first. A running --web would hold it and break a later --cli/--tui OAuth flow at its registered redirect URI, and docs/mcp-app-review.md has those two running side by side. This listener has a documented dynamic fallback, so this listener yields: the default is now 6278 (6277 skipped — v1's retired proxy port, which the migration guide tells people to stop forwarding). A test asserts the two defaults stay distinct. Review comments: - POST /api/app-document now applies hono's bodyLimit BEFORE parsing. The 8MiB check ran after c.req.json() had buffered and parsed the whole request, and bounded only `html` — so bulk in `csp`, or in keys the route never reads, was fully materialized before the 413. - `csp` is validated as an HTTP header value and length-bounded. The controller hands it to res.writeHead() verbatim, where Node throws ERR_INVALID_CHAR synchronously — inside the handler for a later, unrelated request, outside this route's error handling. - The app-origin port now propagates everywhere the other two do: the CLI --print-handoff port-forward command (and its test), the Dockerfile's ENV and EXPOSE, and the remote-review SSH recipe in docs/mcp-app-review.md. Without it an app declaring _meta.ui.domain is unreachable through every documented remote workflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Pre-existing flake, not introduced here — the file is untouched by this branch and the test already carries a comment about failing this way once. The default 1000ms was still too tight: it failed twice consecutively under `npm run ci` (~1021ms, 0 calls) while passing every time the file runs alone, which is a budget signature rather than a broken observer. A generous `waitFor` costs nothing on the passing path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3 on #2100. - MCP_APP_ORIGIN_PORT could equal CLIENT_PORT or MCP_SANDBOX_PORT. The app-origin listener starts BEFORE the web server binds, so that is not a race it loses — it wins the port and the Inspector dies with EADDRINUSE. `CLIENT_PORT=6278` was a valid config before this feature existed. It now resolves to a dynamic port with a warning: of the three listeners this is the only one that can move, and refusing to boot would cost the user the whole Inspector over a feature they may not be using. - The proxy's same-origin refusal covered only its own origin. An app served from the INSPECTOR's origin is no safer: same-origin with the top document means direct access to window.top, its DOM and its storage, and the cross-origin frame in between buys nothing. Both trusted origins are now refused before allow-same-origin is granted. - Replaced the `as unknown as number` on `server.address()` with a real narrowing of the string/null members of its return type. - MCP_APP_ORIGIN_PORT added to the v1→v2 migration guide's env table, its Docker recipe, and a troubleshooting entry for the symptom this actually produces (the app renders, but its own backend rejects `Origin: null`). - clients/web/README.md now states which failure is outside the fallback guarantee — a published document the browser cannot reach — and why there is no honest signal to fall back on. Docs and code no longer disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 4 on #2100. All three findings are the same error: clients/web/README.md documents the limitation correctly, and every port-forwarding blurb then said the opposite. An unpublished/unforwarded port is NOT the opaque-origin fallback case. The backend's listener binds fine inside the container, so publication succeeds and the browser is handed a URL it cannot reach — the frame stays blank, with no console warning, because a cross-origin navigation failure is not observable from the page. The fallback and its warning cover the failures the BACKEND can see: no listener, a port that never bound, an older backend, a network error. Corrected in the root README's Docker blurb and the migration guide's third-port recipe. The migration guide's troubleshooting entry had the wrong symptom for the same reason — a missing forward produces a blank frame, not an app whose requests are rejected — so it now leads with that and keeps the rejected-requests case beside it as the neighbouring symptom, since the two have opposite causes and opposite console output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 5 on #2100. - The store bounded entry COUNT only, and the route admits 8Mi code units per document with a one-hour TTL — ~256Mi retainable on documents a server under test chooses the size of. Added a 64Mi retained-size budget evicted oldest-first alongside the count bound, with a running total kept in step with the map through a single `drop` seam so it cannot drift. - The isolation claim was wrong about cookies. A separate port makes a separate ORIGIN, so DOM access, localStorage, sessionStorage and IndexedDB are isolated — but cookies are keyed by host and path, ignoring port, so an app served here shares the 127.0.0.1 jar with the Inspector and every other loopback service. Not a hole in the Inspector's own auth (the API token is a header plus a window global and sessionStorage, none of them cookies) and not closable without a distinct host this listener cannot mint, so it is stated plainly in both the module header and clients/web/README.md rather than left as a claim that is false for one of the three surfaces it names. - Removed a shell-escaping artifact I left in the migration guide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
d040c5f to
6011d4dCompareUh oh!
There was an error while loading. Please reload this page.
Closes#2056
Problem
An MCP App is rendered by handing its HTML to the sandbox proxy as
srcdoc, in an iframe sandboxed withoutallow-same-origin— the isolation model from #1565. That gives the app document an opaque origin, so every request it makes carriesOrigin: null. No CORS policy, OAuth callback, or API-key allowlist can admit that._meta.ui.domainis the spec field by which a server asks its host for a stable, dedicated origin precisely to solve this. The Inspector was dropping it on the floor:sendSandboxResourceReadynever saw it, and nothing consumed it.Measured on the two showcase servers below, same widget, same code path up to the render:
location.originmcp-app-http.json(nodomain)about:srcdocnullmcp-app-domain-http.json(domaindeclared)http://127.0.0.1:6278/app-document/<id>http://127.0.0.1:6278What this does
The spec is explicit that
domainis host-dependent — "the format and validation rules for this field are determined by each host. Servers MUST consult host-specific documentation" — and the Inspector owns no domain infrastructure; it cannot servemy-app.example.com. What it can provide is a real, stable HTTP origin on loopback. So it treats the field as a request, not an address:domainopts the resource in. The value itself is never parsed, matched, or reserved — declare whatever your production host expects.MCP_APP_ORIGIN_PORT(default6278, in the same627xfamily as web6274and sandbox6275so the three forward together), serves the wrapped document under an unguessable path. Not6276— that is the fixed CLI/TUI loopback OAuth callback, which OAuth apps pre-register and which therefore cannot move;6277is skipped as v1's retired proxy port.<meta>thesrcdocpath must rely on, since a header applies before any of the app's bytes are parsed — plus aframe-ancestorsrestricting who may frame it.allow-same-origin, which is what makes the origin real rather than opaque.Apps that declare no
domainare completely unaffected: samesrcdocrender, same opaque origin, and the listener is never touched.Why
allow-same-originhere is not a #1565 regressionThe #1565 rationale is that the app "cannot touch this proxy's DOM, so it cannot bypass its own CSP by executing in the parent's realm". That still holds, because the property doing the work is the origin, not the opacity: the listener is on its own port, so the app document is cross-origin to both the sandbox proxy and the Inspector, and same-origin policy blocks the reach either way.
Three things keep the grant narrow:
src, never from the server-suppliedsandboxstring, which is still unconditionally stripped ofallow-same-origin.http(s)URL is honored; a relative path, ajavascript:ordata:URL falls through to thesrcdocpath.The proxy also now targets its relayed messages at the frame's real origin instead of
"*", so a frame that navigated itself elsewhere stops receiving host messages. The opaque default keeps"*"("null"is not a validtargetOrigin).One shared origin, not one per app
Every domain-declaring app is served from the same port, keyed by path. That is a deliberate trade: it delivers the property the field exists for — a real, allowlistable origin — without minting a port or a DNS name per app. The consequence, documented in the web README and in the module header, is that this origin is not a per-app isolation boundary: two such apps share its
localStorage,sessionStorage, and cookies. They stay isolated from the sandbox proxy and from the Inspector.Every failure falls back rather than blanking the app
No listener, a port that never bound, an older backend with no
POST /api/app-document, a network error, a malformed body — each renders the app the default (opaque-origin) way and logs a console warning naming_meta.ui.domain.publishAppDocumentnever throws for the same reason: losing the real origin degrades what the app can reach, and the developer can see why; losing the app itself would be worse.Why the browser hands the bytes back
The app's HTML arrives over the MCP connection, which only the browser holds — the backend has no client of its own to read the
ui://resource with. So the one route to a real HTTP origin is a POST of the document the browser already wrapped, through the authenticated/api/*surface. The route is an option oncreateRemoteApprather than a route built there, because the listener is aclients/web/serverconcern;core/owns only the authenticated seam. Same shape as the existingsandboxUrloption.The
frame-ancestorstrap this hitThe first working build still failed:
frame-ancestorsis checked against every ancestor, not just the parent. A published document is framed by the sandbox proxy, which is framed by the Inspector page — so admitting only the proxy's origin blocked the frame outright, rendering achrome-error://frame that never reached the bridge.appDocumentEmbeddersadmits both, and the smoke below is what caught it.Tests
Every new file clears the ≥90 per-file gate on all four dimensions (
app-origin-controller.ts100/98.9/100/92.4,publishAppDocument.ts100 across the board,createAppBridgeFactory.ts100/100/100/97.2).app-origin-controller.test.ts(new, 33 cases) — the document is reachable at the URLpublishhands back; the CSP arrives as a header withframe-ancestors; ids are distinct and hex-32; unknown id, non-GET, and any other path 404; publish-before-start and publish-after-close returnnull; the cap evicts the oldest and the TTL expires;EADDRINUSEretries once on a dynamic port and warns loudly; a non-EADDRINUSElisten failure resolves rather than hanging (the backendsawait start()during boot); the retry latch is bounded.app-document-route.test.ts(new, 11 cases) — the publish round-trip, both 503 shapes (no publisher, publisher declines), 400 on every malformed body, 413 on an oversized document, and that the route is behind the auth token like every other/api/*.publishAppDocument.test.ts(new, 10 cases) — the POST shape, and that all five failure modes resolvenullrather than throwing.createAppBridgeFactory.test.ts— the domain path publishes the same wrapped document thesrcdocpath would render and passes its URL assrc; an empty / whitespace / non-string / absentdomainnever publishes; both no-publisher and null-result fall back with the warning.web-server-config.test.ts—appOriginPortresolution, and that it deliberately does not share the sandbox'sSERVER_PORTfallback (a separate listener must be pinnable on its own).smoke:web:appgrows a second phase againstmcp-app-domain-http.json, asserting the frame was navigated to a published document and that its ownlocation.originis a real http origin. Both phases end atdata-app-status="ready", so nothing short of reading the frame's origin can tell them apart — which is exactly why theframe-ancestorsbug above was invisible to the unit tests.Drive-by
clients/web/src/test/core/react/useServers.test.tsxpassedmetadata: []whereRequestMetadatais expected. That has hadtsc -b— and therefore CI — red onv2/mainsince #2087, and blocks the mandatory pre-push gate. Fixed here (metadata: {}) so this branch can be verified; happy to split it out if you'd rather land it on its own.Screenshots
The change is deliberately not visual — the widget must render identically on both paths, which is what these show. The measurable difference is the origin table at the top.
Verification
npm run cipasses.