diff --git a/src/exec/runner.ts b/src/exec/runner.ts index bbb3f2a39..60cde4be6 100644 --- a/src/exec/runner.ts +++ b/src/exec/runner.ts @@ -20,6 +20,7 @@ import { registerSourceCredential, } from "../config/source-credentials.js"; import { formatDirectorSystemPrompt } from "../agent/directors/identity.js"; +import { formatMcpTrustQuestion } from "../trust/project-trust.js"; import { DIRECTOR_REGISTRY } from "../agent/directors/registry.js"; import type { DirectorId, DirectorPackage } from "../agent/directors/types.js"; import { submitOutputDefinition } from "../agent/director.js"; @@ -165,14 +166,7 @@ const logger = getLogger([LOG_NAMESPACE_ROOT, "exec"]); const SELECTED_PROVIDER_FAILURE = "SelectedProviderFailure"; export function formatExecMcpTrustQuestion(server: MCPServerConfig): string { - return ( - `Trust local MCP server "${server.name}" for this project?` + - (server.command !== undefined - ? `\nCommand: ${server.command}${(server.args ?? []).length > 0 ? ` ${(server.args ?? []).join(" ")}` : ""}` - : server.url !== undefined - ? `\nURL: ${server.url}` - : "") - ); + return formatMcpTrustQuestion(server); } export async function refreshSelectedProviderCredential( diff --git a/src/mcp/client.ts b/src/mcp/client.ts index e804d1e5f..2121ade02 100644 --- a/src/mcp/client.ts +++ b/src/mcp/client.ts @@ -13,6 +13,7 @@ import { normalizeMCPServerURL } from "./auth-store.js"; import type { ResolvedMCPServerConfig } from "./exa.js"; import type { McpToolAnnotations } from "./tool-permissions.js"; import { buildStdioMcpProcessEnv } from "./stdio-env.js"; +import { isHttpServer } from "./is-http-server.js"; import { MCP_CLIENT_NAME } from "../branding.js"; export interface MCPTool { @@ -95,13 +96,6 @@ export interface MCPConnectOptions { onDisconnect?: () => void; } -function isHttpServer(config: ResolvedMCPServerConfig): boolean { - return ( - config.type === "http" || - (config.type === undefined && config.url !== undefined) - ); -} - export function unwrapToolContent(content: unknown): string { if (!Array.isArray(content) || content.length === 0) return ""; return content diff --git a/src/mcp/is-http-server.test.ts b/src/mcp/is-http-server.test.ts new file mode 100644 index 000000000..6f774b35c --- /dev/null +++ b/src/mcp/is-http-server.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, test } from "bun:test"; +import { isHttpServer } from "./is-http-server.js"; + +describe("isHttpServer", () => { + test("HTTP wins when type is unset and url is set, even with command", () => { + const config = { command: "run", url: "https://mcp.example.test" }; + expect(isHttpServer(config)).toBe(true); + }); + + test("type http wins even when command is also set", () => { + const config = { + type: "http" as const, + command: "run", + url: "https://mcp.example.test", + }; + expect(isHttpServer(config)).toBe(true); + }); + + test("type stdio is not HTTP even when url is set", () => { + expect( + isHttpServer({ + type: "stdio", + url: "https://mcp.example.test", + }), + ).toBe(false); + }); + + test("unset type and url is not HTTP", () => { + expect(isHttpServer({})).toBe(false); + }); +}); diff --git a/src/mcp/is-http-server.ts b/src/mcp/is-http-server.ts new file mode 100644 index 000000000..e0dce6dff --- /dev/null +++ b/src/mcp/is-http-server.ts @@ -0,0 +1,15 @@ +/** + * Connect-path transport: HTTP wins when `type` is `"http"`, or when `type` is + * unset and `url` is present — even if `command` is also set. Trust-prompt + * display must use this same predicate so the operator grants the identity + * that `connectMCPServer` will actually open. + */ +export function isHttpServer(config: { + type?: "stdio" | "http"; + url?: string; +}): boolean { + return ( + config.type === "http" || + (config.type === undefined && config.url !== undefined) + ); +} diff --git a/src/trust/project-trust.ts b/src/trust/project-trust.ts index 3afe52927..fda7075bf 100644 --- a/src/trust/project-trust.ts +++ b/src/trust/project-trust.ts @@ -7,6 +7,7 @@ import { type } from "arktype"; import { getLogger } from "@intx/log"; import type { MCPServerConfig } from "../config/settings.js"; import { isBuiltinExaMCPServer } from "../mcp/exa.js"; +import { isHttpServer } from "../mcp/is-http-server.js"; import { LOG_NAMESPACE_ROOT, SETTINGS_DIR_NAME } from "../branding.js"; const logger = getLogger([LOG_NAMESPACE_ROOT, "trust"]); @@ -298,6 +299,72 @@ export function mcpServerFingerprint(server: MCPServerConfig): string { return createHash("sha256").update(payload).digest("hex"); } +// Display-only quoting for the MCP trust prompt: argv, name, and url all pass +// through the same escape so a newline, quote, or Unicode/C1 line break cannot +// spoof extra prompt lines. An arg containing whitespace (or a quote, or empty) +// renders double-quoted so ["a b"] and ["a", "b"] never look alike. Approval +// identity still comes from mcpServerFingerprint above, never from this rendering. +function isTrustPromptControlChar(code: number): boolean { + return ( + code <= 0x1f || + code === 0x7f || + (code >= 0x80 && code <= 0x9f) || + code === 0x2028 || + code === 0x2029 + ); +} + +function escapeMcpTrustText(value: string): string { + const named = value + .replace(/\\/g, "\\\\") + .replace(/"/g, '\\"') + .replace(/\n/g, "\\n") + .replace(/\r/g, "\\r") + .replace(/\t/g, "\\t"); + let escaped = ""; + for (const ch of named) { + const code = ch.charCodeAt(0); + if (!isTrustPromptControlChar(code)) { + escaped += ch; + continue; + } + escaped += + code <= 0xff + ? `\\x${code.toString(16).toUpperCase().padStart(2, "0")}` + : `\\u${code.toString(16).toUpperCase().padStart(4, "0")}`; + } + return escaped; +} + +function quoteMcpTrustArg(arg: string): string { + const needsQuotes = + arg === "" || + /[\s"]/.test(arg) || + [...arg].some((ch) => isTrustPromptControlChar(ch.charCodeAt(0))); + if (!needsQuotes) return arg; + return `"${escapeMcpTrustText(arg)}"`; +} + +function formatMcpSpawnCommand(command: string, args: string[]): string { + const head = quoteMcpTrustArg(command); + return args.length === 0 + ? head + : `${head} ${args.map(quoteMcpTrustArg).join(" ")}`; +} + +export function formatMcpTrustQuestion(server: MCPServerConfig): string { + const header = `Trust local MCP server "${escapeMcpTrustText(server.name)}" for this project?`; + if (isHttpServer(server)) { + return server.url !== undefined + ? `${header}\nURL: ${quoteMcpTrustArg(server.url)}` + : header; + } + if (server.command !== undefined) { + return `${header}\nCommand: ${formatMcpSpawnCommand(server.command, server.args ?? [])}`; + } + return header; +} + export function isMcpServerTrusted( store: ProjectTrustStore, server: MCPServerConfig, diff --git a/src/tui/runner/session.ts b/src/tui/runner/session.ts index 542ee3818..40e6c24d3 100644 --- a/src/tui/runner/session.ts +++ b/src/tui/runner/session.ts @@ -14,6 +14,7 @@ import { EventEmitter } from "node:events"; import { shellTimeoutFromSettings, toolWatchdogFromSettings, + type MCPServerConfig, } from "../../config/settings.js"; import { isCodexProviderName } from "../../config/codex-providers.js"; import { peekSourceCredentialSecret } from "../../config/source-credentials.js"; @@ -126,6 +127,11 @@ import { } from "./state.js"; import { createParkedOverlayAbortBinding } from "./parked-overlay-abort.js"; import { createTUISettingsWriters } from "./settings-writers.js"; +import { formatMcpTrustQuestion } from "../../trust/project-trust.js"; + +export function formatTuiMcpTrustQuestion(server: MCPServerConfig): string { + return formatMcpTrustQuestion(server); +} export async function assembleTUISession( state: RunnerState, @@ -406,13 +412,7 @@ export async function assembleTUISession( const timeout = approvalTimeout(); const event: OperatorGateEvent = { id: randomUUID(), - question: - `Trust local MCP server "${server.name}" for this project?` + - (server.command !== undefined - ? `\nCommand: ${server.command}${(server.args ?? []).length > 0 ? ` ${(server.args ?? []).join(" ")}` : ""}` - : server.url !== undefined - ? `\nURL: ${server.url}` - : ""), + question: formatTuiMcpTrustQuestion(server), options: ["Trust and connect", "Deny"], resolve: finish, ...(timeout !== undefined ? timeout : {}), diff --git a/tests/unit/exec/runner.test.ts b/tests/unit/exec/runner.test.ts index 264f9382f..073d80ba0 100644 --- a/tests/unit/exec/runner.test.ts +++ b/tests/unit/exec/runner.test.ts @@ -124,6 +124,57 @@ describe("exec MCP trust prompt", () => { }); }); +describe("exec MCP trust prompt argv boundaries", () => { + test("quotes an arg containing whitespace", () => { + expect( + formatExecMcpTrustQuestion({ + name: "notes", + command: "server", + args: ["--dir", "/tmp/my work"], + }), + ).toBe( + 'Trust local MCP server "notes" for this project?\nCommand: server --dir "/tmp/my work"', + ); + }); + + test("renders one spaced arg distinctly from two args", () => { + const one = formatExecMcpTrustQuestion({ + name: "s", + command: "run", + args: ["a b"], + }); + const two = formatExecMcpTrustQuestion({ + name: "s", + command: "run", + args: ["a", "b"], + }); + expect(one).toContain('"a b"'); + expect(one).not.toBe(two); + }); + + test("quotes empty args so they stay visible", () => { + const question = formatExecMcpTrustQuestion({ + name: "s", + command: "run", + args: [""], + }); + expect(question).toContain('""'); + expect(question).not.toBe( + formatExecMcpTrustQuestion({ name: "s", command: "run", args: [] }), + ); + }); + + test("escapes quotes inside a quoted arg", () => { + expect( + formatExecMcpTrustQuestion({ + name: "s", + command: "run", + args: ['say "hi"'], + }), + ).toContain('"say \\"hi\\""'); + }); +}); + describe("formatCaughtError", () => { test("prefers Error.message and stringifies other values", () => { expect(formatCaughtError(new Error("disk full"))).toBe("disk full"); diff --git a/tests/unit/project-trust.test.ts b/tests/unit/project-trust.test.ts index a4d121d4d..dd41cae47 100644 --- a/tests/unit/project-trust.test.ts +++ b/tests/unit/project-trust.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { filterMcpServersForConnect, + formatMcpTrustQuestion, isMcpServerTrusted, isPluginTrusted, loadProjectTrust, @@ -358,6 +359,199 @@ describe("project-trust", () => { } }); + test("trust question quotes whitespace args so argv boundaries stay visible", () => { + const one = formatMcpTrustQuestion({ + name: "s", + command: "run", + args: ["a b"], + }); + const two = formatMcpTrustQuestion({ + name: "s", + command: "run", + args: ["a", "b"], + }); + expect(one).toBe( + 'Trust local MCP server "s" for this project?\nCommand: run "a b"', + ); + expect(one).not.toBe(two); + }); + + test("trust question escapes control characters so args stay single-line", () => { + const question = formatMcpTrustQuestion({ + name: "s", + command: "run", + args: ["x\nTrust local MCP server evil", "a\tb", "c\rd"], + }); + // Only the structural header/Command separator newline may remain. + const lines = question.split("\n"); + expect(lines).toHaveLength(2); + for (const line of lines) { + for (const ch of line) { + const code = ch.charCodeAt(0); + expect(code > 0x1f && code !== 0x7f).toBe(true); + } + } + expect(question).toContain('"x\\nTrust local MCP server evil"'); + expect(question).toContain('"a\\tb"'); + expect(question).toContain('"c\\rd"'); + }); + + test("trust question quotes a spaced binary path so the command is unambiguous", () => { + expect( + formatMcpTrustQuestion({ + name: "s", + command: "/tmp/my tool/server", + args: ["--dir", "/tmp/work"], + }), + ).toBe( + 'Trust local MCP server "s" for this project?\nCommand: "/tmp/my tool/server" --dir /tmp/work', + ); + expect( + formatMcpTrustQuestion({ name: "s", command: "/tmp/my tool/server" }), + ).toBe( + 'Trust local MCP server "s" for this project?\nCommand: "/tmp/my tool/server"', + ); + }); + + test("trust question leaves plain args unquoted and hides secrets", () => { + expect( + formatMcpTrustQuestion({ + name: "filesystem", + command: "npx", + args: ["-y", "@modelcontextprotocol/server-filesystem", "/tmp/work"], + }), + ).toBe( + 'Trust local MCP server "filesystem" for this project?\nCommand: npx -y @modelcontextprotocol/server-filesystem /tmp/work', + ); + const question = formatMcpTrustQuestion({ + name: "private", + command: "private-server", + env: { API_TOKEN: "super-secret" }, + }); + expect(question).toBe( + 'Trust local MCP server "private" for this project?\nCommand: private-server', + ); + expect(question).not.toContain("super-secret"); + }); + + test("trust question shows an HTTP server URL", () => { + expect( + formatMcpTrustQuestion({ + name: "remote", + type: "http", + url: "https://mcp.example.test/api", + }), + ).toBe( + 'Trust local MCP server "remote" for this project?\nURL: https://mcp.example.test/api', + ); + }); + + test("trust question shows URL not Command when command, args, and url are set without type", () => { + const question = formatMcpTrustQuestion({ + name: "s", + command: "run", + args: ["--secret"], + url: "https://mcp.example.test/api", + }); + expect(question).toContain("\nURL: https://mcp.example.test/api"); + expect(question).not.toContain("Command:"); + expect(question).not.toContain("run"); + }); + + test("trust question shows URL when type is http even if command is also set", () => { + const question = formatMcpTrustQuestion({ + name: "s", + type: "http", + command: "run", + url: "https://mcp.example.test/api", + }); + expect(question).toContain("\nURL: https://mcp.example.test/api"); + expect(question).not.toContain("Command:"); + }); + + test("trust question still shows Command when type is stdio even if url is set", () => { + const question = formatMcpTrustQuestion({ + name: "s", + type: "stdio", + command: "run", + args: ["a"], + url: "https://mcp.example.test/api", + }); + expect(question).toContain("\nCommand: run a"); + expect(question).not.toContain("URL:"); + }); + + test("trust question escapes name so a newline or quote cannot inject extra Command lines", () => { + const question = formatMcpTrustQuestion({ + name: 's"\nCommand: evil', + command: "run", + args: ["a"], + }); + const lines = question.split("\n"); + expect(lines).toHaveLength(2); + expect(lines[0]?.startsWith("Trust local MCP server")).toBe(true); + expect(lines[1]).toBe("Command: run a"); + expect(question).not.toContain("\nCommand: evil"); + expect(question).toContain("\\n"); + expect(question).toContain('\\"'); + }); + + test("trust question escapes url so an embedded newline stays single-line", () => { + const question = formatMcpTrustQuestion({ + name: "remote", + type: "http", + url: "https://mcp.example.test/api\nCommand: evil", + }); + const lines = question.split("\n"); + expect(lines).toHaveLength(2); + expect(lines[1]?.startsWith("URL:")).toBe(true); + expect(question).not.toContain("\nCommand:"); + expect(question).toContain("\\n"); + }); + + test("trust question escapes Unicode line breaks and C1 controls in args", () => { + const question = formatMcpTrustQuestion({ + name: "s", + command: "run", + args: ["x\u2028y", "a\u0085b"], + }); + expect(question.split("\n")).toHaveLength(2); + expect(question).not.toContain("\u2028"); + expect(question).not.toContain("\u0085"); + for (const line of question.split("\n")) { + for (const ch of line) { + const code = ch.charCodeAt(0); + expect( + code > 0x1f && + code !== 0x7f && + !(code >= 0x80 && code <= 0x9f) && + code !== 0x2028 && + code !== 0x2029, + ).toBe(true); + } + } + }); + + test("mcp fingerprint still hashes command and url together", () => { + const mixed: MCPServerConfig = { + name: "s", + command: "run", + args: ["a"], + url: "https://evil.test", + }; + expect(mcpServerFingerprint(mixed)).toBe( + "d06726e3489e2513056178f392b492a77aef02b239c8922c5a6cdca0b4fd886d", + ); + expect( + mcpServerFingerprint({ + name: "s", + type: "http", + command: "run", + url: "https://mcp.example.test", + }), + ).toBe("75b80b4878d818362a918027917cd149c0d948d509b8c0c08b7406fd69de53b9"); + }); + test("readProjectTrustStore: malformed file with wrong types, missing fields, and extra fields drops bad entries and ignores unknown keys", async () => { const { cwd, home, cleanup } = await scratch(); try { diff --git a/tests/unit/tui/mcp-trust-prompt-parity.test.ts b/tests/unit/tui/mcp-trust-prompt-parity.test.ts new file mode 100644 index 000000000..b6781b13b --- /dev/null +++ b/tests/unit/tui/mcp-trust-prompt-parity.test.ts @@ -0,0 +1,95 @@ +import { describe, expect, test } from "bun:test"; +import type { MCPServerConfig } from "../../../src/config/settings.js"; +import { formatExecMcpTrustQuestion } from "../../../src/exec/runner.js"; +import { formatTuiMcpTrustQuestion } from "../../../src/tui/runner/session.js"; + +const parityCases: MCPServerConfig[] = [ + { + name: "plain-stdio", + command: "node", + args: ["server.js", "--port", "3000"], + }, + { name: "no-args", command: "node" }, + { name: "empty-args", command: "node", args: [] }, + { name: "spaced-path", command: "server", args: ["--dir", "/tmp/my work"] }, + { name: "one-spaced-arg", command: "run", args: ["a b"] }, + { name: "two-plain-args", command: "run", args: ["a", "b"] }, + { name: "tab-arg", command: "run", args: ["a\tb"] }, + { name: "quoted-arg", command: "run", args: ['say "hi"'] }, + { name: "empty-string-arg", command: "run", args: [""] }, + { + name: "with-secrets", + command: "private-server", + args: ["--token", "super secret"], + env: { API_TOKEN: "super-secret" }, + }, + { name: "http-server", type: "http", url: "https://mcp.example.test/api" }, + { + name: "http-wins-no-type", + command: "run", + args: ["a"], + url: "https://mcp.example.test/api", + }, + { + name: "http-typed-with-command", + type: "http", + command: "run", + url: "https://mcp.example.test/api", + }, + { name: 'inject\nCommand: evil"', command: "run", args: ["a"] }, + { + name: "url-newline", + type: "http", + url: "https://mcp.example.test/api\nCommand: evil", + }, + { name: "unicode-break", command: "run", args: ["x\u2028y", "a\u0085b"] }, + { name: "bare-name" }, +]; + +describe("MCP trust prompt TTY/TUI parity", () => { + for (const server of parityCases) { + test(`TTY and TUI render "${server.name}" identically`, () => { + expect(formatTuiMcpTrustQuestion(server)).toBe( + formatExecMcpTrustQuestion(server), + ); + }); + } + + test('both surfaces keep ["a b"] distinct from ["a", "b"]', () => { + const oneArg: MCPServerConfig = { + name: "s", + command: "run", + args: ["a b"], + }; + const twoArgs: MCPServerConfig = { + name: "s", + command: "run", + args: ["a", "b"], + }; + for (const format of [ + formatExecMcpTrustQuestion, + formatTuiMcpTrustQuestion, + ]) { + expect(format(oneArg)).toContain('"a b"'); + expect(format(oneArg)).not.toBe(format(twoArgs)); + } + }); + + test("both surfaces render tab/newline args escaped with no raw control characters", () => { + const server: MCPServerConfig = { + name: "s", + command: "run", + args: ["a\tb", "x\ny"], + }; + for (const format of [ + formatExecMcpTrustQuestion, + formatTuiMcpTrustQuestion, + ]) { + const rendered = format(server); + expect(rendered).toContain('"a\\tb"'); + expect(rendered).toContain('"x\\ny"'); + expect(rendered).not.toContain("\t"); + expect(rendered.split("\n")).toHaveLength(2); + } + }); +});