Address MCP app safety review feedback

Signed-off-by: Andrew Harvard <aharvard@squareup.com>

Author: aharvard

crates/goose-cli/src/cli.rs

@@ -1103,7 +1103,11 @@ async fn handle_serve_command(host: String, port: u16, builtins: Vec<String>) ->
     info!("Starting ACP server on {}", addr);
 
     let listener = tokio::net::TcpListener::bind(addr).await?;
-    axum::serve(listener, router).await?;
+    axum::serve(
+        listener,
+        router.into_make_service_with_connect_info::<SocketAddr>(),
+    )
+    .await?;
 
     Ok(())
 }

crates/goose/src/acp/mcp_app_proxy.rs

@@ -1,12 +1,13 @@
 use axum::{
-    extract::{Query, State},
-    http::{header, HeaderMap, HeaderValue, StatusCode},
+    extract::{ConnectInfo, Query, State},
+    http::{header, HeaderValue, StatusCode},
     response::{Html, IntoResponse, Response},
     routing::{get, post},
     Json, Router,
 };
 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;
+use std::net::SocketAddr;
 use std::sync::Arc;
 use std::time::Duration;
 use std::time::Instant;
@@ -147,35 +148,8 @@ fn is_valid_dns_label(label: &str) -> bool {
         && label.chars().all(|c| c.is_ascii_alphanumeric() || c == '-')
 }
 
-fn request_host_is_loopback(headers: &HeaderMap) -> bool {
-    headers
-        .get(header::HOST)
-        .and_then(|host| host.to_str().ok())
-        .is_some_and(authority_is_loopback)
-}
-
-fn authority_is_loopback(authority: &str) -> bool {
-    if authority.contains('@') {
-        return false;
-    }
-
-    let host = if let Some(remainder) = authority.strip_prefix('[') {
-        remainder
-            .split_once(']')
-            .map(|(host, _)| host)
-            .unwrap_or(remainder)
-    } else {
-        authority.split(':').next().unwrap_or(authority)
-    };
-
-    let host = host.to_ascii_lowercase();
-    host == "localhost"
-        || host
-            .parse::<std::net::Ipv4Addr>()
-            .is_ok_and(|addr| addr.is_loopback())
-        || host
-            .parse::<std::net::Ipv6Addr>()
-            .is_ok_and(|addr| addr.is_loopback())
+fn peer_addr_is_loopback(peer_addr: &SocketAddr) -> bool {
+    peer_addr.ip().is_loopback()
 }
 
 fn parse_domains(domains: Option<&String>) -> Vec<String> {
@@ -248,16 +222,16 @@ fn build_outer_csp(
 
 async fn mcp_app_proxy(
     State(state): State<AppState>,
-    headers: HeaderMap,
+    ConnectInfo(peer_addr): ConnectInfo<SocketAddr>,
     Query(params): Query<ProxyQuery>,
 ) -> Response {
     if params.secret != state.secret_key {
         return (StatusCode::UNAUTHORIZED, "Unauthorized").into_response();
     }
-    if !request_host_is_loopback(&headers) {
+    if !peer_addr_is_loopback(&peer_addr) {
         return (
             StatusCode::BAD_REQUEST,
-            "MCP app proxy is only available from a loopback host",
+            "MCP app proxy is only available to loopback clients",
         )
             .into_response();
     }
@@ -289,16 +263,16 @@ async fn mcp_app_proxy(
 
 async fn store_guest_html(
     State(state): State<AppState>,
-    headers: HeaderMap,
+    ConnectInfo(peer_addr): ConnectInfo<SocketAddr>,
     Json(body): Json<StoreGuestBody>,
 ) -> Response {
     if body.secret != state.secret_key {
         return (StatusCode::UNAUTHORIZED, "Unauthorized").into_response();
     }
-    if !request_host_is_loopback(&headers) {
+    if !peer_addr_is_loopback(&peer_addr) {
         return (
             StatusCode::BAD_REQUEST,
-            "MCP app guest storage is only available from a loopback host",
+            "MCP app guest storage is only available to loopback clients",
         )
             .into_response();
     }
@@ -414,7 +388,8 @@ pub(crate) fn routes(secret_key: String) -> Router {
 
 #[cfg(test)]
 mod tests {
-    use super::{authority_is_loopback, normalize_csp_source, parse_domains};
+    use super::{normalize_csp_source, parse_domains, peer_addr_is_loopback};
+    use std::net::SocketAddr;
 
     #[test]
     fn normalizes_url_sources_to_origins() {
@@ -470,10 +445,15 @@ mod tests {
     }
 
     #[test]
-    fn detects_loopback_authorities() {
-        assert!(authority_is_loopback("127.0.0.1:12345"));
-        assert!(authority_is_loopback("localhost:12345"));
-        assert!(authority_is_loopback("[::1]:12345"));
-        assert!(!authority_is_loopback("example.test"));
+    fn detects_loopback_peer_addresses() {
+        assert!(peer_addr_is_loopback(
+            &"127.0.0.1:12345".parse::<SocketAddr>().unwrap()
+        ));
+        assert!(peer_addr_is_loopback(
+            &"[::1]:12345".parse::<SocketAddr>().unwrap()
+        ));
+        assert!(!peer_addr_is_loopback(
+            &"192.168.1.10:12345".parse::<SocketAddr>().unwrap()
+        ));
     }
 }

ui/goose2/src/features/chat/ui/McpAppView.tsx

@@ -4,7 +4,6 @@ import {
   type McpUiHostContext,
 } from "@mcp-ui/client";
 import type { GooseToolCallResponse } from "@aaif/goose-sdk";
-import { openUrl } from "@tauri-apps/plugin-opener";
 import { useCallback, useEffect, useMemo, useRef, useState } from "react";
 import { useTranslation } from "react-i18next";
 import packageJson from "../../../../package.json";
@@ -15,9 +14,11 @@ import type {
   McpAppPayload,
   ToolResponseContent,
 } from "@/shared/types/messages";
+import { LinkSafetyModal } from "@/shared/ui/ai-elements/link-safety-modal";
 import type { McpAppMessageHandler } from "./mcpAppTypes";
 import { extractRenderableMcpAppDocument } from "./mcpAppPayload";
 import { useIframeColorScheme } from "./useIframeColorScheme";
+import { useMcpAppOpenLink } from "./useMcpAppOpenLink";
 import { useMcpAppSandbox } from "./useMcpAppSandbox";
 
 interface McpAppViewProps {
@@ -128,6 +129,12 @@ export function McpAppView({
   const [containerWidth, setContainerWidth] = useState<number | null>(null);
   const autoScrollTimersRef = useRef<number[]>([]);
   const rootRef = useRef<HTMLDivElement>(null);
+  const {
+    handleConfirmOpenLink,
+    handleOpenLink,
+    handleOpenLinkModalClose,
+    pendingOpenLinkUrl,
+  } = useMcpAppOpenLink();
   useIframeColorScheme(rootRef, resolvedTheme);
 
   const renderableDocument = useMemo(
@@ -281,11 +288,6 @@ export function McpAppView({
     [containerWidth, inlineHeight, payload, resolvedTheme],
   );
 
-  const handleOpenLink = useCallback(async ({ url }: { url: string }) => {
-    await openUrl(url);
-    return { status: "success" as const };
-  }, []);
-
   const handleMessage = useCallback(
     async ({ role, content }: MessageParams) => {
       if (role !== "user" || !onSendMessage) {
@@ -447,6 +449,12 @@ export function McpAppView({
           )}
         </div>
       )}
+      <LinkSafetyModal
+        isOpen={pendingOpenLinkUrl !== null}
+        onClose={handleOpenLinkModalClose}
+        onOpenLink={handleConfirmOpenLink}
+        url={pendingOpenLinkUrl ?? ""}
+      />
     </div>
   );
 }

ui/goose2/src/features/chat/ui/__tests__/McpAppView.test.tsx

@@ -1,4 +1,5 @@
 import type { AppRendererProps, RequestHandlerExtra } from "@mcp-ui/client";
+import { openUrl } from "@tauri-apps/plugin-opener";
 import { fireEvent, render, screen, waitFor } from "@testing-library/react";
 import { beforeEach, describe, expect, it, vi } from "vitest";
 import { McpAppView } from "../McpAppView";
@@ -136,6 +137,8 @@ describe("McpAppView nested tool calls", () => {
     mocks.extMethod.mockReset();
     mocks.getClient.mockReset();
     mocks.resolvedTheme = "dark";
+    vi.mocked(openUrl).mockReset();
+    vi.mocked(openUrl).mockResolvedValue(undefined);
     mocks.getClient.mockResolvedValue({
       extMethod: mocks.extMethod,
     });
@@ -271,6 +274,69 @@ describe("McpAppView nested tool calls", () => {
     expect(getLatestAppRendererProps().onFallbackRequest).toBeUndefined();
   });
 
+  it("confirms safe app open-link requests before opening the URL", async () => {
+    render(
+      <McpAppView
+        payload={createPayload()}
+        toolResponse={createToolResponse()}
+      />,
+    );
+
+    await waitFor(() => {
+      expect(screen.getByTestId("mock-app-renderer")).toBeInTheDocument();
+    });
+
+    const promise = getLatestAppRendererProps().onOpenLink?.(
+      { url: "https://example.com" },
+      {} as RequestHandlerExtra,
+    );
+
+    if (!promise) {
+      throw new Error("Expected onOpenLink to be registered");
+    }
+
+    expect(openUrl).not.toHaveBeenCalled();
+
+    await waitFor(() => {
+      expect(screen.getByText("https://example.com/")).toBeInTheDocument();
+    });
+
+    fireEvent.click(screen.getByRole("button", { name: "Open link" }));
+
+    await waitFor(() => {
+      expect(openUrl).toHaveBeenCalledWith("https://example.com/");
+    });
+    await expect(promise).resolves.toEqual({});
+  });
+
+  it("blocks app open-link requests for non-web URL schemes", async () => {
+    render(
+      <McpAppView
+        payload={createPayload()}
+        toolResponse={createToolResponse()}
+      />,
+    );
+
+    await waitFor(() => {
+      expect(screen.getByTestId("mock-app-renderer")).toBeInTheDocument();
+    });
+
+    const result = await getLatestAppRendererProps().onOpenLink?.(
+      { url: "file:///private/tmp/secrets.txt" },
+      {} as RequestHandlerExtra,
+    );
+
+    expect(result).toEqual(
+      expect.objectContaining({
+        isError: true,
+      }),
+    );
+    expect(openUrl).not.toHaveBeenCalled();
+    expect(
+      screen.queryByText("file:///private/tmp/secrets.txt"),
+    ).not.toBeInTheDocument();
+  });
+
   it("keeps the iframe color scheme aligned with the host theme", async () => {
     mocks.resolvedTheme = "light";
     const payload = createPayload();

ui/goose2/src/features/chat/ui/useMcpAppOpenLink.ts

@@ -0,0 +1,88 @@
+import type { AppRendererProps } from "@mcp-ui/client";
+import { openUrl } from "@tauri-apps/plugin-opener";
+import { useCallback, useEffect, useRef, useState } from "react";
+
+type OpenLinkParams = Parameters<
+  NonNullable<AppRendererProps["onOpenLink"]>
+>[0];
+type OpenLinkResult = Awaited<
+  ReturnType<NonNullable<AppRendererProps["onOpenLink"]>>
+>;
+
+function normalizeOpenLinkUrl(url: string): string | null {
+  try {
+    const parsed = new URL(url);
+    return parsed.protocol === "http:" || parsed.protocol === "https:"
+      ? parsed.href
+      : null;
+  } catch {
+    return null;
+  }
+}
+
+export function useMcpAppOpenLink() {
+  const [pendingOpenLinkUrl, setPendingOpenLinkUrl] = useState<string | null>(
+    null,
+  );
+  const pendingOpenLinkResolverRef = useRef<
+    ((result: OpenLinkResult) => void) | null
+  >(null);
+
+  useEffect(
+    () => () => {
+      pendingOpenLinkResolverRef.current?.({ isError: true });
+      pendingOpenLinkResolverRef.current = null;
+    },
+    [],
+  );
+
+  const finishOpenLinkRequest = useCallback((result: OpenLinkResult) => {
+    pendingOpenLinkResolverRef.current?.(result);
+    pendingOpenLinkResolverRef.current = null;
+    setPendingOpenLinkUrl(null);
+  }, []);
+
+  const handleOpenLink = useCallback(async ({ url }: OpenLinkParams) => {
+    const safeUrl = normalizeOpenLinkUrl(url);
+    if (!safeUrl) {
+      return {
+        isError: true,
+        message: "Only http and https links can be opened.",
+      };
+    }
+
+    pendingOpenLinkResolverRef.current?.({
+      isError: true,
+      message: "Another open-link request replaced this request.",
+    });
+
+    return new Promise<OpenLinkResult>((resolve) => {
+      pendingOpenLinkResolverRef.current = resolve;
+      setPendingOpenLinkUrl(safeUrl);
+    });
+  }, []);
+
+  const handleOpenLinkModalClose = useCallback(() => {
+    finishOpenLinkRequest({ isError: true });
+  }, [finishOpenLinkRequest]);
+
+  const handleConfirmOpenLink = useCallback(
+    async (url: string) => {
+      try {
+        await openUrl(url);
+        finishOpenLinkRequest({});
+      } catch (error) {
+        finishOpenLinkRequest({ isError: true });
+        throw error;
+      }
+    },
+    [finishOpenLinkRequest],
+  );
+
+  return {
+    handleConfirmOpenLink,
+    handleOpenLink,
+    handleOpenLinkModalClose,
+    pendingOpenLinkUrl,
+  };
+}

ui/goose2/src/shared/ui/ai-elements/link-safety-modal.tsx

@@ -14,12 +14,14 @@ import {
 interface LinkSafetyModalProps {
   isOpen: boolean;
   onClose: () => void;
+  onOpenLink?: (url: string) => Promise<void>;
   url: string;
 }
 
 export function LinkSafetyModal({
   isOpen,
   onClose,
+  onOpenLink,
   url,
 }: LinkSafetyModalProps) {
   const { t } = useTranslation("common");
@@ -39,12 +41,12 @@ export function LinkSafetyModal({
 
   const handleOpen = useCallback(async () => {
     try {
-      await openUrl(url);
+      await (onOpenLink ?? openUrl)(url);
     } catch (e: unknown) {
       console.error("[linkSafety] openUrl failed:", e);
     }
     onClose();
-  }, [url, onClose]);
+  }, [url, onClose, onOpenLink]);
 
   const handleCopy = useCallback(() => {
     if (isCopied) return;