Conversation
- Enhanced `ToolCallWithResponse` to transform tool results and pass tool input to `McpAppRenderer`. - Updated `McpAppRenderer` to accept and handle tool input and result props. - Added utility functions to create notifications for tool input and result. - Modified mock app data to display tool input and result in the UI. - Updated documentation to reflect the implemented notifications.
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
I wanted to finish this, but I'm fried, let's sync tomorrow?
| if request.uri().path() == "/status" | ||
| || request.uri().path() == "/mcp-ui-proxy" | ||
| || request.uri().path() == "/mcp-app-proxy" | ||
| { |
There was a problem hiding this comment.
is three the time we introduce an allow list?
There was a problem hiding this comment.
/mcp-ui-proxy will eventually go away, and I don't have strong opinions here. Maybe if we start opening up more routes?
| @@ -0,0 +1,53 @@ | |||
| use axum::{ | |||
There was a problem hiding this comment.
this is very similar to mcp_ui_proxy.rs
I would suggest to just change the route to
/proxy/mcp_app and /proxy/mcp_ui (or /mcp-proxy/ui etc)
and then just serve the relvant HMTL based on the parameter
There was a problem hiding this comment.
Whoops... Yesterday, I dug pretty deep into the iframe approach and realized I messed some things up. I think I got it right now. I just pushed this new commit. Sorry for the churn.
Before: The outer sandbox had a static, permissive CSP that allowed any HTTPS domain, relying on JavaScript to build and inject a restrictive CSP into the inner iframe.
After: The outer sandbox CSP is now dynamically built in Rust based on the app's declared domains, making it the single source of truth — the inner iframe needs no CSP since it can't exceed the outer's restrictions.
Based on these changes, mcp_app_proxy is now unique from mcp_ui_proxy — and implemented closer to spec.
Do we consolidate code into a single route that handles both MCP-UI and MCP Apps (with CSP construction) HTML, or keep them as separate files? Thinking ahead to the MCP-UI deprecation, can we keep them separate?
| console.error('Error fetching MCP App Proxy URL:', error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
we should find a way to do this through the API instead of hand composing this, adding the secret, checking the secret by hand. for one thing we could extend the auto checker of the secret to also look in, say __secret
There was a problem hiding this comment.
Do you think we should address that as a fast follow-up or incorporate it into this PR?
…erer * origin/main: (26 commits) Don't persist ephemeral extensions when resuming sessions (#5974) chore(deps): bump mdast-util-to-hast from 13.2.0 to 13.2.1 in /ui/desktop (#5939) chore(deps): bump node-forge from 1.3.1 to 1.3.2 in /documentation (#5898) Add Scorecard supply-chain security workflow (#5810) Don't show subagent tool when we're a subagent (#6125) Fix keyboard shortcut conflict for Focus Goose Window (#5809) feat(goose-cli): add feature to disable update (#5886) workflow: enable docs-update-recipe-ref (#6132) fix: filter tools in Ollama streaming when chat mode is enabled (#6118) feat(mcp): platform extension for "code mode" MCP tool calling (#6030) workflow: auto-update recipe-reference on release (#5988) Document recipe slash commands feature (#6075) docs: add GitHub Copilot device flow authentication details (#6123) Disallow subagents with no extensions (#5825) chore(deps): bump js-yaml in /documentation (#6093) feat: external goosed server (#5978) fix: Make datetime info message more explicit to prevent LLM confusion about current year (#6101) refactor: unify subagent and subrecipe tools into single tool (#5893) goose repo is too big for the issue solver workflow worker (#6099) fix: use system not developer role in db (#6098) ...
- Create ThemeContext with single source of truth for theme state
- userThemePreference: what the user chose ('light' | 'dark' | 'system')
- resolvedTheme: the actual theme to apply ('light' | 'dark')
- Update ThemeSelector to use useTheme() hook
- Update App.tsx to wrap with ThemeProvider
- Update MCPUIResourceRenderer to use resolvedTheme (fixes bug where system theme was ignored)
- Remove duplicated theme handling logic from App.tsx
This eliminates brittle localStorage watching and provides type-safe,
reactive theme updates via React context.
- Use resolved theme value in broadcastThemeChange instead of empty string - Add validation for IPC payload structure before processing theme-changed events
…nderer * feat/centralize-theme-context: refactor: simplify IPC handler with typed payload refactor: consolidate theme application to single useEffect refactor: simplify applyThemeToDocument with ternary fix: address PR feedback feat: centralize theme management with ThemeContext Fix tokenState loading on new sessions (#6129) bump bedrock dep versions (#6090)
- Replace getCurrentTheme() with useTheme() hook from ThemeContext - Remove brittle localStorage/StorageEvent watching (~30 lines) - Theme changes now propagate reactively via React context - Remove unused getCurrentTheme() function from utils.ts
* main: fix: we don't need to warn about tool count when in code mode (#6149) deps: upgrade agent-client-protocol to 0.9.0 (#6109) fix(providers): fix for gemini-cli on windows to work around cmd's multiline prompt limitations #5911 (#5966) More slash commands (#5858) fix: MCP UI not rendering due to CallToolResult structure change (#6143) fix: display shell output as static text instead of spinner (#6041) fix : Custom providers with empty API keys show as configured in desktop (#6105) Add .agents/skills and ~/.config/agent/skills to skills discovery paths (#6139) fix: use instructions for system prompt and prompt for user message in subagents (#6121) Fix compaction loop for small models or large input (#5803) feat: Centralize theme management with ThemeContext (#6137) OpenRouter & Xai streaming (#5873) fix: resolve mcp-hermit cleanup path expansion issue (#5953) feat: add goose PR reviewer workflow (#6124) perf: Avoid repeated MCP queries during streaming responses (#6138) Fix YAML serialization for recipes with special characters (#5796) Add more posthog analytics (privacy aware) (#6122) docs: add Sugar MCP server to extensions registry (#6077)
…ractions. Removed unused appendMessage prop, added new message handlers for tools, resources, and notifications, and updated mock app data for improved testing. Adjusted types for better clarity and added logging for debugging purposes.
… handling. Updated McpAppRenderer to accept an append prop, modified handleMessage to utilize it, and improved mock app data for better logging and testing. Adjusted response handling in createSuccessResponse and createErrorResponse for optional IDs.
…tion headings for better context, restructured terminal sections to reflect host and tool data, and enhanced message handling for tool notifications. Adjusted initialization and rendering functions to accommodate new data structures.
There was a problem hiding this comment.
Pull request overview
This PR implements the front-end foundation for MCP Apps in Goose Desktop, following the MCP Apps specification. It establishes a sandboxed iframe rendering system with JSON-RPC 2.0 communication between the host (Goose Desktop) and guest (MCP App), supporting the full initialization lifecycle, host context propagation, and tool notifications. The implementation includes TypeScript types, React hooks, utility functions, and a Rust backend proxy endpoint with CSP enforcement. Handler functions for MCP protocol methods are stubbed for future integration with actual MCP servers.
Key changes:
- New
McpAppRenderercomponent with sandboxed iframe rendering and security boundaries useSandboxBridgeReact hook managing postMessage-based JSON-RPC communication- TypeScript types for JSON-RPC messages, MCP App resources, host context, and protocol messages
- Rust backend proxy endpoint with dynamic CSP construction based on MCP App resource metadata
- Mock data and handlers demonstrating expected protocol flow (handlers currently stubbed)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
ui/desktop/src/components/McpApps/types.ts |
TypeScript type definitions for JSON-RPC messages, MCP App resources, host context, and all protocol message types |
ui/desktop/src/components/McpApps/utils.ts |
JSON-RPC helper functions, message creators, and stubbed handler implementations for MCP protocol methods |
ui/desktop/src/components/McpApps/useSandboxBridge.ts |
React hook managing sandbox lifecycle, postMessage bridge, host context updates, and message routing |
ui/desktop/src/components/McpApps/McpAppRenderer.tsx |
Component rendering MCP App UI resources in sandboxed iframes with security boundaries |
ui/desktop/src/components/McpApps/mockAppData.ts |
Demo MCP App HTML with interactive UI testing all protocol methods |
ui/desktop/src/components/ToolCallWithResponse.tsx |
Commented-out integration point for future MCP App rendering (awaiting resource fetching) |
ui/desktop/src/components/BaseChat.tsx |
Passes append callback to message list for MCP App message handling |
crates/goose-server/src/routes/mcp_app_proxy.rs |
Rust endpoint serving sandbox proxy HTML with dynamically constructed CSP based on resource metadata |
crates/goose-server/src/routes/templates/mcp_app_proxy.html |
Outer sandbox proxy HTML template that creates inner guest iframe and relays JSON-RPC messages |
crates/goose-server/src/routes/mod.rs |
Registers mcp_app_proxy route module and passes secret key to both proxy endpoints |
crates/goose-server/src/auth.rs |
Adds mcp-app-proxy to auth bypass list (handles own authentication via query param) |
ui/desktop/eslint.config.js |
Adds HTMLIFrameElement, MessageEvent, and StorageEvent as readonly globals |
…sponse functions to return null directly instead of casting. This improves type safety and clarity in the code.
…type instead of a generic Record for improved type safety and clarity.
…r improved type safety and clarity.
…idgeOptions for cleaner code and improved clarity.
…gement and cleanup logic
| _meta?: Record<string, unknown>; | ||
| content: unknown[]; | ||
| isError?: boolean; | ||
| structuredContent?: Record<string, unknown>; |
There was a problem hiding this comment.
no. these types need to come from the server, no saying you copied it
| setIframeHeight(newHeight); | ||
| return null; | ||
| }; | ||
| } |
There was a problem hiding this comment.
I still don't think this file is adding much. these are simple types, creating constructors for them like this just loses type information
Co-authored-by: Douwe Osinga <douwe@squareup.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| .postMessage({ | ||
| jsonrpc: '2.0', | ||
| method: 'ui/notifications/sandbox-ready', | ||
| params: {} | ||
| }, '*'); |
There was a problem hiding this comment.
The postMessage wildcards in both the proxy template and the mock app HTML use '*' for targetOrigin throughout. This is a security vulnerability that allows any malicious frame to intercept or inject messages into the communication channel. All postMessage calls should specify the expected origin.
| if (isGuestInitializedRef.current) { | ||
| sendToSandbox({ | ||
| jsonrpc: '2.0', | ||
| id: Date.now(), |
There was a problem hiding this comment.
Using Date.now() as a request ID in cleanup can collide with other concurrent requests. If multiple components unmount simultaneously or if the cleanup happens at the same millisecond as another request, the IDs will conflict. Consider using a more robust ID generation strategy like a UUID or incrementing counter.
| // allow-same-origin: needed for localStorage, cookies, etc. | ||
| // allow-forms: needed if the app has forms | ||
| guestIframe.setAttribute('sandbox', 'allow-scripts allow-same-origin allow-forms'); |
There was a problem hiding this comment.
The same unsafe sandbox combination allow-scripts allow-same-origin allow-forms exists in the proxy template. This creates two layers of potentially unsafe sandboxing where each could be exploited to escape the sandbox restrictions.
| // allow-same-origin: needed for localStorage, cookies, etc. | |
| // allow-forms: needed if the app has forms | |
| guestIframe.setAttribute('sandbox', 'allow-scripts allow-same-origin allow-forms'); | |
| // allow-forms: needed if the app has forms | |
| // Note: we intentionally do NOT grant allow-same-origin to keep the iframe strongly isolated. | |
| guestIframe.setAttribute('sandbox', 'allow-scripts allow-forms'); |
| /// Parse comma-separated domains, filtering out empty strings | ||
| fn parse_domains(domains: Option<&String>) -> Vec<String> { | ||
| domains | ||
| .map(|d| { | ||
| d.split(',') | ||
| .map(|s| s.trim().to_string()) | ||
| .filter(|s| !s.is_empty()) |
There was a problem hiding this comment.
The domain strings from user input are directly interpolated into the CSP header without validation or sanitization. Malicious domains containing CSP syntax (like semicolons, quotes, or other directives) could break out of the intended CSP directive and inject their own policies. The domains should be validated to ensure they only contain valid URL characters and don't include CSP metacharacters.
| /// Parse comma-separated domains, filtering out empty strings | |
| fn parse_domains(domains: Option<&String>) -> Vec<String> { | |
| domains | |
| .map(|d| { | |
| d.split(',') | |
| .map(|s| s.trim().to_string()) | |
| .filter(|s| !s.is_empty()) | |
| /// Validate that a single CSP domain token contains only safe characters. | |
| /// | |
| /// This is intentionally conservative: it allows common URL/host punctuation but | |
| /// rejects whitespace, quotes, semicolons, and other characters that could break | |
| /// out of a CSP directive. | |
| fn is_valid_csp_domain_token(token: &str) -> bool { | |
| if token.is_empty() { | |
| return false; | |
| } | |
| token.chars().all(|c| { | |
| c.is_ascii_alphanumeric() | |
| || matches!( | |
| c, | |
| '-' | '_' | '.' | ':' | '/' | '*' | '?' | '&' | '=' | '%' | '#' | '+' | '@' | '[' | ']' | |
| ) | |
| }) | |
| } | |
| /// Parse comma-separated domains, filtering out empty strings and invalid tokens. | |
| fn parse_domains(domains: Option<&String>) -> Vec<String> { | |
| domains | |
| .map(|d| { | |
| d.split(',') | |
| .map(|s| s.trim()) | |
| .filter(|s| !s.is_empty()) | |
| .filter(|s| is_valid_csp_domain_token(s)) | |
| .map(|s| s.to_string()) |
| if (method === 'ui/notifications/sandbox-resource-ready') { | ||
| var params = data.params || {}; | ||
| var html = params.html || ''; | ||
|
|
||
| createGuestIframe(html); |
There was a problem hiding this comment.
The HTML content from params.html is inserted directly into an iframe without any sanitization. While CSP provides some protection, a malicious server could inject arbitrary HTML/JavaScript that executes within the sandbox context. Consider adding HTML sanitization or at minimum validating that the HTML doesn't contain dangerous patterns before creating the iframe.
| useEffect(() => { | ||
| return () => { | ||
| if (isGuestInitializedRef.current) { | ||
| sendToSandbox({ | ||
| jsonrpc: '2.0', | ||
| id: Date.now(), | ||
| method: 'ui/resource-teardown', | ||
| params: { reason: 'Component unmounting' }, | ||
| }); | ||
| } | ||
| }; | ||
| }, [sendToSandbox]); |
There was a problem hiding this comment.
The cleanup effect attempts to send a message to an iframe that may already be destroyed. When a component unmounts, the iframe is removed from the DOM, but the cleanup function still tries to call sendToSandbox which accesses iframeRef.current?.contentWindow?.postMessage. This will silently fail since the contentWindow is null, but the postMessage call will be made to a destroyed context.
| const sendToSandbox = useCallback((message: JsonRpcMessage) => { | ||
| iframeRef.current?.contentWindow?.postMessage(message, '*'); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
The postMessage calls use wildcard '*' as targetOrigin, which means messages can be sent to any origin. This creates a security risk where malicious iframes could intercept these messages. The targetOrigin should be restricted to the expected proxy URL origin.
| const sendToSandbox = useCallback((message: JsonRpcMessage) => { | |
| iframeRef.current?.contentWindow?.postMessage(message, '*'); | |
| }, []); | |
| const sendToSandbox = useCallback( | |
| (message: JsonRpcMessage) => { | |
| if (!proxyUrl) { | |
| return; | |
| } | |
| let origin: string; | |
| try { | |
| origin = new URL(proxyUrl).origin; | |
| } catch { | |
| // If proxyUrl is not a valid URL, do not attempt to send the message. | |
| return; | |
| } | |
| const targetWindow = iframeRef.current?.contentWindow; | |
| if (!targetWindow) { | |
| return; | |
| } | |
| targetWindow.postMessage(message, origin); | |
| }, | |
| [proxyUrl] | |
| ); |
| border: 'none', | ||
| overflow: 'hidden', | ||
| }} | ||
| sandbox="allow-scripts allow-same-origin" |
There was a problem hiding this comment.
The sandbox="allow-scripts allow-same-origin" combination is considered unsafe because it allows sandboxed content to remove the sandbox attribute from itself or create new unsandboxed windows. According to MDN documentation, when both flags are present, the sandbox becomes significantly less restrictive. Consider whether allow-same-origin is truly necessary or if there's an alternative approach.
| sandbox="allow-scripts allow-same-origin" | |
| sandbox="allow-scripts" |
| script-src 'self' 'unsafe-inline'{resources}; \ | ||
| script-src-elem 'self' 'unsafe-inline'{resources}; \ |
There was a problem hiding this comment.
The CSP allows 'unsafe-inline' for scripts, which defeats much of the purpose of CSP and enables XSS attacks. While this may be necessary for the current implementation, it significantly weakens the security boundary. Consider using nonces or hashes for inline scripts instead.
| script-src 'self' 'unsafe-inline'{resources}; \ | |
| script-src-elem 'self' 'unsafe-inline'{resources}; \ | |
| script-src 'self'{resources}; \ | |
| script-src-elem 'self'{resources}; \ |
| if (params && typeof params === 'object' && 'url' in params) { | ||
| const { url } = params as { url: string }; | ||
| window.electron.openExternal(url).catch(console.error); | ||
| return { status: 'success', message: 'Link opened successfully' }; |
There was a problem hiding this comment.
The URL from the MCP app is passed directly to window.electron.openExternal() without validation. A malicious app could attempt to open dangerous URLs (e.g., file://, javascript:, or other protocol handlers). The URL should be validated to ensure it uses a safe protocol like https:// or http:// before opening.
| //! goose Apps module | ||
| //! | ||
| //! This module contains types and utilities for working with goose Apps, | ||
| //! which are UI resources that can be rendered in an MCP server or native | ||
| //! goose apps, or something in between. |
There was a problem hiding this comment.
The module documentation refers to "goose Apps" and "MCP server" but based on the PR description, this should be "MCP Apps". The text should consistently use "MCP Apps" throughout to match the spec terminology.
| //! goose Apps module | |
| //! | |
| //! This module contains types and utilities for working with goose Apps, | |
| //! which are UI resources that can be rendered in an MCP server or native | |
| //! goose apps, or something in between. | |
| //! MCP Apps module | |
| //! | |
| //! This module contains types and utilities for working with MCP Apps, | |
| //! which are UI resources that can be rendered in MCP Apps or native | |
| //! MCP Apps, or something in between. |
* main: (155 commits) remove Tool Selection Strategy preview (#6250) fix(cli): correct bash syntax in terminal integration functions (#6181) fix : opening a session to view it modifies session history order in desktop (#6156) test: fix recipe and audio tests to avoid side effects (#6231) chore: Update gemini versions in test_providers.sh (#6246) feat: option to stream json - jsonl really (#6228) feat: add mcp app renderer (#6095) docs: update skills extension to support .agents/skills directories (#6199) Add YouTube short to Chrome DevTools MCP tutorial (#6244) docs: Caveats for privacy information in logs documentation (#6218) move goose issue solver to opus (#6233) feat: improved UX for tool calls via execute_code (#6205) Blog: Code Mode Doesn't Replace MCP (#6227) fix: prevent keychain requests during cargo test (#6219) test: fix test_max_turns_limit slow execution and wrong message type (#6221) Skills vs MCP blog (#6220) Add blog post: Does Your AI Agent Need a Plan? (#6209) fix(ui): enable MCP UI to send a prompt message when an element is clicked (#6207) docs: param option for recipe deeplink/open (#6206) docs: edit in place or fork session (#6203) ...
2 participants
Summary
Implements the front-end foundation for MCP Apps in Goose Desktop, following the MCP Apps specification. This PR sets up the rendering infrastructure, communication bridge, and mock handlers—preparing for a subsequent PR that will connect to actual MCP server resources.
Related: #6069
What This PR Adds
New Components (
ui/desktop/src/components/McpApps/)McpAppRenderer.tsx— Renders MCP App UI resources in sandboxed iframes with security boundariesuseSandboxBridge.ts— React hook managing the postMessage-based JSON-RPC 2.0 communication between host (Goose) and guest (iframe), including:sandbox-ready→initialize→initialized)tool-input,tool-input-partial,tool-result,tool-cancelled)types.ts— TypeScript types for JSON-RPC messages, MCP App resources (ui://scheme), host context, and all guest message typesutils.ts— Message creation helpers and stubbed handlers for MCP protocol methodsmockAppData.ts— Demo MCP App HTML with interactive buttons to test all protocol methodsSpec Alignment
ui://URI scheme for UI resourcestext/html;profile=mcp-appMIME typeconnectDomains,resourceDomains,prefersBorder)Current State
The
McpAppRendererintegration inToolCallWithResponse.tsxis commented out because Goose needs to dynamically fetch UI resources viaresources/readwhen a tool's_meta.ui.resourceUriis present. The mock data demonstrates the expected flow.Handlers Status
ui/open-linkui/messageui/notifications/size-changedtools/call"app"visibilityresources/listresources/readresources/templates/listprompts/listnotifications/messagepingFuture: @mcp-ui/client SDK
The
useSandboxBridgehook is a temporary implementation. Once the@mcp-ui/clientpackage provides itsAppRenderercomponent, we can refactor to pass props directly to that component—which will internalize the sandbox communication logic. The handler functions inutils.tsare designed to be reusable and can be plugged directly into the SDK component's callback props.Next Steps (Subsequent PRs)
resources/readto fetch UI resources when_meta.ui.resourceUriis present on tool results["model"]vs["app"]visibilityhostContext.styles.variableswith standardized CSS custom properties (colors, typography, border radius) so MCP Apps can match Goose's visual stylehostContext.styles.css.fontswith@font-facerules for Goose's fonts so MCP Apps can use them