Merge pull request #2639 from modelcontextprotocol/v2/fix/2638-merge-review-findings

fix: v2.10.0 merge review findings — TUI error redaction, mcpdo daemon flush, CodeQL #78-#81

Author: cliffhall

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);

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 {

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 });
   });
 

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<void> {
   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);
 });

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<typeof import("../src/utils/errorText.js")>();
+  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) });

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")),

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: [],

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);

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);

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<string, { glyph: string; color: string }> = {
@@ -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;