Skip to content

Conversation

@alexhancock
Copy link
Collaborator

@alexhancock alexhancock commented Nov 14, 2025

Moves the mcp ui proxy from desktop ts -> goosed

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-proxy endpoint 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;
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mcp_ui_proxy.rs module file doesn't exist yet. This will cause a compilation error.

Copilot uses AI. Check for mistakes.
next: Next,
) -> Result<Response, StatusCode> {
if request.uri().path() == "/status" {
if request.uri().path() == "/status" || request.uri().path() == "/mcp-ui-proxy" {
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if request.uri().path() == "/status" || request.uri().path() == "/mcp-ui-proxy" {
if request.uri().path() == "/status" {

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 alexhancock force-pushed the feat/mcp-ui-improvements-rust-server-goosed branch from 5d2cd63 to a7770d0 Compare November 14, 2025 22:41
Router,
};

const MCP_UI_PROXY_HTML: &str = r#"<!doctype html>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

axum::extract::State(secret_key): axum::extract::State<String>,
Query(params): Query<ProxyQuery>,
) -> Response {
if params.secret != secret_key {
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
const baseUrl = await window.electron.getGoosedHostPort();
const secretKey = await window.electron.getSecretKey();
if (baseUrl && secretKey) {
setProxyUrl(`${baseUrl}/mcp-ui-proxy?secret=${encodeURIComponent(secretKey)}`);
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
setProxyUrl(`${baseUrl}/mcp-ui-proxy?secret=${encodeURIComponent(secretKey)}`);
setProxyUrl(`${baseUrl}/mcp-ui-proxy`);

Copilot uses AI. Check for mistakes.
next: Next,
) -> Result<Response, StatusCode> {
if request.uri().path() == "/status" {
if request.uri().path() == "/status" || request.uri().path() == "/mcp-ui-proxy" {
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if request.uri().path() == "/status" || request.uri().path() == "/mcp-ui-proxy" {
if request.uri().path() == "/status" {

Copilot uses AI. Check for mistakes.
-->
<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"/>
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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"/>

Copilot uses AI. Check for mistakes.
@alexhancock alexhancock force-pushed the feat/mcp-ui-improvements-rust-server-goosed branch from 31c6b36 to 20c8740 Compare November 17, 2025 19:53
Copy link
Collaborator

@aharvard aharvard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@aharvard aharvard merged commit b6740fb into feat/mcp-ui-improvements Nov 17, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants