feat: MCP UI proxy to goose-server#5749
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the MCP UI proxy functionality from a local Electron-managed HTTP server to a centralized endpoint on the goose-server. The changes remove complex client-side proxy security infrastructure (token generation, header injection, WebContents whitelisting) in favor of a simpler server-side implementation.
Key changes:
- Removes local proxy server initialization and IPC handlers from the Electron desktop app
- Updates the MCP UI renderer to fetch proxy URL from goose-server instead of Electron main process
- Adds
/mcp-ui-proxyendpoint to goose-server with auth bypass
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/desktop/src/preload.ts | Removes getMcpUIProxyUrl IPC API definition |
| ui/desktop/src/main.ts | Removes proxy initialization call |
| ui/desktop/src/components/MCPUIResourceRenderer.tsx | Updates to construct proxy URL from goosed host/port |
| ui/desktop/src/api/types.gen.ts | Generated TypeScript types for new endpoint |
| ui/desktop/src/api/sdk.gen.ts | Generated SDK client for new endpoint |
| ui/desktop/openapi.json | OpenAPI schema update for new endpoint |
| crates/goose-server/src/routes/mod.rs | Registers new mcp_ui_proxy route module |
| crates/goose-server/src/openapi.rs | Registers new endpoint in OpenAPI docs |
| crates/goose-server/src/auth.rs | Bypasses authentication for /mcp-ui-proxy |
| pub mod audio; | ||
| pub mod config_management; | ||
| pub mod errors; | ||
| pub mod mcp_ui_proxy; |
There was a problem hiding this comment.
The mcp_ui_proxy.rs module file doesn't exist yet. This will cause a compilation error.
| next: Next, | ||
| ) -> Result<Response, StatusCode> { | ||
| if request.uri().path() == "/status" { | ||
| if request.uri().path() == "/status" || request.uri().path() == "/mcp-ui-proxy" { |
There was a problem hiding this comment.
Bypassing authentication for /mcp-ui-proxy removes the security protections from the original implementation (token validation, origin checks, WebContents whitelisting). This allows unauthenticated access to MCP UI resources. Consider requiring the X-Secret-Key header or document why this endpoint must be public.
| if request.uri().path() == "/status" || request.uri().path() == "/mcp-ui-proxy" { | |
| if request.uri().path() == "/status" { |
There was a problem hiding this comment.
I support simplifying the complexity of security protections in the Node implementation. I think our goal should be to prevent access to the route outside of Goose (like directly in-browser and/or via CURL).
alexhancock
force-pushed
the
feat/mcp-ui-improvements-rust-server-goosed
branch
from
5d2cd63 to
a7770d0
Compare
| Router, | ||
| }; | ||
|
|
||
| const MCP_UI_PROXY_HTML: &str = r#"<!doctype html> |
There was a problem hiding this comment.
To make this easier to maintain, can we keep this HTML as a standalone file and inline it like we do in the autovisualiser mod.rs?
| axum::extract::State(secret_key): axum::extract::State<String>, | ||
| Query(params): Query<ProxyQuery>, | ||
| ) -> Response { | ||
| if params.secret != secret_key { |
There was a problem hiding this comment.
Timing attack vulnerability: string comparison using != operator is not constant-time. An attacker could use timing differences to guess the secret key character by character. Use a constant-time comparison function instead, such as subtle::ConstantTimeEq from the subtle crate.
| const baseUrl = await window.electron.getGoosedHostPort(); | ||
| const secretKey = await window.electron.getSecretKey(); | ||
| if (baseUrl && secretKey) { | ||
| setProxyUrl(`${baseUrl}/mcp-ui-proxy?secret=${encodeURIComponent(secretKey)}`); |
There was a problem hiding this comment.
Security exposure: the secret key is exposed in the URL query parameter, which can be logged in browser history, server logs, and referrer headers. Consider using a POST request with the secret in the request body, or implementing a session-based approach where the authentication happens separately from the proxy URL.
| setProxyUrl(`${baseUrl}/mcp-ui-proxy?secret=${encodeURIComponent(secretKey)}`); | |
| setProxyUrl(`${baseUrl}/mcp-ui-proxy`); |
| next: Next, | ||
| ) -> Result<Response, StatusCode> { | ||
| if request.uri().path() == "/status" { | ||
| if request.uri().path() == "/status" || request.uri().path() == "/mcp-ui-proxy" { |
There was a problem hiding this comment.
Authentication bypass: /mcp-ui-proxy is excluded from the global auth middleware check (line 13), but performs its own authentication using query parameters. This creates an inconsistent security model. If the query parameter check fails or is bypassed, the endpoint becomes publicly accessible. Consider either: (1) keeping /mcp-ui-proxy in the middleware and moving secret validation there, or (2) ensuring the endpoint's authentication is robust enough to stand alone.
| if request.uri().path() == "/status" || request.uri().path() == "/mcp-ui-proxy" { | |
| if request.uri().path() == "/status" { |
| --> | ||
| <meta | ||
| http-equiv="Content-Security-Policy" | ||
| content="default-src 'self'; script-src * 'wasm-unsafe-eval' 'unsafe-inline' 'unsafe-eval' blob:; style-src * 'unsafe-inline'; font-src *; connect-src *; frame-src *; media-src *; base-uri 'self'; upgrade-insecure-requests"/> |
There was a problem hiding this comment.
Security risk: the CSP includes 'unsafe-eval' in script-src, which allows eval(), new Function(), and similar dangerous operations. This significantly weakens the CSP's protection against XSS attacks. Unless absolutely required for MCP-UI functionality, consider removing 'unsafe-eval' and using safer alternatives like 'wasm-unsafe-eval' (which you already have) for WebAssembly.
| content="default-src 'self'; script-src * 'wasm-unsafe-eval' 'unsafe-inline' 'unsafe-eval' blob:; style-src * 'unsafe-inline'; font-src *; connect-src *; frame-src *; media-src *; base-uri 'self'; upgrade-insecure-requests"/> | |
| content="default-src 'self'; script-src * 'wasm-unsafe-eval' 'unsafe-inline' blob:; style-src * 'unsafe-inline'; font-src *; connect-src *; frame-src *; media-src *; base-uri 'self'; upgrade-insecure-requests"/> |
alexhancock
force-pushed
the
feat/mcp-ui-improvements-rust-server-goosed
branch
from
31c6b36 to
20c8740
Compare
Moves the mcp ui proxy from desktop ts -> goosed