Skip to content

Commit db3371b

Browse files
authored
Merge branch 'v2/main' into v2/chore/2621-dco-required-ruleset
2 parents 62b6c7a + b9a898a commit db3371b

12 files changed

Lines changed: 143 additions & 23 deletions

File tree

‎clients/cli/__tests__/completion.test.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,11 @@ import {
2121
parseCompletionShell,
2222
registerCompletionOption,
2323
renderCompletion,
24+
renderZsh,
2425
type CompletionShell,
2526
} from "../src/completion.js";
2627
import { ONE_SHOT_METHODS } from "@inspector/core/cli/handlers/method-types.js";
28+
import { CATALOG_WRITE_METHODS } from "../src/handlers/servers-write.js";
2729

2830
async function script(shell: CompletionShell): Promise<string> {
2931
const result = await runCli(["--completion", shell]);
@@ -94,6 +96,28 @@ describe("--completion", () => {
9496
expect(out).toContain(method);
9597
}
9698
});
99+
100+
it("offers the catalog write methods the CLI implements (#2629)", async () => {
101+
// Read from servers-write itself, not from CATALOG_METHODS, so a write
102+
// method the completion list forgets fails here.
103+
expect(CATALOG_METHODS).toEqual(
104+
expect.arrayContaining([...CATALOG_WRITE_METHODS]),
105+
);
106+
const out = await script("bash");
107+
for (const method of CATALOG_WRITE_METHODS) {
108+
expect(out).toContain(method);
109+
}
110+
});
111+
112+
it("offers no --method the CLI would reject as unsupported", async () => {
113+
for (const method of VALUE_CHOICES["--method"]!) {
114+
const result = await runCli(["--method", method]);
115+
expect(result.stderr, method).not.toContain("Unsupported method");
116+
}
117+
// Control: an unknown method is rejected by the same check.
118+
const bogus = await runCli(["--method", "servers/bogus"]);
119+
expect(bogus.stderr).toContain("Unsupported method: servers/bogus");
120+
});
97121
});
98122

99123
describe("collectCompletionFlags", () => {
@@ -162,6 +186,14 @@ describe("collectCompletionFlags", () => {
162186
});
163187

164188
describe("shell helpers", () => {
189+
it("escapes backslashes before colons in a zsh _describe name (CodeQL #78)", () => {
190+
const out = renderZsh([
191+
{ long: "--a\\b:c", takesValue: false, description: "Desc" },
192+
]);
193+
// Name --a\b:c → --a\\b\:c, then ":" and the description.
194+
expect(out).toContain("'--a\\\\b\\:c:Desc'");
195+
});
196+
165197
it("parseCompletionShell / isCompletionShell", () => {
166198
expect(isCompletionShell("zsh")).toBe(true);
167199
expect(isCompletionShell("csh")).toBe(false);
@@ -227,6 +259,18 @@ describe.skipIf(!hasShell("bash"))("bash script", () => {
227259
expect(
228260
await complete(["mcp-inspector", "--cli", "--transport", ""]),
229261
).toEqual(["stdio", "sse", "http"]);
262+
expect(
263+
await complete(["mcp-inspector", "--cli", "--method", "servers/"]),
264+
).toEqual([
265+
"servers/list",
266+
"servers/show",
267+
"servers/add",
268+
"servers/edit",
269+
"servers/remove",
270+
]);
271+
expect(
272+
await complete(["mcp-inspector", "--cli", "--output-format", ""]),
273+
).toEqual(["raw", "json"]);
230274
// A free-form value offers nothing (the shell falls back to files).
231275
expect(
232276
await complete(["mcp-inspector", "--cli", "--tool-name", "--"]),

‎clients/cli/src/cli.ts‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import { Command } from "commander";
22
import {
3+
CATALOG_METHODS,
34
emitCompletionIfRequested,
5+
isCatalogMethod,
46
registerCompletionOption,
57
} from "./completion.js";
68
type McpResponse = Record<string, unknown>;
@@ -21,7 +23,6 @@ import {
2123
showServerEntry,
2224
} from "@inspector/core/cli/handlers/servers-list.js";
2325
import {
24-
CATALOG_WRITE_METHODS,
2526
isCatalogWriteMethod,
2627
runCatalogWrite,
2728
} from "./handlers/servers-write.js";
@@ -1268,13 +1269,9 @@ async function parseArgs(argv?: string[]): Promise<ParseResult> {
12681269
"Method is required. Use --method to specify the method to invoke.",
12691270
);
12701271
}
1271-
const isCatalogMethod =
1272-
options.method === "servers/list" ||
1273-
options.method === "servers/show" ||
1274-
isCatalogWriteMethod(options.method);
1275-
if (!isCatalogMethod && !isOneShotMethod(options.method)) {
1272+
if (!isCatalogMethod(options.method) && !isOneShotMethod(options.method)) {
12761273
throw new Error(
1277-
`Unsupported method: ${options.method}. Supported --cli methods: ${ONE_SHOT_METHODS.join(", ")}, servers/list, servers/show, ${CATALOG_WRITE_METHODS.join(", ")}.`,
1274+
`Unsupported method: ${options.method}. Supported --cli methods: ${[...ONE_SHOT_METHODS, ...CATALOG_METHODS].join(", ")}.`,
12781275
);
12791276
}
12801277
if (options.rename !== undefined && options.method !== "servers/edit") {

‎clients/cli/src/completion.ts‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ import type { Command, Option } from "commander";
2525
import { LoggingLevelSchema } from "@modelcontextprotocol/core";
2626
import { ONE_SHOT_METHODS } from "@inspector/core/cli/handlers/method-types.js";
2727
import { awaitableLog } from "@inspector/core/cli/utils/awaitable-log.js";
28+
import { OUTPUT_FILE_FORMATS } from "@inspector/core/cli/handlers/output-file.js";
29+
import { CATALOG_WRITE_METHODS } from "./handlers/servers-write.js";
2830

2931
export const COMPLETION_SHELLS = ["bash", "zsh", "fish"] as const;
3032
export type CompletionShell = (typeof COMPLETION_SHELLS)[number];
@@ -39,8 +41,20 @@ const MODE_FLAGS: readonly CompletionFlag[] = [
3941
{ long: "--tui", takesValue: false, description: "Run the terminal UI" },
4042
];
4143

42-
/** Catalog-only methods `parseArgs` accepts alongside `ONE_SHOT_METHODS`. */
43-
export const CATALOG_METHODS = ["servers/list", "servers/show"] as const;
44+
/**
45+
* Catalog-only methods `parseArgs` accepts alongside `ONE_SHOT_METHODS` —
46+
* the reads plus the writes `servers-write` implements. `parseArgs` validates
47+
* `--method` against this same list, so the two cannot drift (#2629).
48+
*/
49+
export const CATALOG_METHODS = [
50+
"servers/list",
51+
"servers/show",
52+
...CATALOG_WRITE_METHODS,
53+
] as const;
54+
55+
export function isCatalogMethod(method: string): boolean {
56+
return (CATALOG_METHODS as readonly string[]).includes(method);
57+
}
4458

4559
/**
4660
* Finite value sets for flags whose values the CLI validates in a custom
@@ -51,6 +65,7 @@ export const VALUE_CHOICES: Readonly<Record<string, readonly string[]>> = {
5165
"--transport": ["stdio", "sse", "http"],
5266
"--log-level": Object.values(LoggingLevelSchema.enum),
5367
"--format": ["text", "json"],
68+
"--output-format": OUTPUT_FILE_FORMATS,
5469
"--protocol-era": ["legacy", "auto", "modern"],
5570
"--completion": COMPLETION_SHELLS,
5671
};
@@ -198,9 +213,14 @@ complete -o default -F ${FUNCTION_NAME} ${COMPLETION_COMMAND}
198213
`;
199214
}
200215

201-
/** `name:description` for zsh `_describe`; colons in the name are escaped. */
216+
/**
217+
* `name:description` for zsh `_describe`. `_describe` reads `\` as an escape
218+
* and the first unescaped `:` as the separator, so backslashes in the name are
219+
* escaped first, then colons (CodeQL #78).
220+
*/
202221
function zshDescribeEntry(name: string, description: string): string {
203-
return shQuote(`${name.replace(/:/g, "\\:")}:${description}`);
222+
const escaped = name.replace(/\\/g, "\\\\").replace(/:/g, "\\:");
223+
return shQuote(`${escaped}:${description}`);
204224
}
205225

206226
export function renderZsh(flags: readonly CompletionFlag[]): string {

‎clients/mcpdo/__tests__/connection-stored-auth.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ describe("connection stored-auth helpers", () => {
144144
hasRefreshToken: true,
145145
});
146146
expect(
147-
list.servers.find((s) => s.url.includes("example.com")),
147+
list.servers.find((s) => s.url === "https://example.com/mcp"),
148148
).toMatchObject({ hasTokens: true, hasRefreshToken: true });
149149
expect(list.servers.find((s) => s.url.includes("other"))).toMatchObject({
150150
hasTokens: true,
@@ -190,7 +190,7 @@ describe("connection stored-auth helpers", () => {
190190

191191
const list = await listStoredAuth();
192192
expect(
193-
list.servers.find((s) => s.url.includes("example.com")),
193+
list.servers.find((s) => s.url === "https://example.com/mcp"),
194194
).toMatchObject({ hasTokens: true, hasRefreshToken: true });
195195
});
196196

‎clients/mcpdo/src/daemon/run.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { DaemonServer } from "./server.js";
77
import { generateDaemonToken, getDaemonTokenFromEnv } from "./auth.js";
88
import { ensureDaemonDir } from "./paths.js";
99
import { disallowMemorySecretStoreFallback } from "@inspector/core/auth/node/secret-store-selection.js";
10+
import { awaitableError } from "@inspector/core/cli/utils/awaitable-log.js";
1011

1112
// Name the process `mcpdod` (Unix d-suffix convention) so `ps`/`pgrep`/`pkill`
1213
// see the daemon under a greppable name instead of a bare `node .../mcpdod.js`.
@@ -44,8 +45,10 @@ async function main(): Promise<void> {
4445
await server.start();
4546
}
4647

47-
main().catch((error: unknown) => {
48+
main().catch(async (error: unknown) => {
4849
const message = error instanceof Error ? error.message : String(error);
49-
process.stderr.write(`mcpdo daemon: ${message}\n`);
50+
// Exit only once the write has been performed: on a pipe or file stderr is
51+
// asynchronous, and process.exit() would discard the diagnostic (#2638).
52+
await awaitableError(`mcpdo daemon: ${message}\n`);
5053
process.exit(1);
5154
});

‎clients/tui/__tests__/RootsModal.test.tsx‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,14 @@ import type { Root } from "@modelcontextprotocol/client";
55
import type { InspectorClient } from "@inspector/core/mcp/index.js";
66

77
vi.mock("ink-form", () => import("./helpers/inkFormMock.js"));
8+
// Passthrough spy: the modal's frame is empty under ink-testing-library (see
9+
// below), so the redaction test asserts the error reached the display boundary.
10+
vi.mock("../src/utils/errorText.js", async (importOriginal) => {
11+
const actual =
12+
await importOriginal<typeof import("../src/utils/errorText.js")>();
13+
return { ...actual, errorMessage: vi.fn(actual.errorMessage) };
14+
});
15+
import * as errorText from "../src/utils/errorText.js";
816

917
import { RootsModal, rootFromForm } from "../src/components/RootsModal.js";
1018

@@ -156,6 +164,23 @@ describe("RootsModal", () => {
156164
expect(setRoots).toHaveBeenCalledTimes(1);
157165
});
158166

167+
it("shows a failed save through the redacting display boundary (#2638)", async () => {
168+
const failure = new Error(
169+
"Request failed: https://auth.example/cb?code=s3cret&state=ok",
170+
);
171+
const setRoots = vi.fn(async () => {
172+
throw failure;
173+
});
174+
const { stdin } = renderModal({ inspectorClient: fakeClient(setRoots) });
175+
await tick();
176+
stdin.write("x");
177+
await tick();
178+
expect(errorText.errorMessage).toHaveBeenCalledWith(failure);
179+
expect(vi.mocked(errorText.errorMessage).mock.results.at(-1)?.value).toBe(
180+
"Request failed: https://auth.example/cb?code=%5BREDACTED%5D&state=ok",
181+
);
182+
});
183+
159184
it("reports a non-Error failure", async () => {
160185
const setRoots = vi.fn(() => Promise.reject("nope"));
161186
const { stdin } = renderModal({ inspectorClient: fakeClient(setRoots) });

‎clients/tui/__tests__/SubscriptionsTab.test.tsx‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,22 @@ describe("SubscriptionsTab", () => {
155155
expect(lastFrame()).toContain("does not support resource subscriptions");
156156
});
157157

158+
it("redacts URL query secrets in a surfaced failure (#2638)", async () => {
159+
const client = fakeClient({
160+
subscribeToResource: vi.fn(async () => {
161+
throw new Error(
162+
"Request failed: https://auth.example/cb?code=s3cret&state=ok",
163+
);
164+
}),
165+
});
166+
const { stdin, lastFrame } = renderTab({ inspectorClient: client });
167+
stdin.write("\r");
168+
await tick();
169+
const frame = (lastFrame() ?? "").replace(/\s+/g, "");
170+
expect(frame).toContain("code=%5BREDACTED%5D&state=ok");
171+
expect(frame).not.toContain("s3cret");
172+
});
173+
158174
it("surfaces a non-Error failure", async () => {
159175
const client = fakeClient({
160176
subscribeToResource: vi.fn(() => Promise.reject("nope")),

‎clients/tui/__tests__/TasksTab.test.tsx‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,21 @@ describe("TasksTab", () => {
160160
expect(lastFrame()).toContain("list failed");
161161
});
162162

163+
it("redacts URL query secrets in a surfaced failure (#2638)", async () => {
164+
const { stdin, lastFrame } = renderTab({
165+
onRefresh: vi.fn(async () => {
166+
throw new Error(
167+
"Request failed: https://auth.example/cb?code=s3cret&state=ok",
168+
);
169+
}),
170+
});
171+
stdin.write("f");
172+
await tick();
173+
const frame = (lastFrame() ?? "").replace(/\s+/g, "");
174+
expect(frame).toContain("code=%5BREDACTED%5D&state=ok");
175+
expect(frame).not.toContain("s3cret");
176+
});
177+
163178
it("surfaces a non-Error failure with no task selected", async () => {
164179
const { stdin, lastFrame } = renderTab({
165180
tasks: [],

‎clients/tui/src/components/RootsModal.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import { Form, type FormStructure } from "ink-form";
1818
import type { Root } from "@modelcontextprotocol/client";
1919
import type { InspectorClient } from "@inspector/core/mcp/index.js";
2020
import { useSelectableList } from "../hooks/useSelectableList.js";
21+
import { errorMessage } from "../utils/errorText.js";
2122

2223
export const ADD_ROOT_FORM: FormStructure = {
2324
title: "Add Root",
@@ -89,7 +90,7 @@ export function RootsModal({
8990
await inspectorClient.setRoots(next);
9091
setMode("list");
9192
} catch (err) {
92-
setError(err instanceof Error ? err.message : String(err));
93+
setError(errorMessage(err));
9394
} finally {
9495
savingRef.current = false;
9596
setSaving(false);

‎clients/tui/src/components/SubscriptionsTab.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
findNestedAuthError,
2929
} from "@inspector/core/auth/challenge.js";
3030
import { useSelectableList } from "../hooks/useSelectableList.js";
31+
import { errorMessage } from "../utils/errorText.js";
3132
import {
3233
resourceUpdateFeed,
3334
subscribableResources,
@@ -116,7 +117,7 @@ export function SubscriptionsTab({
116117
onAuthRecoveryRequired?.(authErr);
117118
return;
118119
}
119-
setError(err instanceof Error ? err.message : String(err));
120+
setError(errorMessage(err));
120121
} finally {
121122
inFlightRef.current = false;
122123
setPendingUri(null);

0 commit comments

Comments
 (0)