diff --git a/clients/cli/__tests__/completion.test.ts b/clients/cli/__tests__/completion.test.ts index ffa3414ac..4c7043428 100644 --- a/clients/cli/__tests__/completion.test.ts +++ b/clients/cli/__tests__/completion.test.ts @@ -21,6 +21,7 @@ import { parseCompletionShell, registerCompletionOption, renderCompletion, + renderZsh, type CompletionShell, } from "../src/completion.js"; import { ONE_SHOT_METHODS } from "@inspector/core/cli/handlers/method-types.js"; @@ -185,6 +186,14 @@ describe("collectCompletionFlags", () => { }); describe("shell helpers", () => { + it("escapes backslashes before colons in a zsh _describe name (CodeQL #78)", () => { + const out = renderZsh([ + { long: "--a\\b:c", takesValue: false, description: "Desc" }, + ]); + // Name --a\b:c → --a\\b\:c, then ":" and the description. + expect(out).toContain("'--a\\\\b\\:c:Desc'"); + }); + it("parseCompletionShell / isCompletionShell", () => { expect(isCompletionShell("zsh")).toBe(true); expect(isCompletionShell("csh")).toBe(false); diff --git a/clients/cli/src/completion.ts b/clients/cli/src/completion.ts index cbfb1482a..90cfdf4b7 100644 --- a/clients/cli/src/completion.ts +++ b/clients/cli/src/completion.ts @@ -213,9 +213,14 @@ complete -o default -F ${FUNCTION_NAME} ${COMPLETION_COMMAND} `; } -/** `name:description` for zsh `_describe`; colons in the name are escaped. */ +/** + * `name:description` for zsh `_describe`. `_describe` reads `\` as an escape + * and the first unescaped `:` as the separator, so backslashes in the name are + * escaped first, then colons (CodeQL #78). + */ function zshDescribeEntry(name: string, description: string): string { - return shQuote(`${name.replace(/:/g, "\\:")}:${description}`); + const escaped = name.replace(/\\/g, "\\\\").replace(/:/g, "\\:"); + return shQuote(`${escaped}:${description}`); } export function renderZsh(flags: readonly CompletionFlag[]): string { diff --git a/clients/mcpdo/__tests__/connection-stored-auth.test.ts b/clients/mcpdo/__tests__/connection-stored-auth.test.ts index b6895ab05..4f1596ef5 100644 --- a/clients/mcpdo/__tests__/connection-stored-auth.test.ts +++ b/clients/mcpdo/__tests__/connection-stored-auth.test.ts @@ -144,7 +144,7 @@ describe("connection stored-auth helpers", () => { hasRefreshToken: true, }); expect( - list.servers.find((s) => s.url.includes("example.com")), + list.servers.find((s) => s.url === "https://example.com/mcp"), ).toMatchObject({ hasTokens: true, hasRefreshToken: true }); expect(list.servers.find((s) => s.url.includes("other"))).toMatchObject({ hasTokens: true, @@ -190,7 +190,7 @@ describe("connection stored-auth helpers", () => { const list = await listStoredAuth(); expect( - list.servers.find((s) => s.url.includes("example.com")), + list.servers.find((s) => s.url === "https://example.com/mcp"), ).toMatchObject({ hasTokens: true, hasRefreshToken: true }); }); diff --git a/clients/mcpdo/src/daemon/run.ts b/clients/mcpdo/src/daemon/run.ts index bd7daa6dc..c34273d2a 100644 --- a/clients/mcpdo/src/daemon/run.ts +++ b/clients/mcpdo/src/daemon/run.ts @@ -7,6 +7,7 @@ import { DaemonServer } from "./server.js"; import { generateDaemonToken, getDaemonTokenFromEnv } from "./auth.js"; import { ensureDaemonDir } from "./paths.js"; import { disallowMemorySecretStoreFallback } from "@inspector/core/auth/node/secret-store-selection.js"; +import { awaitableError } from "@inspector/core/cli/utils/awaitable-log.js"; // Name the process `mcpdod` (Unix d-suffix convention) so `ps`/`pgrep`/`pkill` // see the daemon under a greppable name instead of a bare `node .../mcpdod.js`. @@ -44,8 +45,10 @@ async function main(): Promise { await server.start(); } -main().catch((error: unknown) => { +main().catch(async (error: unknown) => { const message = error instanceof Error ? error.message : String(error); - process.stderr.write(`mcpdo daemon: ${message}\n`); + // Exit only once the write has been performed: on a pipe or file stderr is + // asynchronous, and process.exit() would discard the diagnostic (#2638). + await awaitableError(`mcpdo daemon: ${message}\n`); process.exit(1); }); diff --git a/clients/tui/__tests__/RootsModal.test.tsx b/clients/tui/__tests__/RootsModal.test.tsx index 9ad5219f1..9a6249dcf 100644 --- a/clients/tui/__tests__/RootsModal.test.tsx +++ b/clients/tui/__tests__/RootsModal.test.tsx @@ -5,6 +5,14 @@ import type { Root } from "@modelcontextprotocol/client"; import type { InspectorClient } from "@inspector/core/mcp/index.js"; vi.mock("ink-form", () => import("./helpers/inkFormMock.js")); +// Passthrough spy: the modal's frame is empty under ink-testing-library (see +// below), so the redaction test asserts the error reached the display boundary. +vi.mock("../src/utils/errorText.js", async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, errorMessage: vi.fn(actual.errorMessage) }; +}); +import * as errorText from "../src/utils/errorText.js"; import { RootsModal, rootFromForm } from "../src/components/RootsModal.js"; @@ -156,6 +164,23 @@ describe("RootsModal", () => { expect(setRoots).toHaveBeenCalledTimes(1); }); + it("shows a failed save through the redacting display boundary (#2638)", async () => { + const failure = new Error( + "Request failed: https://auth.example/cb?code=s3cret&state=ok", + ); + const setRoots = vi.fn(async () => { + throw failure; + }); + const { stdin } = renderModal({ inspectorClient: fakeClient(setRoots) }); + await tick(); + stdin.write("x"); + await tick(); + expect(errorText.errorMessage).toHaveBeenCalledWith(failure); + expect(vi.mocked(errorText.errorMessage).mock.results.at(-1)?.value).toBe( + "Request failed: https://auth.example/cb?code=%5BREDACTED%5D&state=ok", + ); + }); + it("reports a non-Error failure", async () => { const setRoots = vi.fn(() => Promise.reject("nope")); const { stdin } = renderModal({ inspectorClient: fakeClient(setRoots) }); diff --git a/clients/tui/__tests__/SubscriptionsTab.test.tsx b/clients/tui/__tests__/SubscriptionsTab.test.tsx index 4af825d40..236551a87 100644 --- a/clients/tui/__tests__/SubscriptionsTab.test.tsx +++ b/clients/tui/__tests__/SubscriptionsTab.test.tsx @@ -155,6 +155,22 @@ describe("SubscriptionsTab", () => { expect(lastFrame()).toContain("does not support resource subscriptions"); }); + it("redacts URL query secrets in a surfaced failure (#2638)", async () => { + const client = fakeClient({ + subscribeToResource: vi.fn(async () => { + throw new Error( + "Request failed: https://auth.example/cb?code=s3cret&state=ok", + ); + }), + }); + const { stdin, lastFrame } = renderTab({ inspectorClient: client }); + stdin.write("\r"); + await tick(); + const frame = (lastFrame() ?? "").replace(/\s+/g, ""); + expect(frame).toContain("code=%5BREDACTED%5D&state=ok"); + expect(frame).not.toContain("s3cret"); + }); + it("surfaces a non-Error failure", async () => { const client = fakeClient({ subscribeToResource: vi.fn(() => Promise.reject("nope")), diff --git a/clients/tui/__tests__/TasksTab.test.tsx b/clients/tui/__tests__/TasksTab.test.tsx index 6984d887d..43d7f2c98 100644 --- a/clients/tui/__tests__/TasksTab.test.tsx +++ b/clients/tui/__tests__/TasksTab.test.tsx @@ -160,6 +160,21 @@ describe("TasksTab", () => { expect(lastFrame()).toContain("list failed"); }); + it("redacts URL query secrets in a surfaced failure (#2638)", async () => { + const { stdin, lastFrame } = renderTab({ + onRefresh: vi.fn(async () => { + throw new Error( + "Request failed: https://auth.example/cb?code=s3cret&state=ok", + ); + }), + }); + stdin.write("f"); + await tick(); + const frame = (lastFrame() ?? "").replace(/\s+/g, ""); + expect(frame).toContain("code=%5BREDACTED%5D&state=ok"); + expect(frame).not.toContain("s3cret"); + }); + it("surfaces a non-Error failure with no task selected", async () => { const { stdin, lastFrame } = renderTab({ tasks: [], diff --git a/clients/tui/src/components/RootsModal.tsx b/clients/tui/src/components/RootsModal.tsx index f503a63a7..1514329ce 100644 --- a/clients/tui/src/components/RootsModal.tsx +++ b/clients/tui/src/components/RootsModal.tsx @@ -18,6 +18,7 @@ import { Form, type FormStructure } from "ink-form"; import type { Root } from "@modelcontextprotocol/client"; import type { InspectorClient } from "@inspector/core/mcp/index.js"; import { useSelectableList } from "../hooks/useSelectableList.js"; +import { errorMessage } from "../utils/errorText.js"; export const ADD_ROOT_FORM: FormStructure = { title: "Add Root", @@ -89,7 +90,7 @@ export function RootsModal({ await inspectorClient.setRoots(next); setMode("list"); } catch (err) { - setError(err instanceof Error ? err.message : String(err)); + setError(errorMessage(err)); } finally { savingRef.current = false; setSaving(false); diff --git a/clients/tui/src/components/SubscriptionsTab.tsx b/clients/tui/src/components/SubscriptionsTab.tsx index d216604f9..6742df559 100644 --- a/clients/tui/src/components/SubscriptionsTab.tsx +++ b/clients/tui/src/components/SubscriptionsTab.tsx @@ -28,6 +28,7 @@ import { findNestedAuthError, } from "@inspector/core/auth/challenge.js"; import { useSelectableList } from "../hooks/useSelectableList.js"; +import { errorMessage } from "../utils/errorText.js"; import { resourceUpdateFeed, subscribableResources, @@ -116,7 +117,7 @@ export function SubscriptionsTab({ onAuthRecoveryRequired?.(authErr); return; } - setError(err instanceof Error ? err.message : String(err)); + setError(errorMessage(err)); } finally { inFlightRef.current = false; setPendingUri(null); diff --git a/clients/tui/src/components/TasksTab.tsx b/clients/tui/src/components/TasksTab.tsx index fa55a7122..6dbb94634 100644 --- a/clients/tui/src/components/TasksTab.tsx +++ b/clients/tui/src/components/TasksTab.tsx @@ -20,6 +20,7 @@ import type { CallToolResult, Task } from "@modelcontextprotocol/client"; import type { InspectorClient } from "@inspector/core/mcp/index.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import { useSelectableList } from "../hooks/useSelectableList.js"; +import { errorMessage } from "../utils/errorText.js"; /** Glyph and color per task status; unknown statuses fall back to gray. */ const STATUS_STYLE: Record = { @@ -47,10 +48,6 @@ export function hasTaskResult(status: string): boolean { return status === "completed" || status === "failed"; } -function errorMessage(err: unknown): string { - return err instanceof Error ? err.message : String(err); -} - interface TasksTabProps { tasks: Task[]; inspectorClient: InspectorClient | null; diff --git a/clients/web/src/test/core/auth/oauth-namespace-ledger.test.ts b/clients/web/src/test/core/auth/oauth-namespace-ledger.test.ts index 75fc7905b..ada92ca16 100644 --- a/clients/web/src/test/core/auth/oauth-namespace-ledger.test.ts +++ b/clients/web/src/test/core/auth/oauth-namespace-ledger.test.ts @@ -132,9 +132,10 @@ describe("recordNamespaceKeys", () => { it("does not rewrite the ledger when every key is already recorded", async () => { await recordNamespaceKeys(stateFile, store, NS1, [SERVER], [ISSUER]); const before = readFileSync(ledgerFile, "utf8"); - // A sentinel the rewrite would replace. - writeFileSync(ledgerFile, before.replace("{", "{ ")); + // A sentinel the rewrite would replace: one space after the opening brace. + writeFileSync(ledgerFile, before.replace(/^\{/, "{ ")); const sentinel = readFileSync(ledgerFile, "utf8"); + expect(sentinel).not.toBe(before); await recordNamespaceKeys(stateFile, store, NS1, [SERVER], [ISSUER]); expect(readFileSync(ledgerFile, "utf8")).toBe(sentinel); });