feat(mcp-app): satisfy the spec origin requirement + wire per-resource permissions - #1604
Merged
Merged
Conversation
…e permissions This is the security-model change #1600 deferred to its own PR: give the inner app frame `allow-same-origin` so it satisfies the MCP Apps spec's origin requirement (apps.mdx:474-475 MUSTs `allow-scripts` AND `allow-same-origin`), and wire the spec's per-resource `permissions` end to end so they actually work. The inner frame now inherits the per-server proxy origin (`lvis-mcp-app://<hex(serverId)>`) — a real, NON-opaque origin — instead of an opaque one. Measured in a real Electron 43 <webview> (test/e2e/ui/mcp-app-permissions.spec.ts): camera, microphone, and geolocation all work when a resource declares them and are denied fail-closed when it does not; clipboardWrite is excluded (a script-initiated clipboard write is refused even when delegated). Before (opaque origin) only geolocation worked. Plumbing (host-computed, never app-supplied — same trust rule as the CSP): - `McpUiResourcePermissions` threaded through the read model (`PluginUiResourceDecl` / `McpUiResourceMeta` / `McpUiResourceRead`, both the external `resources/read` arm and the plugin loopback arm) + the manifest schema. - `shared/mcp-app-permissions.ts`: the single capability table. Main derives BOTH the inner-frame Permissions-Policy `allow` attribute and the Electron session grant from the declaration. A closed enum: a declaration can only SELECT. - The `allow` attribute rides a host-served meta tag in the host-owned proxy document (never the renderer-forwarded bridge), set on the frame by the relay preload; `createMcpAppProxySession` carries the declaration + computed allow. - Electron `setPermissionRequestHandler`/`setPermissionCheckHandler` on the card's per-server session, deny-by-default, keyed off the proxy token. Media-kind fix (measured necessity, NOT predicted by the prior analysis): for a same-origin inner frame the `allow` attribute does NOT gate camera vs microphone — a mic-only card could open the camera until the Electron handler started matching on `details.mediaTypes`. That kind check is the real per-feature gate for camera/microphone, proven by the `mic-only` e2e card. Also rewrites the load-bearing prose that asserted the opposite of the spec (preload, bridge-contract, csp, types, McpAppView, manifest schema — commit 9090c5e's reasoning), and fixes the html-preview-partition test's mock sessions that lacked the permission handlers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H1hqXuV9sfFEM7KAg8g5PE
🚨 Cross-Cutting Review Gate 위반이 PR은 민감 영역 클러스터 조건을 충족합니다. 감지 사유: 최근 14일 이내에 민감 영역을 건드리는 PR이 3개 이상 머지되었습니다 (rolling-window cluster). 민감 영역 (Sensitive Areas):
요구 사항: 이 PR은 머지 전에 Cross-Cutting Review Gate를 통과해야 합니다.
|
…ions Address the 3-lane cluster review of PR #1604 (origin flip + per-resource permissions): - FIX 1 (security): isElectronPermissionGranted now fail-CLOSES the collapsed Electron `media` permission when the requested kind is indeterminate. A kindless `media` ask/check no longer falls back to a coarse string match (which was fail-OPEN — a mic-only card could be granted `media` for a camera request that arrived with no mediaTypes). Geolocation (no kind) is unchanged. Rewrote the JSDoc and updated the unit tests to the new semantics. - FIX 2 (architect): proxy-session lifetime = card MOUNT lifetime, reclaimed by disposeMcpAppProxySession on unmount (#1600) as the authoritative path. Raised the FIFO cap (64 -> 4096) to a pure leak backstop far above any plausible count of simultaneously-mounted cards, so it can no longer silently FIFO-evict a still-live camera/mic/geo card's grant in a busy chat. Added tests: a live session survives 200 later mints; dispose frees it promptly. - FIX 3 (critic): rewrote the MIC_ONLY e2e comment that claimed Permissions Policy refuses the mic-only camera request. The split is enforced by the Electron media-kind request handler (details.mediaTypes), NOT the `allow` attribute — the exact claim the PR measured wrong. - FIX 4 (critic): added a lockstep test binding the schema's mcpUiResourcePermissions property keys to MCP_APP_PERMISSION_FEATURES, so adding a feature to one without the other fails a test instead of drifting. - FIX 5 (architect): documented at INNER_SANDBOX_ATTR that the origin flip and the permissions plumbing MUST be reverted together (reverting only the sandbox attr re-creates the #1600 unhonored-knob). Accepted set (camera/mic/geo), the origin flip value, and the arm-convergence plumbing are untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H1hqXuV9sfFEM7KAg8g5PE
🚨 Cross-Cutting Review Gate 위반이 PR은 민감 영역 클러스터 조건을 충족합니다. 감지 사유: 최근 14일 이내에 민감 영역을 건드리는 PR이 3개 이상 머지되었습니다 (rolling-window cluster). 민감 영역 (Sensitive Areas):
요구 사항: 이 PR은 머지 전에 Cross-Cutting Review Gate를 통과해야 합니다.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
This is the security-model change #1600 (MCP Apps) deliberately deferred to its own PR because it touches
preload/protocol: it gives the card's inner frameallow-same-originso it satisfies the MCP Apps spec's origin requirement, which is also what makes the spec's per-resourcepermissions(camera / microphone / geolocation / clipboardWrite) actually work.Spec (
node_modules/@modelcontextprotocol/ext-apps/.../specification/2026-01-26/apps.mdx:474-475) MUSTs: (1) Host and Sandbox have different origins, (2) the Sandbox hasallow-scriptsANDallow-same-origin. We already satisfied (1) structurally (rendererfile://vs a per-serverlvis-mcp-app://<hex(serverId)>custom scheme). We violated (2) — the inner frame wassandbox="allow-scripts"(opaque origin), which is exactly whypermissionscould not work. This PR setsINNER_SANDBOX_ATTR = "allow-scripts allow-same-origin"and wirespermissionsend to end, host-computed and never app-supplied (same trust rule as the CSP).Measured results — before vs after (real Electron
<webview>, production path)test/e2e/ui/mcp-app-permissions.spec.tsruns the real browser APIs inside a real inner frame and records per-feature outcomes. The prior analysis measured the before (opaque origin); this PR measured the after (Electron 43 / Chromium, Windows):allow-same-origin) declaredSecurityError: Invalid security originNotAllowedErrordeniedSecurityError: Invalid security originNotAllowedErrordeniedNotAllowedError: Write permission deniedSo the origin change unlocks camera + microphone (previously impossible on the opaque origin) and keeps geolocation working — 3 of 4 now work when declared, all denied fail-closed when not.
OPAQUE_ORIGIN:falseandsandbox="allow-scripts allow-same-origin"confirmed on every card.clipboardWrite stays excluded from the capability table and the manifest schema: measured, a script-initiated async clipboard write is refused even when
clipboard-writeis delegated (no transient user activation), so honoring it is impossible — declaring it fails manifest validation loudly rather than being a silent no-op.The e2e proves fail-closed at three layers:
absentcard (declares nothing) gets noallowattribute and every feature denied;revokedcard declares everything, then its proxy session is dropped: theallowattribute is present (Permissions-Policy passes) yet the feature is still denied, which isolates the Electron permission handler and proves it is installed and denies (Electron's default is GRANT);mic-onlycard declares microphone only and is granted its mic but denied the camera.Attack-surface argument — why
allow-same-originon a per-server proxy origin is safeThe inner
srcdocframe inherits the proxy's per-server origin, not the renderer'sfile://. The containment never rested on the opaque origin; it rests on invariants I re-verified against the merged code:standard:true, secure:true(src/main/mcp-app-protocol.ts:50-51); theprotocol.handlecallback 403s on an authority/token mismatch (src/main/mcp-app-protocol.ts:255-256); tokens arerandomUUID; injective hex encode (src/shared/mcp-app-partition.ts).contextIsolationis true, so the relay preload runs in an isolated world that same-origin DOM access cannot cross;nodeIntegrationInSubFramesis false, so the inner frame has no preload of its own; the preload's own top-frame guard (src/mcp-app-preload.ts:56,window.parent === window && protocol === "lvis-mcp-app:") refuses to relay from the inner frame. Isolation is force-set at attach for the detached window path (src/main/window-manager.ts:597-602) and set via the<webview webpreferences>attribute for the inline path (src/ui/renderer/components/McpAppView.tsx:589) — see the correction note below.<style>, plus the host-computedallowmeta).shouldBlockGlobalWebviewNavigationpermits onlylvis-mcp-app:/data:/about:(src/main/webview-navigation-policy.ts:24); the per-serverwebRequestgate opens only declared origins.shared/mcp-app-permissions.tsis a closed capability table; main derives both theallowattribute and the Electron grant from the resource declaration; the renderer/app supply neither. Theallowstring rides a host-served meta tag in the host-owned proxy document, not the renderer-forwarded bridge.The only genuine cost of
allow-same-originis intra-server storage sharing between two cards of the same server (they now share the per-server proxy origin'slocalStorage/IndexedDB) — which is exactly what the spec andbasic-hostintend (same app ⇒ same origin). Cross-server isolation is unchanged (distinct partition + distinct scheme authority).allow-formsis deliberately not added: the spec does not MUST it andform-actiondefaults to'none', so it would be inert; add it later only with evidence.Two things that did NOT match the prior analysis's prediction
allowattribute does NOT gate camera vs microphone for a same-origin frame. Amic-onlycard (allow="microphone") was able to open the camera viagetUserMedia({video})until the Electron handler started matching ondetails.mediaTypes. camera+microphone collapse into Electron's singlemediapermission, so the media-kind check ininstallDeclaredPermissionGateis the real per-feature chokepoint — not theallowattribute. This is covered by themic-onlye2e card and unit tests, and is the reason the handler threadsmediaKinds.will-attach-webviewhandler. The prior trace citedwindow-manager.ts:600as the isolation guarantee, butconfigureSideBrowserWebviewAttachignores thelvis-mcp-app:partition — the inline card'scontextIsolation=yescomes from the<webview webpreferences>attribute (McpAppView.tsx:589) plus Electron's defaultnodeIntegrationInSubFrames=false. The force-set atwindow-manager.ts:597-602applies to the detached window path. The isolated-world-preload guarantee holds for both, via different mechanisms.Gates
npx tsc --noEmit→ rc=0.bunx vitest run src/mcp src/ipc src/plugins src/main src/shared→ 235 files, 2762 passed, 0 failed.bunx playwright test test/e2e/ui/mcp-app-permissions.spec.ts→ 1 passed (real<webview>, per-feature outcomes recorded above).html-preview-partitiontest's mock sessions that lackedsetPermissionRequestHandler/setPermissionCheckHandler.Review
This is a security-model change in
preload/protocol, so per CLAUDE.md §Cross-Cutting Review Gate it needs its own 3-agent cluster review (architect + critic + security-reviewer) and thecluster-review-passedlabel before merge. I have NOT applied the label — that is the orchestrator's attested decision.Required cross-repo companions (BLOCKER for plugins declaring
permissions)This host PR re-adds
permissions+ themcpUiResourcePermissionsdefinition to the host-ownedschemas/plugin-manifest.schema.json(accepted set: camera / microphone / geolocation;clipboardWritedeliberately excluded). The SDK and marketplace schema mirrors were just synced WITHOUT it, so they are now out of lockstep with the host SOT. Before any plugin can declarepermissions, two companion PRs must land:permissions+mcpUiResourcePermissionsto the@lvis/plugin-sdkschema mirror. Without it, the SDK pre-commit AJV validation rejects any manifest declaringpermissions.Land these as a lockstep set (the host schema is the SOT; a plugin declaring
permissionsagainst an un-synced SDK/marketplace fails validation loudly, never silently). The orchestrator will prepare the SDK + marketplace companions; this note records the requirement so the host schema re-add is not mistaken for a complete, plugin-usable feature on its own.🤖 Generated with Claude Code
Cluster Review (CLAUDE.md §Cross-Cutting Review Gate)
3 lanes over the preload/protocol origin-model change. Verdict after fixes (
436dd84f): all findings resolved.allow=frame gains nothing, because the token-keyed, deny-by-default Electron handler (keyed off the host-minted proxy URL, not the frame'sallow) is the real gate, proven by therevoked+mic-onlye2e. Fixed MINOR-1: the collapsedmediapermission now fails CLOSED when the requested kind is indeterminate (was fail-open).MCP_APP_PERMISSION_FEATURES; clipboardWrite exclusion is clean; no over-gating. Fixed: the falsemic-onlyharness comment (claimed Permissions-Policy gates camera-vs-mic; it is the Electron media-kind handler) + added a schema↔SoT lockstep test.permissions/mcpUiResourcePermissionswhile the SDK + marketplace mirrors are (post-feat(mcp-app): complete the MCP Apps program — plugin ui:// serving, the full app→host surface, and ['app'] visibility made real #1600) synced without it. Companion SDK + marketplace schema PRs re-adding it are required before any plugin declarespermissionsand will be shipped in the same session immediately after this merges (the same flow as feat(mcp-app): complete the MCP Apps program — plugin ui:// serving, the full app→host surface, and ['app'] visibility made real #1600/refactor(mcp): rename _meta wire namespace xyz.lvis/ → lvisai/ (host, transitional dual-read) #1601 → SDK#219/marketplace#193). Also fixed the emergent LRU defect (the proxy session became a long-lived permission authority but was FIFO-capped at 64 → a live camera card could be silently revoked; cap raised to a leak-backstop 4096 with dispose-on-unmount as the authoritative reclaim) and a revert-coupling note (origin + permissions revert together).