From 2bb98b0639ad5c49df08cd7de0b39c840d65e7b0 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 16:38:24 -0700 Subject: [PATCH 01/13] feat(cli): catalog listing, auth flags, and OAuth browser UX Add servers/list and servers/show, --relogin / --stored-auth-only, browser OAuth navigation with OSC 8 links, and extract method handlers under clients/cli/src/handlers. Tracks #1781. Co-authored-by: Cursor --- clients/cli/README.md | 31 +- clients/cli/__tests__/README.md | 72 --- .../clear-stored-auth-for-relogin.test.ts | 68 +++ .../__tests__/cli-oauth-navigation.test.ts | 114 +++++ clients/cli/__tests__/cliOAuth.test.ts | 71 +++ clients/cli/__tests__/emit-result.test.ts | 35 ++ clients/cli/__tests__/format-output.test.ts | 50 ++ clients/cli/__tests__/metadata.test.ts | 37 ++ clients/cli/__tests__/method-types.test.ts | 36 ++ clients/cli/__tests__/methods.test.ts | 33 +- clients/cli/__tests__/open-url.test.ts | 28 ++ .../__tests__/programmatic-ergonomics.test.ts | 46 ++ .../cli/__tests__/run-method-mocks.test.ts | 232 +++++++++ clients/cli/__tests__/run-method.test.ts | 249 +++++++++ clients/cli/__tests__/servers-list.test.ts | 286 +++++++++++ clients/cli/__tests__/style.test.ts | 40 ++ clients/cli/package-lock.json | 149 ++++++ clients/cli/package.json | 1 + .../cli/src/clear-stored-auth-for-relogin.ts | 27 + clients/cli/src/cli-oauth-navigation.ts | 46 ++ clients/cli/src/cli.ts | 476 ++++-------------- clients/cli/src/cliOAuth.ts | 36 ++ clients/cli/src/handlers/collect-app-info.ts | 35 ++ clients/cli/src/handlers/connect-timeout.ts | 35 ++ clients/cli/src/handlers/consume-outcome.ts | 37 ++ clients/cli/src/handlers/emit-result.ts | 46 ++ clients/cli/src/handlers/format-output.ts | 18 + clients/cli/src/handlers/method-types.ts | 116 +++++ clients/cli/src/handlers/run-method.ts | 310 ++++++++++++ clients/cli/src/handlers/servers-list.ts | 212 ++++++++ clients/cli/src/open-url.ts | 6 + clients/cli/src/style.ts | 76 +++ clients/cli/tsup.config.ts | 4 +- 33 files changed, 2585 insertions(+), 473 deletions(-) create mode 100644 clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts create mode 100644 clients/cli/__tests__/cli-oauth-navigation.test.ts create mode 100644 clients/cli/__tests__/format-output.test.ts create mode 100644 clients/cli/__tests__/method-types.test.ts create mode 100644 clients/cli/__tests__/open-url.test.ts create mode 100644 clients/cli/__tests__/run-method-mocks.test.ts create mode 100644 clients/cli/__tests__/run-method.test.ts create mode 100644 clients/cli/__tests__/servers-list.test.ts create mode 100644 clients/cli/__tests__/style.test.ts create mode 100644 clients/cli/src/clear-stored-auth-for-relogin.ts create mode 100644 clients/cli/src/cli-oauth-navigation.ts create mode 100644 clients/cli/src/handlers/collect-app-info.ts create mode 100644 clients/cli/src/handlers/connect-timeout.ts create mode 100644 clients/cli/src/handlers/consume-outcome.ts create mode 100644 clients/cli/src/handlers/emit-result.ts create mode 100644 clients/cli/src/handlers/format-output.ts create mode 100644 clients/cli/src/handlers/method-types.ts create mode 100644 clients/cli/src/handlers/run-method.ts create mode 100644 clients/cli/src/handlers/servers-list.ts create mode 100644 clients/cli/src/open-url.ts create mode 100644 clients/cli/src/style.ts diff --git a/clients/cli/README.md b/clients/cli/README.md index 7c49d3a35..8c75df3ef 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -1,16 +1,16 @@ # MCP Inspector CLI Client -The CLI mode enables programmatic interaction with MCP servers from the command line. It is ideal for scripting, automation, continuous integration, and establishing an efficient feedback loop with AI coding assistants. +CLI for the Inspector: connect, run a `--method`, disconnect. Invoked as `mcp-inspector --cli`. ## Running the CLI -You can run the CLI client directly via `npx`: +You can run the CLI via `npx`: ```bash npx @modelcontextprotocol/inspector --cli node build/index.js ``` -The CLI mode supports operations across tools, resources, and prompts, returning structured JSON output. +Supports tools, resources, and prompts (plus `--method servers/list` / `servers/show` for catalog entries without connecting). ### Examples @@ -98,7 +98,7 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | Option | Description | | ----------------------------- | ----------------------------------------------------------------------------------------- | -| `--method ` | MCP method to invoke. Supports `initialize` (connect-only probe → `{serverInfo, protocolVersion, capabilities, instructions}`), `tools/list`, `tools/call`, `resources/list`, `resources/read`, `resources/templates/list`, `prompts/list`, `prompts/get`, `logging/setLevel`. | +| `--method ` | MCP method to invoke. Supports `initialize` (connect-only probe → `{serverInfo, protocolVersion, capabilities, instructions}`), `tools/list`, `tools/call`, `resources/list`, `resources/read`, `resources/templates/list`, `prompts/list`, `prompts/get`, `logging/setLevel`, plus catalog-only `servers/list` / `servers/show` (no MCP connect). Stream / session-only methods (e.g. `logging/tail`) are rejected. | | `--tool-name ` | Tool name (for `tools/call`). | | `--tool-arg ` | Tool argument; repeat for multiple. Use `key='{"json":true}'` for JSON. Values are coerced (JSON-parsed, so `count=1` becomes a number). | | `--tool-args-json ` | Tool arguments as a single JSON object (e.g. `'{"zip":"10001"}'`). Passed verbatim — no `key=value` coercion, so `"012"` stays a string. Mutually exclusive with `--tool-arg`. | @@ -111,6 +111,8 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--connect-timeout ` | Connection timeout in ms. Defaults to `15000` for ad-hoc `--server-url`/target runs (so a black-holed host fails fast) and to the file-level timeout for `--catalog`/`--config` runs. `0` disables the timeout. | | `--app-info` | Probe a tool's MCP App UI metadata without invoking it. With `--method tools/call --tool-name `: prints one JSON line (`hasApp`, `resourceUri`, `csp`, `permissions`, `domain`, …) and exits `0` if the tool has an app or `2` (`no_app`) if not. With `--method tools/list`: emits NDJSON — one app-info line per tool over a single connection. | | `--format ` | Output format. `text` (default) pretty-prints the result. `json` emits a single JSON object on stdout (`{ "result": … }`, plus `{ "appInfo": … }` as a sibling key for App tools) with no banners, so the whole output pipes cleanly into `jq`. | +| `--relogin` | Ignore any stored OAuth for this run’s server URL before connect; interactive login still only runs if the server requires auth. No-op for stdio (no URL-keyed store entry). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth`. | +| `--stored-auth-only` | Never start interactive OAuth / step-up; use the shared store if present, otherwise fail with `auth_required`. | #### App probing (`--app-info`) and machine-readable output (`--format json`) @@ -140,10 +142,12 @@ A `tools/call` that returns `isError:true` still prints its payload but exits `5 ### CLI-specific (OAuth for HTTP servers) -The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:6276/oauth/callback` by default). On connect **401** or mid-session interactive auth (re-login / step-up), it: +The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:6276/oauth/callback` by default). + +**CLI (`mcp-inspector --cli`):** on connect **401** or mid-session interactive auth (re-login / step-up), it: 1. Starts the callback listener on `--callback-url` (or `MCP_OAUTH_CALLBACK_URL`) -2. Prints the authorization URL to the console (`ConsoleNavigation`) +2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** on a TTY; non-TTY / CI prints a plain URL only and never launches a browser 3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC **Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues; **N** exits with an error. EMA step-up re-mints silently (no prompt). @@ -256,17 +260,16 @@ While the Web Client provides a rich visual interface, the CLI is designed for: Like the other clients, the CLI self-validates from its own folder: ```bash -npm run validate # format:check && lint && test:coverage +npm run validate # format:check && lint && test (fast; no coverage gate) npm test # build test-servers + binary, then run all tests -npm run test:coverage # build + tests under the per-file coverage gate +npm run test:coverage # build + tests under the per-file ≥90 coverage gate ``` -The CLI's `test:coverage` **builds the binary first** (its out-of-process -`e2e.test.ts` spawns it, so it must run against a fresh build). `validate` -therefore folds the build into `test:coverage` rather than repeating it — it is -`format:check && lint && test:coverage`, with no separate `build` step (the -other clients, whose tests don't spawn their bundle, keep an explicit `build`). -The repo-root `validate:cli` just delegates here. +The CLI's `test` / `test:coverage` **build the binary first** (out-of-process +`e2e.test.ts` spawns it). `validate` is `format:check && lint && test` with no +separate `build` step (`pretest` builds). Repo-root `validate:cli` delegates +here; the coverage gate is `npm run coverage` / `coverage:cli` (also in +`npm run ci`), matching AGENTS.md. Tests run the CLI **in-process** (importing `runCli()`) so `src/` is measured under coverage, with a thin out-of-process spawn layer for the real binary. See diff --git a/clients/cli/__tests__/README.md b/clients/cli/__tests__/README.md index 09aec386f..ca7793506 100644 --- a/clients/cli/__tests__/README.md +++ b/clients/cli/__tests__/README.md @@ -1,73 +1 @@ # CLI Tests - -## Running Tests - -```bash -# Run all tests (pretest builds test-servers + the CLI binary) -npm test - -# Run all tests under the per-file coverage gate -npm run test:coverage - -# Run in watch mode (useful for test file changes) -npm run test:watch - -# Run specific test file -npm run test:cli # cli.test.ts -npm run test:cli-tools # tools.test.ts -npm run test:cli-headers # headers.test.ts -npm run test:cli-metadata # metadata.test.ts -npx vitest run oauth-interactive.test.ts cliOAuth.test.ts # OAuth interactive smoke parity -``` - -## How the CLI is exercised - -Most tests run the CLI **in-process**: `helpers/cli-runner.ts` imports `runCli()` -from `../src/cli.ts` and invokes it directly, capturing `process.stdout`/`stderr` -and mapping thrown errors to an exit code. This is what lets vitest's v8 coverage -instrument `clients/cli/src` (#1484). Because `commander` uses `.exitOverride()`, -a parse/usage error throws instead of calling `process.exit` and tearing down the -worker. - -`e2e.test.ts` is the deliberately thin **out-of-process** layer: it spawns the -**built** `build/index.js` as a real subprocess to cover the shebang, the -`index.ts` `isMain` bootstrap, and the actual `process.exit` codes — the only -parts the in-process runner can't reach. (`scripts/smoke-cli.mjs` at the repo -root provides a further end-to-end check of the binary.) - -## Test Files - -- `cli.test.ts` - Basic CLI functionality: CLI mode, environment variables, config files, resources, prompts, logging, transport types -- `tools.test.ts` - Tool-related tests: Tool discovery, JSON argument parsing, error handling, prompts -- `headers.test.ts` - Header parsing and validation -- `metadata.test.ts` - Metadata functionality: General metadata, tool-specific metadata, parsing, merging, validation -- `methods.test.ts` - Method/option-validation paths not covered elsewhere (resource templates, missing/invalid options, the `--` target separator) -- `error-handler.test.ts` - The binary's `handleError` error sink, exercised in-process with `process.exit` stubbed -- `oauth-runner.test.ts` - OAuth flag wiring (`--client-config`, `--callback-url`, CIMD overrides) -- `cliOAuth.test.ts` - Unit tests for `cliOAuth.ts` (step-up confirm, helper wiring, retry) -- `oauth-interactive.test.ts` - **Integration** smoke parity for CLI interactive OAuth: connect-time callback server + step-up **y/N** against composable `TestServerHttp` (auto-completes authorize URL programmatically; not a subprocess binary e2e) -- `e2e.test.ts` - Out-of-process spawn of the built binary (exit codes + boot; no OAuth) - -## Helpers - -The `helpers/` directory contains shared utilities: - -- `cli-runner.ts` - Invokes `runCli()` in-process and captures stdout/stderr + exit code -- `assertions.ts` - Custom assertion helpers for CLI output validation -- `fixtures.ts` - Test config/catalog file generators and temporary directory management - -HTTP/SSE test servers (for transport tests) come from the -`@modelcontextprotocol/inspector-test-server` package (`createTestServerHttp`), -and the bundled stdio server is launched via `getTestMcpServerCommand()`. - -## Notes - -- Test files run in separate forked processes (`pool: 'forks'`), so the in-process - runner's `process.std*.write` patching never overlaps across files; tests within - a file run sequentially -- Config files use `crypto.randomUUID()` for uniqueness -- HTTP/SSE servers use dynamic port allocation to avoid conflicts -- Coverage is enforced per-file (lines ≥ 90, statements ≥ 85, functions ≥ 80, - branches ≥ 50). `src/index.ts` (the binary bootstrap) is excluded because it - only runs in the spawned binary, which coverage can't instrument -- All tests use the built-in MCP test servers — no external/registry dependencies diff --git a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts new file mode 100644 index 000000000..0e4ecee93 --- /dev/null +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -0,0 +1,68 @@ +import { describe, it, expect, afterEach } from "vitest"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { + getStateFilePath, + resetNodeOAuthStorageCache, +} from "@inspector/core/auth/node/storage-node.js"; +import { clearStoredAuthForRelogin } from "../src/clear-stored-auth-for-relogin.js"; + +describe("clearStoredAuthForRelogin", () => { + let dir: string | undefined; + let prevPath: string | undefined; + + afterEach(() => { + if (prevPath === undefined) + delete process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + else process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = prevPath; + resetNodeOAuthStorageCache(); + if (dir) { + fs.rmSync(dir, { recursive: true, force: true }); + dir = undefined; + } + }); + + it("no-ops when serverUrl is missing or blank", async () => { + await clearStoredAuthForRelogin(undefined); + await clearStoredAuthForRelogin(" "); + }); + + it("clears a URL-keyed entry and tolerates non-URL keys", async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-")); + const file = path.join(dir, "oauth.json"); + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + "https://example.com/mcp": { + tokens: { access_token: "a", token_type: "Bearer" }, + }, + "not a url": { + tokens: { access_token: "b", token_type: "Bearer" }, + }, + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + expect(getStateFilePath()).toBe(file); + + await clearStoredAuthForRelogin("https://example.com/mcp"); + let blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://example.com/mcp"]).toBeUndefined(); + expect(blob.servers["not a url"]).toBeDefined(); + + // normalizeServerUrl catch path — clears under the raw key + await clearStoredAuthForRelogin("not a url"); + blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["not a url"]).toBeUndefined(); + }); +}); diff --git a/clients/cli/__tests__/cli-oauth-navigation.test.ts b/clients/cli/__tests__/cli-oauth-navigation.test.ts new file mode 100644 index 000000000..4cc9a2dbf --- /dev/null +++ b/clients/cli/__tests__/cli-oauth-navigation.test.ts @@ -0,0 +1,114 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { createCliOAuthNavigation } from "../src/cli-oauth-navigation.js"; +import { openUrl } from "../src/open-url.js"; + +vi.mock("../src/open-url.js", () => ({ + openUrl: vi.fn().mockResolvedValue(undefined), +})); + +const openUrlMock = vi.mocked(openUrl); + +describe("createCliOAuthNavigation", () => { + afterEach(() => { + openUrlMock.mockClear(); + openUrlMock.mockResolvedValue(undefined); + }); + + it("prints OSC 8 link and opens browser on a TTY", async () => { + const lines: string[] = []; + const openBrowser = vi.fn().mockResolvedValue(undefined); + const nav = createCliOAuthNavigation({ + isTTY: true, + // Force ANSI on even when the test runner exports NO_COLOR. + noColorEnv: "", + write: (line) => lines.push(line), + openBrowser, + }); + const url = new URL("https://as.example/authorize?x=1"); + nav.navigateToAuthorization(url); + await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce()); + expect(lines.join("")).toContain("Please navigate to:"); + expect(lines.join("")).toContain( + "\u001b]8;;https://as.example/authorize?x=1\u0007", + ); + expect(openBrowser).toHaveBeenCalledWith( + "https://as.example/authorize?x=1", + ); + expect(openUrlMock).not.toHaveBeenCalled(); + }); + + it("prints a plain URL and does not open a browser when not a TTY", async () => { + const lines: string[] = []; + const openBrowser = vi.fn(); + const nav = createCliOAuthNavigation({ + isTTY: false, + write: (line) => lines.push(line), + openBrowser, + }); + nav.navigateToAuthorization(new URL("https://as.example/authorize")); + await vi.waitFor(() => expect(lines.length).toBe(1)); + expect(lines.join("")).toBe( + "Please navigate to: https://as.example/authorize\n", + ); + expect(lines.join("")).not.toContain("\u001b]8;;"); + expect(openBrowser).not.toHaveBeenCalled(); + expect(openUrlMock).not.toHaveBeenCalled(); + }); + + it("skips OSC 8 when NO_COLOR is set but still opens on a TTY", async () => { + const lines: string[] = []; + const openBrowser = vi.fn().mockResolvedValue(undefined); + const nav = createCliOAuthNavigation({ + isTTY: true, + noColorEnv: "1", + write: (line) => lines.push(line), + openBrowser, + }); + nav.navigateToAuthorization(new URL("https://as.example/a")); + await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce()); + expect(lines.join("")).toBe("Please navigate to: https://as.example/a\n"); + expect(lines.join("")).not.toContain("\u001b]8;;"); + }); + + it("swallows browser-open failures after printing the URL", async () => { + const lines: string[] = []; + const openBrowser = vi.fn().mockRejectedValue(new Error("no browser")); + const nav = createCliOAuthNavigation({ + isTTY: true, + noColorEnv: "1", + write: (line) => lines.push(line), + openBrowser, + }); + nav.navigateToAuthorization(new URL("https://as.example/a")); + await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce()); + expect(lines.join("")).toContain("Please navigate to:"); + }); + + it("writes to stderr and uses openUrl by default on a TTY", async () => { + const writeSpy = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); + const ttyDesc = Object.getOwnPropertyDescriptor(process.stderr, "isTTY"); + Object.defineProperty(process.stderr, "isTTY", { + configurable: true, + get: () => true, + }); + try { + const nav = createCliOAuthNavigation({ + noColorEnv: "1", + }); + nav.navigateToAuthorization(new URL("https://as.example/default")); + await vi.waitFor(() => expect(openUrlMock).toHaveBeenCalledOnce()); + expect(openUrlMock).toHaveBeenCalledWith("https://as.example/default"); + const written = writeSpy.mock.calls.map((c) => String(c[0])).join(""); + expect(written).toContain( + "Please navigate to: https://as.example/default", + ); + } finally { + writeSpy.mockRestore(); + if (ttyDesc) { + Object.defineProperty(process.stderr, "isTTY", ttyDesc); + } + } + }); +}); diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index f9d191539..f075f13ca 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -453,5 +453,76 @@ describe("cliOAuth", () => { ), ).rejects.toThrow("nope (401)"); }); + + it("fails with AUTH_REQUIRED under --stored-auth-only without interactive OAuth", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const connect = vi.fn().mockRejectedValue( + new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { + reason: "token_expired", + }), + ); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), + }; + + await expect( + connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + { storedAuthOnly: true }, + ), + ).rejects.toMatchObject({ exitCode: 3 }); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("fails stored-auth-only on plain unauthorized errors", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const connect = vi + .fn() + .mockRejectedValue(new Error("Connection failed for server (401)")); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }; + + await expect( + connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + { storedAuthOnly: true }, + ), + ).rejects.toMatchObject({ exitCode: 3 }); + expect(runSpy).not.toHaveBeenCalled(); + }); + }); + + it("withCliAuthRecoveryRetry respects storedAuthOnly", async () => { + const fn = vi.fn().mockRejectedValue( + new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { + reason: "token_expired", + }), + ); + await expect( + withCliAuthRecoveryRetry( + { + checkAuthChallengeSatisfied: vi.fn(), + } as never, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + fn, + undefined, + { storedAuthOnly: true }, + ), + ).rejects.toMatchObject({ exitCode: 3 }); }); }); diff --git a/clients/cli/__tests__/emit-result.test.ts b/clients/cli/__tests__/emit-result.test.ts index df4e8fc6f..dfa16b379 100644 --- a/clients/cli/__tests__/emit-result.test.ts +++ b/clients/cli/__tests__/emit-result.test.ts @@ -31,6 +31,22 @@ describe("emitResult", () => { process.stdout.write = originalWrite; }); + it("uses a default appInfo shell when --app-info has no collected info", async () => { + const promise = emitResult(undefined as never, undefined, { + appInfo: true, + toolName: "missing-app", + format: "text", + }); + await expect(promise).rejects.toBeInstanceOf(CliExitCodeError); + await promise.catch((e: CliExitCodeError) => { + expect(e.exitCode).toBe(2); + }); + expect(JSON.parse(stdout.trim())).toMatchObject({ + hasApp: false, + toolName: "missing-app", + }); + }); + it("throws TOOL_ERROR (code:tool_is_error) when the result has isError:true", async () => { const promise = emitResult({ isError: true }, undefined, { toolName: "boom", @@ -108,6 +124,25 @@ describe("collectAppInfo", () => { expect(info.resourceError).toBe("string failure"); }); + it("stringifies a non-Error throw from extractAppInfo", async () => { + const extract = await import("@inspector/core/mcp/apps.js"); + const spy = vi + .spyOn(extract, "extractAppInfo") + .mockImplementationOnce(() => { + throw "boom-string"; + }); + try { + const info = await collectAppInfo( + { readResource: vi.fn() } as never, + { name: "t", inputSchema: { type: "object" as const } }, + undefined, + ); + expect(info.resourceError).toBe("boom-string"); + } finally { + spy.mockRestore(); + } + }); + it("folds a malformed _meta.ui.resourceUri into {hasApp:false, resourceError} instead of throwing", async () => { // extractAppInfo throws when the advertised UI URI is not a `ui://` string; // collectAppInfo must tolerate that so the tools/list --app-info NDJSON loop diff --git a/clients/cli/__tests__/format-output.test.ts b/clients/cli/__tests__/format-output.test.ts new file mode 100644 index 000000000..14d633207 --- /dev/null +++ b/clients/cli/__tests__/format-output.test.ts @@ -0,0 +1,50 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { writeFormattedResult } from "../src/handlers/format-output.js"; + +describe("writeFormattedResult", () => { + let originalWrite: typeof process.stdout.write; + + afterEach(() => { + if (originalWrite) process.stdout.write = originalWrite; + }); + + function captureStdout(): { get: () => string; restore: () => void } { + let out = ""; + originalWrite = process.stdout.write; + process.stdout.write = ((chunk: unknown, ...rest: unknown[]) => { + out += String(chunk); + const cb = rest.find((x) => typeof x === "function") as + | (() => void) + | undefined; + cb?.(); + return true; + }) as typeof process.stdout.write; + return { + get: () => out, + restore: () => { + process.stdout.write = originalWrite; + }, + }; + } + + it("defaults to pretty text and supports the json envelope", async () => { + const cap = captureStdout(); + try { + await writeFormattedResult({ ok: 1 }); + expect(cap.get()).toContain('"ok": 1'); + expect(cap.get()).not.toContain('"result"'); + + // reset buffer by re-capturing + cap.restore(); + const cap2 = captureStdout(); + try { + await writeFormattedResult({ ok: 2 }, "json"); + expect(JSON.parse(cap2.get())).toEqual({ result: { ok: 2 } }); + } finally { + cap2.restore(); + } + } finally { + cap.restore(); + } + }); +}); diff --git a/clients/cli/__tests__/metadata.test.ts b/clients/cli/__tests__/metadata.test.ts index ac2876f6c..b5867d50e 100644 --- a/clients/cli/__tests__/metadata.test.ts +++ b/clients/cli/__tests__/metadata.test.ts @@ -428,6 +428,43 @@ describe("Metadata Tests", () => { } }); + it("JSON.stringifies object/array metadata (not String → [object Object])", async () => { + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createEchoTool()], + }); + + try { + await server.start(); + const result = await runCli([ + server.url, + "--cli", + "--method", + "tools/list", + "--metadata", + 'nested={"key":"value"}', + "list=[1,2,3]", + "flag=true", + "--transport", + "http", + ]); + + expectCliSuccess(result); + + const recordedRequests = server.getRecordedRequests(); + const toolsListRequest = recordedRequests.find( + (r) => r.method === "tools/list", + ); + expect(toolsListRequest?.metadata).toEqual({ + nested: '{"key":"value"}', + list: "[1,2,3]", + flag: "true", + }); + } finally { + await server.stop(); + } + }); + it("should handle special characters", async () => { const server = createTestServerHttp({ serverInfo: createTestServerInfo(), diff --git a/clients/cli/__tests__/method-types.test.ts b/clients/cli/__tests__/method-types.test.ts new file mode 100644 index 000000000..42c710256 --- /dev/null +++ b/clients/cli/__tests__/method-types.test.ts @@ -0,0 +1,36 @@ +import { describe, it, expect } from "vitest"; +import { + isOneShotMethod, + metaValueToString, + ONE_SHOT_METHODS, + SESSION_RPC_METHODS, +} from "../src/handlers/method-types.js"; + +describe("SESSION_RPC_METHODS", () => { + it("lists the full RPC method set supported by runMethod", () => { + expect(SESSION_RPC_METHODS).toContain("tools/list"); + expect(SESSION_RPC_METHODS).toContain("tools/call"); + expect(SESSION_RPC_METHODS).toContain("logging/tail"); + expect(SESSION_RPC_METHODS).toContain("roots/set"); + expect(new Set(SESSION_RPC_METHODS).size).toBe(SESSION_RPC_METHODS.length); + }); +}); + +describe("ONE_SHOT_METHODS", () => { + it("excludes stream and session-only methods", () => { + expect(isOneShotMethod("tools/list")).toBe(true); + expect(isOneShotMethod("logging/setLevel")).toBe(true); + expect(isOneShotMethod("logging/tail")).toBe(false); + expect(isOneShotMethod("resources/subscribe")).toBe(false); + expect(isOneShotMethod("tasks/list")).toBe(false); + expect(ONE_SHOT_METHODS).not.toContain("logging/tail"); + }); +}); + +describe("metaValueToString", () => { + it("passes strings through and JSON-encodes structured values", () => { + expect(metaValueToString("plain")).toBe("plain"); + expect(metaValueToString({ a: 1 })).toBe('{"a":1}'); + expect(metaValueToString([1, 2])).toBe("[1,2]"); + }); +}); diff --git a/clients/cli/__tests__/methods.test.ts b/clients/cli/__tests__/methods.test.ts index 0fb720b36..a41459586 100644 --- a/clients/cli/__tests__/methods.test.ts +++ b/clients/cli/__tests__/methods.test.ts @@ -59,20 +59,43 @@ describe("CLI method coverage", () => { expect(Array.isArray(json.tools)).toBe(true); }); - it("throws for an unsupported method after connecting", async () => { - // Reaches the final `else` in callMethod: the connection succeeds, none of - // the known method branches match, so it throws "Unsupported method". - const result = await runCli([ + it("rejects unsupported and stream methods before connecting", async () => { + const unknown = await runCli([ command, ...args, "--cli", "--method", "definitely/not-a-real-method", ]); + expectCliFailure(unknown); + expectOutputContains(unknown, "Unsupported method"); + expectOutputContains(unknown, "definitely/not-a-real-method"); + // Stream methods used to hang via consumeMethodOutcome; now rejected. + const tail = await runCli([ + command, + ...args, + "--cli", + "--method", + "logging/tail", + ]); + expectCliFailure(tail); + expectOutputContains(tail, "Unsupported method"); + expectOutputContains(tail, "logging/tail"); + }); + + it("rejects bad methods before --use-stored-auth network work", async () => { + // No oauth.json / refresh — must fail on allowlist, not no_stored_token. + const result = await runCli([ + "--method", + "logging/tail", + "--use-stored-auth", + "--server-url", + "https://example.com/mcp", + ]); expectCliFailure(result); expectOutputContains(result, "Unsupported method"); - expectOutputContains(result, "definitely/not-a-real-method"); + expect(result.stderr).not.toMatch(/No stored OAuth token/); }); it("rejects an invalid --transport value before connecting", async () => { diff --git a/clients/cli/__tests__/open-url.test.ts b/clients/cli/__tests__/open-url.test.ts new file mode 100644 index 000000000..304dd2570 --- /dev/null +++ b/clients/cli/__tests__/open-url.test.ts @@ -0,0 +1,28 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const openMock = vi.fn().mockResolvedValue(undefined); + +vi.mock("open", () => ({ + default: (...args: unknown[]) => openMock(...args), +})); + +describe("openUrl", () => { + beforeEach(() => { + openMock.mockClear(); + openMock.mockResolvedValue(undefined); + }); + + it("forwards a string URL to open", async () => { + const { openUrl } = await import("../src/open-url.js"); + await openUrl("https://example.com/auth"); + expect(openMock).toHaveBeenCalledWith("https://example.com/auth"); + }); + + it("forwards URL.href for URL instances", async () => { + const { openUrl } = await import("../src/open-url.js"); + await openUrl(new URL("https://example.com/callback?code=1")); + expect(openMock).toHaveBeenCalledWith( + "https://example.com/callback?code=1", + ); + }); +}); diff --git a/clients/cli/__tests__/programmatic-ergonomics.test.ts b/clients/cli/__tests__/programmatic-ergonomics.test.ts index 63ea39cbd..17888498e 100644 --- a/clients/cli/__tests__/programmatic-ergonomics.test.ts +++ b/clients/cli/__tests__/programmatic-ergonomics.test.ts @@ -135,6 +135,52 @@ describe("--tool-args-json", () => { }); }); +describe("CLI --relogin flag conflicts", () => { + it("rejects --relogin with --stored-auth-only", async () => { + const result = await runCli([ + "--relogin", + "--stored-auth-only", + "--server-url", + "https://example.com/mcp", + "--method", + "tools/list", + ]); + expectCliFailure(result); + expect(result.stderr).toMatch( + /--relogin cannot be combined with --stored-auth-only/, + ); + }); + + it("rejects --relogin with --use-stored-auth", async () => { + const result = await runCli([ + "--relogin", + "--use-stored-auth", + "--server-url", + "https://example.com/mcp", + "--method", + "tools/list", + ]); + expectCliFailure(result); + expect(result.stderr).toMatch(/--use-stored-auth/); + }); + + it("accepts --relogin and clears stored auth before connect", async () => { + // Unreachable port: proves --relogin runs (clears store) then fails connect. + const result = await runCli([ + "--relogin", + "--server-url", + "http://127.0.0.1:1/mcp", + "--transport", + "http", + "--connect-timeout", + "200", + "--method", + "initialize", + ]); + expectCliFailure(result); + }); +}); + describe("MCP_CATALOG_PATH with an ad-hoc target", () => { it("does not conflict with an ad-hoc target (env catalog is ignored)", async () => { const { command, args } = getTestMcpServerCommand(); diff --git a/clients/cli/__tests__/run-method-mocks.test.ts b/clients/cli/__tests__/run-method-mocks.test.ts new file mode 100644 index 000000000..4fda0c519 --- /dev/null +++ b/clients/cli/__tests__/run-method-mocks.test.ts @@ -0,0 +1,232 @@ +import { describe, it, expect, vi } from "vitest"; +import { runMethod } from "../src/handlers/run-method.js"; +import type { InspectorClient } from "@inspector/core/mcp/index.js"; + +function mockClient(overrides: Partial = {}): InspectorClient { + const base = { + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + subscribeToResource: vi.fn().mockResolvedValue(undefined), + unsubscribeFromResource: vi.fn().mockResolvedValue(undefined), + getCompletions: vi.fn().mockResolvedValue({ values: ["a"] }), + getRequestorTask: vi.fn().mockResolvedValue({ taskId: "t1" }), + cancelRequestorTask: vi.fn().mockResolvedValue(undefined), + getRequestorTaskResult: vi.fn().mockResolvedValue({ content: [] }), + getRoots: vi.fn().mockReturnValue([]), + setRoots: vi.fn().mockResolvedValue(undefined), + setLoggingLevel: vi.fn().mockResolvedValue(undefined), + getServerInfo: vi.fn().mockReturnValue({ name: "t" }), + getProtocolVersion: vi.fn().mockReturnValue("1"), + getCapabilities: vi.fn().mockReturnValue({}), + getInstructions: vi.fn().mockReturnValue(undefined), + callTool: vi.fn(), + callToolStream: vi.fn(), + readResource: vi.fn(), + getPrompt: vi.fn(), + }; + return { ...base, ...overrides } as unknown as InspectorClient; +} + +vi.mock("@inspector/core/mcp/state/index.js", async (importOriginal) => { + const actual = + await importOriginal(); + class FakeManaged { + setMetadata = vi.fn(); + refresh = vi.fn().mockResolvedValue(undefined); + destroy = vi.fn(); + getTools = vi + .fn() + .mockReturnValue([ + { name: "echo", description: "e", inputSchema: { type: "object" } }, + ]); + getResources = vi.fn().mockReturnValue([]); + getResourceTemplates = vi.fn().mockReturnValue([]); + getPrompts = vi.fn().mockReturnValue([{ name: "p" }]); + getTasks = vi.fn().mockReturnValue([{ taskId: "t1" }]); + } + class FakeLog { + static last: FakeLog | undefined; + addEventListener = vi.fn(); + removeEventListener = vi.fn(); + destroy = vi.fn(); + constructor() { + FakeLog.last = this; + } + } + return { + ...actual, + ManagedToolsState: FakeManaged, + ManagedResourcesState: FakeManaged, + ManagedResourceTemplatesState: FakeManaged, + ManagedPromptsState: FakeManaged, + ManagedRequestorTasksState: FakeManaged, + MessageLogState: FakeLog, + }; +}); + +describe("runMethod (mocked client)", () => { + it("covers subscribe stream, tasks, complete, and app-info call", async () => { + const client = mockClient({ + callTool: vi.fn().mockResolvedValue({ + result: { content: [{ type: "text", text: "ok" }] }, + toolName: "echo", + params: {}, + timestamp: new Date(), + success: true, + }), + callToolStream: vi.fn().mockResolvedValue({ + result: { content: [] }, + toolName: "echo", + params: {}, + timestamp: new Date(), + success: true, + }), + readResource: vi.fn().mockResolvedValue({ + result: { contents: [] }, + }), + }); + + const sub = await runMethod(client, { + method: "resources/subscribe", + uri: "test://x", + }); + expect(sub.kind).toBe("stream"); + if (sub.kind === "stream") { + const lines: unknown[] = []; + const stop = sub.start((o) => lines.push(o)); + expect(lines[0]).toMatchObject({ type: "subscribed" }); + // Simulate update event + const listener = ( + client.addEventListener as ReturnType + ).mock.calls.find((c) => c[0] === "resourceUpdated")?.[1] as ( + ev: Event, + ) => void; + listener?.( + new CustomEvent("resourceUpdated", { detail: { uri: "test://x" } }), + ); + expect( + lines.some( + (l) => (l as { type?: string }).type === "resources/updated", + ), + ).toBe(true); + stop(); + } + + const tasks = await runMethod(client, { method: "tasks/list" }); + expect(tasks.kind).toBe("result"); + + const got = await runMethod(client, { + method: "tasks/get", + taskId: "t1", + }); + expect(got.kind).toBe("result"); + + const cancelled = await runMethod(client, { + method: "tasks/cancel", + taskId: "t1", + }); + expect(cancelled.kind).toBe("result"); + + const result = await runMethod(client, { + method: "tasks/result", + taskId: "t1", + }); + expect(result.kind).toBe("result"); + + const complete = await runMethod(client, { + method: "prompts/complete", + completeRefType: "ref/prompt", + completeRef: "p", + completeArgName: "a", + completeArgValue: "x", + }); + expect(complete.kind).toBe("result"); + + const completeRes = await runMethod(client, { + method: "prompts/complete", + completeRefType: "ref/resource", + completeRef: "test://x", + completeArgName: "a", + }); + expect(completeRes.kind).toBe("result"); + + const appOnly = await runMethod(client, { + method: "tools/call", + toolName: "echo", + appInfo: true, + }); + expect(appOnly.kind).toBe("result"); + + const tasked = await runMethod(client, { + method: "tools/call", + toolName: "echo", + task: true, + toolArg: { message: "x" }, + }); + expect(tasked.kind).toBe("result"); + expect(client.callToolStream).toHaveBeenCalled(); + + vi.mocked(client.callTool).mockResolvedValueOnce({ + result: null, + error: "Tool call failed hard", + toolName: "echo", + params: {}, + timestamp: new Date(), + success: false, + } as never); + const nullResult = await runMethod(client, { + method: "tools/call", + toolName: "echo", + toolArg: { message: "x" }, + }); + expect(nullResult.kind).toBe("result"); + if (nullResult.kind === "result") { + expect(nullResult.result).toMatchObject({ isError: true }); + } + + await expect(runMethod(client, { method: "tasks/cancel" })).rejects.toThrow( + /tasks\/cancel/, + ); + await expect(runMethod(client, { method: "tasks/result" })).rejects.toThrow( + /tasks\/result/, + ); + + await expect( + runMethod(client, { + method: "roots/set", + rootsJson: "not-json", + }), + ).rejects.toThrow(/roots-json is invalid/); + + const tail = await runMethod(client, { method: "logging/tail" }); + expect(tail.kind).toBe("stream"); + if (tail.kind === "stream") { + const lines: unknown[] = []; + const stop = tail.start((o) => lines.push(o)); + const logMod = await import("@inspector/core/mcp/state/index.js"); + const FakeLog = logMod.MessageLogState as unknown as { + last?: { + addEventListener: ReturnType; + }; + }; + const listener = FakeLog.last?.addEventListener.mock.calls[0]?.[1] as + | ((ev: Event) => void) + | undefined; + listener?.( + new CustomEvent("message", { + detail: { + direction: "notification", + message: { method: "notifications/message" }, + }, + }), + ); + listener?.( + new CustomEvent("message", { + detail: { direction: "request", message: { method: "tools/list" } }, + }), + ); + expect(lines).toHaveLength(1); + stop(); + } + }); +}); diff --git a/clients/cli/__tests__/run-method.test.ts b/clients/cli/__tests__/run-method.test.ts new file mode 100644 index 000000000..242e0cd03 --- /dev/null +++ b/clients/cli/__tests__/run-method.test.ts @@ -0,0 +1,249 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-server"; +import { InspectorClient } from "@inspector/core/mcp/index.js"; +import { createTransportNode } from "@inspector/core/mcp/node/index.js"; +import { runMethod } from "../src/handlers/run-method.js"; +import { consumeMethodOutcome } from "../src/handlers/consume-outcome.js"; + +describe("runMethod", () => { + let client: InspectorClient | undefined; + + afterEach(async () => { + if (client) { + await client.disconnect().catch(() => {}); + client = undefined; + } + }); + + async function connectStdio(): Promise { + const { command, args } = getTestMcpServerCommand(); + client = new InspectorClient( + { type: "stdio", command, args }, + { + environment: { transport: createTransportNode }, + clientIdentity: { name: "test", version: "0.0.0" }, + progress: false, + sample: false, + elicit: false, + }, + ); + await client.connect(); + return client; + } + + it("lists tools and reads initialize fields", async () => { + const c = await connectStdio(); + const tools = await runMethod(c, { method: "tools/list" }); + expect(tools.kind).toBe("result"); + if (tools.kind === "result") { + expect( + (tools.result.tools as unknown[] | undefined)?.length, + ).toBeGreaterThan(0); + } + + const init = await runMethod(c, { method: "initialize" }); + expect(init.kind).toBe("result"); + }); + + it("returns ndjson lines for tools/list --app-info", async () => { + const c = await connectStdio(); + const outcome = await runMethod(c, { + method: "tools/list", + appInfo: true, + }); + expect(outcome.kind).toBe("ndjson"); + if (outcome.kind === "ndjson") { + expect(outcome.lines.length).toBeGreaterThan(0); + } + }); + + it("lists resources and prompts", async () => { + const c = await connectStdio(); + const resources = await runMethod(c, { method: "resources/list" }); + expect(resources.kind).toBe("result"); + const prompts = await runMethod(c, { method: "prompts/list" }); + expect(prompts.kind).toBe("result"); + const templates = await runMethod(c, { + method: "resources/templates/list", + }); + expect(templates.kind).toBe("result"); + }); + + it("sets logging level", async () => { + const c = await connectStdio(); + const outcome = await runMethod(c, { + method: "logging/setLevel", + logLevel: "info", + }); + expect(outcome.kind).toBe("result"); + }); + + it("roots/list and roots/set round-trip", async () => { + const c = await connectStdio(); + const listed = await runMethod(c, { method: "roots/list" }); + expect(listed.kind).toBe("result"); + const set = await runMethod(c, { + method: "roots/set", + rootsJson: JSON.stringify([{ uri: "file:///tmp", name: "tmp" }]), + }); + expect(set.kind).toBe("result"); + if (set.kind === "result") { + expect(set.result.roots).toEqual([{ uri: "file:///tmp", name: "tmp" }]); + } + }); + + it("consumeMethodOutcome writes result json", async () => { + let stdout = ""; + const original = process.stdout.write; + process.stdout.write = ((chunk: unknown, ...rest: unknown[]) => { + stdout += typeof chunk === "string" ? chunk : String(chunk); + const cb = rest.find((r) => typeof r === "function") as + | (() => void) + | undefined; + cb?.(); + return true; + }) as typeof process.stdout.write; + try { + await consumeMethodOutcome( + { kind: "result", result: { ok: true } }, + { format: "json" }, + ); + expect(JSON.parse(stdout.trim())).toEqual({ result: { ok: true } }); + } finally { + process.stdout.write = original; + } + }); + + it("rejects unknown methods", async () => { + const c = await connectStdio(); + await expect(runMethod(c, { method: "nope/method" })).rejects.toThrow( + /Unsupported method/, + ); + }); + + it("calls echo tool and rejects missing tool name", async () => { + const c = await connectStdio(); + await expect(runMethod(c, { method: "tools/call" })).rejects.toThrow( + /Tool name is required/, + ); + + const missing = runMethod(c, { + method: "tools/call", + toolName: "does-not-exist", + }); + await expect(missing).rejects.toMatchObject({ exitCode: 5 }); + + const called = await runMethod(c, { + method: "tools/call", + toolName: "echo", + toolArg: { message: "hi" }, + format: "json", + }); + expect(called.kind).toBe("result"); + }); + + it("reads a resource and gets a prompt", async () => { + const c = await connectStdio(); + const read = await runMethod(c, { + method: "resources/read", + uri: "test://env", + }); + expect(read.kind).toBe("result"); + + await expect(runMethod(c, { method: "resources/read" })).rejects.toThrow( + /URI is required/, + ); + + const prompts = await runMethod(c, { method: "prompts/list" }); + expect(prompts.kind).toBe("result"); + if (prompts.kind === "result") { + const list = prompts.result.prompts as { name: string }[]; + if (list.length > 0) { + const got = await runMethod(c, { + method: "prompts/get", + promptName: list[0]!.name, + }); + expect(got.kind).toBe("result"); + } + } + }); + + it("returns a stream starter for logging/tail", async () => { + const c = await connectStdio(); + const outcome = await runMethod(c, { method: "logging/tail" }); + expect(outcome.kind).toBe("stream"); + if (outcome.kind === "stream") { + const stop = outcome.start(() => {}); + stop(); + } + }); + + it("requires uri for subscribe/unsubscribe", async () => { + const c = await connectStdio(); + await expect( + runMethod(c, { method: "resources/subscribe" }), + ).rejects.toThrow(/URI is required/); + await expect( + runMethod(c, { method: "resources/unsubscribe" }), + ).rejects.toThrow(/URI is required/); + }); + + it("rejects bad roots/set JSON and missing logging level", async () => { + const c = await connectStdio(); + await expect(runMethod(c, { method: "logging/setLevel" })).rejects.toThrow( + /Log level is required/, + ); + await expect( + runMethod(c, { method: "roots/set", rootsJson: "{}" }), + ).rejects.toThrow(/must be a JSON array/); + await expect(runMethod(c, { method: "roots/set" })).rejects.toThrow( + /roots\/set requires/, + ); + await expect(runMethod(c, { method: "tasks/get" })).rejects.toThrow( + /Task id is required/, + ); + await expect(runMethod(c, { method: "prompts/get" })).rejects.toThrow( + /Prompt name is required/, + ); + await expect(runMethod(c, { method: "prompts/complete" })).rejects.toThrow( + /prompts\/complete requires/, + ); + }); + + it("consumeMethodOutcome writes ndjson and stream until SIGINT", async () => { + let stdout = ""; + const original = process.stdout.write; + process.stdout.write = ((chunk: unknown, ...rest: unknown[]) => { + stdout += typeof chunk === "string" ? chunk : String(chunk); + const cb = rest.find((r) => typeof r === "function") as + | (() => void) + | undefined; + cb?.(); + return true; + }) as typeof process.stdout.write; + try { + await consumeMethodOutcome( + { kind: "ndjson", lines: [{ a: 1 }, { b: 2 }] }, + {}, + ); + expect(stdout.trim().split("\n")).toHaveLength(2); + + const streamPromise = consumeMethodOutcome( + { + kind: "stream", + label: "t", + start: (write) => { + write({ hi: true }); + queueMicrotask(() => process.emit("SIGINT")); + return () => {}; + }, + }, + {}, + ); + await streamPromise; + expect(stdout).toContain('"hi":true'); + } finally { + process.stdout.write = original; + } + }); +}); diff --git a/clients/cli/__tests__/servers-list.test.ts b/clients/cli/__tests__/servers-list.test.ts new file mode 100644 index 000000000..6860e29f0 --- /dev/null +++ b/clients/cli/__tests__/servers-list.test.ts @@ -0,0 +1,286 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { runCli } from "./helpers/cli-runner.js"; +import { + createSampleTestConfig, + deleteConfigFile, +} from "./helpers/fixtures.js"; +import { expectCliSuccess } from "./helpers/assertions.js"; +import { + annotateServerEntriesWithSessions, + listServerEntries, + sanitizeServerConfig, + sanitizeServerSettings, + showServerEntry, + summarizeServerConfig, +} from "../src/handlers/servers-list.js"; +import type { + InspectorServerSettings, + MCPServerConfig, +} from "@inspector/core/mcp/types.js"; +import { expectCliFailure } from "./helpers/assertions.js"; + +describe("summarizeServerConfig", () => { + it("summarises stdio, sse, and streamable-http configs", () => { + expect( + summarizeServerConfig({ + type: "stdio", + command: "node", + args: ["server.js"], + }), + ).toEqual({ type: "stdio", detail: "node server.js" }); + expect( + summarizeServerConfig({ + type: "stdio", + command: "node", + }), + ).toEqual({ type: "stdio", detail: "node" }); + expect( + summarizeServerConfig({ + type: "sse", + url: "http://localhost/sse", + }), + ).toEqual({ type: "sse", detail: "http://localhost/sse" }); + expect( + summarizeServerConfig({ + type: "streamable-http", + url: "http://localhost/mcp", + } as MCPServerConfig), + ).toEqual({ type: "streamable-http", detail: "http://localhost/mcp" }); + expect( + summarizeServerConfig({ + type: "streamable-http", + } as MCPServerConfig), + ).toEqual({ type: "streamable-http", detail: "" }); + }); +}); + +describe("annotateServerEntriesWithSessions", () => { + const entries = [ + { name: "a", type: "stdio", detail: "node a" }, + { name: "b", type: "stdio", detail: "node b" }, + ]; + + it("returns entries unchanged when there are no sessions", () => { + expect(annotateServerEntriesWithSessions(entries, [])).toBe(entries); + }); + + it("marks matching entry names and MRU", () => { + expect( + annotateServerEntriesWithSessions(entries, [ + { name: "b", isMru: true }, + { name: "other" }, + ]), + ).toEqual([ + { name: "a", type: "stdio", detail: "node a" }, + { name: "b", type: "stdio", detail: "node b", session: "b", isMru: true }, + ]); + }); + + it("omits isMru when the session is not MRU", () => { + expect( + annotateServerEntriesWithSessions(entries, [{ name: "a", isMru: false }]), + ).toEqual([ + { name: "a", type: "stdio", detail: "node a", session: "a" }, + { name: "b", type: "stdio", detail: "node b" }, + ]); + }); +}); + +describe("listServerEntries / --method servers/list", () => { + let configPath: string | undefined; + + afterEach(() => { + if (configPath) { + deleteConfigFile(configPath); + configPath = undefined; + } + }); + + it("lists catalog entries without connecting", async () => { + configPath = createSampleTestConfig(); + const entries = await listServerEntries({ configPath }); + expect(entries.map((e) => e.name).sort()).toEqual([ + "test-http", + "test-stdio", + ]); + expect(entries.find((e) => e.name === "test-stdio")?.type).toBe("stdio"); + }); + + it("works via --method servers/list", async () => { + configPath = createSampleTestConfig(); + const result = await runCli([ + "--config", + configPath, + "--method", + "servers/list", + "--format", + "json", + ]); + expectCliSuccess(result); + const body = JSON.parse(result.stdout) as { + result: { servers: { name: string }[] }; + }; + expect(body.result.servers.map((s) => s.name).sort()).toEqual([ + "test-http", + "test-stdio", + ]); + }); +}); + +describe("showServerEntry / servers/show", () => { + let configPath: string | undefined; + + afterEach(() => { + if (configPath) { + deleteConfigFile(configPath); + configPath = undefined; + } + }); + + it("redacts env values, init headers, and sensitive settings fields", () => { + expect( + sanitizeServerConfig({ + type: "stdio", + command: "node", + env: { SECRET: "value", HELLO: "world" }, + }), + ).toEqual({ + type: "stdio", + command: "node", + env: { SECRET: "[redacted]", HELLO: "[redacted]" }, + }); + + expect( + sanitizeServerConfig({ + type: "streamable-http", + url: "https://example.com/mcp", + requestInit: { + headers: { + Authorization: "Bearer secret", + "X-Custom": "ok", + }, + }, + eventSourceInit: { + headers: { + "X-Api-Key": "k", + Accept: "text/event-stream", + }, + }, + } as MCPServerConfig), + ).toMatchObject({ + requestInit: { + headers: { + Authorization: "[redacted]", + "X-Custom": "ok", + }, + }, + eventSourceInit: { + headers: { + "X-Api-Key": "[redacted]", + Accept: "text/event-stream", + }, + }, + }); + + expect( + sanitizeServerConfig({ + type: "streamable-http", + url: "https://example.com/mcp", + requestInit: { + headers: [ + ["Authorization", "Bearer secret"], + ["X-Custom", "ok"], + ], + }, + } as MCPServerConfig), + ).toMatchObject({ + requestInit: { + headers: [ + ["Authorization", "[redacted]"], + ["X-Custom", "ok"], + ], + }, + }); + + const settings: InspectorServerSettings = { + headers: [ + { key: "Authorization", value: "Bearer x" }, + { key: "X-Custom", value: "ok" }, + ], + metadata: [], + env: [ + { key: "TOKEN", value: "secret" }, + { key: "", value: "still-secret" }, + ], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 60_000, + maxFetchRequests: 100, + roots: [], + oauthClientSecret: "shh", + oauthClientId: "cid", + }; + const sanitized = sanitizeServerSettings(settings); + expect(sanitized.oauthClientSecret).toBe("[redacted]"); + expect(sanitized.oauthClientId).toBe("cid"); + expect(sanitized.headers).toEqual([ + { key: "Authorization", value: "[redacted]" }, + { key: "X-Custom", value: "ok" }, + ]); + expect(sanitized.env).toEqual([ + { key: "TOKEN", value: "[redacted]" }, + { key: "", value: "[redacted]" }, + ]); + }); + + it("shows one resolved entry without connecting", async () => { + configPath = createSampleTestConfig(); + const entry = await showServerEntry("test-stdio", { configPath }); + expect(entry.name).toBe("test-stdio"); + expect(entry.type).toBe("stdio"); + expect(entry.config).toMatchObject({ + type: "stdio", + command: expect.any(String), + }); + expect(entry.config.env).toEqual({ HELLO: "[redacted]" }); + }); + + it("works via --method servers/show --server", async () => { + configPath = createSampleTestConfig(); + const result = await runCli([ + "--config", + configPath, + "--method", + "servers/show", + "--server", + "test-http", + "--format", + "json", + ]); + expectCliSuccess(result); + const body = JSON.parse(result.stdout) as { + result: { name: string; type: string }; + }; + expect(body.result.name).toBe("test-http"); + expect(body.result.type).toBe("streamable-http"); + }); + + it("rejects servers/show without --server", async () => { + configPath = createSampleTestConfig(); + const result = await runCli([ + "--config", + configPath, + "--method", + "servers/show", + ]); + expectCliFailure(result); + expect(result.stderr).toMatch(/--server/); + }); + + it("rejects unknown server names", async () => { + configPath = createSampleTestConfig(); + await expect(showServerEntry("nope", { configPath })).rejects.toThrow( + /not found/, + ); + }); +}); diff --git a/clients/cli/__tests__/style.test.ts b/clients/cli/__tests__/style.test.ts new file mode 100644 index 000000000..f9b8cfeff --- /dev/null +++ b/clients/cli/__tests__/style.test.ts @@ -0,0 +1,40 @@ +import { describe, it, expect } from "vitest"; +import { + createStyle, + resolveAnsiEnabled, + styleFromOpts, + PLAIN, +} from "../src/style.js"; + +describe("resolveAnsiEnabled", () => { + it("is off for --plain, json, NO_COLOR, and non-TTY", () => { + expect(resolveAnsiEnabled({ plain: true, isTTY: true })).toBe(false); + expect(resolveAnsiEnabled({ format: "json", isTTY: true })).toBe(false); + expect(resolveAnsiEnabled({ isTTY: true, noColorEnv: "1" })).toBe(false); + expect(resolveAnsiEnabled({ isTTY: false, noColorEnv: "" })).toBe(false); + }); + + it("is on for TTY text without plain/NO_COLOR", () => { + expect( + resolveAnsiEnabled({ isTTY: true, noColorEnv: "", format: "text" }), + ).toBe(true); + expect(styleFromOpts({ isTTY: true, noColorEnv: "" }).ansi).toBe(true); + }); +}); + +describe("createStyle", () => { + it("PLAIN is identity; ansi wraps SGR and OSC 8", () => { + expect(PLAIN.bold("x")).toBe("x"); + expect(PLAIN.link("https://example.com")).toBe("https://example.com"); + + const s = createStyle(true); + expect(s.bold("x")).toContain("\u001b[1m"); + expect(s.dim("x")).toContain("\u001b[2m"); + expect(s.red("x")).toContain("\u001b[31m"); + expect(s.link("https://example.com", "ex")).toContain( + "\u001b]8;;https://example.com", + ); + expect(s.link("https://example.com", "ex")).toContain("ex"); + expect(s.link("", "ex")).toBe("ex"); + }); +}); diff --git a/clients/cli/package-lock.json b/clients/cli/package-lock.json index 2760136d3..36d3aad62 100644 --- a/clients/cli/package-lock.json +++ b/clients/cli/package-lock.json @@ -16,6 +16,7 @@ "ajv": "^8.17.1", "atomically": "^2.1.1", "commander": "^13.1.0", + "open": "^10.2.0", "pino": "^9.14.0", "undici": "^8.5.0", "zod": "^4.3.6" @@ -2449,6 +2450,21 @@ "concat-map": "0.0.1" } }, + "node_modules/bundle-name": { + "version": "4.1.0", + "resolved": "https://registry.npmjs.org/bundle-name/-/bundle-name-4.1.0.tgz", + "integrity": "sha512-tjwM5exMg6BGRI+kNmTntNsvdZS1X8BFYS6tnJ2hdH0kVxM6/eVZ2xy+FqStSWvYmtfFMDLIxurorHwDKfDz5Q==", + "license": "MIT", + "dependencies": { + "run-applescript": "^7.0.0" + }, + "engines": { + "node": ">=18" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/bundle-require": { "version": "5.1.0", "resolved": "https://registry.npmjs.org/bundle-require/-/bundle-require-5.1.0.tgz", @@ -2726,6 +2742,46 @@ "dev": true, "license": "MIT" }, + "node_modules/default-browser": { + "version": "5.5.0", + "resolved": "https://registry.npmjs.org/default-browser/-/default-browser-5.5.0.tgz", + "integrity": "sha512-H9LMLr5zwIbSxrmvikGuI/5KGhZ8E2zH3stkMgM5LpOWDutGM2JZaj460Udnf1a+946zc7YBgrqEWwbk7zHvGw==", + "license": "MIT", + "dependencies": { + "bundle-name": "^4.1.0", + "default-browser-id": "^5.0.0" + }, + "engines": { + "node": ">=18" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, + "node_modules/default-browser-id": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/default-browser-id/-/default-browser-id-5.0.1.tgz", + "integrity": "sha512-x1VCxdX4t+8wVfd1so/9w+vQ4vx7lKd2Qp5tDRutErwmR85OgmfX7RlLRMWafRMY7hbEiXIbudNrjOAPa/hL8Q==", + "license": "MIT", + "engines": { + "node": ">=18" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, + "node_modules/define-lazy-prop": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/define-lazy-prop/-/define-lazy-prop-3.0.0.tgz", + "integrity": "sha512-N+MeXYoqr3pOgn8xfyRPREN7gHakLYjhsHhWGT3fWAiL4IkAt0iDw14QiiEm2bE30c5XX5q0FtAA3CK5f9/BUg==", + "license": "MIT", + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/depd": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/depd/-/depd-2.0.0.tgz", @@ -3583,6 +3639,21 @@ "node": ">= 0.10" } }, + "node_modules/is-docker": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/is-docker/-/is-docker-3.0.0.tgz", + "integrity": "sha512-eljcgEDlEns/7AXFosB5K/2nCM4P7FQPkGc/DWLy5rmFEWvZayGrik1d9/QIY5nJ4f9YsVvBkA6kJpHn9rISdQ==", + "license": "MIT", + "bin": { + "is-docker": "cli.js" + }, + "engines": { + "node": "^12.20.0 || ^14.13.1 || >=16.0.0" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/is-extglob": { "version": "2.1.1", "resolved": "https://registry.npmjs.org/is-extglob/-/is-extglob-2.1.1.tgz", @@ -3606,6 +3677,24 @@ "node": ">=0.10.0" } }, + "node_modules/is-inside-container": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/is-inside-container/-/is-inside-container-1.0.0.tgz", + "integrity": "sha512-KIYLCCJghfHZxqjYBE7rEy0OBuTd5xCHS7tHVgvCLkx7StIoaxwNW3hCALgEUjFfeRk+MG/Qxmp/vtETEF3tRA==", + "license": "MIT", + "dependencies": { + "is-docker": "^3.0.0" + }, + "bin": { + "is-inside-container": "cli.js" + }, + "engines": { + "node": ">=14.16" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/is-promise": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/is-promise/-/is-promise-4.0.0.tgz", @@ -3613,6 +3702,21 @@ "license": "MIT", "peer": true }, + "node_modules/is-wsl": { + "version": "3.1.1", + "resolved": "https://registry.npmjs.org/is-wsl/-/is-wsl-3.1.1.tgz", + "integrity": "sha512-e6rvdUCiQCAuumZslxRJWR/Doq4VpPR82kqclvcS0efgt430SlGIk05vdCN58+VrzgtIcfNODjozVielycD4Sw==", + "license": "MIT", + "dependencies": { + "is-inside-container": "^1.0.0" + }, + "engines": { + "node": ">=16" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/isexe": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/isexe/-/isexe-2.0.0.tgz", @@ -4311,6 +4415,24 @@ "wrappy": "1" } }, + "node_modules/open": { + "version": "10.2.0", + "resolved": "https://registry.npmjs.org/open/-/open-10.2.0.tgz", + "integrity": "sha512-YgBpdJHPyQ2UE5x+hlSXcnejzAvD0b22U2OuAP+8OnlJT+PjWPxtgmGqKKc+RgTM63U9gN0YzrYc71R2WT/hTA==", + "license": "MIT", + "dependencies": { + "default-browser": "^5.2.1", + "define-lazy-prop": "^3.0.0", + "is-inside-container": "^1.0.0", + "wsl-utils": "^0.1.0" + }, + "engines": { + "node": ">=18" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/optionator": { "version": "0.9.4", "resolved": "https://registry.npmjs.org/optionator/-/optionator-0.9.4.tgz", @@ -4832,6 +4954,18 @@ "node": ">= 18" } }, + "node_modules/run-applescript": { + "version": "7.1.0", + "resolved": "https://registry.npmjs.org/run-applescript/-/run-applescript-7.1.0.tgz", + "integrity": "sha512-DPe5pVFaAsinSaV6QjQ6gdiedWDcRCbUuiQfQa2wmWV7+xC9bGulGI8+TdRmoFkAPaBXk8CrAbnlY2ISniJ47Q==", + "license": "MIT", + "engines": { + "node": ">=18" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/safe-stable-stringify": { "version": "2.5.0", "resolved": "https://registry.npmjs.org/safe-stable-stringify/-/safe-stable-stringify-2.5.0.tgz", @@ -5693,6 +5827,21 @@ "license": "ISC", "peer": true }, + "node_modules/wsl-utils": { + "version": "0.1.0", + "resolved": "https://registry.npmjs.org/wsl-utils/-/wsl-utils-0.1.0.tgz", + "integrity": "sha512-h3Fbisa2nKGPxCpm89Hk33lBLsnaGBvctQopaBSOW/uIs6FTe1ATyAnKFJrzVs9vpGdsTe73WF3V4lIsk4Gacw==", + "license": "MIT", + "dependencies": { + "is-wsl": "^3.1.0" + }, + "engines": { + "node": ">=18" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/yocto-queue": { "version": "0.1.0", "resolved": "https://registry.npmjs.org/yocto-queue/-/yocto-queue-0.1.0.tgz", diff --git a/clients/cli/package.json b/clients/cli/package.json index 9ef0e9bd8..2f277877c 100644 --- a/clients/cli/package.json +++ b/clients/cli/package.json @@ -40,6 +40,7 @@ "ajv": "^8.17.1", "atomically": "^2.1.1", "commander": "^13.1.0", + "open": "^10.2.0", "pino": "^9.14.0", "undici": "^8.5.0", "zod": "^4.3.6" diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts new file mode 100644 index 000000000..850c14f99 --- /dev/null +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -0,0 +1,27 @@ +import { + NodeOAuthStorage, + resetNodeOAuthStorageCache, +} from "@inspector/core/auth/node/storage-node.js"; + +/** Same canonicalisation as CLI `normalizeServerUrl` (avoid cycles). */ +function normalizeServerUrl(serverUrl: string): string { + try { + return new URL(serverUrl).href; + } catch { + return serverUrl; + } +} + +/** + * Drop stored OAuth state for an HTTP(S) server URL so the next connect cannot + * silently reuse tokens (`--relogin`). No-op when `serverUrl` is missing. + */ +export async function clearStoredAuthForRelogin( + serverUrl: string | undefined, +): Promise { + if (!serverUrl?.trim()) return; + const url = normalizeServerUrl(serverUrl.trim()); + const storage = new NodeOAuthStorage(); + await storage.clear(url); + resetNodeOAuthStorageCache(); +} diff --git a/clients/cli/src/cli-oauth-navigation.ts b/clients/cli/src/cli-oauth-navigation.ts new file mode 100644 index 000000000..03352a697 --- /dev/null +++ b/clients/cli/src/cli-oauth-navigation.ts @@ -0,0 +1,46 @@ +import { CallbackNavigation } from "@inspector/core/auth/index.js"; +import { openUrl } from "./open-url.js"; +import { createStyle, resolveAnsiEnabled } from "./style.js"; + +export type CliOAuthNavigationOptions = { + /** Override TTY detection (tests). Defaults to stderr.isTTY. */ + isTTY?: boolean; + /** Override NO_COLOR (tests). */ + noColorEnv?: string | undefined; + /** Write the prompt line (defaults to stderr). */ + write?: (line: string) => void; + /** Open the browser (defaults to {@link openUrl}). */ + openBrowser?: (url: string) => Promise; +}; + +/** + * CLI OAuth navigation: print the authorization URL (OSC 8 when TTY allows + * ANSI) and auto-open the default browser on a TTY. Non-TTY / CI: plain URL + * only — never launches a browser. + */ +export function createCliOAuthNavigation( + options: CliOAuthNavigationOptions = {}, +): CallbackNavigation { + return new CallbackNavigation(async (url) => { + const href = url.href; + const tty = + options.isTTY !== undefined + ? options.isTTY + : process.stderr.isTTY === true; + const style = createStyle( + resolveAnsiEnabled({ + isTTY: tty, + noColorEnv: options.noColorEnv, + }), + ); + const write = + options.write ?? ((line: string) => process.stderr.write(line)); + write(`Please navigate to: ${style.link(href)}\n`); + if (!tty) return; + try { + await (options.openBrowser ?? openUrl)(href); + } catch { + // URL already printed; browser open is best-effort. + } + }); +} diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index f63815bca..b48bfb7cb 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -6,18 +6,15 @@ import type { MCPServerConfig, InspectorClientEnvironment, } from "@inspector/core/mcp/types.js"; +import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js"; import { - DEFAULT_MAX_FETCH_REQUESTS, - DEFAULT_TASK_TTL_MS, - eraToVersionNegotiation, -} from "@inspector/core/mcp/types.js"; + DEFAULT_CONNECT_TIMEOUT_MS, + withConnectTimeout, +} from "./handlers/connect-timeout.js"; +import { listServerEntries, showServerEntry } from "./handlers/servers-list.js"; +import { writeFormattedResult } from "./handlers/format-output.js"; +import { clearStoredAuthForRelogin } from "./clear-stored-auth-for-relogin.js"; import { InspectorClient } from "@inspector/core/mcp/index.js"; -import { - ManagedToolsState, - ManagedResourcesState, - ManagedResourceTemplatesState, - ManagedPromptsState, -} from "@inspector/core/mcp/state/index.js"; import { createTransportNode, loadServerEntries, @@ -26,9 +23,18 @@ import { parseHeaderPair, } from "@inspector/core/mcp/node/index.js"; import type { JsonValue } from "@inspector/core/mcp/index.js"; -import { extractAppInfo } from "@inspector/core/mcp/apps.js"; -import type { AppInfo } from "@inspector/core/mcp/apps.js"; import { getStateFilePath } from "@inspector/core/auth/node/storage-node.js"; +import { consumeMethodOutcome } from "./handlers/consume-outcome.js"; +import { runMethod } from "./handlers/run-method.js"; +import { + isOneShotMethod, + metaValueToString, + ONE_SHOT_METHODS, + type MethodArgs, +} from "./handlers/method-types.js"; +export type { CliAppInfo } from "./handlers/method-types.js"; +export { emitResult } from "./handlers/emit-result.js"; +export { collectAppInfo } from "./handlers/collect-app-info.js"; import { parseOAuthPersistBlob, serializeOAuthPersistBlob, @@ -46,11 +52,9 @@ import type { OAuthTokens, } from "@modelcontextprotocol/client"; import { CliExitCodeError, EXIT_CODES } from "./error-handler.js"; -import { - ConsoleNavigation, - MutableRedirectUrlProvider, -} from "@inspector/core/auth/index.js"; +import { MutableRedirectUrlProvider } from "@inspector/core/auth/index.js"; import { NodeOAuthStorage } from "@inspector/core/auth/node/index.js"; +import { createCliOAuthNavigation } from "./cli-oauth-navigation.js"; import { connectInspectorWithOAuth, withCliAuthRecoveryRetry, @@ -79,61 +83,10 @@ export const validLogLevels: LoggingLevel[] = Object.values( /** Client identity name the CLI reports to servers. */ const CLI_CLIENT_NAME = "inspector-cli"; -/** - * Default connect timeout (ms) for ad-hoc server invocations. Without this an - * unreachable server (e.g. a partner edge that drops the SYN) hangs the CLI - * indefinitely; the value is generous enough for cold-start OAuth discovery - * round-trips while still failing fast on a black-holed host. - */ -export const DEFAULT_CONNECT_TIMEOUT_MS = 15000; +export { DEFAULT_CONNECT_TIMEOUT_MS, withConnectTimeout }; type OutputFormat = "text" | "json"; -type MethodArgs = { - method?: string; - promptName?: string; - promptArgs?: Record; - uri?: string; - logLevel?: LoggingLevel; - toolName?: string; - toolArg?: Record; - toolMeta?: Record; - metadata?: Record; - appInfo?: boolean; - format?: OutputFormat; -}; - -/** - * {@link AppInfo} plus a CLI-only `resourceError` so a `resources/read` failure - * during the probe is reported instead of being silently swallowed (which would - * make "no CSP declared" indistinguishable from "resource unreadable"). - */ -export type CliAppInfo = AppInfo & { resourceError?: string }; - -/** - * Discriminated outcome from {@link callMethod}'s per-method runner. Most - * methods return a `result` (with optional collected `appInfo`) for - * {@link emitResult} to format; the `tools/list --app-info` NDJSON path writes - * its lines itself and reports `emitted` so the caller skips a second write. - */ -type MethodOutcome = - | { kind: "result"; result: McpResponse; appInfo?: CliAppInfo } - | { kind: "emitted" }; - -/** - * Tear down a managed list state if it was created for this method call. - * - * The `managed*State` locals below are assigned inside the `runMethod` closure, - * so TypeScript's control-flow analysis keeps them narrowed to `null` at the - * outer `finally` block (a closure "might not have run"). Routing the teardown - * through this helper — whose parameter type is the shared `destroy()` shape — - * both documents that and calls `destroy()` on whatever the closure actually - * assigned at runtime, with no `as` cast. - */ -function destroyManagedState(state: { destroy(): void } | null): void { - state?.destroy(); -} - async function callMethod( serverConfig: MCPServerConfig, serverSettings: InspectorServerSettings | undefined, @@ -141,6 +94,7 @@ async function callMethod( clientConfig: ClientConfig, cliAuthOverrides: RunnerClientConfigOverrides, callbackUrlConfig: RunnerOAuthCallbackConfig, + storedAuthOnly: boolean, ): Promise { // Version comes from the single source of truth — the root package.json — // via the shared core reader, not the CLI's own manifest. @@ -158,7 +112,7 @@ async function callMethod( formatRunnerOAuthRedirectUrl(callbackUrlConfig); environment.oauth = { storage: new NodeOAuthStorage(), - navigation: new ConsoleNavigation(), + navigation: createCliOAuthNavigation(), redirectUrlProvider, }; } @@ -185,182 +139,6 @@ async function callMethod( ...clientAuthOptions, }); - let managedToolsState: ManagedToolsState | null = null; - let managedResourcesState: ManagedResourcesState | null = null; - let managedResourceTemplatesState: ManagedResourceTemplatesState | null = - null; - let managedPromptsState: ManagedPromptsState | null = null; - - const runMethod = async (): Promise => { - let result: McpResponse; - let appInfo: CliAppInfo | undefined; - - if (args.method === "tools/list" || args.method === "tools/call") { - managedToolsState = new ManagedToolsState(inspectorClient); - managedToolsState.setMetadata(args.metadata); - await managedToolsState.refresh(); - } - - if (args.method === "resources/list") { - managedResourcesState = new ManagedResourcesState(inspectorClient); - managedResourcesState.setMetadata(args.metadata); - await managedResourcesState.refresh(); - } else if (args.method === "resources/templates/list") { - managedResourceTemplatesState = new ManagedResourceTemplatesState( - inspectorClient, - ); - managedResourceTemplatesState.setMetadata(args.metadata); - await managedResourceTemplatesState.refresh(); - } else if (args.method === "prompts/list") { - managedPromptsState = new ManagedPromptsState(inspectorClient); - managedPromptsState.setMetadata(args.metadata); - await managedPromptsState.refresh(); - } - - if (args.method === "tools/list") { - const tools = managedToolsState!.getTools(); - if (args.appInfo) { - // NDJSON: one app-info line per tool, all on a single connection. A - // caller that wants only the App tools can `| jq -c 'select(.hasApp)'`. - // collectAppInfo never throws — a tool with a malformed `_meta.ui` - // surfaces as `{hasApp:false, resourceError}` — so one bad tool can't - // abort the whole listing. Emitted verbatim as NDJSON regardless of - // --format (the list-probe shape is fixed; --format json only reshapes - // the single-result paths). - for (const tool of tools) { - const info = await collectAppInfo( - inspectorClient, - tool, - args.metadata, - ); - await awaitableLog(JSON.stringify(info) + "\n"); - } - return { kind: "emitted" }; - } - result = { tools }; - } else if (args.method === "tools/call") { - if (!args.toolName) { - throw new Error( - "Tool name is required for tools/call method. Use --tool-name to specify the tool name.", - ); - } - - const tool = managedToolsState! - .getTools() - .find((t) => t.name === args.toolName); - if (!tool) { - // Distinct from `isError:true` and (for --app-info) from "tool has no - // app": the named tool does not exist on the server. Exit TOOL_ERROR - // with `code: "tool_not_found"` so a caller can tell a typo/rename - // apart from a real tool failure or a no-app probe result. - throw new CliExitCodeError( - EXIT_CODES.TOOL_ERROR, - `Tool '${args.toolName}' not found on server.`, - { code: "tool_not_found" }, - ); - } - - // Only collect app-info when the caller asked for it (`--app-info` or - // `--format json`); a plain text-mode `tools/call` shouldn't fail just - // because the tool's `_meta.ui.resourceUri` is malformed or its resource - // is unreadable. - if (args.appInfo || args.format === "json") { - appInfo = await collectAppInfo(inspectorClient, tool, args.metadata); - } - if (args.appInfo) { - // --app-info: probe-only — emit the app metadata and skip the tool - // call entirely. The no-app exit code is handled in emitResult. - result = { ...appInfo }; - } else { - const invocation = await inspectorClient.callTool( - tool, - args.toolArg || {}, - args.metadata, - args.toolMeta, - ); - if (invocation.result !== null) { - result = invocation.result; - } else { - /* v8 ignore next 9 -- unreachable: InspectorClient.callTool either - throws on a tool-execution/transport error (caught by the outer - try) or returns an invocation whose `result` is guaranteed - non-null (attemptToolCall throws if finalResult is falsy). This - else is defensive only. */ - result = { - content: [ - { - type: "text" as const, - text: invocation.error || "Tool call failed", - }, - ], - isError: true, - }; - } - } - } else if (args.method === "resources/list") { - result = { - resources: managedResourcesState!.getResources(), - }; - } else if (args.method === "resources/read") { - if (!args.uri) { - throw new Error( - "URI is required for resources/read method. Use --uri to specify the resource URI.", - ); - } - - const invocation = await inspectorClient.readResource( - args.uri, - args.metadata, - ); - result = invocation.result; - } else if (args.method === "resources/templates/list") { - result = { - resourceTemplates: - managedResourceTemplatesState!.getResourceTemplates(), - }; - } else if (args.method === "prompts/list") { - result = { prompts: managedPromptsState!.getPrompts() }; - } else if (args.method === "prompts/get") { - if (!args.promptName) { - throw new Error( - "Prompt name is required for prompts/get method. Use --prompt-name to specify the prompt name.", - ); - } - - const invocation = await inspectorClient.getPrompt( - args.promptName, - args.promptArgs || {}, - args.metadata, - ); - result = invocation.result; - } else if (args.method === "initialize") { - // Connect-only probe: emit the cached InitializeResult fields so a - // caller can read serverInfo / protocolVersion / capabilities / - // instructions without picking a list method. - result = { - serverInfo: inspectorClient.getServerInfo(), - protocolVersion: inspectorClient.getProtocolVersion(), - capabilities: inspectorClient.getCapabilities(), - instructions: inspectorClient.getInstructions(), - }; - } else if (args.method === "logging/setLevel") { - if (!args.logLevel) { - throw new Error( - "Log level is required for logging/setLevel method. Use --log-level to specify the log level.", - ); - } - - await inspectorClient.setLoggingLevel(args.logLevel); - result = {}; - } else { - throw new Error( - `Unsupported method: ${args.method}. Supported methods include: initialize, tools/list, tools/call, resources/list, resources/read, resources/templates/list, prompts/list, prompts/get, logging/setLevel`, - ); - } - - return { kind: "result", result, appInfo }; - }; - try { await connectInspectorWithOAuth( inspectorClient, @@ -368,6 +146,7 @@ async function callMethod( redirectUrlProvider, callbackUrlConfig, serverSettings, + { storedAuthOnly }, ); const outcome = await withCliAuthRecoveryRetry( @@ -375,117 +154,17 @@ async function callMethod( redirectUrlProvider, callbackUrlConfig, serverSettings, - runMethod, + () => runMethod(inspectorClient, args), + undefined, + { storedAuthOnly }, ); - // The NDJSON `tools/list --app-info` path already wrote its lines; every - // other method hands its result to emitResult for format/exit handling. - if (outcome.kind === "result") { - await emitResult(outcome.result, outcome.appInfo, args); - } + await consumeMethodOutcome(outcome, args); } finally { - destroyManagedState(managedToolsState); - destroyManagedState(managedResourcesState); - destroyManagedState(managedResourceTemplatesState); - destroyManagedState(managedPromptsState); await inspectorClient.disconnect(); } } -/** - * Write the method result (and any app-info) to stdout, honouring `--format` - * and `--app-info`, then map `isError`/no-app outcomes onto the exit-code map. - * Extracted from `callMethod` so the format/exit handling is in one place. - */ -export async function emitResult( - result: McpResponse, - appInfo: CliAppInfo | undefined, - args: MethodArgs, -): Promise { - const json = args.format === "json"; - - if (args.appInfo) { - const info: CliAppInfo = appInfo ?? { - hasApp: false, - toolName: args.toolName ?? "", - }; - // Single-line JSON either way; --format json wraps it under an `appInfo` - // key so the envelope shape is uniform with the non-probe path. - await awaitableLog(JSON.stringify(json ? { appInfo: info } : info) + "\n"); - if (!info.hasApp) { - throw new CliExitCodeError( - EXIT_CODES.NO_APP, - `Tool '${args.toolName}' has no MCP App UI resource (_meta.ui.resourceUri).`, - ); - } - return; - } - - if (json) { - // One JSON object on stdout — `result` plus, when present, `appInfo` as a - // sibling key. No `--- MCP App Info ---` banner, so `| jq` works for App - // tools as well as plain ones. - const envelope: Record = { result }; - if (appInfo?.hasApp) envelope.appInfo = appInfo; - await awaitableLog(JSON.stringify(envelope) + "\n"); - } else { - // Text mode emits the result only; app-info is not collected on this path - // (use `--format json` or `--app-info` to get it). - await awaitableLog(JSON.stringify(result, null, 2) + "\n"); - } - - // A tool that returned `isError:true` (or whose call failed) is still printed - // above so the caller sees the payload, but the process exits TOOL_ERROR so - // `&&` chains don't proceed on a failed call. - if ((result as { isError?: unknown }).isError === true) { - throw new CliExitCodeError( - EXIT_CODES.TOOL_ERROR, - `Tool '${args.toolName}' returned isError:true.`, - { code: "tool_is_error" }, - ); - } -} - -/** - * Build the CLI's app-info for a tool: extract the tool-side `_meta.ui` and, - * when the tool advertises a UI resource, follow it with a `resources/read` so - * the resource-side csp/permissions/domain are included. - * - * Never throws — the two failure modes both fold into a `{hasApp:false, - * resourceError}` result rather than propagating: - * - a malformed `_meta.ui.resourceUri` (extractAppInfo throws), so the - * `tools/list --app-info` NDJSON loop stays per-tool tolerant (one bad tool - * can't abort the whole listing); - * - a `resources/read` failure, since "tool says it has an app but the - * resource is unreadable" is itself a useful probe result. - */ -export async function collectAppInfo( - client: Pick, - tool: Parameters[0], - metadata: Record | undefined, -): Promise { - let base: AppInfo; - try { - base = extractAppInfo(tool); - } catch (e) { - return { - hasApp: false, - toolName: tool.name, - resourceError: e instanceof Error ? e.message : String(e), - }; - } - if (!base.hasApp || base.resourceUri === undefined) return base; - try { - const read = await client.readResource(base.resourceUri, metadata); - return extractAppInfo(tool, read.result); - } catch (e) { - return { - ...base, - resourceError: e instanceof Error ? e.message : String(e), - }; - } -} - /** * Canonicalise a server URL the same way the web inspector does before storing * OAuth state (`new URL().href` lowercases the host, normalises the scheme, and @@ -754,32 +433,6 @@ function buildHandoff( }; } -/** - * Apply a connection timeout to a resolved server's settings, building a - * minimal {@link InspectorServerSettings} when none came from the file. Ad-hoc - * invocations get {@link DEFAULT_CONNECT_TIMEOUT_MS} so a black-holed host - * fails fast; catalog/config invocations keep their file-level timeout unless - * `--connect-timeout` is passed explicitly. - */ -export function withConnectTimeout( - settings: InspectorServerSettings | undefined, - connectionTimeout: number | undefined, -): InspectorServerSettings | undefined { - if (connectionTimeout === undefined) return settings; - if (settings) return { ...settings, connectionTimeout }; - return { - headers: [], - metadata: [], - env: [], - connectionTimeout, - requestTimeout: 0, - taskTtl: DEFAULT_TASK_TTL_MS, - maxFetchRequests: DEFAULT_MAX_FETCH_REQUESTS, - autoRefreshOnListChanged: false, - roots: [], - }; -} - function parseKeyValuePair( value: string, previous: Record = {}, @@ -815,6 +468,7 @@ type ParseResult = clientSecret?: string; clientMetadataUrl?: string; callbackUrl?: string; + storedAuthOnly?: boolean; } // Short-circuit modes (`--list-stored-auth`, `--print-handoff`) do their own // output and need no server connection; runCli returns immediately. @@ -998,6 +652,14 @@ async function parseArgs(argv?: string[]): Promise { "--use-stored-auth", "Read the OAuth access token for --server-url from the OAuth state file (written by the web inspector) and inject it as Authorization: Bearer.", ) + .option( + "--stored-auth-only", + "Never start interactive OAuth; use the shared store if present, otherwise fail with auth_required. No-op when the server does not require auth.", + ) + .option( + "--relogin", + "Ignore stored OAuth for this run (HTTP/SSE URL keys only); interactive login runs only if the server requires auth. No-op for stdio / servers with no stored entry", + ) .option( "--wait-for-auth ", "Poll the OAuth state file until a token for --server-url appears (or the timeout elapses), then proceed as if --use-stored-auth were set. Use after handing off to a human to complete OAuth in a browser.", @@ -1050,11 +712,24 @@ async function parseArgs(argv?: string[]): Promise { clientMetadataUrl?: string; callbackUrl?: string; useStoredAuth?: boolean; + storedAuthOnly?: boolean; + relogin?: boolean; waitForAuth?: number; listStoredAuth?: boolean; printHandoff?: boolean; }; + if (options.relogin) { + if (options.storedAuthOnly) { + throw new Error("--relogin cannot be combined with --stored-auth-only"); + } + if (options.useStoredAuth || options.waitForAuth !== undefined) { + throw new Error( + "--relogin cannot be combined with --use-stored-auth or --wait-for-auth", + ); + } + } + // State-path precedence (getStateFilePath): MCP_INSPECTOR_OAUTH_STATE_PATH → // /oauth.json → ~/.mcp-inspector/storage/oauth.json — the // same file the web backend writes, so tokens are shared across surfaces. @@ -1083,6 +758,21 @@ async function parseArgs(argv?: string[]): Promise { return { shortCircuit: true }; } + // Validate --method before stored-auth network work (refresh / wait) so a + // typo or stream method fails locally without burning a token round-trip. + if (!options.method) { + throw new Error( + "Method is required. Use --method to specify the method to invoke.", + ); + } + const isCatalogMethod = + options.method === "servers/list" || options.method === "servers/show"; + if (!isCatalogMethod && !isOneShotMethod(options.method)) { + throw new Error( + `Unsupported method: ${options.method}. Supported --cli methods: ${ONE_SHOT_METHODS.join(", ")}, servers/list, servers/show.`, + ); + } + // Honour MCP_CATALOG_PATH only when no ad-hoc target is given. Applying it // unconditionally meant a homespace that exports the env var could never run // `--server-url …` (serverSourceConflict rejects catalog + ad-hoc). @@ -1172,6 +862,29 @@ async function parseArgs(argv?: string[]): Promise { }; } + // Catalog list / show — no MCP connection. + if (options.method === "servers/list") { + const servers = await listServerEntries(serverOptions); + await writeFormattedResult( + { servers }, + options.format === "json" ? "json" : "text", + ); + return { shortCircuit: true }; + } + if (options.method === "servers/show") { + if (!options.server?.trim()) { + throw new Error( + "servers/show requires --server to select a catalog entry.", + ); + } + const server = await showServerEntry(options.server, serverOptions); + await writeFormattedResult( + server, + options.format === "json" ? "json" : "text", + ); + return { shortCircuit: true }; + } + // Shared with the TUI: resolves the catalog/config source (or ad-hoc target), // enforces the conflict matrix, and lifts disk headers/timeouts/OAuth into // per-server settings. `--server` selects one when the file has several. @@ -1186,10 +899,8 @@ async function parseArgs(argv?: string[]): Promise { options.connectTimeout ?? (adHoc ? DEFAULT_CONNECT_TIMEOUT_MS : undefined), ); - if (!options.method) { - throw new Error( - "Method is required. Use --method to specify the method to invoke.", - ); + if (options.relogin && "url" in serverConfig && serverConfig.url) { + await clearStoredAuthForRelogin(serverConfig.url); } if ( @@ -1241,7 +952,7 @@ async function parseArgs(argv?: string[]): Promise { ? Object.fromEntries( Object.entries(options.metadata).map(([key, value]) => [ key, - String(value), + metaValueToString(value), ]), ) : undefined, @@ -1249,7 +960,7 @@ async function parseArgs(argv?: string[]): Promise { ? Object.fromEntries( Object.entries(options.toolMetadata).map(([key, value]) => [ key, - String(value), + metaValueToString(value), ]), ) : undefined, @@ -1266,6 +977,7 @@ async function parseArgs(argv?: string[]): Promise { clientSecret: options.clientSecret, clientMetadataUrl: options.clientMetadataUrl, callbackUrl: options.callbackUrl, + storedAuthOnly: options.storedAuthOnly === true, }; } @@ -1282,6 +994,7 @@ export async function runCli(argv?: string[]): Promise { clientSecret, clientMetadataUrl, callbackUrl, + storedAuthOnly, } = parsed; const clientConfig = await loadRunnerClientConfig({ clientConfigPath }); const callbackUrlConfig = parseRunnerOAuthCallbackUrl(callbackUrl); @@ -1296,5 +1009,6 @@ export async function runCli(argv?: string[]): Promise { clientMetadataUrl, }, callbackUrlConfig, + storedAuthOnly === true, ); } diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index 24aa60582..a15bc0c4f 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -17,6 +17,21 @@ import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; import { isOAuthCapableServerConfig } from "@inspector/core/client/runner.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; import { createInterface } from "node:readline/promises"; +import { CliExitCodeError, EXIT_CODES } from "./error-handler.js"; + +export type CliOAuthConnectOptions = { + /** + * When true, never open interactive OAuth / step-up prompts. Use the shared + * store if it can satisfy the challenge; otherwise fail with AUTH_REQUIRED. + */ + storedAuthOnly?: boolean; +}; + +function storedAuthOnlyFailure(message: string): never { + throw new CliExitCodeError(EXIT_CODES.AUTH_REQUIRED, message, { + code: "auth_required", + }); +} /** Standard-OAuth step-up (not EMA silent re-mint). */ export function isStandardOAuthStepUp( @@ -111,6 +126,7 @@ export async function connectInspectorWithOAuth( redirectUrlProvider: MutableRedirectUrlProvider, callbackUrlConfig: RunnerOAuthCallbackConfig, serverSettings?: InspectorServerSettings, + options?: CliOAuthConnectOptions, ): Promise { try { await inspectorClient.connect(); @@ -126,6 +142,12 @@ export async function connectInspectorWithOAuth( await inspectorClient.connect(); return; } + if (options?.storedAuthOnly) { + storedAuthOnlyFailure( + err.message || + "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", + ); + } await handleCliAuthRecoveryRequired( inspectorClient, err, @@ -138,6 +160,13 @@ export async function connectInspectorWithOAuth( } if (isUnauthorizedError(err)) { + if (options?.storedAuthOnly) { + storedAuthOnlyFailure( + err instanceof Error + ? err.message + : "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", + ); + } await inspectorClient.disconnect().catch(() => {}); await runCliInteractiveOAuth( inspectorClient, @@ -163,6 +192,7 @@ export async function withCliAuthRecoveryRetry( serverSettings: InspectorServerSettings | undefined, fn: () => Promise, confirmStepUp: () => Promise = confirmStepUpFromStdin, + options?: CliOAuthConnectOptions, ): Promise { try { return await fn(); @@ -170,6 +200,12 @@ export async function withCliAuthRecoveryRetry( if (!(err instanceof AuthRecoveryRequiredError)) { throw err; } + if (options?.storedAuthOnly) { + storedAuthOnlyFailure( + err.message || + "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", + ); + } await handleCliAuthRecoveryRequired( inspectorClient, err, diff --git a/clients/cli/src/handlers/collect-app-info.ts b/clients/cli/src/handlers/collect-app-info.ts new file mode 100644 index 000000000..aa2df1fc1 --- /dev/null +++ b/clients/cli/src/handlers/collect-app-info.ts @@ -0,0 +1,35 @@ +import { InspectorClient } from "@inspector/core/mcp/index.js"; +import { extractAppInfo } from "@inspector/core/mcp/apps.js"; +import type { AppInfo } from "@inspector/core/mcp/apps.js"; +import type { CliAppInfo } from "./method-types.js"; + +/** + * Build the CLI's app-info for a tool. Never throws — failures fold into + * `{hasApp:false, resourceError}` so list probes stay per-tool tolerant. + */ +export async function collectAppInfo( + client: Pick, + tool: Parameters[0], + metadata: Record | undefined, +): Promise { + let base: AppInfo; + try { + base = extractAppInfo(tool); + } catch (e) { + return { + hasApp: false, + toolName: tool.name, + resourceError: e instanceof Error ? e.message : String(e), + }; + } + if (!base.hasApp || base.resourceUri === undefined) return base; + try { + const read = await client.readResource(base.resourceUri, metadata); + return extractAppInfo(tool, read.result); + } catch (e) { + return { + ...base, + resourceError: e instanceof Error ? e.message : String(e), + }; + } +} diff --git a/clients/cli/src/handlers/connect-timeout.ts b/clients/cli/src/handlers/connect-timeout.ts new file mode 100644 index 000000000..7d5a7b2cc --- /dev/null +++ b/clients/cli/src/handlers/connect-timeout.ts @@ -0,0 +1,35 @@ +import { + DEFAULT_MAX_FETCH_REQUESTS, + DEFAULT_TASK_TTL_MS, + type InspectorServerSettings, +} from "@inspector/core/mcp/types.js"; + +/** + * Default connect timeout (ms) for ad-hoc server invocations. Without this an + * unreachable server hangs the CLI indefinitely. + */ +export const DEFAULT_CONNECT_TIMEOUT_MS = 15000; + +/** + * Apply a connection timeout to a resolved server's settings, building a + * minimal {@link InspectorServerSettings} when none came from the file. + */ +export function withConnectTimeout( + settings: InspectorServerSettings | undefined, + connectionTimeout: number | undefined, +): InspectorServerSettings | undefined { + if (connectionTimeout === undefined) return settings; + if (settings) return { ...settings, connectionTimeout }; + return { + headers: [], + metadata: [], + env: [], + connectionTimeout, + requestTimeout: 0, + taskTtl: DEFAULT_TASK_TTL_MS, + maxFetchRequests: DEFAULT_MAX_FETCH_REQUESTS, + autoRefreshOnListChanged: false, + paginatedLists: false, + roots: [], + }; +} diff --git a/clients/cli/src/handlers/consume-outcome.ts b/clients/cli/src/handlers/consume-outcome.ts new file mode 100644 index 000000000..89b4ac7a8 --- /dev/null +++ b/clients/cli/src/handlers/consume-outcome.ts @@ -0,0 +1,37 @@ +import { awaitableLog } from "../utils/awaitable-log.js"; +import { emitResult } from "./emit-result.js"; +import type { MethodArgs, MethodOutcome } from "./method-types.js"; + +/** + * Write a {@link MethodOutcome} to stdout (result / NDJSON / long-lived stream). + * Stream methods stay attached until SIGINT/SIGTERM. + */ +export async function consumeMethodOutcome( + outcome: MethodOutcome, + args: MethodArgs, +): Promise { + if (outcome.kind === "result") { + await emitResult(outcome.result, outcome.appInfo, args); + return; + } + if (outcome.kind === "ndjson") { + for (const line of outcome.lines) { + await awaitableLog(JSON.stringify(line) + "\n"); + } + return; + } + + await new Promise((resolve) => { + const stop = outcome.start((obj) => { + void awaitableLog(JSON.stringify(obj) + "\n"); + }); + const onSignal = () => { + stop(); + process.off("SIGINT", onSignal); + process.off("SIGTERM", onSignal); + resolve(); + }; + process.on("SIGINT", onSignal); + process.on("SIGTERM", onSignal); + }); +} diff --git a/clients/cli/src/handlers/emit-result.ts b/clients/cli/src/handlers/emit-result.ts new file mode 100644 index 000000000..b4d370deb --- /dev/null +++ b/clients/cli/src/handlers/emit-result.ts @@ -0,0 +1,46 @@ +import { awaitableLog } from "../utils/awaitable-log.js"; +import { CliExitCodeError, EXIT_CODES } from "../error-handler.js"; +import type { CliAppInfo, McpResponse, MethodArgs } from "./method-types.js"; + +/** + * Write the method result (and any app-info) to stdout, honouring `--format` + * and `--app-info`, then map `isError`/no-app outcomes onto the exit-code map. + */ +export async function emitResult( + result: McpResponse, + appInfo: CliAppInfo | undefined, + args: MethodArgs, +): Promise { + const json = args.format === "json"; + + if (args.appInfo) { + const info: CliAppInfo = appInfo ?? { + hasApp: false, + toolName: args.toolName ?? "", + }; + await awaitableLog(JSON.stringify(json ? { appInfo: info } : info) + "\n"); + if (!info.hasApp) { + throw new CliExitCodeError( + EXIT_CODES.NO_APP, + `Tool '${args.toolName}' has no MCP App UI resource (_meta.ui.resourceUri).`, + ); + } + return; + } + + if (json) { + const envelope: Record = { result }; + if (appInfo?.hasApp) envelope.appInfo = appInfo; + await awaitableLog(JSON.stringify(envelope) + "\n"); + } else { + await awaitableLog(JSON.stringify(result, null, 2) + "\n"); + } + + if ((result as { isError?: unknown }).isError === true) { + throw new CliExitCodeError( + EXIT_CODES.TOOL_ERROR, + `Tool '${args.toolName}' returned isError:true.`, + { code: "tool_is_error" }, + ); + } +} diff --git a/clients/cli/src/handlers/format-output.ts b/clients/cli/src/handlers/format-output.ts new file mode 100644 index 000000000..d6ceaa975 --- /dev/null +++ b/clients/cli/src/handlers/format-output.ts @@ -0,0 +1,18 @@ +import { awaitableLog } from "../utils/awaitable-log.js"; + +export type OutputFormat = "text" | "json"; + +/** + * Write a command result to stdout. Text = pretty-printed JSON; json = single + * `{ "result": … }` envelope (same family as CLI `--format json`). + */ +export async function writeFormattedResult( + result: unknown, + format: OutputFormat = "text", +): Promise { + if (format === "json") { + await awaitableLog(JSON.stringify({ result }) + "\n"); + return; + } + await awaitableLog(JSON.stringify(result, null, 2) + "\n"); +} diff --git a/clients/cli/src/handlers/method-types.ts b/clients/cli/src/handlers/method-types.ts new file mode 100644 index 000000000..ce5344a21 --- /dev/null +++ b/clients/cli/src/handlers/method-types.ts @@ -0,0 +1,116 @@ +import type { JsonValue } from "@inspector/core/mcp/index.js"; +import type { AppInfo } from "@inspector/core/mcp/apps.js"; +import type { LoggingLevel } from "@modelcontextprotocol/client"; +import type { OutputFormat } from "./format-output.js"; + +export type { OutputFormat }; + +/** + * {@link AppInfo} plus a CLI-only `resourceError` so a `resources/read` failure + * during the probe is reported instead of being silently swallowed. + */ +export type CliAppInfo = AppInfo & { resourceError?: string }; + +/** Arguments for a single MCP method invocation via {@link runMethod}. */ +export type MethodArgs = { + method?: string; + promptName?: string; + promptArgs?: Record; + uri?: string; + logLevel?: LoggingLevel; + toolName?: string; + toolArg?: Record; + toolMeta?: Record; + metadata?: Record; + appInfo?: boolean; + format?: OutputFormat; + /** Task id for tasks/get, tasks/cancel, tasks/result. */ + taskId?: string; + /** When true, tools/call uses callToolStream (task-augmented). */ + task?: boolean; + /** roots/set payload (JSON array of {uri, name?}). */ + rootsJson?: string; + /** prompts/complete: argument name / value / ref. */ + completeRefType?: "ref/prompt" | "ref/resource"; + completeRef?: string; + completeArgName?: string; + completeArgValue?: string; +}; + +export type McpResponse = Record; + +/** + * Discriminated outcome from {@link runMethod}. Most methods return a `result` + * for formatting; `tools/list --app-info` returns `ndjson` lines for the + * caller to write. Stream methods return `stream` for a long-lived consumer. + */ +export type MethodOutcome = + | { kind: "result"; result: McpResponse; appInfo?: CliAppInfo } + /** One JSON object per line (e.g. tools/list --app-info). Caller writes stdout. */ + | { kind: "ndjson"; lines: unknown[] } + | { + kind: "stream"; + /** Human label for errors. */ + label: string; + /** Subscribe and push NDJSON object lines; return an unsubscribe. */ + start: (writeLine: (obj: unknown) => void) => () => void; + }; + +/** Methods supported by {@link runMethod}. */ +export const SESSION_RPC_METHODS = [ + "initialize", + "tools/list", + "tools/call", + "resources/list", + "resources/read", + "resources/templates/list", + "resources/subscribe", + "resources/unsubscribe", + "prompts/list", + "prompts/get", + "prompts/complete", + "logging/setLevel", + "logging/tail", + "tasks/list", + "tasks/get", + "tasks/cancel", + "tasks/result", + "roots/list", + "roots/set", +] as const; + +export type SessionRpcMethod = (typeof SESSION_RPC_METHODS)[number]; + +/** + * Methods accepted by `mcp-inspector --cli` (plus catalog-only + * `servers/list` / `servers/show`, handled before connect). Stream methods and + * long-lived RPCs (`logging/tail`, `tasks/*`, …) are rejected so the CLI + * never hangs waiting for SIGINT. + */ +export const ONE_SHOT_METHODS = [ + "initialize", + "tools/list", + "tools/call", + "resources/list", + "resources/read", + "resources/templates/list", + "prompts/list", + "prompts/get", + "logging/setLevel", +] as const; + +export type OneShotMethod = (typeof ONE_SHOT_METHODS)[number]; + +export function isOneShotMethod(method: string): method is OneShotMethod { + return (ONE_SHOT_METHODS as readonly string[]).includes(method); +} + +/** + * Encode a metadata / tool-metadata value for MethodArgs (string map). + * Strings pass through; objects/arrays use JSON.stringify so structure is kept + * (avoids String(obj) → "[object Object]"). + */ +export function metaValueToString(value: JsonValue): string { + if (typeof value === "string") return value; + return JSON.stringify(value); +} diff --git a/clients/cli/src/handlers/run-method.ts b/clients/cli/src/handlers/run-method.ts new file mode 100644 index 000000000..8d94e2978 --- /dev/null +++ b/clients/cli/src/handlers/run-method.ts @@ -0,0 +1,310 @@ +import type { InspectorClient } from "@inspector/core/mcp/index.js"; +import type { Root } from "@modelcontextprotocol/client"; +import { + ManagedToolsState, + ManagedResourcesState, + ManagedResourceTemplatesState, + ManagedPromptsState, + ManagedRequestorTasksState, + MessageLogState, +} from "@inspector/core/mcp/state/index.js"; +import { CliExitCodeError, EXIT_CODES } from "../error-handler.js"; +import { collectAppInfo } from "./collect-app-info.js"; +import type { + CliAppInfo, + McpResponse, + MethodArgs, + MethodOutcome, +} from "./method-types.js"; + +/** + * Run one MCP method against a connected {@link InspectorClient}. + * Core method dispatch used by the CLI (and other Inspector Node runners). + */ +export async function runMethod( + inspectorClient: InspectorClient, + args: MethodArgs & { method: string }, +): Promise { + let managedToolsState: ManagedToolsState | null = null; + let managedResourcesState: ManagedResourcesState | null = null; + let managedResourceTemplatesState: ManagedResourceTemplatesState | null = + null; + let managedPromptsState: ManagedPromptsState | null = null; + let managedTasksState: ManagedRequestorTasksState | null = null; + + try { + let result: McpResponse; + let appInfo: CliAppInfo | undefined; + + if (args.method === "tools/list" || args.method === "tools/call") { + managedToolsState = new ManagedToolsState(inspectorClient); + managedToolsState.setMetadata(args.metadata); + await managedToolsState.refresh(); + } + + if (args.method === "resources/list") { + managedResourcesState = new ManagedResourcesState(inspectorClient); + managedResourcesState.setMetadata(args.metadata); + await managedResourcesState.refresh(); + } else if (args.method === "resources/templates/list") { + managedResourceTemplatesState = new ManagedResourceTemplatesState( + inspectorClient, + ); + managedResourceTemplatesState.setMetadata(args.metadata); + await managedResourceTemplatesState.refresh(); + } else if (args.method === "prompts/list") { + managedPromptsState = new ManagedPromptsState(inspectorClient); + managedPromptsState.setMetadata(args.metadata); + await managedPromptsState.refresh(); + } else if (args.method === "tasks/list") { + managedTasksState = new ManagedRequestorTasksState(inspectorClient); + await managedTasksState.refresh(); + } + + if (args.method === "tools/list") { + const tools = managedToolsState!.getTools(); + if (args.appInfo) { + const lines: unknown[] = []; + for (const tool of tools) { + lines.push( + await collectAppInfo(inspectorClient, tool, args.metadata), + ); + } + return { kind: "ndjson", lines }; + } + result = { tools }; + } else if (args.method === "tools/call") { + if (!args.toolName) { + throw new Error( + "Tool name is required for tools/call method. Use --tool-name to specify the tool name.", + ); + } + + const tool = managedToolsState! + .getTools() + .find((t) => t.name === args.toolName); + if (!tool) { + throw new CliExitCodeError( + EXIT_CODES.TOOL_ERROR, + `Tool '${args.toolName}' not found on server.`, + { code: "tool_not_found" }, + ); + } + + if (args.appInfo || args.format === "json") { + appInfo = await collectAppInfo(inspectorClient, tool, args.metadata); + } + if (args.appInfo) { + result = { ...appInfo }; + } else { + const invocation = args.task + ? await inspectorClient.callToolStream( + tool, + args.toolArg || {}, + args.metadata, + args.toolMeta, + ) + : await inspectorClient.callTool( + tool, + args.toolArg || {}, + args.metadata, + args.toolMeta, + ); + if (invocation.result !== null) { + result = invocation.result; + } else { + result = { + content: [ + { + type: "text" as const, + text: invocation.error || "Tool call failed", + }, + ], + isError: true, + }; + } + } + } else if (args.method === "resources/list") { + result = { resources: managedResourcesState!.getResources() }; + } else if (args.method === "resources/read") { + if (!args.uri) { + throw new Error( + "URI is required for resources/read method. Use --uri to specify the resource URI.", + ); + } + const invocation = await inspectorClient.readResource( + args.uri, + args.metadata, + ); + result = invocation.result; + } else if (args.method === "resources/templates/list") { + result = { + resourceTemplates: + managedResourceTemplatesState!.getResourceTemplates(), + }; + } else if (args.method === "resources/subscribe") { + if (!args.uri) { + throw new Error( + "URI is required for resources/subscribe. Use --uri to specify the resource URI.", + ); + } + await inspectorClient.subscribeToResource(args.uri); + return { + kind: "stream", + label: "resources/subscribe", + start: (writeLine) => { + writeLine({ type: "subscribed", uri: args.uri }); + const onUpdate = (ev: Event) => { + const detail = (ev as CustomEvent<{ uri: string }>).detail; + writeLine({ + type: "resources/updated", + uri: detail?.uri ?? args.uri, + }); + }; + inspectorClient.addEventListener("resourceUpdated", onUpdate); + return () => { + inspectorClient.removeEventListener("resourceUpdated", onUpdate); + void inspectorClient.unsubscribeFromResource(args.uri!); + }; + }, + }; + } else if (args.method === "resources/unsubscribe") { + if (!args.uri) { + throw new Error( + "URI is required for resources/unsubscribe. Use --uri to specify the resource URI.", + ); + } + await inspectorClient.unsubscribeFromResource(args.uri); + result = { unsubscribed: true, uri: args.uri }; + } else if (args.method === "prompts/list") { + result = { prompts: managedPromptsState!.getPrompts() }; + } else if (args.method === "prompts/get") { + if (!args.promptName) { + throw new Error( + "Prompt name is required for prompts/get method. Use --prompt-name to specify the prompt name.", + ); + } + const invocation = await inspectorClient.getPrompt( + args.promptName, + args.promptArgs || {}, + args.metadata, + ); + result = invocation.result; + } else if (args.method === "prompts/complete") { + if (!args.completeRefType || !args.completeRef || !args.completeArgName) { + throw new Error( + "prompts/complete requires --complete-ref-type, --complete-ref, and --complete-arg-name.", + ); + } + const ref = + args.completeRefType === "ref/prompt" + ? ({ type: "ref/prompt", name: args.completeRef } as const) + : ({ type: "ref/resource", uri: args.completeRef } as const); + result = await inspectorClient.getCompletions( + ref, + args.completeArgName, + args.completeArgValue ?? "", + undefined, + args.metadata, + ); + } else if (args.method === "initialize") { + result = { + serverInfo: inspectorClient.getServerInfo(), + protocolVersion: inspectorClient.getProtocolVersion(), + capabilities: inspectorClient.getCapabilities(), + instructions: inspectorClient.getInstructions(), + }; + } else if (args.method === "logging/setLevel") { + if (!args.logLevel) { + throw new Error( + "Log level is required for logging/setLevel method. Use --log-level to specify the log level.", + ); + } + await inspectorClient.setLoggingLevel(args.logLevel); + result = {}; + } else if (args.method === "logging/tail") { + return { + kind: "stream", + label: "logging/tail", + start: (writeLine) => { + const log = new MessageLogState(inspectorClient); + const onMessage = (ev: Event) => { + const entry = (ev as CustomEvent).detail as { + direction?: string; + message?: { method?: string }; + }; + // Follow server logging notifications only (not full RPC history). + if ( + entry.direction === "notification" && + entry.message?.method === "notifications/message" + ) { + writeLine(entry); + } + }; + log.addEventListener("message", onMessage); + return () => { + log.removeEventListener("message", onMessage); + log.destroy(); + }; + }, + }; + } else if (args.method === "tasks/list") { + result = { tasks: managedTasksState!.getTasks() }; + } else if (args.method === "tasks/get") { + if (!args.taskId) { + throw new Error("Task id is required for tasks/get. Use --task-id."); + } + result = { + task: await inspectorClient.getRequestorTask(args.taskId), + }; + } else if (args.method === "tasks/cancel") { + if (!args.taskId) { + throw new Error("Task id is required for tasks/cancel. Use --task-id."); + } + await inspectorClient.cancelRequestorTask(args.taskId); + result = { cancelled: true, taskId: args.taskId }; + } else if (args.method === "tasks/result") { + if (!args.taskId) { + throw new Error("Task id is required for tasks/result. Use --task-id."); + } + // SDK return type is a protocol result object; we only need a JSON bag. + result = (await inspectorClient.getRequestorTaskResult( + args.taskId, + )) as McpResponse; + } else if (args.method === "roots/list") { + result = { roots: inspectorClient.getRoots() }; + } else if (args.method === "roots/set") { + if (!args.rootsJson) { + throw new Error( + "roots/set requires --roots-json ''.", + ); + } + let roots: Root[]; + try { + const parsed: unknown = JSON.parse(args.rootsJson); + if (!Array.isArray(parsed)) { + throw new Error("must be a JSON array"); + } + roots = parsed as Root[]; + } catch (e) { + throw new Error( + `--roots-json is invalid: ${e instanceof Error ? e.message : String(e)}`, + ); + } + await inspectorClient.setRoots(roots); + result = { roots: inspectorClient.getRoots() }; + } else { + throw new Error( + `Unsupported method: ${args.method}. Supported methods include: initialize, tools/*, resources/*, prompts/*, logging/*, tasks/*, roots/*.`, + ); + } + + return { kind: "result", result, appInfo }; + } finally { + managedToolsState?.destroy(); + managedResourcesState?.destroy(); + managedResourceTemplatesState?.destroy(); + managedPromptsState?.destroy(); + managedTasksState?.destroy(); + } +} diff --git a/clients/cli/src/handlers/servers-list.ts b/clients/cli/src/handlers/servers-list.ts new file mode 100644 index 000000000..8188f7035 --- /dev/null +++ b/clients/cli/src/handlers/servers-list.ts @@ -0,0 +1,212 @@ +import type { + InspectorServerSettings, + MCPServerConfig, +} from "@inspector/core/mcp/types.js"; +import { + loadServerEntries, + selectServerEntry, + type ServerLoadOptions, +} from "@inspector/core/mcp/node/index.js"; + +/** One catalog/config entry as returned by `servers/list`. */ +export type ServerListEntry = { + name: string; + type: string; + /** Command line, URL, or other short identity for display. */ + detail: string; + /** + * Optional live-session name when a caller annotates catalog entries + * with connected sessions (omitted for plain catalog listing). + */ + session?: string; + /** True when that session is the most-recently-used connected session. */ + isMru?: boolean; +}; + +/** Minimal session shape needed to annotate catalog entries. */ +export type SessionListRef = { + name: string; + isMru?: boolean; +}; + +/** + * Mark catalog entries that have a live session with the same name. + * Does not mutate `entries`. + */ +export function annotateServerEntriesWithSessions( + entries: ServerListEntry[], + sessions: SessionListRef[], +): ServerListEntry[] { + if (sessions.length === 0) return entries; + const byName = new Map(sessions.map((s) => [s.name, s] as const)); + return entries.map((entry) => { + const session = byName.get(entry.name); + if (!session) return entry; + return { + ...entry, + session: session.name, + ...(session.isMru === true ? { isMru: true } : {}), + }; + }); +} + +/** Detail view for `servers/show` (secrets redacted). */ +export type ServerShowEntry = { + name: string; + type: string; + detail: string; + config: Record; + settings?: Record; +}; + +const REDACTED = "[redacted]"; + +/** + * Summarise an {@link MCPServerConfig} for catalog listing (no connection). + */ +export function summarizeServerConfig(config: MCPServerConfig): { + type: string; + detail: string; +} { + // Narrow on the URL-bearing transports first — stdio's `type` is optional, so + // an else-after-`=== "stdio"` check would still leave `StdioServerConfig` in + // the residual union (see `StdioServerConfig` in core/mcp/types.ts). + if (config.type === "sse" || config.type === "streamable-http") { + return { type: config.type, detail: config.url ?? "" }; + } + const args = config.args?.length ? ` ${config.args.join(" ")}` : ""; + return { type: "stdio", detail: `${config.command}${args}` }; +} + +/** + * Load catalog/config entries and return a sorted name + summary list. + */ +export async function listServerEntries( + serverOptions: ServerLoadOptions = {}, +): Promise { + const entries = await loadServerEntries(serverOptions); + return Object.entries(entries) + .map(([name, resolved]) => { + const { type, detail } = summarizeServerConfig(resolved.config); + return { name, type, detail }; + }) + .sort((a, b) => a.name.localeCompare(b.name)); +} + +/** + * Resolve one catalog/config entry for `servers/show` (no MCP connection). + * Secret-bearing fields (env values, OAuth client secret, sensitive headers) + * are replaced with {@link REDACTED}. + */ +export async function showServerEntry( + serverName: string, + serverOptions: ServerLoadOptions = {}, +): Promise { + const name = serverName.trim(); + if (!name) { + throw new Error("servers/show requires a server name."); + } + const entries = await loadServerEntries(serverOptions); + const selected = selectServerEntry(entries, name); + const { type, detail } = summarizeServerConfig(selected.config); + const result: ServerShowEntry = { + name, + type, + detail, + config: sanitizeServerConfig(selected.config), + }; + if (selected.settings) { + result.settings = sanitizeServerSettings(selected.settings); + } + return result; +} + +/** Visible for tests. */ +export function sanitizeServerConfig( + config: MCPServerConfig, +): Record { + const out: Record = { ...config }; + if ("env" in config && config.env) { + out.env = redactStringRecord(config.env); + } + if ("requestInit" in config && isPlainObject(config.requestInit)) { + out.requestInit = sanitizeInitRecord(config.requestInit); + } + if ("eventSourceInit" in config && isPlainObject(config.eventSourceInit)) { + out.eventSourceInit = sanitizeInitRecord(config.eventSourceInit); + } + return out; +} + +/** Visible for tests. */ +export function sanitizeServerSettings( + settings: InspectorServerSettings, +): Record { + const out: Record = { + ...settings, + headers: settings.headers.map((h) => ({ + key: h.key, + value: isSensitiveHeader(h.key) ? REDACTED : h.value, + })), + env: settings.env.map((e) => ({ + key: e.key, + value: REDACTED, + })), + }; + if (settings.oauthClientSecret !== undefined) { + out.oauthClientSecret = REDACTED; + } + return out; +} + +function redactStringRecord( + record: Record, +): Record { + return Object.fromEntries(Object.keys(record).map((key) => [key, REDACTED])); +} + +function isPlainObject(value: unknown): value is Record { + return value !== null && typeof value === "object" && !Array.isArray(value); +} + +/** Redact sensitive header values inside requestInit / eventSourceInit. */ +function sanitizeInitRecord( + init: Record, +): Record { + const out: Record = { ...init }; + if (isPlainObject(init.headers)) { + out.headers = Object.fromEntries( + Object.entries(init.headers).map(([key, value]) => [ + key, + isSensitiveHeader(key) ? REDACTED : value, + ]), + ); + } else if (Array.isArray(init.headers)) { + // HeadersInit pair form: [["Authorization", "Bearer …"], …] + out.headers = init.headers.map((entry) => { + if ( + Array.isArray(entry) && + entry.length >= 2 && + typeof entry[0] === "string" + ) { + const key = entry[0]; + const value = entry[1]; + return [key, isSensitiveHeader(key) ? REDACTED : value]; + } + return entry; + }); + } + return out; +} + +function isSensitiveHeader(key: string): boolean { + const k = key.toLowerCase(); + return ( + k === "authorization" || + k.includes("secret") || + k.includes("token") || + k.includes("password") || + k.includes("api-key") || + k.includes("apikey") + ); +} diff --git a/clients/cli/src/open-url.ts b/clients/cli/src/open-url.ts new file mode 100644 index 000000000..f5b42a8ac --- /dev/null +++ b/clients/cli/src/open-url.ts @@ -0,0 +1,6 @@ +import open from "open"; + +/** Open a URL in the user's default browser (best-effort). */ +export async function openUrl(url: string | URL): Promise { + await open(typeof url === "string" ? url : url.href); +} diff --git a/clients/cli/src/style.ts b/clients/cli/src/style.ts new file mode 100644 index 000000000..315edb381 --- /dev/null +++ b/clients/cli/src/style.ts @@ -0,0 +1,76 @@ +import type { OutputFormat } from "./handlers/format-output.js"; + +export type Style = { + /** Whether ANSI styling is enabled. */ + readonly ansi: boolean; + bold: (s: string) => string; + dim: (s: string) => string; + red: (s: string) => string; + yellow: (s: string) => string; + green: (s: string) => string; + cyan: (s: string) => string; + /** OSC 8 hyperlink when ansi; otherwise plain label (defaulting to uri). */ + link: (uri: string, label?: string) => string; +}; + +const identity = (s: string): string => s; + +function sgr(enabled: boolean, open: number, close: number) { + if (!enabled) return identity; + return (s: string): string => `\u001b[${open}m${s}\u001b[${close}m`; +} + +/** + * Build a style helper. When `ansi` is false, all methods are identity + * (and `link` returns the label only — no OSC 8). + */ +export function createStyle(ansi: boolean): Style { + return { + ansi, + bold: sgr(ansi, 1, 22), + dim: sgr(ansi, 2, 22), + red: sgr(ansi, 31, 39), + yellow: sgr(ansi, 33, 39), + green: sgr(ansi, 32, 39), + cyan: sgr(ansi, 36, 39), + link: (uri: string, label?: string): string => { + const text = label ?? uri; + if (!ansi || !uri) return text; + // OSC 8 hyperlink: ESC ] 8 ; ; URI BEL text ESC ] 8 ; ; BEL + return `\u001b]8;;${uri}\u0007${text}\u001b]8;;\u0007`; + }, + }; +} + +/** Plain (no ANSI) style — default for tests and `--plain`. */ +export const PLAIN: Style = createStyle(false); + +export type ResolveAnsiOptions = { + /** `--plain` — force no ANSI. */ + plain?: boolean; + /** `--format`; json never uses ANSI for the payload path. */ + format?: OutputFormat; + /** Override TTY detection (tests). */ + isTTY?: boolean; + /** Override `NO_COLOR` (tests). */ + noColorEnv?: string | undefined; +}; + +/** + * Decide whether session human output should use ANSI. + * Off when: `--plain`, `NO_COLOR` set, `--format json`, or stdout is not a TTY. + */ +export function resolveAnsiEnabled(opts: ResolveAnsiOptions = {}): boolean { + if (opts.plain) return false; + if (opts.format === "json") return false; + const noColor = + opts.noColorEnv !== undefined ? opts.noColorEnv : process.env.NO_COLOR; + if (noColor !== undefined && noColor !== "") return false; + const tty = + opts.isTTY !== undefined ? opts.isTTY : process.stdout.isTTY === true; + return tty; +} + +export function styleFromOpts(opts: ResolveAnsiOptions = {}): Style { + return createStyle(resolveAnsiEnabled(opts)); +} diff --git a/clients/cli/tsup.config.ts b/clients/cli/tsup.config.ts index d65d37ec3..5a58c1ab8 100644 --- a/clients/cli/tsup.config.ts +++ b/clients/cli/tsup.config.ts @@ -6,7 +6,9 @@ const dirname = path.dirname(fileURLToPath(import.meta.url)); const repoRoot = path.resolve(dirname, '../..'); export default defineConfig({ - entry: ['src/index.ts'], + entry: { + index: 'src/index.ts', + }, format: ['esm'], outDir: 'build', clean: true, From 5197b3766189f2b477c82f212e0153ac047cba9b Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 17:00:29 -0700 Subject: [PATCH 02/13] =?UTF-8?q?fix(cli):=20address=20#1782=20review=20?= =?UTF-8?q?=E2=80=94=20relogin=20clear,=20stored-auth-only,=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clear both raw and normalized OAuth keys on --relogin; check the shared store before bailing under --stored-auth-only mid-session; restore the tests README link target; document servers/show redaction limits; keep mcpi groundwork with TODO(#1432) markers. Co-authored-by: Cursor --- clients/cli/README.md | 2 + clients/cli/__tests__/README.md | 20 ++++++++ .../clear-stored-auth-for-relogin.test.ts | 44 +++++++++++++++++ clients/cli/__tests__/cliOAuth.test.ts | 38 ++++++++++++++- clients/cli/__tests__/methods.test.ts | 2 +- .../cli/src/clear-stored-auth-for-relogin.ts | 15 +++++- clients/cli/src/cli.ts | 47 ++++++++++--------- clients/cli/src/cliOAuth.ts | 6 +++ clients/cli/src/handlers/method-types.ts | 8 +++- clients/cli/src/handlers/run-method.ts | 12 +++++ clients/cli/src/handlers/servers-list.ts | 7 ++- clients/cli/src/style.ts | 7 +++ core/auth/node/storage-node.ts | 3 +- 13 files changed, 180 insertions(+), 31 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 8c75df3ef..60dd5a081 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -114,6 +114,8 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--relogin` | Ignore any stored OAuth for this run’s server URL before connect; interactive login still only runs if the server requires auth. No-op for stdio (no URL-keyed store entry). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth`. | | `--stored-auth-only` | Never start interactive OAuth / step-up; use the shared store if present, otherwise fail with `auth_required`. | +`servers/show` redacts secret-bearing fields (`env` values, sensitive headers in `requestInit` / `eventSourceInit` / settings, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. + #### App probing (`--app-info`) and machine-readable output (`--format json`) `--app-info` inspects a tool's [MCP App](https://modelcontextprotocol.io) UI posture **without calling the tool**, so a pipeline can decide whether to open a browser before touching one: diff --git a/clients/cli/__tests__/README.md b/clients/cli/__tests__/README.md index ca7793506..75eb42ada 100644 --- a/clients/cli/__tests__/README.md +++ b/clients/cli/__tests__/README.md @@ -1 +1,21 @@ # CLI Tests + +Tests live next to the suite entrypoints under `__tests__/` and run via Vitest. + +- Most tests import `runCli()` **in-process** (see `helpers/cli-runner.ts`) so + `clients/cli/src` is measured under the coverage gate. +- `e2e.test.ts` (and root `scripts/smoke-cli.mjs`) spawn the built binary for + shebang / `process.exit` paths — `pretest` builds `test-servers` + the CLI + bundle first. + +Useful scripts (from `clients/cli/`): + +```bash +npm test # pretest build + all tests +npm run test:cli # subset: cli.test.ts +npm run test:cli-tools # subset: tools.test.ts +npm run test:coverage # build + ≥90 per-file coverage gate +``` + +OAuth interactive tests may open a loopback callback; they stub or drive that +path in-process rather than requiring a real browser. diff --git a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts index 0e4ecee93..1da26d7f5 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -65,4 +65,48 @@ describe("clearStoredAuthForRelogin", () => { }; expect(blob.servers["not a url"]).toBeUndefined(); }); + + it("clears both raw and URL-normalised keys (bare origin / mixed-case host)", async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-norm-")); + const file = path.join(dir, "oauth.json"); + // Runtime storage keys by the transport's raw url string — often without a + // trailing slash / with mixed-case host — while new URL().href normalises. + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + "https://example.com": { + tokens: { access_token: "bare", token_type: "Bearer" }, + }, + "https://example.com/": { + tokens: { access_token: "slash", token_type: "Bearer" }, + }, + "https://Example.com/mcp": { + tokens: { access_token: "mixed", token_type: "Bearer" }, + }, + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + + await clearStoredAuthForRelogin("https://example.com"); + let blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://example.com"]).toBeUndefined(); + expect(blob.servers["https://example.com/"]).toBeUndefined(); + expect(blob.servers["https://Example.com/mcp"]).toBeDefined(); + + await clearStoredAuthForRelogin("https://Example.com/mcp"); + blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://Example.com/mcp"]).toBeUndefined(); + // Normalised key (lowercased host) is cleared too when distinct. + expect(blob.servers["https://example.com/mcp"]).toBeUndefined(); + }); }); diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index f075f13ca..68848b4cd 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -506,6 +506,7 @@ describe("cliOAuth", () => { }); it("withCliAuthRecoveryRetry respects storedAuthOnly", async () => { + const checkAuthChallengeSatisfied = vi.fn().mockResolvedValue(false); const fn = vi.fn().mockRejectedValue( new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), { reason: "token_expired", @@ -514,7 +515,7 @@ describe("cliOAuth", () => { await expect( withCliAuthRecoveryRetry( { - checkAuthChallengeSatisfied: vi.fn(), + checkAuthChallengeSatisfied, } as never, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, @@ -524,5 +525,40 @@ describe("cliOAuth", () => { { storedAuthOnly: true }, ), ).rejects.toMatchObject({ exitCode: 3 }); + expect(checkAuthChallengeSatisfied).toHaveBeenCalledOnce(); + }); + + it("withCliAuthRecoveryRetry under storedAuthOnly retries when the store satisfies the challenge", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const challenge = { + reason: "token_expired" as const, + }; + const fn = vi + .fn() + .mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + challenge, + ), + ) + .mockResolvedValueOnce("ok"); + const checkAuthChallengeSatisfied = vi.fn().mockResolvedValue(true); + + const result = await withCliAuthRecoveryRetry( + { + checkAuthChallengeSatisfied, + } as never, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + fn, + undefined, + { storedAuthOnly: true }, + ); + + expect(result).toBe("ok"); + expect(fn).toHaveBeenCalledTimes(2); + expect(checkAuthChallengeSatisfied).toHaveBeenCalledWith(challenge); + expect(runSpy).not.toHaveBeenCalled(); }); }); diff --git a/clients/cli/__tests__/methods.test.ts b/clients/cli/__tests__/methods.test.ts index a41459586..c808edcaa 100644 --- a/clients/cli/__tests__/methods.test.ts +++ b/clients/cli/__tests__/methods.test.ts @@ -71,7 +71,7 @@ describe("CLI method coverage", () => { expectOutputContains(unknown, "Unsupported method"); expectOutputContains(unknown, "definitely/not-a-real-method"); - // Stream methods used to hang via consumeMethodOutcome; now rejected. + // Long-lived / stream methods are rejected by the CLI method allowlist. const tail = await runCli([ command, ...args, diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index 850c14f99..ae6d29902 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -15,13 +15,24 @@ function normalizeServerUrl(serverUrl: string): string { /** * Drop stored OAuth state for an HTTP(S) server URL so the next connect cannot * silently reuse tokens (`--relogin`). No-op when `serverUrl` is missing. + * + * Clears **both** the raw URL and the `new URL().href`-normalised form. Runtime + * OAuth storage is keyed by the transport's raw `url` string, while some writers + * (and earlier clear paths) use the normalised key — mirroring + * `findStoredServerState` in `cli.ts`, which already tries both on read. */ export async function clearStoredAuthForRelogin( serverUrl: string | undefined, ): Promise { if (!serverUrl?.trim()) return; - const url = normalizeServerUrl(serverUrl.trim()); + const raw = serverUrl.trim(); + const normalized = normalizeServerUrl(raw); const storage = new NodeOAuthStorage(); - await storage.clear(url); + await storage.clear(raw); + if (normalized !== raw) { + await storage.clear(normalized); + } + // Drop the in-process singleton so the next connect cannot reuse a cleared + // entry from the NodeOAuthStorage cache. resetNodeOAuthStorageCache(); } diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index b48bfb7cb..ec83d23e2 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -797,6 +797,30 @@ async function parseArgs(argv?: string[]): Promise { headers: options.header as Record | undefined, }; + // Catalog list / show — no MCP connection. Run before stored-auth refresh so + // a catalog-only command never triggers a token round-trip it won't use. + if (options.method === "servers/list") { + const servers = await listServerEntries(serverOptions); + await writeFormattedResult( + { servers }, + options.format === "json" ? "json" : "text", + ); + return { shortCircuit: true }; + } + if (options.method === "servers/show") { + if (!options.server?.trim()) { + throw new Error( + "servers/show requires --server to select a catalog entry.", + ); + } + const server = await showServerEntry(options.server, serverOptions); + await writeFormattedResult( + server, + options.format === "json" ? "json" : "text", + ); + return { shortCircuit: true }; + } + if (options.waitForAuth !== undefined || options.useStoredAuth) { if (!options.serverUrl) { throw new Error( @@ -862,29 +886,6 @@ async function parseArgs(argv?: string[]): Promise { }; } - // Catalog list / show — no MCP connection. - if (options.method === "servers/list") { - const servers = await listServerEntries(serverOptions); - await writeFormattedResult( - { servers }, - options.format === "json" ? "json" : "text", - ); - return { shortCircuit: true }; - } - if (options.method === "servers/show") { - if (!options.server?.trim()) { - throw new Error( - "servers/show requires --server to select a catalog entry.", - ); - } - const server = await showServerEntry(options.server, serverOptions); - await writeFormattedResult( - server, - options.format === "json" ? "json" : "text", - ); - return { shortCircuit: true }; - } - // Shared with the TUI: resolves the catalog/config source (or ad-hoc target), // enforces the conflict matrix, and lifts disk headers/timeouts/OAuth into // per-server settings. `--server` selects one when the file has several. diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index a15bc0c4f..86a6086e5 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -200,6 +200,12 @@ export async function withCliAuthRecoveryRetry( if (!(err instanceof AuthRecoveryRequiredError)) { throw err; } + // Match connectInspectorWithOAuth: give the shared store a chance to + // satisfy the challenge (already-satisfied / EMA re-mint) before bailing + // under --stored-auth-only or opening interactive OAuth. + if (await inspectorClient.checkAuthChallengeSatisfied(err.authChallenge)) { + return await fn(); + } if (options?.storedAuthOnly) { storedAuthOnlyFailure( err.message || diff --git a/clients/cli/src/handlers/method-types.ts b/clients/cli/src/handlers/method-types.ts index ce5344a21..b0505425e 100644 --- a/clients/cli/src/handlers/method-types.ts +++ b/clients/cli/src/handlers/method-types.ts @@ -56,7 +56,13 @@ export type MethodOutcome = start: (writeLine: (obj: unknown) => void) => () => void; }; -/** Methods supported by {@link runMethod}. */ +/** + * Full method set supported by {@link runMethod}. + * + * TODO(#1432): several of these (subscribe, tasks, roots, logging/tail, …) are + * not exposed by `mcp-inspector --cli` today; they exist for the experimental + * session CLI (`mcpi`) and other Node runners that share this dispatcher. + */ export const SESSION_RPC_METHODS = [ "initialize", "tools/list", diff --git a/clients/cli/src/handlers/run-method.ts b/clients/cli/src/handlers/run-method.ts index 8d94e2978..9e50bab66 100644 --- a/clients/cli/src/handlers/run-method.ts +++ b/clients/cli/src/handlers/run-method.ts @@ -20,6 +20,10 @@ import type { /** * Run one MCP method against a connected {@link InspectorClient}. * Core method dispatch used by the CLI (and other Inspector Node runners). + * + * TODO(#1432): stream / task / roots / subscribe paths are exercised by the + * experimental session CLI (`mcpi`); `mcp-inspector --cli` only admits + * {@link ONE_SHOT_METHODS} plus catalog `servers/*`. */ export async function runMethod( inspectorClient: InspectorClient, @@ -64,6 +68,10 @@ export async function runMethod( if (args.method === "tools/list") { const tools = managedToolsState!.getTools(); if (args.appInfo) { + // Collect every probe first, then emit NDJSON as a batch. A piped + // consumer therefore sees lines only after all tools are probed (not + // as each completes). Preferable to a partial stream when one tool's + // probe fails mid-list; collectAppInfo never throws for a single tool. const lines: unknown[] = []; for (const tool of tools) { lines.push( @@ -84,6 +92,8 @@ export async function runMethod( .getTools() .find((t) => t.name === args.toolName); if (!tool) { + // Distinct from tools/call isError:true (tool ran, reported failure) + // and from "no App" under --app-info — the tool simply isn't listed. throw new CliExitCodeError( EXIT_CODES.TOOL_ERROR, `Tool '${args.toolName}' not found on server.`, @@ -91,6 +101,8 @@ export async function runMethod( ); } + // collectAppInfo only for --app-info / --format json — text mode skips + // the resources/read probe so a plain tools/call stays a single RPC. if (args.appInfo || args.format === "json") { appInfo = await collectAppInfo(inspectorClient, tool, args.metadata); } diff --git a/clients/cli/src/handlers/servers-list.ts b/clients/cli/src/handlers/servers-list.ts index 8188f7035..d4d5873e2 100644 --- a/clients/cli/src/handlers/servers-list.ts +++ b/clients/cli/src/handlers/servers-list.ts @@ -32,6 +32,9 @@ export type SessionListRef = { /** * Mark catalog entries that have a live session with the same name. * Does not mutate `entries`. + * + * TODO(#1432): consumed by the experimental session CLI (`mcpi`); kept here so + * that client can reuse catalog listing without duplicating this helper. */ export function annotateServerEntriesWithSessions( entries: ServerListEntry[], @@ -144,11 +147,11 @@ export function sanitizeServerSettings( ): Record { const out: Record = { ...settings, - headers: settings.headers.map((h) => ({ + headers: (settings.headers ?? []).map((h) => ({ key: h.key, value: isSensitiveHeader(h.key) ? REDACTED : h.value, })), - env: settings.env.map((e) => ({ + env: (settings.env ?? []).map((e) => ({ key: e.key, value: REDACTED, })), diff --git a/clients/cli/src/style.ts b/clients/cli/src/style.ts index 315edb381..4cf9c70bc 100644 --- a/clients/cli/src/style.ts +++ b/clients/cli/src/style.ts @@ -1,5 +1,12 @@ import type { OutputFormat } from "./handlers/format-output.js"; +/** + * ANSI / OSC 8 styling helpers. + * + * TODO(#1432): the CLI OAuth path only needs {@link Style.link} today; bold / + * color helpers and {@link styleFromOpts} are used by the experimental session + * CLI (`mcpi`) human formatter. + */ export type Style = { /** Whether ANSI styling is enabled. */ readonly ansi: boolean; diff --git a/core/auth/node/storage-node.ts b/core/auth/node/storage-node.ts index cad693e77..4bbdeb4f5 100644 --- a/core/auth/node/storage-node.ts +++ b/core/auth/node/storage-node.ts @@ -53,7 +53,8 @@ function getSharedMemory(stateFilePath?: string): OAuthMemoryStore { /** * Drop cached in-memory and {@link NodeOAuthStorage} instances for a path. - * @internal Test isolation only. + * Used for test isolation and by CLI `--relogin` after clearing on-disk state + * so the next connect cannot reuse a stale in-process storage singleton. */ export function resetNodeOAuthStorageCache(stateFilePath?: string): void { const key = getStateFilePath(stateFilePath); From 73e2b52f16773e958c14fd607685bc92d730b9dd Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 17:25:47 -0700 Subject: [PATCH 03/13] fix(cli): gate OAuth browser open and harden #1782 review follow-ups Arm auto-open only for CLI-owned interactive OAuth, honor MCP_AUTO_OPEN_ENABLED, move --relogin clear after validation, skip keychain on servers/list, and cover secret redaction after rehydrate. Co-authored-by: Cursor --- clients/cli/README.md | 12 +-- .../__tests__/cli-oauth-navigation.test.ts | 90 ++++++++++++++++++- .../__tests__/programmatic-ergonomics.test.ts | 21 +++++ clients/cli/__tests__/servers-list.test.ts | 50 +++++++++++ .../cli/src/clear-stored-auth-for-relogin.ts | 5 +- clients/cli/src/cli-oauth-navigation.ts | 53 ++++++++++- clients/cli/src/cli.ts | 29 ++++-- clients/cli/src/cliOAuth.ts | 44 +++++++-- clients/cli/src/handlers/consume-outcome.ts | 3 + clients/cli/src/handlers/servers-list.ts | 8 +- clients/cli/src/style.ts | 3 +- 11 files changed, 284 insertions(+), 34 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 60dd5a081..b0fcb7805 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -111,8 +111,8 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--connect-timeout ` | Connection timeout in ms. Defaults to `15000` for ad-hoc `--server-url`/target runs (so a black-holed host fails fast) and to the file-level timeout for `--catalog`/`--config` runs. `0` disables the timeout. | | `--app-info` | Probe a tool's MCP App UI metadata without invoking it. With `--method tools/call --tool-name `: prints one JSON line (`hasApp`, `resourceUri`, `csp`, `permissions`, `domain`, …) and exits `0` if the tool has an app or `2` (`no_app`) if not. With `--method tools/list`: emits NDJSON — one app-info line per tool over a single connection. | | `--format ` | Output format. `text` (default) pretty-prints the result. `json` emits a single JSON object on stdout (`{ "result": … }`, plus `{ "appInfo": … }` as a sibling key for App tools) with no banners, so the whole output pipes cleanly into `jq`. | -| `--relogin` | Ignore any stored OAuth for this run’s server URL before connect; interactive login still only runs if the server requires auth. No-op for stdio (no URL-keyed store entry). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth`. | -| `--stored-auth-only` | Never start interactive OAuth / step-up; use the shared store if present, otherwise fail with `auth_required`. | +| `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. No-op for stdio (no URL-keyed store entry). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth`. | +| `--stored-auth-only` | Never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail with `auth_required`. | `servers/show` redacts secret-bearing fields (`env` values, sensitive headers in `requestInit` / `eventSourceInit` / settings, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. @@ -262,15 +262,15 @@ While the Web Client provides a rich visual interface, the CLI is designed for: Like the other clients, the CLI self-validates from its own folder: ```bash -npm run validate # format:check && lint && test (fast; no coverage gate) +npm run validate # format:check && lint && typecheck && test (fast; no coverage gate) npm test # build test-servers + binary, then run all tests npm run test:coverage # build + tests under the per-file ≥90 coverage gate ``` The CLI's `test` / `test:coverage` **build the binary first** (out-of-process -`e2e.test.ts` spawns it). `validate` is `format:check && lint && test` with no -separate `build` step (`pretest` builds). Repo-root `validate:cli` delegates -here; the coverage gate is `npm run coverage` / `coverage:cli` (also in +`e2e.test.ts` spawns it). `validate` is `format:check && lint && typecheck && test` +with no separate `build` step (`pretest` builds). Repo-root `validate:cli` +delegates here; the coverage gate is `npm run coverage` / `coverage:cli` (also in `npm run ci`), matching AGENTS.md. Tests run the CLI **in-process** (importing `runCli()`) so `src/` is measured diff --git a/clients/cli/__tests__/cli-oauth-navigation.test.ts b/clients/cli/__tests__/cli-oauth-navigation.test.ts index 4cc9a2dbf..d8600f971 100644 --- a/clients/cli/__tests__/cli-oauth-navigation.test.ts +++ b/clients/cli/__tests__/cli-oauth-navigation.test.ts @@ -1,5 +1,8 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { createCliOAuthNavigation } from "../src/cli-oauth-navigation.js"; +import { + createCliOAuthNavigation, + resolveCliAutoOpenEnabled, +} from "../src/cli-oauth-navigation.js"; import { openUrl } from "../src/open-url.js"; vi.mock("../src/open-url.js", () => ({ @@ -8,13 +11,32 @@ vi.mock("../src/open-url.js", () => ({ const openUrlMock = vi.mocked(openUrl); +describe("resolveCliAutoOpenEnabled", () => { + it("honors MCP_AUTO_OPEN_ENABLED true/false and defaults off under VITEST", () => { + expect( + resolveCliAutoOpenEnabled({ + MCP_AUTO_OPEN_ENABLED: "true", + VITEST: "true", + }), + ).toBe(true); + expect( + resolveCliAutoOpenEnabled({ + MCP_AUTO_OPEN_ENABLED: "false", + VITEST: undefined, + }), + ).toBe(false); + expect(resolveCliAutoOpenEnabled({ VITEST: "true" })).toBe(false); + expect(resolveCliAutoOpenEnabled({})).toBe(true); + }); +}); + describe("createCliOAuthNavigation", () => { afterEach(() => { openUrlMock.mockClear(); openUrlMock.mockResolvedValue(undefined); }); - it("prints OSC 8 link and opens browser on a TTY", async () => { + it("prints OSC 8 link and opens browser when armed on a TTY", async () => { const lines: string[] = []; const openBrowser = vi.fn().mockResolvedValue(undefined); const nav = createCliOAuthNavigation({ @@ -23,6 +45,8 @@ describe("createCliOAuthNavigation", () => { noColorEnv: "", write: (line) => lines.push(line), openBrowser, + autoOpenControl: { armed: true }, + autoOpenEnabled: true, }); const url = new URL("https://as.example/authorize?x=1"); nav.navigateToAuthorization(url); @@ -37,6 +61,56 @@ describe("createCliOAuthNavigation", () => { expect(openUrlMock).not.toHaveBeenCalled(); }); + it("prints the URL but does not open a browser when disarmed (SDK-during-connect default)", async () => { + const lines: string[] = []; + const openBrowser = vi.fn(); + const nav = createCliOAuthNavigation({ + isTTY: true, + noColorEnv: "1", + write: (line) => lines.push(line), + openBrowser, + autoOpenEnabled: true, + // no autoOpenControl → never armed + }); + nav.navigateToAuthorization(new URL("https://as.example/authorize")); + await vi.waitFor(() => expect(lines.length).toBe(1)); + expect(lines.join("")).toContain("Please navigate to:"); + expect(openBrowser).not.toHaveBeenCalled(); + }); + + it("does not open a browser when disableAutoOpen is set", async () => { + const lines: string[] = []; + const openBrowser = vi.fn(); + const nav = createCliOAuthNavigation({ + isTTY: true, + noColorEnv: "1", + write: (line) => lines.push(line), + openBrowser, + autoOpenControl: { armed: true }, + autoOpenEnabled: true, + disableAutoOpen: true, + }); + nav.navigateToAuthorization(new URL("https://as.example/authorize")); + await vi.waitFor(() => expect(lines.length).toBe(1)); + expect(openBrowser).not.toHaveBeenCalled(); + }); + + it("does not open a browser when autoOpenEnabled is false", async () => { + const lines: string[] = []; + const openBrowser = vi.fn(); + const nav = createCliOAuthNavigation({ + isTTY: true, + noColorEnv: "1", + write: (line) => lines.push(line), + openBrowser, + autoOpenControl: { armed: true }, + autoOpenEnabled: false, + }); + nav.navigateToAuthorization(new URL("https://as.example/authorize")); + await vi.waitFor(() => expect(lines.length).toBe(1)); + expect(openBrowser).not.toHaveBeenCalled(); + }); + it("prints a plain URL and does not open a browser when not a TTY", async () => { const lines: string[] = []; const openBrowser = vi.fn(); @@ -44,6 +118,8 @@ describe("createCliOAuthNavigation", () => { isTTY: false, write: (line) => lines.push(line), openBrowser, + autoOpenControl: { armed: true }, + autoOpenEnabled: true, }); nav.navigateToAuthorization(new URL("https://as.example/authorize")); await vi.waitFor(() => expect(lines.length).toBe(1)); @@ -55,7 +131,7 @@ describe("createCliOAuthNavigation", () => { expect(openUrlMock).not.toHaveBeenCalled(); }); - it("skips OSC 8 when NO_COLOR is set but still opens on a TTY", async () => { + it("skips OSC 8 when NO_COLOR is set but still opens on a TTY when armed", async () => { const lines: string[] = []; const openBrowser = vi.fn().mockResolvedValue(undefined); const nav = createCliOAuthNavigation({ @@ -63,6 +139,8 @@ describe("createCliOAuthNavigation", () => { noColorEnv: "1", write: (line) => lines.push(line), openBrowser, + autoOpenControl: { armed: true }, + autoOpenEnabled: true, }); nav.navigateToAuthorization(new URL("https://as.example/a")); await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce()); @@ -78,13 +156,15 @@ describe("createCliOAuthNavigation", () => { noColorEnv: "1", write: (line) => lines.push(line), openBrowser, + autoOpenControl: { armed: true }, + autoOpenEnabled: true, }); nav.navigateToAuthorization(new URL("https://as.example/a")); await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce()); expect(lines.join("")).toContain("Please navigate to:"); }); - it("writes to stderr and uses openUrl by default on a TTY", async () => { + it("writes to stderr and uses openUrl by default when armed on a TTY", async () => { const writeSpy = vi .spyOn(process.stderr, "write") .mockImplementation(() => true); @@ -96,6 +176,8 @@ describe("createCliOAuthNavigation", () => { try { const nav = createCliOAuthNavigation({ noColorEnv: "1", + autoOpenControl: { armed: true }, + autoOpenEnabled: true, }); nav.navigateToAuthorization(new URL("https://as.example/default")); await vi.waitFor(() => expect(openUrlMock).toHaveBeenCalledOnce()); diff --git a/clients/cli/__tests__/programmatic-ergonomics.test.ts b/clients/cli/__tests__/programmatic-ergonomics.test.ts index 17888498e..0318d8278 100644 --- a/clients/cli/__tests__/programmatic-ergonomics.test.ts +++ b/clients/cli/__tests__/programmatic-ergonomics.test.ts @@ -181,6 +181,27 @@ describe("CLI --relogin flag conflicts", () => { }); }); +describe("CLI --stored-auth-only wiring", () => { + it("accepts the flag and fails connect without interactive OAuth prompts", async () => { + // Unreachable host: proves the flag is accepted through parse → callMethod + // and does not hang on an interactive OAuth prompt. + const result = await runCli([ + "--stored-auth-only", + "--server-url", + "http://127.0.0.1:1/mcp", + "--transport", + "http", + "--connect-timeout", + "200", + "--method", + "initialize", + ]); + expectCliFailure(result); + expect(result.stderr).not.toMatch(/Please navigate to:/); + expect(result.stderr).not.toMatch(/Proceed with step-up/); + }); +}); + describe("MCP_CATALOG_PATH with an ad-hoc target", () => { it("does not conflict with an ad-hoc target (env catalog is ignored)", async () => { const { command, args } = getTestMcpServerCommand(); diff --git a/clients/cli/__tests__/servers-list.test.ts b/clients/cli/__tests__/servers-list.test.ts index 6860e29f0..fbb9f9b60 100644 --- a/clients/cli/__tests__/servers-list.test.ts +++ b/clients/cli/__tests__/servers-list.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, afterEach } from "vitest"; import { runCli } from "./helpers/cli-runner.js"; import { createSampleTestConfig, + createTestConfig, deleteConfigFile, } from "./helpers/fixtures.js"; import { expectCliSuccess } from "./helpers/assertions.js"; @@ -18,6 +19,11 @@ import type { MCPServerConfig, } from "@inspector/core/mcp/types.js"; import { expectCliFailure } from "./helpers/assertions.js"; +import { + InMemorySecretStore, + SECRET_FIELD_OAUTH_CLIENT_SECRET, + envSecretField, +} from "@inspector/core/auth/node/secret-store.js"; describe("summarizeServerConfig", () => { it("summarises stdio, sse, and streamable-http configs", () => { @@ -245,6 +251,50 @@ describe("showServerEntry / servers/show", () => { expect(entry.config.env).toEqual({ HELLO: "[redacted]" }); }); + it("redacts secrets rehydrated from an injected secret store", async () => { + configPath = createTestConfig({ + mcpServers: { + "secret-stdio": { + type: "stdio", + command: "node", + env: { HELLO: "" }, + }, + "secret-http": { + type: "streamable-http", + url: "https://example.com/mcp", + // Disk keeps clientId only; secret lives in the store. + oauth: { clientId: "cid" }, + } as MCPServerConfig, + }, + }); + const secretStore = new InMemorySecretStore(); + await secretStore.set( + "secret-stdio", + envSecretField("HELLO"), + "top-secret-env", + ); + await secretStore.set( + "secret-http", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "top-secret-oauth", + ); + + const stdio = await showServerEntry("secret-stdio", { + configPath, + secretStore, + }); + expect(stdio.config.env).toEqual({ HELLO: "[redacted]" }); + expect(JSON.stringify(stdio)).not.toContain("top-secret-env"); + + const http = await showServerEntry("secret-http", { + configPath, + secretStore, + }); + expect(http.settings?.oauthClientSecret).toBe("[redacted]"); + expect(http.settings?.oauthClientId).toBe("cid"); + expect(JSON.stringify(http)).not.toContain("top-secret-oauth"); + }); + it("works via --method servers/show --server", async () => { configPath = createSampleTestConfig(); const result = await runCli([ diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index ae6d29902..bcbdc880c 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -13,8 +13,9 @@ function normalizeServerUrl(serverUrl: string): string { } /** - * Drop stored OAuth state for an HTTP(S) server URL so the next connect cannot - * silently reuse tokens (`--relogin`). No-op when `serverUrl` is missing. + * Delete stored OAuth state for an HTTP(S) server URL from the shared store + * (`--relogin`) so the next connect cannot silently reuse tokens. This removes + * the store entry (not a per-run ignore). No-op when `serverUrl` is missing. * * Clears **both** the raw URL and the `new URL().href`-normalised form. Runtime * OAuth storage is keyed by the transport's raw `url` string, while some writers diff --git a/clients/cli/src/cli-oauth-navigation.ts b/clients/cli/src/cli-oauth-navigation.ts index 03352a697..34a901955 100644 --- a/clients/cli/src/cli-oauth-navigation.ts +++ b/clients/cli/src/cli-oauth-navigation.ts @@ -2,6 +2,16 @@ import { CallbackNavigation } from "@inspector/core/auth/index.js"; import { openUrl } from "./open-url.js"; import { createStyle, resolveAnsiEnabled } from "./style.js"; +/** + * Arms browser auto-open only for the CLI-owned interactive OAuth flow (the + * one with a listening callback server). Left disarmed during + * `inspectorClient.connect()` so an SDK-internal `auth()` cannot open a + * browser (or a second doomed tab) before CLI gates run. + */ +export type CliOAuthAutoOpenControl = { + armed: boolean; +}; + export type CliOAuthNavigationOptions = { /** Override TTY detection (tests). Defaults to stderr.isTTY. */ isTTY?: boolean; @@ -11,12 +21,40 @@ export type CliOAuthNavigationOptions = { write?: (line: string) => void; /** Open the browser (defaults to {@link openUrl}). */ openBrowser?: (url: string) => Promise; + /** + * When set, browser open is allowed only while {@link CliOAuthAutoOpenControl.armed} + * is true. When omitted, auto-open is never armed (print URL only). + */ + autoOpenControl?: CliOAuthAutoOpenControl; + /** Hard-disable browser open (e.g. `--stored-auth-only`). */ + disableAutoOpen?: boolean; + /** + * Override {@link resolveCliAutoOpenEnabled} (tests). When omitted, uses + * `MCP_AUTO_OPEN_ENABLED` / `VITEST` the same way the web server does. + */ + autoOpenEnabled?: boolean; }; /** - * CLI OAuth navigation: print the authorization URL (OSC 8 when TTY allows - * ANSI) and auto-open the default browser on a TTY. Non-TTY / CI: plain URL - * only — never launches a browser. + * Mirror of web `resolveAutoOpen` (`clients/web/server/web-server-config.ts`): + * - `MCP_AUTO_OPEN_ENABLED=true` → always open + * - `MCP_AUTO_OPEN_ENABLED=false` → never open + * - otherwise → open unless `VITEST` is set + */ +export function resolveCliAutoOpenEnabled( + env: NodeJS.ProcessEnv = process.env, +): boolean { + const flag = env.MCP_AUTO_OPEN_ENABLED; + if (flag === "true") return true; + if (flag === "false") return false; + return !env.VITEST; +} + +/** + * CLI OAuth navigation: always print the authorization URL (OSC 8 when TTY + * allows ANSI). Browser open is gated by {@link CliOAuthAutoOpenControl} + * (armed only for the CLI interactive flow), `--stored-auth-only`, TTY, + * and {@link resolveCliAutoOpenEnabled}. */ export function createCliOAuthNavigation( options: CliOAuthNavigationOptions = {}, @@ -36,7 +74,14 @@ export function createCliOAuthNavigation( const write = options.write ?? ((line: string) => process.stderr.write(line)); write(`Please navigate to: ${style.link(href)}\n`); - if (!tty) return; + + const envAllows = + options.autoOpenEnabled !== undefined + ? options.autoOpenEnabled + : resolveCliAutoOpenEnabled(); + const armed = options.autoOpenControl?.armed === true; + if (options.disableAutoOpen || !armed || !envAllows || !tty) return; + try { await (options.openBrowser ?? openUrl)(href); } catch { diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index ec83d23e2..d9a0f57d3 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -95,7 +95,14 @@ async function callMethod( cliAuthOverrides: RunnerClientConfigOverrides, callbackUrlConfig: RunnerOAuthCallbackConfig, storedAuthOnly: boolean, + relogin: boolean, ): Promise { + // Clear after parse-time validation so a bad flag combo never deletes store + // entries. Deletes the shared URL-keyed OAuth entry (not "ignore for this run"). + if (relogin && "url" in serverConfig && serverConfig.url) { + await clearStoredAuthForRelogin(serverConfig.url); + } + // Version comes from the single source of truth — the root package.json — // via the shared core reader, not the CLI's own manifest. const clientIdentity = { @@ -107,12 +114,18 @@ async function callMethod( transport: createTransportNode, }; const redirectUrlProvider = new MutableRedirectUrlProvider(); + // Disarmed until the CLI-owned interactive OAuth flow runs — SDK `auth()` + // during connect must not open a browser before `--stored-auth-only` / gates. + const autoOpenControl = { armed: false }; if (isOAuthCapableServerConfig(serverConfig)) { redirectUrlProvider.redirectUrl = formatRunnerOAuthRedirectUrl(callbackUrlConfig); environment.oauth = { storage: new NodeOAuthStorage(), - navigation: createCliOAuthNavigation(), + navigation: createCliOAuthNavigation({ + autoOpenControl, + disableAutoOpen: storedAuthOnly, + }), redirectUrlProvider, }; } @@ -146,7 +159,7 @@ async function callMethod( redirectUrlProvider, callbackUrlConfig, serverSettings, - { storedAuthOnly }, + { storedAuthOnly, autoOpenControl }, ); const outcome = await withCliAuthRecoveryRetry( @@ -156,7 +169,7 @@ async function callMethod( serverSettings, () => runMethod(inspectorClient, args), undefined, - { storedAuthOnly }, + { storedAuthOnly, autoOpenControl }, ); await consumeMethodOutcome(outcome, args); @@ -469,6 +482,7 @@ type ParseResult = clientMetadataUrl?: string; callbackUrl?: string; storedAuthOnly?: boolean; + relogin?: boolean; } // Short-circuit modes (`--list-stored-auth`, `--print-handoff`) do their own // output and need no server connection; runCli returns immediately. @@ -658,7 +672,7 @@ async function parseArgs(argv?: string[]): Promise { ) .option( "--relogin", - "Ignore stored OAuth for this run (HTTP/SSE URL keys only); interactive login runs only if the server requires auth. No-op for stdio / servers with no stored entry", + "Delete stored OAuth for this server URL from the shared store before connect (HTTP/SSE URL keys only); interactive login runs only if the server requires auth. No-op for stdio / servers with no stored entry", ) .option( "--wait-for-auth ", @@ -900,10 +914,6 @@ async function parseArgs(argv?: string[]): Promise { options.connectTimeout ?? (adHoc ? DEFAULT_CONNECT_TIMEOUT_MS : undefined), ); - if (options.relogin && "url" in serverConfig && serverConfig.url) { - await clearStoredAuthForRelogin(serverConfig.url); - } - if ( options.appInfo && options.method !== "tools/call" && @@ -979,6 +989,7 @@ async function parseArgs(argv?: string[]): Promise { clientMetadataUrl: options.clientMetadataUrl, callbackUrl: options.callbackUrl, storedAuthOnly: options.storedAuthOnly === true, + relogin: options.relogin === true, }; } @@ -996,6 +1007,7 @@ export async function runCli(argv?: string[]): Promise { clientMetadataUrl, callbackUrl, storedAuthOnly, + relogin, } = parsed; const clientConfig = await loadRunnerClientConfig({ clientConfigPath }); const callbackUrlConfig = parseRunnerOAuthCallbackUrl(callbackUrl); @@ -1011,5 +1023,6 @@ export async function runCli(argv?: string[]): Promise { }, callbackUrlConfig, storedAuthOnly === true, + relogin === true, ); } diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index 86a6086e5..8be2874be 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -18,6 +18,7 @@ import { isOAuthCapableServerConfig } from "@inspector/core/client/runner.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; import { createInterface } from "node:readline/promises"; import { CliExitCodeError, EXIT_CODES } from "./error-handler.js"; +import type { CliOAuthAutoOpenControl } from "./cli-oauth-navigation.js"; export type CliOAuthConnectOptions = { /** @@ -25,6 +26,11 @@ export type CliOAuthConnectOptions = { * store if it can satisfy the challenge; otherwise fail with AUTH_REQUIRED. */ storedAuthOnly?: boolean; + /** + * Arms browser auto-open only around the CLI-owned interactive OAuth flow + * (callback server listening). See {@link createCliOAuthNavigation}. + */ + autoOpenControl?: CliOAuthAutoOpenControl; }; function storedAuthOnlyFailure(message: string): never { @@ -63,6 +69,19 @@ async function promptStepUpConfirm( return confirmStepUp(); } +async function withArmedAutoOpen( + control: CliOAuthAutoOpenControl | undefined, + fn: () => Promise, +): Promise { + if (!control) return fn(); + control.armed = true; + try { + return await fn(); + } finally { + control.armed = false; + } +} + export async function runCliInteractiveOAuth( client: InspectorClient, redirectUrlProvider: MutableRedirectUrlProvider, @@ -70,16 +89,19 @@ export async function runCliInteractiveOAuth( options?: { authorizationUrl?: URL; authChallenge?: AuthChallenge; + autoOpenControl?: CliOAuthAutoOpenControl; }, ): Promise { - const result = await runRunnerInteractiveOAuth({ - client, - redirectUrlProvider, - callbackListen: callbackUrlConfig, - createCallbackServer: createOAuthCallbackServer, - authorizationUrl: options?.authorizationUrl, - authChallenge: options?.authChallenge, - }); + const result = await withArmedAutoOpen(options?.autoOpenControl, () => + runRunnerInteractiveOAuth({ + client, + redirectUrlProvider, + callbackListen: callbackUrlConfig, + createCallbackServer: createOAuthCallbackServer, + authorizationUrl: options?.authorizationUrl, + authChallenge: options?.authChallenge, + }), + ); if (result.kind === "insufficient_scope") { throw new Error(stepUpInsufficientScopeMessage(result.challenge)); @@ -96,6 +118,7 @@ export async function handleCliAuthRecoveryRequired( callbackUrlConfig: RunnerOAuthCallbackConfig, serverSettings?: InspectorServerSettings, confirmStepUp: () => Promise = confirmStepUpFromStdin, + autoOpenControl?: CliOAuthAutoOpenControl, ): Promise { if (isStandardOAuthStepUp(error.authChallenge, serverSettings)) { if (await client.checkAuthChallengeSatisfied(error.authChallenge)) { @@ -114,6 +137,7 @@ export async function handleCliAuthRecoveryRequired( await runCliInteractiveOAuth(client, redirectUrlProvider, callbackUrlConfig, { authorizationUrl: error.authorizationUrl, + autoOpenControl, ...(error.authChallenge.reason === "insufficient_scope" && { authChallenge: error.authChallenge, }), @@ -154,6 +178,8 @@ export async function connectInspectorWithOAuth( redirectUrlProvider, callbackUrlConfig, serverSettings, + confirmStepUpFromStdin, + options?.autoOpenControl, ); await inspectorClient.connect(); return; @@ -172,6 +198,7 @@ export async function connectInspectorWithOAuth( inspectorClient, redirectUrlProvider, callbackUrlConfig, + { autoOpenControl: options?.autoOpenControl }, ); await inspectorClient.connect(); return; @@ -219,6 +246,7 @@ export async function withCliAuthRecoveryRetry( callbackUrlConfig, serverSettings, confirmStepUp, + options?.autoOpenControl, ); process.stderr.write("Authorization complete. Retrying…\n"); return await fn(); diff --git a/clients/cli/src/handlers/consume-outcome.ts b/clients/cli/src/handlers/consume-outcome.ts index 89b4ac7a8..3147098c4 100644 --- a/clients/cli/src/handlers/consume-outcome.ts +++ b/clients/cli/src/handlers/consume-outcome.ts @@ -5,6 +5,9 @@ import type { MethodArgs, MethodOutcome } from "./method-types.js"; /** * Write a {@link MethodOutcome} to stdout (result / NDJSON / long-lived stream). * Stream methods stay attached until SIGINT/SIGTERM. + * + * TODO(#1432): long-lived stream path does not yet handle EPIPE / stdout error + * (session CLI / `mcpi` follow-up). */ export async function consumeMethodOutcome( outcome: MethodOutcome, diff --git a/clients/cli/src/handlers/servers-list.ts b/clients/cli/src/handlers/servers-list.ts index d4d5873e2..755d27495 100644 --- a/clients/cli/src/handlers/servers-list.ts +++ b/clients/cli/src/handlers/servers-list.ts @@ -2,6 +2,7 @@ import type { InspectorServerSettings, MCPServerConfig, } from "@inspector/core/mcp/types.js"; +import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store.js"; import { loadServerEntries, selectServerEntry, @@ -83,11 +84,16 @@ export function summarizeServerConfig(config: MCPServerConfig): { /** * Load catalog/config entries and return a sorted name + summary list. + * Uses an empty in-memory secret store by default so listing never touches the + * OS keychain (names/types/details do not need rehydrated secrets). */ export async function listServerEntries( serverOptions: ServerLoadOptions = {}, ): Promise { - const entries = await loadServerEntries(serverOptions); + const entries = await loadServerEntries({ + ...serverOptions, + secretStore: serverOptions.secretStore ?? new InMemorySecretStore(), + }); return Object.entries(entries) .map(([name, resolved]) => { const { type, detail } = summarizeServerConfig(resolved.config); diff --git a/clients/cli/src/style.ts b/clients/cli/src/style.ts index 4cf9c70bc..dc2ed15c5 100644 --- a/clients/cli/src/style.ts +++ b/clients/cli/src/style.ts @@ -65,7 +65,8 @@ export type ResolveAnsiOptions = { /** * Decide whether session human output should use ANSI. - * Off when: `--plain`, `NO_COLOR` set, `--format json`, or stdout is not a TTY. + * Off when: `--plain`, `NO_COLOR` set, `--format json`, or the caller's stream + * is not a TTY (override via `isTTY`; default checks `process.stdout`). */ export function resolveAnsiEnabled(opts: ResolveAnsiOptions = {}): boolean { if (opts.plain) return false; From f4a010d412691967d0efaa5b04920681ff47b3c7 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 17:35:42 -0700 Subject: [PATCH 04/13] fix(cli): silence disarmed OAuth URLs and round-3 review nits Suppress authorize URL print when navigation is disarmed or --stored-auth-only; document MCP_AUTO_OPEN_ENABLED; redact Cookie/auth headers; keep servers/show unit tests off the OS keychain. Co-authored-by: Cursor --- clients/cli/README.md | 3 +- .../__tests__/cli-oauth-navigation.test.ts | 15 ++++---- clients/cli/__tests__/servers-list.test.ts | 20 ++++++++--- clients/cli/src/cli-oauth-navigation.ts | 34 ++++++++++++------- clients/cli/src/handlers/servers-list.ts | 3 +- 5 files changed, 50 insertions(+), 25 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index b0fcb7805..e721b6b7e 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -149,7 +149,7 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 **CLI (`mcp-inspector --cli`):** on connect **401** or mid-session interactive auth (re-login / step-up), it: 1. Starts the callback listener on `--callback-url` (or `MCP_OAUTH_CALLBACK_URL`) -2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** on a TTY; non-TTY / CI prints a plain URL only and never launches a browser +2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** on a TTY; non-TTY / CI prints a plain URL only and never launches a browser. Opt out of the browser launch with `MCP_AUTO_OPEN_ENABLED=false` (same 3-way resolve as the web launcher: `true` / `false` / default off under `VITEST`). `--stored-auth-only` never prints or opens. 3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC **Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues; **N** exits with an error. EMA step-up re-mints silently (no prompt). @@ -182,6 +182,7 @@ Register `http://127.0.0.1:6276/oauth/callback` on static or enterprise IdPs tha | `--client-secret ` | — | OAuth client secret; overrides `client.json`. | | `--client-metadata-url ` | — | CIMD metadata URL; overrides `client.json`. | | `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | Redirect URI sent to the authorization server (default: `http://127.0.0.1:6276/oauth/callback`). | +| — | `MCP_AUTO_OPEN_ENABLED` | Browser auto-open for interactive OAuth: `true` / `false` / unset (open unless `VITEST` is set). Same semantics as the web launcher / `Dockerfile`. | **Example** — list tools on an OAuth-protected server using stored tokens and CIMD from the command line: diff --git a/clients/cli/__tests__/cli-oauth-navigation.test.ts b/clients/cli/__tests__/cli-oauth-navigation.test.ts index d8600f971..d3d17fb1e 100644 --- a/clients/cli/__tests__/cli-oauth-navigation.test.ts +++ b/clients/cli/__tests__/cli-oauth-navigation.test.ts @@ -61,7 +61,7 @@ describe("createCliOAuthNavigation", () => { expect(openUrlMock).not.toHaveBeenCalled(); }); - it("prints the URL but does not open a browser when disarmed (SDK-during-connect default)", async () => { + it("is a silent no-op when disarmed (SDK-during-connect default)", async () => { const lines: string[] = []; const openBrowser = vi.fn(); const nav = createCliOAuthNavigation({ @@ -73,12 +73,13 @@ describe("createCliOAuthNavigation", () => { // no autoOpenControl → never armed }); nav.navigateToAuthorization(new URL("https://as.example/authorize")); - await vi.waitFor(() => expect(lines.length).toBe(1)); - expect(lines.join("")).toContain("Please navigate to:"); + // Navigation is fire-and-forget; give the microtask a turn. + await Promise.resolve(); + expect(lines).toEqual([]); expect(openBrowser).not.toHaveBeenCalled(); }); - it("does not open a browser when disableAutoOpen is set", async () => { + it("is a silent no-op when disableAutoOpen is set (stored-auth-only)", async () => { const lines: string[] = []; const openBrowser = vi.fn(); const nav = createCliOAuthNavigation({ @@ -91,11 +92,12 @@ describe("createCliOAuthNavigation", () => { disableAutoOpen: true, }); nav.navigateToAuthorization(new URL("https://as.example/authorize")); - await vi.waitFor(() => expect(lines.length).toBe(1)); + await Promise.resolve(); + expect(lines).toEqual([]); expect(openBrowser).not.toHaveBeenCalled(); }); - it("does not open a browser when autoOpenEnabled is false", async () => { + it("prints but does not open a browser when autoOpenEnabled is false", async () => { const lines: string[] = []; const openBrowser = vi.fn(); const nav = createCliOAuthNavigation({ @@ -108,6 +110,7 @@ describe("createCliOAuthNavigation", () => { }); nav.navigateToAuthorization(new URL("https://as.example/authorize")); await vi.waitFor(() => expect(lines.length).toBe(1)); + expect(lines.join("")).toContain("Please navigate to:"); expect(openBrowser).not.toHaveBeenCalled(); }); diff --git a/clients/cli/__tests__/servers-list.test.ts b/clients/cli/__tests__/servers-list.test.ts index fbb9f9b60..f6893cb56 100644 --- a/clients/cli/__tests__/servers-list.test.ts +++ b/clients/cli/__tests__/servers-list.test.ts @@ -163,6 +163,9 @@ describe("showServerEntry / servers/show", () => { requestInit: { headers: { Authorization: "Bearer secret", + Cookie: "session=abc", + "X-Auth": "t", + "Proxy-Authorization": "Basic x", "X-Custom": "ok", }, }, @@ -177,6 +180,9 @@ describe("showServerEntry / servers/show", () => { requestInit: { headers: { Authorization: "[redacted]", + Cookie: "[redacted]", + "X-Auth": "[redacted]", + "Proxy-Authorization": "[redacted]", "X-Custom": "ok", }, }, @@ -241,7 +247,10 @@ describe("showServerEntry / servers/show", () => { it("shows one resolved entry without connecting", async () => { configPath = createSampleTestConfig(); - const entry = await showServerEntry("test-stdio", { configPath }); + const entry = await showServerEntry("test-stdio", { + configPath, + secretStore: new InMemorySecretStore(), + }); expect(entry.name).toBe("test-stdio"); expect(entry.type).toBe("stdio"); expect(entry.config).toMatchObject({ @@ -329,8 +338,11 @@ describe("showServerEntry / servers/show", () => { it("rejects unknown server names", async () => { configPath = createSampleTestConfig(); - await expect(showServerEntry("nope", { configPath })).rejects.toThrow( - /not found/, - ); + await expect( + showServerEntry("nope", { + configPath, + secretStore: new InMemorySecretStore(), + }), + ).rejects.toThrow(/not found/); }); }); diff --git a/clients/cli/src/cli-oauth-navigation.ts b/clients/cli/src/cli-oauth-navigation.ts index 34a901955..cf3b778ef 100644 --- a/clients/cli/src/cli-oauth-navigation.ts +++ b/clients/cli/src/cli-oauth-navigation.ts @@ -3,10 +3,10 @@ import { openUrl } from "./open-url.js"; import { createStyle, resolveAnsiEnabled } from "./style.js"; /** - * Arms browser auto-open only for the CLI-owned interactive OAuth flow (the - * one with a listening callback server). Left disarmed during - * `inspectorClient.connect()` so an SDK-internal `auth()` cannot open a - * browser (or a second doomed tab) before CLI gates run. + * Arms URL print + browser auto-open only for the CLI-owned interactive OAuth + * flow (the one with a listening callback server). Left disarmed during + * `inspectorClient.connect()` so an SDK-internal `auth()` cannot print a + * doomed authorize URL (or open a second browser tab) before CLI gates run. */ export type CliOAuthAutoOpenControl = { armed: boolean; @@ -22,11 +22,16 @@ export type CliOAuthNavigationOptions = { /** Open the browser (defaults to {@link openUrl}). */ openBrowser?: (url: string) => Promise; /** - * When set, browser open is allowed only while {@link CliOAuthAutoOpenControl.armed} - * is true. When omitted, auto-open is never armed (print URL only). + * When set, print + browser open are allowed only while + * {@link CliOAuthAutoOpenControl.armed} is true. When omitted, navigation is + * a no-op (SDK-during-connect default). */ autoOpenControl?: CliOAuthAutoOpenControl; - /** Hard-disable browser open (e.g. `--stored-auth-only`). */ + /** + * Suppress print + browser open entirely (e.g. `--stored-auth-only`). A + * disarmed/disabled navigation is never actionable — no callback server is + * listening for that authorize URL. + */ disableAutoOpen?: boolean; /** * Override {@link resolveCliAutoOpenEnabled} (tests). When omitted, uses @@ -51,15 +56,19 @@ export function resolveCliAutoOpenEnabled( } /** - * CLI OAuth navigation: always print the authorization URL (OSC 8 when TTY - * allows ANSI). Browser open is gated by {@link CliOAuthAutoOpenControl} - * (armed only for the CLI interactive flow), `--stored-auth-only`, TTY, - * and {@link resolveCliAutoOpenEnabled}. + * CLI OAuth navigation: print the authorization URL (OSC 8 when TTY allows + * ANSI) and optionally open the browser — but only when armed for the + * CLI-owned interactive flow. Disarmed / `--stored-auth-only` navigations are + * silent no-ops so SDK-internal `auth()` during connect cannot emit an + * uncompletable "Please navigate to:" line. */ export function createCliOAuthNavigation( options: CliOAuthNavigationOptions = {}, ): CallbackNavigation { return new CallbackNavigation(async (url) => { + const armed = options.autoOpenControl?.armed === true; + if (options.disableAutoOpen || !armed) return; + const href = url.href; const tty = options.isTTY !== undefined @@ -79,8 +88,7 @@ export function createCliOAuthNavigation( options.autoOpenEnabled !== undefined ? options.autoOpenEnabled : resolveCliAutoOpenEnabled(); - const armed = options.autoOpenControl?.armed === true; - if (options.disableAutoOpen || !armed || !envAllows || !tty) return; + if (!envAllows || !tty) return; try { await (options.openBrowser ?? openUrl)(href); diff --git a/clients/cli/src/handlers/servers-list.ts b/clients/cli/src/handlers/servers-list.ts index 755d27495..c0a1c4f67 100644 --- a/clients/cli/src/handlers/servers-list.ts +++ b/clients/cli/src/handlers/servers-list.ts @@ -211,7 +211,8 @@ function sanitizeInitRecord( function isSensitiveHeader(key: string): boolean { const k = key.toLowerCase(); return ( - k === "authorization" || + k.includes("auth") || + k.includes("cookie") || k.includes("secret") || k.includes("token") || k.includes("password") || From fad2c13a6a1c72f4e7490a3911c277aa28f73887 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 17:42:23 -0700 Subject: [PATCH 05/13] =?UTF-8?q?test(cli):=20cover=20withArmedAutoOpen=20?= =?UTF-8?q?for=20the=20=E2=89=A590=20branch=20gate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed on cliOAuth.ts branch coverage after the auto-open arming helper landed; exercise the armed path through runCliInteractiveOAuth. Co-authored-by: Cursor --- clients/cli/__tests__/cliOAuth.test.ts | 31 ++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index 68848b4cd..7114f63a7 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -82,6 +82,37 @@ describe("cliOAuth", () => { expect(stderrSpy).toHaveBeenCalledWith("Authorization complete.\n"); }); + it("runCliInteractiveOAuth arms autoOpenControl only for the interactive flow", async () => { + const autoOpenControl = { armed: false }; + let armedDuringCall = false; + vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth").mockImplementation( + async () => { + armedDuringCall = autoOpenControl.armed; + return { kind: "success" }; + }, + ); + const client = { + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }; + const stderrSpy = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); + + await runCliInteractiveOAuth( + client, + new MutableRedirectUrlProvider(), + { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, + { autoOpenControl }, + ); + + expect(armedDuringCall).toBe(true); + expect(autoOpenControl.armed).toBe(false); + expect(stderrSpy).toHaveBeenCalledWith("Authorization complete.\n"); + }); + it("runCliInteractiveOAuth throws when scopes remain insufficient", async () => { const challenge = { reason: "insufficient_scope" as const, From 61a8b985c7b3c90524be589e0bebd5993b03e4e9 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 19:10:04 -0700 Subject: [PATCH 06/13] fix(cli): mid-RPC unauthorized recovery and clear round-4 nits Mirror connect's UnauthorizedError path in withCliAuthRecoveryRetry, let MCP_AUTO_OPEN_ENABLED=true force open off-TTY, mock open-url in the in-process runner, reject --relogin on catalog methods, restore the __tests__ README inventory, and narrow OAuth helpers to structural client types. Co-authored-by: Cursor --- clients/cli/README.md | 4 +- clients/cli/__tests__/README.md | 52 ++++++++- .../__tests__/cli-oauth-navigation.test.ts | 51 ++++++++- clients/cli/__tests__/cliOAuth.test.ts | 106 +++++++++++++++++- clients/cli/__tests__/helpers/cli-runner.ts | 9 ++ .../__tests__/programmatic-ergonomics.test.ts | 6 + clients/cli/src/cli-oauth-navigation.ts | 23 +++- clients/cli/src/cli.ts | 8 ++ clients/cli/src/cliOAuth.ts | 101 +++++++++++------ 9 files changed, 314 insertions(+), 46 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index e721b6b7e..7b9d407ca 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -149,7 +149,7 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 **CLI (`mcp-inspector --cli`):** on connect **401** or mid-session interactive auth (re-login / step-up), it: 1. Starts the callback listener on `--callback-url` (or `MCP_OAUTH_CALLBACK_URL`) -2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** on a TTY; non-TTY / CI prints a plain URL only and never launches a browser. Opt out of the browser launch with `MCP_AUTO_OPEN_ENABLED=false` (same 3-way resolve as the web launcher: `true` / `false` / default off under `VITEST`). `--stored-auth-only` never prints or opens. +2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** when allowed. Default: open on a TTY, plain URL only when non-TTY / CI. `MCP_AUTO_OPEN_ENABLED=false` never opens; `=true` forces open even on a non-TTY (same as the web launcher). `--stored-auth-only` never prints or opens. 3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC **Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues; **N** exits with an error. EMA step-up re-mints silently (no prompt). @@ -182,7 +182,7 @@ Register `http://127.0.0.1:6276/oauth/callback` on static or enterprise IdPs tha | `--client-secret ` | — | OAuth client secret; overrides `client.json`. | | `--client-metadata-url ` | — | CIMD metadata URL; overrides `client.json`. | | `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | Redirect URI sent to the authorization server (default: `http://127.0.0.1:6276/oauth/callback`). | -| — | `MCP_AUTO_OPEN_ENABLED` | Browser auto-open for interactive OAuth: `true` / `false` / unset (open unless `VITEST` is set). Same semantics as the web launcher / `Dockerfile`. | +| — | `MCP_AUTO_OPEN_ENABLED` | Browser auto-open for interactive OAuth: `true` (always open, including non-TTY — same as the web launcher), `false` (never), unset (open on a TTY unless `VITEST` is set). | **Example** — list tools on an OAuth-protected server using stored tokens and CIMD from the command line: diff --git a/clients/cli/__tests__/README.md b/clients/cli/__tests__/README.md index 75eb42ada..712488444 100644 --- a/clients/cli/__tests__/README.md +++ b/clients/cli/__tests__/README.md @@ -1,21 +1,63 @@ # CLI Tests -Tests live next to the suite entrypoints under `__tests__/` and run via Vitest. +Tests live under `__tests__/` and run via Vitest. - Most tests import `runCli()` **in-process** (see `helpers/cli-runner.ts`) so - `clients/cli/src` is measured under the coverage gate. + `clients/cli/src` is measured under the coverage gate. That helper mocks + `open-url` so an armed interactive OAuth path cannot launch a real browser. - `e2e.test.ts` (and root `scripts/smoke-cli.mjs`) spawn the built binary for shebang / `process.exit` paths — `pretest` builds `test-servers` + the CLI bundle first. -Useful scripts (from `clients/cli/`): +## Scripts (from `clients/cli/`) ```bash npm test # pretest build + all tests npm run test:cli # subset: cli.test.ts npm run test:cli-tools # subset: tools.test.ts +npm run test:cli-headers # subset: headers.test.ts +npm run test:cli-metadata # subset: metadata.test.ts npm run test:coverage # build + ≥90 per-file coverage gate +npm run validate # format:check && lint && typecheck && test ``` -OAuth interactive tests may open a loopback callback; they stub or drive that -path in-process rather than requiring a real browser. +## Test files + +| File | Focus | +| --------------------------------------- | ------------------------------------------- | +| `cli.test.ts` | Core connect / method / config matrix | +| `tools.test.ts` | `tools/call` argument coercion | +| `headers.test.ts` | `--header` merging | +| `metadata.test.ts` | `--metadata` / `--tool-metadata` | +| `methods.test.ts` | Method allow-list / rejection | +| `app-info.test.ts` | `--app-info` probe paths | +| `format-json.test.ts` | `--format json` envelopes | +| `format-output.test.ts` | Text/json writers | +| `emit-result.test.ts` | Result emission helpers | +| `method-types.test.ts` | `ONE_SHOT_METHODS` / guards | +| `run-method.test.ts` | Handler dispatch against a real test server | +| `run-method-mocks.test.ts` | Handler edge cases with mocks | +| `servers-list.test.ts` | `servers/list` / `servers/show` + redaction | +| `cliOAuth.test.ts` | Connect / mid-RPC OAuth recovery | +| `cli-oauth-navigation.test.ts` | OSC 8, arm/disarm, `MCP_AUTO_OPEN_ENABLED` | +| `open-url.test.ts` | `open` package wrapper | +| `oauth-runner.test.ts` | Runner client-config / CIMD flags | +| `oauth-interactive.test.ts` | Loopback callback OAuth (in-process) | +| `stored-auth.test.ts` | `--use-stored-auth` / handoff / wait | +| `clear-stored-auth-for-relogin.test.ts` | Dual-key `--relogin` clear | +| `programmatic-ergonomics.test.ts` | Flag conflicts, timeouts, ergonomics | +| `error-handler.test.ts` | Exit codes / error shaping | +| `style.test.ts` | ANSI / OSC 8 helpers | +| `e2e.test.ts` | Out-of-process binary smoke | +| `helpers/assertions.test.ts` | Assertion helpers | + +## Helpers + +| Helper | Role | +| ----------------------- | ---------------------------------------------------------------- | +| `helpers/cli-runner.ts` | In-process `runCli` with stdout/stderr capture + `open-url` mock | +| `helpers/assertions.ts` | `expectCliSuccess` / `expectCliFailure` / output matchers | +| `helpers/fixtures.ts` | Temp config / client.json factories | + +OAuth interactive tests stub or drive the loopback callback in-process rather +than requiring a real browser. diff --git a/clients/cli/__tests__/cli-oauth-navigation.test.ts b/clients/cli/__tests__/cli-oauth-navigation.test.ts index d3d17fb1e..33008be9c 100644 --- a/clients/cli/__tests__/cli-oauth-navigation.test.ts +++ b/clients/cli/__tests__/cli-oauth-navigation.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createCliOAuthNavigation, + isCliAutoOpenForced, resolveCliAutoOpenEnabled, } from "../src/cli-oauth-navigation.js"; import { openUrl } from "../src/open-url.js"; @@ -30,6 +31,14 @@ describe("resolveCliAutoOpenEnabled", () => { }); }); +describe("isCliAutoOpenForced", () => { + it("is true only for the explicit string true", () => { + expect(isCliAutoOpenForced({ MCP_AUTO_OPEN_ENABLED: "true" })).toBe(true); + expect(isCliAutoOpenForced({ MCP_AUTO_OPEN_ENABLED: "false" })).toBe(false); + expect(isCliAutoOpenForced({})).toBe(false); + }); +}); + describe("createCliOAuthNavigation", () => { afterEach(() => { openUrlMock.mockClear(); @@ -114,7 +123,7 @@ describe("createCliOAuthNavigation", () => { expect(openBrowser).not.toHaveBeenCalled(); }); - it("prints a plain URL and does not open a browser when not a TTY", async () => { + it("prints a plain URL and does not open a browser when not a TTY (unless forced)", async () => { const lines: string[] = []; const openBrowser = vi.fn(); const nav = createCliOAuthNavigation({ @@ -134,6 +143,46 @@ describe("createCliOAuthNavigation", () => { expect(openUrlMock).not.toHaveBeenCalled(); }); + it("opens on a non-TTY when forceAutoOpen is set (MCP_AUTO_OPEN_ENABLED=true)", async () => { + const lines: string[] = []; + const openBrowser = vi.fn().mockResolvedValue(undefined); + const nav = createCliOAuthNavigation({ + isTTY: false, + noColorEnv: "1", + write: (line) => lines.push(line), + openBrowser, + autoOpenControl: { armed: true }, + autoOpenEnabled: true, + forceAutoOpen: true, + }); + nav.navigateToAuthorization(new URL("https://as.example/authorize")); + await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce()); + expect(lines.join("")).toContain("Please navigate to:"); + }); + + it("infers force-open from MCP_AUTO_OPEN_ENABLED=true when options omit overrides", async () => { + const prev = process.env.MCP_AUTO_OPEN_ENABLED; + process.env.MCP_AUTO_OPEN_ENABLED = "true"; + try { + const openBrowser = vi.fn().mockResolvedValue(undefined); + const nav = createCliOAuthNavigation({ + isTTY: false, + noColorEnv: "1", + write: () => {}, + openBrowser, + autoOpenControl: { armed: true }, + }); + nav.navigateToAuthorization(new URL("https://as.example/authorize")); + await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce()); + } finally { + if (prev === undefined) { + delete process.env.MCP_AUTO_OPEN_ENABLED; + } else { + process.env.MCP_AUTO_OPEN_ENABLED = prev; + } + } + }); + it("skips OSC 8 when NO_COLOR is set but still opens on a TTY when armed", async () => { const lines: string[] = []; const openBrowser = vi.fn().mockResolvedValue(undefined); diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index 7114f63a7..6274b4b33 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -511,6 +511,36 @@ describe("cliOAuth", () => { expect(runSpy).not.toHaveBeenCalled(); }); + it("under --stored-auth-only reconnects when the store already satisfies", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const connect = vi + .fn() + .mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + { reason: "token_expired" }, + ), + ) + .mockResolvedValueOnce(undefined); + const client = { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(true), + }; + + await connectInspectorWithOAuth( + client, + oauthServerConfig, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + { storedAuthOnly: true }, + ); + + expect(connect).toHaveBeenCalledTimes(2); + expect(runSpy).not.toHaveBeenCalled(); + }); + it("fails stored-auth-only on plain unauthorized errors", async () => { const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); const connect = vi @@ -546,8 +576,13 @@ describe("cliOAuth", () => { await expect( withCliAuthRecoveryRetry( { + connect: vi.fn(), + disconnect: vi.fn(), + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), checkAuthChallengeSatisfied, - } as never, + }, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, undefined, @@ -577,8 +612,13 @@ describe("cliOAuth", () => { const result = await withCliAuthRecoveryRetry( { + connect: vi.fn(), + disconnect: vi.fn(), + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), checkAuthChallengeSatisfied, - } as never, + }, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, undefined, @@ -592,4 +632,66 @@ describe("cliOAuth", () => { expect(checkAuthChallengeSatisfied).toHaveBeenCalledWith(challenge); expect(runSpy).not.toHaveBeenCalled(); }); + + it("withCliAuthRecoveryRetry recovers plain unauthorized errors like connect", async () => { + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + const disconnect = vi.fn().mockResolvedValue(undefined); + const fn = vi + .fn() + .mockRejectedValueOnce(new Error("RPC failed (401)")) + .mockResolvedValueOnce("ok"); + const stderrSpy = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); + + const result = await withCliAuthRecoveryRetry( + { + connect: vi.fn(), + disconnect, + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + fn, + ); + + expect(result).toBe("ok"); + expect(disconnect).toHaveBeenCalledOnce(); + expect(runSpy).toHaveBeenCalledOnce(); + expect(fn).toHaveBeenCalledTimes(2); + expect(stderrSpy).toHaveBeenCalledWith( + "Authorization complete. Retrying…\n", + ); + }); + + it("withCliAuthRecoveryRetry fails stored-auth-only on plain unauthorized errors", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const fn = vi.fn().mockRejectedValue(new Error("RPC failed (401)")); + + await expect( + withCliAuthRecoveryRetry( + { + connect: vi.fn(), + disconnect: vi.fn(), + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + fn, + undefined, + { storedAuthOnly: true }, + ), + ).rejects.toMatchObject({ exitCode: 3 }); + expect(runSpy).not.toHaveBeenCalled(); + }); }); diff --git a/clients/cli/__tests__/helpers/cli-runner.ts b/clients/cli/__tests__/helpers/cli-runner.ts index d5c8abe66..76abd5326 100644 --- a/clients/cli/__tests__/helpers/cli-runner.ts +++ b/clients/cli/__tests__/helpers/cli-runner.ts @@ -1,3 +1,12 @@ +import { vi } from "vitest"; + +// Structural guard: any in-process `runCli` path that arms interactive OAuth +// must not shell out to a real browser. Do not rely on `VITEST` alone — +// `cli-oauth-navigation` also honors `MCP_AUTO_OPEN_ENABLED=true`. +vi.mock("../../src/open-url.js", () => ({ + openUrl: vi.fn().mockResolvedValue(undefined), +})); + import { runCli as invokeCli } from "../../src/cli.js"; import { formatErrorOutput } from "../../src/error-handler.js"; diff --git a/clients/cli/__tests__/programmatic-ergonomics.test.ts b/clients/cli/__tests__/programmatic-ergonomics.test.ts index 0318d8278..7398d3456 100644 --- a/clients/cli/__tests__/programmatic-ergonomics.test.ts +++ b/clients/cli/__tests__/programmatic-ergonomics.test.ts @@ -164,6 +164,12 @@ describe("CLI --relogin flag conflicts", () => { expect(result.stderr).toMatch(/--use-stored-auth/); }); + it("rejects --relogin with catalog-only methods", async () => { + const result = await runCli(["--relogin", "--method", "servers/list"]); + expectCliFailure(result); + expect(result.stderr).toMatch(/servers\/list or servers\/show/); + }); + it("accepts --relogin and clears stored auth before connect", async () => { // Unreachable port: proves --relogin runs (clears store) then fails connect. const result = await runCli([ diff --git a/clients/cli/src/cli-oauth-navigation.ts b/clients/cli/src/cli-oauth-navigation.ts index cf3b778ef..699536985 100644 --- a/clients/cli/src/cli-oauth-navigation.ts +++ b/clients/cli/src/cli-oauth-navigation.ts @@ -38,6 +38,11 @@ export type CliOAuthNavigationOptions = { * `MCP_AUTO_OPEN_ENABLED` / `VITEST` the same way the web server does. */ autoOpenEnabled?: boolean; + /** + * When true, open even if stderr is not a TTY (matches web's explicit + * `MCP_AUTO_OPEN_ENABLED=true`). When omitted, inferred from the env flag. + */ + forceAutoOpen?: boolean; }; /** @@ -55,12 +60,22 @@ export function resolveCliAutoOpenEnabled( return !env.VITEST; } +/** True when `MCP_AUTO_OPEN_ENABLED=true` (explicit force, including non-TTY). */ +export function isCliAutoOpenForced( + env: NodeJS.ProcessEnv = process.env, +): boolean { + return env.MCP_AUTO_OPEN_ENABLED === "true"; +} + /** * CLI OAuth navigation: print the authorization URL (OSC 8 when TTY allows * ANSI) and optionally open the browser — but only when armed for the * CLI-owned interactive flow. Disarmed / `--stored-auth-only` navigations are * silent no-ops so SDK-internal `auth()` during connect cannot emit an * uncompletable "Please navigate to:" line. + * + * Browser open: off when env disables it; on a TTY when enabled; forced open + * (TTY optional) when `MCP_AUTO_OPEN_ENABLED=true` / {@link forceAutoOpen}. */ export function createCliOAuthNavigation( options: CliOAuthNavigationOptions = {}, @@ -88,7 +103,13 @@ export function createCliOAuthNavigation( options.autoOpenEnabled !== undefined ? options.autoOpenEnabled : resolveCliAutoOpenEnabled(); - if (!envAllows || !tty) return; + if (!envAllows) return; + + const forced = + options.forceAutoOpen !== undefined + ? options.forceAutoOpen + : options.autoOpenEnabled === undefined && isCliAutoOpenForced(); + if (!forced && !tty) return; try { await (options.openBrowser ?? openUrl)(href); diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index d9a0f57d3..0bd58064c 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -742,6 +742,14 @@ async function parseArgs(argv?: string[]): Promise { "--relogin cannot be combined with --use-stored-auth or --wait-for-auth", ); } + if ( + options.method === "servers/list" || + options.method === "servers/show" + ) { + throw new Error( + "--relogin cannot be combined with --method servers/list or servers/show (no OAuth connect)", + ); + } } // State-path precedence (getStateFilePath): MCP_INSPECTOR_OAUTH_STATE_PATH → diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index 8be2874be..e90e6264d 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -11,8 +11,8 @@ import { createOAuthCallbackServer, runRunnerInteractiveOAuth, } from "@inspector/core/auth/node/index.js"; +import type { RunnerInteractiveOAuthClient } from "@inspector/core/auth/node/runner-interactive-oauth.js"; import type { RunnerOAuthCallbackConfig } from "@inspector/core/auth/node/runner-oauth-callback.js"; -import type { InspectorClient } from "@inspector/core/mcp/index.js"; import type { InspectorServerSettings } from "@inspector/core/mcp/types.js"; import { isOAuthCapableServerConfig } from "@inspector/core/client/runner.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; @@ -20,6 +20,12 @@ import { createInterface } from "node:readline/promises"; import { CliExitCodeError, EXIT_CODES } from "./error-handler.js"; import type { CliOAuthAutoOpenControl } from "./cli-oauth-navigation.js"; +/** Client surface needed for connect + mid-RPC OAuth recovery. */ +export type CliOAuthClient = RunnerInteractiveOAuthClient & { + connect(): Promise; + disconnect(): Promise; +}; + export type CliOAuthConnectOptions = { /** * When true, never open interactive OAuth / step-up prompts. Use the shared @@ -83,7 +89,7 @@ async function withArmedAutoOpen( } export async function runCliInteractiveOAuth( - client: InspectorClient, + client: RunnerInteractiveOAuthClient, redirectUrlProvider: MutableRedirectUrlProvider, callbackUrlConfig: RunnerOAuthCallbackConfig, options?: { @@ -112,7 +118,7 @@ export async function runCliInteractiveOAuth( } export async function handleCliAuthRecoveryRequired( - client: InspectorClient, + client: RunnerInteractiveOAuthClient, error: AuthRecoveryRequiredError, redirectUrlProvider: MutableRedirectUrlProvider, callbackUrlConfig: RunnerOAuthCallbackConfig, @@ -145,7 +151,7 @@ export async function handleCliAuthRecoveryRequired( } export async function connectInspectorWithOAuth( - inspectorClient: InspectorClient, + inspectorClient: CliOAuthClient, serverConfig: MCPServerConfig, redirectUrlProvider: MutableRedirectUrlProvider, callbackUrlConfig: RunnerOAuthCallbackConfig, @@ -160,13 +166,15 @@ export async function connectInspectorWithOAuth( } if (err instanceof AuthRecoveryRequiredError) { - if ( - await inspectorClient.checkAuthChallengeSatisfied(err.authChallenge) - ) { - await inspectorClient.connect(); - return; - } + // Under --stored-auth-only, give the store one chance then bail — avoid + // calling handle (which would re-check) on the failure path. if (options?.storedAuthOnly) { + if ( + await inspectorClient.checkAuthChallengeSatisfied(err.authChallenge) + ) { + await inspectorClient.connect(); + return; + } storedAuthOnlyFailure( err.message || "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", @@ -209,11 +217,13 @@ export async function connectInspectorWithOAuth( } /** - * Run `fn` once; on {@link AuthRecoveryRequiredError}, complete interactive OAuth - * and retry `fn` a single time (no further recovery attempts). + * Run `fn` once; on auth recovery errors, complete interactive OAuth and retry + * `fn` a single time (no further recovery attempts). Mirrors + * {@link connectInspectorWithOAuth}: handles both + * {@link AuthRecoveryRequiredError} and plain unauthorized errors. */ export async function withCliAuthRecoveryRetry( - inspectorClient: InspectorClient, + inspectorClient: CliOAuthClient, redirectUrlProvider: MutableRedirectUrlProvider, callbackUrlConfig: RunnerOAuthCallbackConfig, serverSettings: InspectorServerSettings | undefined, @@ -224,31 +234,52 @@ export async function withCliAuthRecoveryRetry( try { return await fn(); } catch (err) { - if (!(err instanceof AuthRecoveryRequiredError)) { - throw err; - } - // Match connectInspectorWithOAuth: give the shared store a chance to - // satisfy the challenge (already-satisfied / EMA re-mint) before bailing - // under --stored-auth-only or opening interactive OAuth. - if (await inspectorClient.checkAuthChallengeSatisfied(err.authChallenge)) { + if (err instanceof AuthRecoveryRequiredError) { + // Satisfied-check lives in handleCliAuthRecoveryRequired for the + // interactive path; under --stored-auth-only check once here then bail. + if (options?.storedAuthOnly) { + if ( + await inspectorClient.checkAuthChallengeSatisfied(err.authChallenge) + ) { + return await fn(); + } + storedAuthOnlyFailure( + err.message || + "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", + ); + } + await handleCliAuthRecoveryRequired( + inspectorClient, + err, + redirectUrlProvider, + callbackUrlConfig, + serverSettings, + confirmStepUp, + options?.autoOpenControl, + ); + process.stderr.write("Authorization complete. Retrying…\n"); return await fn(); } - if (options?.storedAuthOnly) { - storedAuthOnlyFailure( - err.message || - "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", + + if (isUnauthorizedError(err)) { + if (options?.storedAuthOnly) { + storedAuthOnlyFailure( + err instanceof Error + ? err.message + : "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", + ); + } + await inspectorClient.disconnect().catch(() => {}); + await runCliInteractiveOAuth( + inspectorClient, + redirectUrlProvider, + callbackUrlConfig, + { autoOpenControl: options?.autoOpenControl }, ); + process.stderr.write("Authorization complete. Retrying…\n"); + return await fn(); } - await handleCliAuthRecoveryRequired( - inspectorClient, - err, - redirectUrlProvider, - callbackUrlConfig, - serverSettings, - confirmStepUp, - options?.autoOpenControl, - ); - process.stderr.write("Authorization complete. Retrying…\n"); - return await fn(); + + throw err; } } From 94ed7c99f3c4a7cd8bf03e12ad66cba1664e0ec1 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 19:26:31 -0700 Subject: [PATCH 07/13] fix(cli): reconnect after mid-RPC OAuth and finish round-5 nits Reconnect before retrying after unauthorized recovery, guard non-OAuth configs, decouple forceAutoOpen, move open-url mock to vitest setupFiles, and reject --relogin with stored-auth short-circuits. Co-authored-by: Cursor --- clients/cli/__tests__/README.md | 16 ++- clients/cli/__tests__/cliOAuth.test.ts | 123 ++++++++++++++++-- clients/cli/__tests__/helpers/cli-runner.ts | 9 -- .../cli/__tests__/helpers/mock-open-url.ts | 10 ++ .../cli/__tests__/oauth-interactive.test.ts | 1 + clients/cli/__tests__/open-url.test.ts | 3 + .../__tests__/programmatic-ergonomics.test.ts | 6 + clients/cli/src/cli-oauth-navigation.ts | 9 +- clients/cli/src/cli.ts | 6 + clients/cli/src/cliOAuth.ts | 17 ++- clients/cli/vitest.config.ts | 1 + 11 files changed, 168 insertions(+), 33 deletions(-) create mode 100644 clients/cli/__tests__/helpers/mock-open-url.ts diff --git a/clients/cli/__tests__/README.md b/clients/cli/__tests__/README.md index 712488444..0e341bbf1 100644 --- a/clients/cli/__tests__/README.md +++ b/clients/cli/__tests__/README.md @@ -3,8 +3,9 @@ Tests live under `__tests__/` and run via Vitest. - Most tests import `runCli()` **in-process** (see `helpers/cli-runner.ts`) so - `clients/cli/src` is measured under the coverage gate. That helper mocks - `open-url` so an armed interactive OAuth path cannot launch a real browser. + `clients/cli/src` is measured under the coverage gate. Suite-wide + `helpers/mock-open-url.ts` (vitest `setupFiles`) mocks `open-url` so an armed + interactive OAuth path cannot launch a real browser. - `e2e.test.ts` (and root `scripts/smoke-cli.mjs`) spawn the built binary for shebang / `process.exit` paths — `pretest` builds `test-servers` + the CLI bundle first. @@ -53,11 +54,12 @@ npm run validate # format:check && lint && typecheck && test ## Helpers -| Helper | Role | -| ----------------------- | ---------------------------------------------------------------- | -| `helpers/cli-runner.ts` | In-process `runCli` with stdout/stderr capture + `open-url` mock | -| `helpers/assertions.ts` | `expectCliSuccess` / `expectCliFailure` / output matchers | -| `helpers/fixtures.ts` | Temp config / client.json factories | +| Helper | Role | +| -------------------------- | --------------------------------------------------------- | +| `helpers/cli-runner.ts` | In-process `runCli` with stdout/stderr capture | +| `helpers/mock-open-url.ts` | Suite-wide `open-url` mock (vitest `setupFiles`) | +| `helpers/assertions.ts` | `expectCliSuccess` / `expectCliFailure` / output matchers | +| `helpers/fixtures.ts` | Temp config / client.json factories | OAuth interactive tests stub or drive the loopback callback in-process rather than requiring a real browser. diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index 6274b4b33..ca78b6cc0 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -31,6 +31,16 @@ const CALLBACK_URL_CONFIG = { pathname: "/oauth/callback", }; +const OAUTH_HTTP_CONFIG = { + type: "streamable-http", + url: "https://as.example/mcp", +} as MCPServerConfig; + +const STDIO_CONFIG = { + type: "stdio", + command: "x", +} as MCPServerConfig; + describe("cliOAuth", () => { afterEach(() => { vi.restoreAllMocks(); @@ -255,7 +265,10 @@ describe("cliOAuth", () => { vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth").mockResolvedValue({ kind: "success", }); + const connect = vi.fn().mockResolvedValue(undefined); const client = { + connect, + disconnect: vi.fn(), authenticate: vi.fn(), beginInteractiveAuthorization: vi.fn(), completeOAuthFlow: vi.fn(), @@ -272,6 +285,7 @@ describe("cliOAuth", () => { const result = await withCliAuthRecoveryRetry( client, + OAUTH_HTTP_CONFIG, new MutableRedirectUrlProvider(), { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, { enterpriseManaged: true }, @@ -280,6 +294,7 @@ describe("cliOAuth", () => { ); expect(result).toBe("ok"); + expect(connect).toHaveBeenCalledOnce(); expect(fn).toHaveBeenCalledTimes(2); }); @@ -355,10 +370,7 @@ describe("cliOAuth", () => { }); describe("connectInspectorWithOAuth recovery branch", () => { - const oauthServerConfig = { - type: "streamable-http", - url: "https://as.example/mcp", - } as MCPServerConfig; + const oauthServerConfig = OAUTH_HTTP_CONFIG; it("resumes without re-auth when storage already satisfies the challenge", async () => { const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); @@ -583,6 +595,7 @@ describe("cliOAuth", () => { completeOAuthFlow: vi.fn(), checkAuthChallengeSatisfied, }, + OAUTH_HTTP_CONFIG, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, undefined, @@ -619,6 +632,7 @@ describe("cliOAuth", () => { completeOAuthFlow: vi.fn(), checkAuthChallengeSatisfied, }, + OAUTH_HTTP_CONFIG, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, undefined, @@ -637,24 +651,36 @@ describe("cliOAuth", () => { const runSpy = vi .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") .mockResolvedValue({ kind: "success" }); + const connect = vi.fn().mockResolvedValue(undefined); const disconnect = vi.fn().mockResolvedValue(undefined); + const callOrder: string[] = []; + connect.mockImplementation(async () => { + callOrder.push("connect"); + }); const fn = vi .fn() - .mockRejectedValueOnce(new Error("RPC failed (401)")) - .mockResolvedValueOnce("ok"); + .mockImplementationOnce(async () => { + callOrder.push("fn1"); + throw new Error("RPC failed (401)"); + }) + .mockImplementationOnce(async () => { + callOrder.push("fn2"); + return "ok"; + }); const stderrSpy = vi .spyOn(process.stderr, "write") .mockImplementation(() => true); const result = await withCliAuthRecoveryRetry( { - connect: vi.fn(), + connect, disconnect, authenticate: vi.fn(), beginInteractiveAuthorization: vi.fn(), completeOAuthFlow: vi.fn(), checkAuthChallengeSatisfied: vi.fn(), }, + OAUTH_HTTP_CONFIG, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, undefined, @@ -664,7 +690,8 @@ describe("cliOAuth", () => { expect(result).toBe("ok"); expect(disconnect).toHaveBeenCalledOnce(); expect(runSpy).toHaveBeenCalledOnce(); - expect(fn).toHaveBeenCalledTimes(2); + expect(connect).toHaveBeenCalledOnce(); + expect(callOrder).toEqual(["fn1", "connect", "fn2"]); expect(stderrSpy).toHaveBeenCalledWith( "Authorization complete. Retrying…\n", ); @@ -684,6 +711,7 @@ describe("cliOAuth", () => { completeOAuthFlow: vi.fn(), checkAuthChallengeSatisfied: vi.fn(), }, + OAUTH_HTTP_CONFIG, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, undefined, @@ -694,4 +722,83 @@ describe("cliOAuth", () => { ).rejects.toMatchObject({ exitCode: 3 }); expect(runSpy).not.toHaveBeenCalled(); }); + + it("withCliAuthRecoveryRetry rethrows unauthorized errors for non-OAuth configs", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const err = new Error("proxied backend failed with status (401)"); + const fn = vi.fn().mockRejectedValue(err); + + await expect( + withCliAuthRecoveryRetry( + { + connect: vi.fn(), + disconnect: vi.fn(), + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }, + STDIO_CONFIG, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + fn, + ), + ).rejects.toBe(err); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("withCliAuthRecoveryRetry rethrows unrelated errors unchanged", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const err = new Error("something else went wrong"); + const fn = vi.fn().mockRejectedValue(err); + + await expect( + withCliAuthRecoveryRetry( + { + connect: vi.fn(), + disconnect: vi.fn(), + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }, + OAUTH_HTTP_CONFIG, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + fn, + ), + ).rejects.toBe(err); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("withCliAuthRecoveryRetry stored-auth-only uses a fallback message for non-Error 401s", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const fn = vi.fn().mockRejectedValue({ status: 401 }); + + await expect( + withCliAuthRecoveryRetry( + { + connect: vi.fn(), + disconnect: vi.fn(), + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }, + OAUTH_HTTP_CONFIG, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + fn, + undefined, + { storedAuthOnly: true }, + ), + ).rejects.toMatchObject({ + exitCode: 3, + message: expect.stringMatching(/--stored-auth-only/), + }); + expect(runSpy).not.toHaveBeenCalled(); + }); }); diff --git a/clients/cli/__tests__/helpers/cli-runner.ts b/clients/cli/__tests__/helpers/cli-runner.ts index 76abd5326..d5c8abe66 100644 --- a/clients/cli/__tests__/helpers/cli-runner.ts +++ b/clients/cli/__tests__/helpers/cli-runner.ts @@ -1,12 +1,3 @@ -import { vi } from "vitest"; - -// Structural guard: any in-process `runCli` path that arms interactive OAuth -// must not shell out to a real browser. Do not rely on `VITEST` alone — -// `cli-oauth-navigation` also honors `MCP_AUTO_OPEN_ENABLED=true`. -vi.mock("../../src/open-url.js", () => ({ - openUrl: vi.fn().mockResolvedValue(undefined), -})); - import { runCli as invokeCli } from "../../src/cli.js"; import { formatErrorOutput } from "../../src/error-handler.js"; diff --git a/clients/cli/__tests__/helpers/mock-open-url.ts b/clients/cli/__tests__/helpers/mock-open-url.ts new file mode 100644 index 000000000..0b8daebb3 --- /dev/null +++ b/clients/cli/__tests__/helpers/mock-open-url.ts @@ -0,0 +1,10 @@ +import { vi } from "vitest"; + +/** + * Structural guard for the whole CLI suite: armed interactive OAuth must not + * shell out to a real browser. Registered via vitest `setupFiles` so it does + * not depend on import order in individual test files. + */ +vi.mock("../../src/open-url.js", () => ({ + openUrl: vi.fn().mockResolvedValue(undefined), +})); diff --git a/clients/cli/__tests__/oauth-interactive.test.ts b/clients/cli/__tests__/oauth-interactive.test.ts index ec5437e6e..51af4052e 100644 --- a/clients/cli/__tests__/oauth-interactive.test.ts +++ b/clients/cli/__tests__/oauth-interactive.test.ts @@ -225,6 +225,7 @@ describe("CLI interactive OAuth (integration)", () => { try { const result = await withCliAuthRecoveryRetry( client, + { type: "streamable-http", url: `${serverUrl}/mcp` }, redirectUrlProvider, callbackUrlConfig, {}, diff --git a/clients/cli/__tests__/open-url.test.ts b/clients/cli/__tests__/open-url.test.ts index 304dd2570..269c5596b 100644 --- a/clients/cli/__tests__/open-url.test.ts +++ b/clients/cli/__tests__/open-url.test.ts @@ -6,6 +6,9 @@ vi.mock("open", () => ({ default: (...args: unknown[]) => openMock(...args), })); +// Suite-wide setup mocks open-url; this file exercises the real wrapper. +vi.unmock("../src/open-url.js"); + describe("openUrl", () => { beforeEach(() => { openMock.mockClear(); diff --git a/clients/cli/__tests__/programmatic-ergonomics.test.ts b/clients/cli/__tests__/programmatic-ergonomics.test.ts index 7398d3456..1c763ce10 100644 --- a/clients/cli/__tests__/programmatic-ergonomics.test.ts +++ b/clients/cli/__tests__/programmatic-ergonomics.test.ts @@ -170,6 +170,12 @@ describe("CLI --relogin flag conflicts", () => { expect(result.stderr).toMatch(/servers\/list or servers\/show/); }); + it("rejects --relogin with --list-stored-auth", async () => { + const result = await runCli(["--relogin", "--list-stored-auth"]); + expectCliFailure(result); + expect(result.stderr).toMatch(/--list-stored-auth or --print-handoff/); + }); + it("accepts --relogin and clears stored auth before connect", async () => { // Unreachable port: proves --relogin runs (clears store) then fails connect. const result = await runCli([ diff --git a/clients/cli/src/cli-oauth-navigation.ts b/clients/cli/src/cli-oauth-navigation.ts index 699536985..e851061cd 100644 --- a/clients/cli/src/cli-oauth-navigation.ts +++ b/clients/cli/src/cli-oauth-navigation.ts @@ -39,8 +39,8 @@ export type CliOAuthNavigationOptions = { */ autoOpenEnabled?: boolean; /** - * When true, open even if stderr is not a TTY (matches web's explicit - * `MCP_AUTO_OPEN_ENABLED=true`). When omitted, inferred from the env flag. + * When true, open even if stderr is not a TTY. Independent of + * {@link autoOpenEnabled}. Defaults to {@link isCliAutoOpenForced}. */ forceAutoOpen?: boolean; }; @@ -105,10 +105,7 @@ export function createCliOAuthNavigation( : resolveCliAutoOpenEnabled(); if (!envAllows) return; - const forced = - options.forceAutoOpen !== undefined - ? options.forceAutoOpen - : options.autoOpenEnabled === undefined && isCliAutoOpenForced(); + const forced = options.forceAutoOpen ?? isCliAutoOpenForced(); if (!forced && !tty) return; try { diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 0bd58064c..b746f0303 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -164,6 +164,7 @@ async function callMethod( const outcome = await withCliAuthRecoveryRetry( inspectorClient, + serverConfig, redirectUrlProvider, callbackUrlConfig, serverSettings, @@ -742,6 +743,11 @@ async function parseArgs(argv?: string[]): Promise { "--relogin cannot be combined with --use-stored-auth or --wait-for-auth", ); } + if (options.listStoredAuth || options.printHandoff) { + throw new Error( + "--relogin cannot be combined with --list-stored-auth or --print-handoff", + ); + } if ( options.method === "servers/list" || options.method === "servers/show" diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index e90e6264d..62c2b26d8 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -217,13 +217,15 @@ export async function connectInspectorWithOAuth( } /** - * Run `fn` once; on auth recovery errors, complete interactive OAuth and retry - * `fn` a single time (no further recovery attempts). Mirrors + * Run `fn` once; on auth recovery errors, complete interactive OAuth, + * reconnect, and retry `fn` a single time. Mirrors * {@link connectInspectorWithOAuth}: handles both - * {@link AuthRecoveryRequiredError} and plain unauthorized errors. + * {@link AuthRecoveryRequiredError} and plain unauthorized errors, and skips + * OAuth machinery for non-OAuth-capable server configs. */ export async function withCliAuthRecoveryRetry( inspectorClient: CliOAuthClient, + serverConfig: MCPServerConfig, redirectUrlProvider: MutableRedirectUrlProvider, callbackUrlConfig: RunnerOAuthCallbackConfig, serverSettings: InspectorServerSettings | undefined, @@ -234,6 +236,10 @@ export async function withCliAuthRecoveryRetry( try { return await fn(); } catch (err) { + if (!isOAuthCapableServerConfig(serverConfig)) { + throw err; + } + if (err instanceof AuthRecoveryRequiredError) { // Satisfied-check lives in handleCliAuthRecoveryRequired for the // interactive path; under --stored-auth-only check once here then bail. @@ -257,6 +263,8 @@ export async function withCliAuthRecoveryRetry( confirmStepUp, options?.autoOpenControl, ); + // completeOAuthFlow only reconnects under directAuthRecovery (off in CLI). + await inspectorClient.connect(); process.stderr.write("Authorization complete. Retrying…\n"); return await fn(); } @@ -276,6 +284,9 @@ export async function withCliAuthRecoveryRetry( callbackUrlConfig, { autoOpenControl: options?.autoOpenControl }, ); + // Mirror connectInspectorWithOAuth: reconnect before retrying the RPC. + // connect() is a no-op when already connected. + await inspectorClient.connect(); process.stderr.write("Authorization complete. Retrying…\n"); return await fn(); } diff --git a/clients/cli/vitest.config.ts b/clients/cli/vitest.config.ts index 4480e1bd3..77e29dedb 100644 --- a/clients/cli/vitest.config.ts +++ b/clients/cli/vitest.config.ts @@ -12,6 +12,7 @@ export default defineConfig({ globals: false, environment: 'node', include: ['__tests__/**/*.test.ts'], + setupFiles: ['__tests__/helpers/mock-open-url.ts'], testTimeout: 15000, // The in-process runner (__tests__/helpers/cli-runner.ts) patches // process.std{out,err}.write to capture CLI output. Test files run in From 243d514457c810ee7d47fb092b451e23c19bf00c Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 21:37:13 -0700 Subject: [PATCH 08/13] fix(cli): fail fast on non-TTY interactive OAuth and round-6 nits Skip interactive OAuth without a TTY unless MCP_AUTO_OPEN_ENABLED=true; document --stored-auth-only for CI; redact settings.metadata; reject --relogin for stdio; clarify AuthRecoveryRequired reconnect comments. Co-authored-by: Cursor --- clients/cli/README.md | 10 +- .../__tests__/cli-oauth-navigation.test.ts | 2 + clients/cli/__tests__/cliOAuth.test.ts | 103 ++++++++++++++++++ .../cli/__tests__/oauth-interactive.test.ts | 5 + .../__tests__/programmatic-ergonomics.test.ts | 13 +++ clients/cli/__tests__/servers-list.test.ts | 9 +- clients/cli/src/cli.ts | 11 +- clients/cli/src/cliOAuth.ts | 44 +++++++- clients/cli/src/handlers/servers-list.ts | 4 + 9 files changed, 190 insertions(+), 11 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 7b9d407ca..e9e92d522 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -111,10 +111,10 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--connect-timeout ` | Connection timeout in ms. Defaults to `15000` for ad-hoc `--server-url`/target runs (so a black-holed host fails fast) and to the file-level timeout for `--catalog`/`--config` runs. `0` disables the timeout. | | `--app-info` | Probe a tool's MCP App UI metadata without invoking it. With `--method tools/call --tool-name `: prints one JSON line (`hasApp`, `resourceUri`, `csp`, `permissions`, `domain`, …) and exits `0` if the tool has an app or `2` (`no_app`) if not. With `--method tools/list`: emits NDJSON — one app-info line per tool over a single connection. | | `--format ` | Output format. `text` (default) pretty-prints the result. `json` emits a single JSON object on stdout (`{ "result": … }`, plus `{ "appInfo": … }` as a sibling key for App tools) with no banners, so the whole output pipes cleanly into `jq`. | -| `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. No-op for stdio (no URL-keyed store entry). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth`. | -| `--stored-auth-only` | Never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail with `auth_required`. | +| `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. Requires an HTTP/SSE URL (rejected for stdio). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth` / catalog short-circuits. | +| `--stored-auth-only` | **CI / non-interactive safe:** never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail immediately with `auth_required`. Prefer this over a bare pipe/CI run that would otherwise attempt interactive login. | -`servers/show` redacts secret-bearing fields (`env` values, sensitive headers in `requestInit` / `eventSourceInit` / settings, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. +`servers/show` redacts secret-bearing fields (`env` values, sensitive headers / `settings.metadata` keys, `requestInit` / `eventSourceInit` headers, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. #### App probing (`--app-info`) and machine-readable output (`--format json`) @@ -149,9 +149,11 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 **CLI (`mcp-inspector --cli`):** on connect **401** or mid-session interactive auth (re-login / step-up), it: 1. Starts the callback listener on `--callback-url` (or `MCP_OAUTH_CALLBACK_URL`) -2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** when allowed. Default: open on a TTY, plain URL only when non-TTY / CI. `MCP_AUTO_OPEN_ENABLED=false` never opens; `=true` forces open even on a non-TTY (same as the web launcher). `--stored-auth-only` never prints or opens. +2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** when allowed. Default: open on a TTY, plain URL only when non-TTY. `MCP_AUTO_OPEN_ENABLED=false` never opens; `=true` forces open even on a non-TTY (same as the web launcher). 3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC +Interactive OAuth (connect-time or mid-RPC) **requires a TTY**, or `MCP_AUTO_OPEN_ENABLED=true`. On a non-TTY without that env (typical CI / piped stderr), the CLI fails fast with `auth_required` instead of waiting up to 15 minutes on the loopback callback. Use **`--stored-auth-only`** for non-interactive runs that should only consume the shared store. + **Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues; **N** exits with an error. EMA step-up re-mints silently (no prompt). **Shared OAuth storage:** the CLI **reuses** tokens from `~/.mcp-inspector/storage/oauth.json` when they already exist (same file as other Inspector clients). That is passive file sharing, not launching another app. diff --git a/clients/cli/__tests__/cli-oauth-navigation.test.ts b/clients/cli/__tests__/cli-oauth-navigation.test.ts index 33008be9c..903491527 100644 --- a/clients/cli/__tests__/cli-oauth-navigation.test.ts +++ b/clients/cli/__tests__/cli-oauth-navigation.test.ts @@ -6,6 +6,8 @@ import { } from "../src/cli-oauth-navigation.js"; import { openUrl } from "../src/open-url.js"; +// Local mock (in addition to suite-wide setupFiles) so this file owns a +// `vi.mocked(openUrl)` handle and does not depend on the global mock shape. vi.mock("../src/open-url.js", () => ({ openUrl: vi.fn().mockResolvedValue(undefined), })); diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index ca78b6cc0..683faaf1d 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -7,6 +7,7 @@ import { handleCliAuthRecoveryRequired, isStandardOAuthStepUp, runCliInteractiveOAuth, + assertInteractiveOAuthAllowed, withCliAuthRecoveryRetry, } from "../src/cliOAuth.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; @@ -41,6 +42,9 @@ const STDIO_CONFIG = { command: "x", } as MCPServerConfig; +/** Unit tests run without a TTY; opt into interactive OAuth explicitly. */ +const INTERACTIVE = { isTTY: true as const }; + describe("cliOAuth", () => { afterEach(() => { vi.restoreAllMocks(); @@ -172,6 +176,8 @@ describe("cliOAuth", () => { { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, {}, async () => false, + undefined, + true, ), ).rejects.toThrow("Step-up authorization declined."); @@ -202,6 +208,8 @@ describe("cliOAuth", () => { { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, {}, async () => true, + undefined, + true, ); expect(runSpy).toHaveBeenCalledWith( @@ -291,6 +299,7 @@ describe("cliOAuth", () => { { enterpriseManaged: true }, fn, async () => true, + INTERACTIVE, ); expect(result).toBe("ok"); @@ -326,6 +335,9 @@ describe("cliOAuth", () => { new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, {}, + undefined, + undefined, + true, ); expect(mockQuestion).toHaveBeenCalled(); @@ -345,6 +357,9 @@ describe("cliOAuth", () => { new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, {}, + undefined, + undefined, + true, ); expect(runSpy).toHaveBeenCalled(); @@ -361,6 +376,9 @@ describe("cliOAuth", () => { new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, {}, + undefined, + undefined, + true, ), ).rejects.toThrow("Step-up authorization declined."); @@ -424,6 +442,8 @@ describe("cliOAuth", () => { oauthServerConfig, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, + undefined, + INTERACTIVE, ); expect(connect).toHaveBeenCalledTimes(2); @@ -450,6 +470,8 @@ describe("cliOAuth", () => { oauthServerConfig, new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, + undefined, + INTERACTIVE, ); expect(client.disconnect).toHaveBeenCalled(); @@ -685,6 +707,8 @@ describe("cliOAuth", () => { CALLBACK_URL_CONFIG, undefined, fn, + undefined, + INTERACTIVE, ); expect(result).toBe("ok"); @@ -801,4 +825,83 @@ describe("cliOAuth", () => { }); expect(runSpy).not.toHaveBeenCalled(); }); + + it("assertInteractiveOAuthAllowed fails fast on a non-TTY", () => { + expect(() => assertInteractiveOAuthAllowed({ isTTY: false })).toThrow( + /Interactive OAuth requires a TTY/, + ); + }); + + it("withCliAuthRecoveryRetry refuses interactive OAuth on a non-TTY", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + const fn = vi.fn().mockRejectedValue(new Error("RPC failed (401)")); + + await expect( + withCliAuthRecoveryRetry( + { + connect: vi.fn(), + disconnect: vi.fn(), + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }, + OAUTH_HTTP_CONFIG, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + fn, + undefined, + { isTTY: false }, + ), + ).rejects.toMatchObject({ + exitCode: 3, + message: expect.stringMatching(/--stored-auth-only/), + }); + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("withCliAuthRecoveryRetry allows non-TTY interactive OAuth when MCP_AUTO_OPEN_ENABLED=true", async () => { + const prev = process.env.MCP_AUTO_OPEN_ENABLED; + process.env.MCP_AUTO_OPEN_ENABLED = "true"; + try { + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + const connect = vi.fn().mockResolvedValue(undefined); + const fn = vi + .fn() + .mockRejectedValueOnce(new Error("RPC failed (401)")) + .mockResolvedValueOnce("ok"); + vi.spyOn(process.stderr, "write").mockImplementation(() => true); + + const result = await withCliAuthRecoveryRetry( + { + connect, + disconnect: vi.fn().mockResolvedValue(undefined), + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }, + OAUTH_HTTP_CONFIG, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + fn, + undefined, + { isTTY: false }, + ); + + expect(result).toBe("ok"); + expect(runSpy).toHaveBeenCalledOnce(); + expect(connect).toHaveBeenCalledOnce(); + } finally { + if (prev === undefined) { + delete process.env.MCP_AUTO_OPEN_ENABLED; + } else { + process.env.MCP_AUTO_OPEN_ENABLED = prev; + } + } + }); }); diff --git a/clients/cli/__tests__/oauth-interactive.test.ts b/clients/cli/__tests__/oauth-interactive.test.ts index 51af4052e..07f925bdb 100644 --- a/clients/cli/__tests__/oauth-interactive.test.ts +++ b/clients/cli/__tests__/oauth-interactive.test.ts @@ -147,6 +147,8 @@ describe("CLI interactive OAuth (integration)", () => { { type: "streamable-http", url: `${serverUrl}/mcp` }, redirectUrlProvider, callbackUrlConfig, + undefined, + { isTTY: true }, ); const tools = await client.listTools(); @@ -206,6 +208,8 @@ describe("CLI interactive OAuth (integration)", () => { { type: "streamable-http", url: `${serverUrl}/mcp` }, redirectUrlProvider, callbackUrlConfig, + undefined, + { isTTY: true }, ); const tools = await client.listTools(); @@ -235,6 +239,7 @@ describe("CLI interactive OAuth (integration)", () => { units: "C", }), async () => true, + { isTTY: true }, ); expect(stepUpPrompted).toBe(true); diff --git a/clients/cli/__tests__/programmatic-ergonomics.test.ts b/clients/cli/__tests__/programmatic-ergonomics.test.ts index 1c763ce10..1c6fdee74 100644 --- a/clients/cli/__tests__/programmatic-ergonomics.test.ts +++ b/clients/cli/__tests__/programmatic-ergonomics.test.ts @@ -176,6 +176,19 @@ describe("CLI --relogin flag conflicts", () => { expect(result.stderr).toMatch(/--list-stored-auth or --print-handoff/); }); + it("rejects --relogin for stdio servers", async () => { + const { command, args } = getTestMcpServerCommand(); + const result = await runCli([ + command, + ...args, + "--relogin", + "--method", + "tools/list", + ]); + expectCliFailure(result); + expect(result.stderr).toMatch(/HTTP\/SSE server URL/); + }); + it("accepts --relogin and clears stored auth before connect", async () => { // Unreachable port: proves --relogin runs (clears store) then fails connect. const result = await runCli([ diff --git a/clients/cli/__tests__/servers-list.test.ts b/clients/cli/__tests__/servers-list.test.ts index f6893cb56..8ac7b599c 100644 --- a/clients/cli/__tests__/servers-list.test.ts +++ b/clients/cli/__tests__/servers-list.test.ts @@ -219,7 +219,10 @@ describe("showServerEntry / servers/show", () => { { key: "Authorization", value: "Bearer x" }, { key: "X-Custom", value: "ok" }, ], - metadata: [], + metadata: [ + { key: "Authorization", value: "Bearer meta" }, + { key: "X-Custom", value: "ok" }, + ], env: [ { key: "TOKEN", value: "secret" }, { key: "", value: "still-secret" }, @@ -239,6 +242,10 @@ describe("showServerEntry / servers/show", () => { { key: "Authorization", value: "[redacted]" }, { key: "X-Custom", value: "ok" }, ]); + expect(sanitized.metadata).toEqual([ + { key: "Authorization", value: "[redacted]" }, + { key: "X-Custom", value: "ok" }, + ]); expect(sanitized.env).toEqual([ { key: "TOKEN", value: "[redacted]" }, { key: "", value: "[redacted]" }, diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index b746f0303..75274414f 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -99,7 +99,12 @@ async function callMethod( ): Promise { // Clear after parse-time validation so a bad flag combo never deletes store // entries. Deletes the shared URL-keyed OAuth entry (not "ignore for this run"). - if (relogin && "url" in serverConfig && serverConfig.url) { + if (relogin) { + if (!("url" in serverConfig && serverConfig.url)) { + throw new Error( + "--relogin requires an HTTP/SSE server URL (no OAuth store entry for stdio)", + ); + } await clearStoredAuthForRelogin(serverConfig.url); } @@ -669,11 +674,11 @@ async function parseArgs(argv?: string[]): Promise { ) .option( "--stored-auth-only", - "Never start interactive OAuth; use the shared store if present, otherwise fail with auth_required. No-op when the server does not require auth.", + "Never start interactive OAuth; use the shared store if present, otherwise fail with auth_required. Preferred for CI/non-interactive runs. No-op when the server does not require auth.", ) .option( "--relogin", - "Delete stored OAuth for this server URL from the shared store before connect (HTTP/SSE URL keys only); interactive login runs only if the server requires auth. No-op for stdio / servers with no stored entry", + "Delete stored OAuth for this server URL from the shared store before connect (HTTP/SSE URL keys only); interactive login runs only if the server requires auth. Rejected for stdio (no URL-keyed store entry)", ) .option( "--wait-for-auth ", diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index 62c2b26d8..c572fc301 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -18,7 +18,10 @@ import { isOAuthCapableServerConfig } from "@inspector/core/client/runner.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; import { createInterface } from "node:readline/promises"; import { CliExitCodeError, EXIT_CODES } from "./error-handler.js"; -import type { CliOAuthAutoOpenControl } from "./cli-oauth-navigation.js"; +import { + isCliAutoOpenForced, + type CliOAuthAutoOpenControl, +} from "./cli-oauth-navigation.js"; /** Client surface needed for connect + mid-RPC OAuth recovery. */ export type CliOAuthClient = RunnerInteractiveOAuthClient & { @@ -37,6 +40,11 @@ export type CliOAuthConnectOptions = { * (callback server listening). See {@link createCliOAuthNavigation}. */ autoOpenControl?: CliOAuthAutoOpenControl; + /** + * Override stderr TTY detection for interactive-OAuth gating (tests). + * Defaults to `process.stderr.isTTY`. + */ + isTTY?: boolean; }; function storedAuthOnlyFailure(message: string): never { @@ -45,6 +53,24 @@ function storedAuthOnlyFailure(message: string): never { }); } +/** + * Interactive OAuth waits up to 15 minutes on the loopback callback. On a + * non-TTY (CI / piped stderr) nobody will complete that flow unless the caller + * explicitly opted into browser open via `MCP_AUTO_OPEN_ENABLED=true`. + */ +export function assertInteractiveOAuthAllowed( + options?: Pick, +): void { + const tty = + options?.isTTY !== undefined + ? options.isTTY + : process.stderr.isTTY === true; + if (tty || isCliAutoOpenForced()) return; + storedAuthOnlyFailure( + "Interactive OAuth requires a TTY (or MCP_AUTO_OPEN_ENABLED=true). For CI/non-interactive runs use --stored-auth-only.", + ); +} + /** Standard-OAuth step-up (not EMA silent re-mint). */ export function isStandardOAuthStepUp( challenge: AuthChallenge, @@ -125,11 +151,13 @@ export async function handleCliAuthRecoveryRequired( serverSettings?: InspectorServerSettings, confirmStepUp: () => Promise = confirmStepUpFromStdin, autoOpenControl?: CliOAuthAutoOpenControl, + isTTY?: boolean, ): Promise { if (isStandardOAuthStepUp(error.authChallenge, serverSettings)) { if (await client.checkAuthChallengeSatisfied(error.authChallenge)) { return; } + assertInteractiveOAuthAllowed({ isTTY }); const proceed = await promptStepUpConfirm( error.authChallenge, confirmStepUp, @@ -139,6 +167,8 @@ export async function handleCliAuthRecoveryRequired( } } else if (await client.checkAuthChallengeSatisfied(error.authChallenge)) { return; + } else { + assertInteractiveOAuthAllowed({ isTTY }); } await runCliInteractiveOAuth(client, redirectUrlProvider, callbackUrlConfig, { @@ -188,7 +218,11 @@ export async function connectInspectorWithOAuth( serverSettings, confirmStepUpFromStdin, options?.autoOpenControl, + options?.isTTY, ); + // Belt-and-braces: this branch never disconnects today, so connect() is + // usually a no-op (already connected). Fresh tokens are picked up from + // storage per request; keep the call if handle later gains a disconnect. await inspectorClient.connect(); return; } @@ -201,6 +235,7 @@ export async function connectInspectorWithOAuth( : "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", ); } + assertInteractiveOAuthAllowed(options); await inspectorClient.disconnect().catch(() => {}); await runCliInteractiveOAuth( inspectorClient, @@ -262,8 +297,10 @@ export async function withCliAuthRecoveryRetry( serverSettings, confirmStepUp, options?.autoOpenControl, + options?.isTTY, ); - // completeOAuthFlow only reconnects under directAuthRecovery (off in CLI). + // Belt-and-braces: this branch never disconnects today, so connect() is + // usually a no-op (already connected). See connectInspectorWithOAuth. await inspectorClient.connect(); process.stderr.write("Authorization complete. Retrying…\n"); return await fn(); @@ -277,6 +314,7 @@ export async function withCliAuthRecoveryRetry( : "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", ); } + assertInteractiveOAuthAllowed(options); await inspectorClient.disconnect().catch(() => {}); await runCliInteractiveOAuth( inspectorClient, @@ -284,7 +322,7 @@ export async function withCliAuthRecoveryRetry( callbackUrlConfig, { autoOpenControl: options?.autoOpenControl }, ); - // Mirror connectInspectorWithOAuth: reconnect before retrying the RPC. + // Load-bearing: disconnect() above closed the session. // connect() is a no-op when already connected. await inspectorClient.connect(); process.stderr.write("Authorization complete. Retrying…\n"); diff --git a/clients/cli/src/handlers/servers-list.ts b/clients/cli/src/handlers/servers-list.ts index c0a1c4f67..78bd896fb 100644 --- a/clients/cli/src/handlers/servers-list.ts +++ b/clients/cli/src/handlers/servers-list.ts @@ -157,6 +157,10 @@ export function sanitizeServerSettings( key: h.key, value: isSensitiveHeader(h.key) ? REDACTED : h.value, })), + metadata: (settings.metadata ?? []).map((m) => ({ + key: m.key, + value: isSensitiveHeader(m.key) ? REDACTED : m.value, + })), env: (settings.env ?? []).map((e) => ({ key: e.key, value: REDACTED, From f01188a3d4060dfa5293217a10b2535d8c8d6f77 Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 21:53:42 -0700 Subject: [PATCH 09/13] fix(cli): admit interactive OAuth when stdin is a TTY (round-7) Gate on stdin||stderr so `2>&1 | tee` still works; fold confirm/auto-open/ TTY into CliOAuthConnectOptions; rename authRequiredFailure; document MCP_AUTO_OPEN_ENABLED=true as non-TTY admit. Co-authored-by: Cursor --- clients/cli/README.md | 4 +- clients/cli/__tests__/cliOAuth.test.ts | 134 ++++++++++++++---- .../cli/__tests__/oauth-interactive.test.ts | 3 +- clients/cli/src/cli.ts | 1 - clients/cli/src/cliOAuth.ts | 50 +++---- 5 files changed, 135 insertions(+), 57 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index e9e92d522..fe199acbe 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -152,7 +152,7 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** when allowed. Default: open on a TTY, plain URL only when non-TTY. `MCP_AUTO_OPEN_ENABLED=false` never opens; `=true` forces open even on a non-TTY (same as the web launcher). 3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC -Interactive OAuth (connect-time or mid-RPC) **requires a TTY**, or `MCP_AUTO_OPEN_ENABLED=true`. On a non-TTY without that env (typical CI / piped stderr), the CLI fails fast with `auth_required` instead of waiting up to 15 minutes on the loopback callback. Use **`--stored-auth-only`** for non-interactive runs that should only consume the shared store. +Interactive OAuth (connect-time or mid-RPC) requires a TTY on **stdin or stderr** (so `2>&1 | tee` still works — stdin stays a TTY), or `MCP_AUTO_OPEN_ENABLED=true`. When neither stdin nor stderr is a TTY and that env is unset (typical CI), the CLI fails fast with `auth_required` instead of waiting up to 15 minutes on the loopback callback. Use **`--stored-auth-only`** for non-interactive runs that should only consume the shared store. **Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues; **N** exits with an error. EMA step-up re-mints silently (no prompt). @@ -184,7 +184,7 @@ Register `http://127.0.0.1:6276/oauth/callback` on static or enterprise IdPs tha | `--client-secret ` | — | OAuth client secret; overrides `client.json`. | | `--client-metadata-url ` | — | CIMD metadata URL; overrides `client.json`. | | `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | Redirect URI sent to the authorization server (default: `http://127.0.0.1:6276/oauth/callback`). | -| — | `MCP_AUTO_OPEN_ENABLED` | Browser auto-open for interactive OAuth: `true` (always open, including non-TTY — same as the web launcher), `false` (never), unset (open on a TTY unless `VITEST` is set). | +| — | `MCP_AUTO_OPEN_ENABLED` | Browser auto-open **and** non-TTY interactive-OAuth admit: `true` (allow interactive OAuth without a TTY **and** force-open the browser — same as the web launcher), `false` (never open), unset (open on a TTY unless `VITEST` is set). For CI that must not hang, prefer `--stored-auth-only`. | **Example** — list tools on an OAuth-protected server using stored tokens and CIMD from the command line: diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index 683faaf1d..0d6c3765c 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -175,9 +175,7 @@ describe("cliOAuth", () => { new MutableRedirectUrlProvider(), { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, {}, - async () => false, - undefined, - true, + { confirmStepUp: async () => false, isTTY: true }, ), ).rejects.toThrow("Step-up authorization declined."); @@ -207,9 +205,7 @@ describe("cliOAuth", () => { new MutableRedirectUrlProvider(), { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, {}, - async () => true, - undefined, - true, + { confirmStepUp: async () => true, isTTY: true }, ); expect(runSpy).toHaveBeenCalledWith( @@ -298,8 +294,7 @@ describe("cliOAuth", () => { { hostname: "127.0.0.1", port: 6276, pathname: "/oauth/callback" }, { enterpriseManaged: true }, fn, - async () => true, - INTERACTIVE, + { confirmStepUp: async () => true, isTTY: true }, ); expect(result).toBe("ok"); @@ -327,17 +322,15 @@ describe("cliOAuth", () => { .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") .mockResolvedValue({ kind: "success" }); - // Omitting the confirmStepUp argument exercises the default - // confirmStepUpFromStdin, which reads from the mocked readline interface. + // Omitting confirmStepUp exercises the default confirmStepUpFromStdin, + // which reads from the mocked readline interface. await handleCliAuthRecoveryRequired( clientNeedingStepUp(), standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, {}, - undefined, - undefined, - true, + INTERACTIVE, ); expect(mockQuestion).toHaveBeenCalled(); @@ -357,9 +350,7 @@ describe("cliOAuth", () => { new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, {}, - undefined, - undefined, - true, + INTERACTIVE, ); expect(runSpy).toHaveBeenCalled(); @@ -376,9 +367,7 @@ describe("cliOAuth", () => { new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, {}, - undefined, - undefined, - true, + INTERACTIVE, ), ).rejects.toThrow("Step-up authorization declined."); @@ -622,7 +611,6 @@ describe("cliOAuth", () => { CALLBACK_URL_CONFIG, undefined, fn, - undefined, { storedAuthOnly: true }, ), ).rejects.toMatchObject({ exitCode: 3 }); @@ -659,7 +647,6 @@ describe("cliOAuth", () => { CALLBACK_URL_CONFIG, undefined, fn, - undefined, { storedAuthOnly: true }, ); @@ -707,7 +694,6 @@ describe("cliOAuth", () => { CALLBACK_URL_CONFIG, undefined, fn, - undefined, INTERACTIVE, ); @@ -740,7 +726,6 @@ describe("cliOAuth", () => { CALLBACK_URL_CONFIG, undefined, fn, - undefined, { storedAuthOnly: true }, ), ).rejects.toMatchObject({ exitCode: 3 }); @@ -816,7 +801,6 @@ describe("cliOAuth", () => { CALLBACK_URL_CONFIG, undefined, fn, - undefined, { storedAuthOnly: true }, ), ).rejects.toMatchObject({ @@ -826,10 +810,106 @@ describe("cliOAuth", () => { expect(runSpy).not.toHaveBeenCalled(); }); - it("assertInteractiveOAuthAllowed fails fast on a non-TTY", () => { + it("assertInteractiveOAuthAllowed fails fast when neither stream is a TTY", () => { expect(() => assertInteractiveOAuthAllowed({ isTTY: false })).toThrow( - /Interactive OAuth requires a TTY/, + /Interactive OAuth requires a TTY on stdin or stderr/, + ); + }); + + it("assertInteractiveOAuthAllowed admits when isTTY override is true (stdin||stderr path)", () => { + expect(() => assertInteractiveOAuthAllowed({ isTTY: true })).not.toThrow(); + }); + + it("assertInteractiveOAuthAllowed admits when stdin is a TTY even if stderr is not", () => { + const stdinDesc = Object.getOwnPropertyDescriptor(process.stdin, "isTTY"); + const stderrDesc = Object.getOwnPropertyDescriptor(process.stderr, "isTTY"); + Object.defineProperty(process.stdin, "isTTY", { + configurable: true, + get: () => true, + }); + Object.defineProperty(process.stderr, "isTTY", { + configurable: true, + get: () => false, + }); + try { + expect(() => assertInteractiveOAuthAllowed()).not.toThrow(); + } finally { + if (stdinDesc) Object.defineProperty(process.stdin, "isTTY", stdinDesc); + else delete (process.stdin as { isTTY?: boolean }).isTTY; + if (stderrDesc) + Object.defineProperty(process.stderr, "isTTY", stderrDesc); + else delete (process.stderr as { isTTY?: boolean }).isTTY; + } + }); + + it("assertInteractiveOAuthAllowed fails when both stdin and stderr are non-TTY", () => { + const stdinDesc = Object.getOwnPropertyDescriptor(process.stdin, "isTTY"); + const stderrDesc = Object.getOwnPropertyDescriptor(process.stderr, "isTTY"); + Object.defineProperty(process.stdin, "isTTY", { + configurable: true, + get: () => false, + }); + Object.defineProperty(process.stderr, "isTTY", { + configurable: true, + get: () => false, + }); + try { + expect(() => assertInteractiveOAuthAllowed()).toThrow( + /Interactive OAuth requires a TTY on stdin or stderr/, + ); + } finally { + if (stdinDesc) Object.defineProperty(process.stdin, "isTTY", stdinDesc); + else delete (process.stdin as { isTTY?: boolean }).isTTY; + if (stderrDesc) + Object.defineProperty(process.stderr, "isTTY", stderrDesc); + else delete (process.stderr as { isTTY?: boolean }).isTTY; + } + }); + + it("runCliInteractiveOAuth is quiet on already_authorized", async () => { + vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth").mockResolvedValue({ + kind: "already_authorized", + }); + const stderrSpy = vi + .spyOn(process.stderr, "write") + .mockImplementation(() => true); + await runCliInteractiveOAuth( + { + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn(), + }, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + ); + expect(stderrSpy).not.toHaveBeenCalledWith("Authorization complete.\n"); + }); + + it("connectInspectorWithOAuth stored-auth-only uses fallback when AuthRecovery message is empty", async () => { + const err = new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + { reason: "token_expired" }, ); + Object.defineProperty(err, "message", { value: "" }); + const connect = vi.fn().mockRejectedValue(err); + await expect( + connectInspectorWithOAuth( + { + connect, + disconnect: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), + }, + OAUTH_HTTP_CONFIG, + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + undefined, + { storedAuthOnly: true }, + ), + ).rejects.toMatchObject({ + exitCode: 3, + message: expect.stringMatching(/--stored-auth-only/), + }); }); it("withCliAuthRecoveryRetry refuses interactive OAuth on a non-TTY", async () => { @@ -851,7 +931,6 @@ describe("cliOAuth", () => { CALLBACK_URL_CONFIG, undefined, fn, - undefined, { isTTY: false }, ), ).rejects.toMatchObject({ @@ -889,7 +968,6 @@ describe("cliOAuth", () => { CALLBACK_URL_CONFIG, undefined, fn, - undefined, { isTTY: false }, ); diff --git a/clients/cli/__tests__/oauth-interactive.test.ts b/clients/cli/__tests__/oauth-interactive.test.ts index 07f925bdb..0508c5ec2 100644 --- a/clients/cli/__tests__/oauth-interactive.test.ts +++ b/clients/cli/__tests__/oauth-interactive.test.ts @@ -238,8 +238,7 @@ describe("CLI interactive OAuth (integration)", () => { city: "NYC", units: "C", }), - async () => true, - { isTTY: true }, + { confirmStepUp: async () => true, isTTY: true }, ); expect(stepUpPrompted).toBe(true); diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 75274414f..eddae3ab4 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -174,7 +174,6 @@ async function callMethod( callbackUrlConfig, serverSettings, () => runMethod(inspectorClient, args), - undefined, { storedAuthOnly, autoOpenControl }, ); diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index c572fc301..ccac5c302 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -41,33 +41,41 @@ export type CliOAuthConnectOptions = { */ autoOpenControl?: CliOAuthAutoOpenControl; /** - * Override stderr TTY detection for interactive-OAuth gating (tests). - * Defaults to `process.stderr.isTTY`. + * Override “human present” detection for interactive-OAuth gating (tests). + * When omitted, production uses `stdin.isTTY || stderr.isTTY` so + * `2>&1 | tee` still works (stdin remains a TTY). */ isTTY?: boolean; + /** Override the step-up [y/N] confirmer (tests). */ + confirmStepUp?: () => Promise; }; -function storedAuthOnlyFailure(message: string): never { +function authRequiredFailure(message: string): never { throw new CliExitCodeError(EXIT_CODES.AUTH_REQUIRED, message, { code: "auth_required", }); } +function storedAuthOnlyFailure(message: string): never { + authRequiredFailure(message); +} + /** - * Interactive OAuth waits up to 15 minutes on the loopback callback. On a - * non-TTY (CI / piped stderr) nobody will complete that flow unless the caller - * explicitly opted into browser open via `MCP_AUTO_OPEN_ENABLED=true`. + * Interactive OAuth waits up to 15 minutes on the loopback callback. Admit the + * flow when a human is present (`stdin` or `stderr` is a TTY — so `2>&1 | tee` + * still works), or when `MCP_AUTO_OPEN_ENABLED=true` (explicit non-TTY / + * automation opt-in, which also force-opens a browser). */ export function assertInteractiveOAuthAllowed( options?: Pick, ): void { - const tty = + const humanPresent = options?.isTTY !== undefined ? options.isTTY - : process.stderr.isTTY === true; - if (tty || isCliAutoOpenForced()) return; - storedAuthOnlyFailure( - "Interactive OAuth requires a TTY (or MCP_AUTO_OPEN_ENABLED=true). For CI/non-interactive runs use --stored-auth-only.", + : process.stdin.isTTY === true || process.stderr.isTTY === true; + if (humanPresent || isCliAutoOpenForced()) return; + authRequiredFailure( + "Interactive OAuth requires a TTY on stdin or stderr (or MCP_AUTO_OPEN_ENABLED=true). For CI/non-interactive runs use --stored-auth-only.", ); } @@ -149,15 +157,14 @@ export async function handleCliAuthRecoveryRequired( redirectUrlProvider: MutableRedirectUrlProvider, callbackUrlConfig: RunnerOAuthCallbackConfig, serverSettings?: InspectorServerSettings, - confirmStepUp: () => Promise = confirmStepUpFromStdin, - autoOpenControl?: CliOAuthAutoOpenControl, - isTTY?: boolean, + options?: CliOAuthConnectOptions, ): Promise { + const confirmStepUp = options?.confirmStepUp ?? confirmStepUpFromStdin; if (isStandardOAuthStepUp(error.authChallenge, serverSettings)) { if (await client.checkAuthChallengeSatisfied(error.authChallenge)) { return; } - assertInteractiveOAuthAllowed({ isTTY }); + assertInteractiveOAuthAllowed(options); const proceed = await promptStepUpConfirm( error.authChallenge, confirmStepUp, @@ -168,12 +175,12 @@ export async function handleCliAuthRecoveryRequired( } else if (await client.checkAuthChallengeSatisfied(error.authChallenge)) { return; } else { - assertInteractiveOAuthAllowed({ isTTY }); + assertInteractiveOAuthAllowed(options); } await runCliInteractiveOAuth(client, redirectUrlProvider, callbackUrlConfig, { authorizationUrl: error.authorizationUrl, - autoOpenControl, + autoOpenControl: options?.autoOpenControl, ...(error.authChallenge.reason === "insufficient_scope" && { authChallenge: error.authChallenge, }), @@ -216,9 +223,7 @@ export async function connectInspectorWithOAuth( redirectUrlProvider, callbackUrlConfig, serverSettings, - confirmStepUpFromStdin, - options?.autoOpenControl, - options?.isTTY, + options, ); // Belt-and-braces: this branch never disconnects today, so connect() is // usually a no-op (already connected). Fresh tokens are picked up from @@ -265,7 +270,6 @@ export async function withCliAuthRecoveryRetry( callbackUrlConfig: RunnerOAuthCallbackConfig, serverSettings: InspectorServerSettings | undefined, fn: () => Promise, - confirmStepUp: () => Promise = confirmStepUpFromStdin, options?: CliOAuthConnectOptions, ): Promise { try { @@ -295,9 +299,7 @@ export async function withCliAuthRecoveryRetry( redirectUrlProvider, callbackUrlConfig, serverSettings, - confirmStepUp, - options?.autoOpenControl, - options?.isTTY, + options, ); // Belt-and-braces: this branch never disconnects today, so connect() is // usually a no-op (already connected). See connectInspectorWithOAuth. From 781be8cd8908da8163777b52ee61404ab343c02a Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 22:07:14 -0700 Subject: [PATCH 10/13] fix(cli): fail fast on step-up without stdin (round-8) Decline [y/N] on readline EOF/close, require a stdin TTY for the default step-up confirmer (force-open admit is not enough), document stderr-only browser auto-open vs stdin||stderr flow admit, and drop the alias wrapper. Co-authored-by: Cursor --- clients/cli/README.md | 4 +- clients/cli/__tests__/cliOAuth.test.ts | 142 +++++++++++++++++++++++- clients/cli/src/cli-oauth-navigation.ts | 19 +++- clients/cli/src/cliOAuth.ts | 65 ++++++++--- 4 files changed, 206 insertions(+), 24 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index fe199acbe..31c643142 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -149,12 +149,12 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 **CLI (`mcp-inspector --cli`):** on connect **401** or mid-session interactive auth (re-login / step-up), it: 1. Starts the callback listener on `--callback-url` (or `MCP_OAUTH_CALLBACK_URL`) -2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** when allowed. Default: open on a TTY, plain URL only when non-TTY. `MCP_AUTO_OPEN_ENABLED=false` never opens; `=true` forces open even on a non-TTY (same as the web launcher). +2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** when allowed. Default: open on a **stderr** TTY (so `2>&1 | tee` prints a plain URL into the log but does not auto-open); plain URL only when stderr is non-TTY. `MCP_AUTO_OPEN_ENABLED=false` never opens; `=true` forces open even on a non-TTY (same as the web launcher). 3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC Interactive OAuth (connect-time or mid-RPC) requires a TTY on **stdin or stderr** (so `2>&1 | tee` still works — stdin stays a TTY), or `MCP_AUTO_OPEN_ENABLED=true`. When neither stdin nor stderr is a TTY and that env is unset (typical CI), the CLI fails fast with `auth_required` instead of waiting up to 15 minutes on the loopback callback. Use **`--stored-auth-only`** for non-interactive runs that should only consume the shared store. -**Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues; **N** exits with an error. EMA step-up re-mints silently (no prompt). +**Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues; **N** (or EOF / closed stdin) declines. This prompt requires a **stdin** TTY — admit-on-stderr or `MCP_AUTO_OPEN_ENABLED=true` alone is not enough (avoids an unbounded hang when stdin is `/dev/null`). EMA step-up re-mints silently (no prompt). **Shared OAuth storage:** the CLI **reuses** tokens from `~/.mcp-inspector/storage/oauth.json` when they already exist (same file as other Inspector clients). That is passive file sharing, not launching another app. diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index 0d6c3765c..03f53b7ed 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -8,22 +8,42 @@ import { isStandardOAuthStepUp, runCliInteractiveOAuth, assertInteractiveOAuthAllowed, + assertStepUpPromptAllowed, withCliAuthRecoveryRetry, } from "../src/cliOAuth.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; +import { createInterface } from "node:readline/promises"; // `confirmStepUpFromStdin` (the default step-up confirmer) reads a line from // stdin via node:readline/promises. Mock the module so the default path can be -// exercised deterministically without real TTY input. +// exercised deterministically without real TTY input. Minimal once/removeListener +// surface covers the EOF/close race in the confirmer. const { mockQuestion, mockClose } = vi.hoisted(() => ({ mockQuestion: vi.fn(), mockClose: vi.fn(), })); vi.mock("node:readline/promises", () => ({ - createInterface: vi.fn(() => ({ - question: mockQuestion, - close: mockClose, - })), + createInterface: vi.fn(() => { + const listeners = new Map void>>(); + const rl = { + question: mockQuestion, + close: mockClose, + once(event: string, cb: (...args: unknown[]) => void) { + let set = listeners.get(event); + if (!set) { + set = new Set(); + listeners.set(event, set); + } + set.add(cb); + return rl; + }, + removeListener(event: string, cb: (...args: unknown[]) => void) { + listeners.get(event)?.delete(cb); + return rl; + }, + }; + return rl; + }), })); const CALLBACK_URL_CONFIG = { @@ -374,6 +394,118 @@ describe("cliOAuth", () => { expect(mockClose).toHaveBeenCalled(); expect(runSpy).not.toHaveBeenCalled(); }); + + it("declines when readline question rejects", async () => { + mockQuestion.mockRejectedValue(new Error("stdin closed")); + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + + await expect( + handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + INTERACTIVE, + ), + ).rejects.toThrow("Step-up authorization declined."); + + expect(runSpy).not.toHaveBeenCalled(); + }); + + it("declines when stdin EOF closes the readline interface before an answer", async () => { + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + vi.mocked(createInterface).mockImplementationOnce(() => { + const listeners = new Map void>>(); + const rl = { + question: vi.fn(() => new Promise(() => {})), + close: mockClose, + once(event: string, cb: (...args: unknown[]) => void) { + let set = listeners.get(event); + if (!set) { + set = new Set(); + listeners.set(event, set); + } + set.add(cb); + return rl; + }, + removeListener(event: string, cb: (...args: unknown[]) => void) { + listeners.get(event)?.delete(cb); + return rl; + }, + }; + queueMicrotask(() => { + for (const cb of listeners.get("close") ?? []) cb(); + }); + return rl as unknown as ReturnType; + }); + + await expect( + handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + INTERACTIVE, + ), + ).rejects.toThrow("Step-up authorization declined."); + + expect(runSpy).not.toHaveBeenCalled(); + }); + }); + + it("assertStepUpPromptAllowed fails without a stdin TTY when using the default confirmer", () => { + expect(() => assertStepUpPromptAllowed({ isTTY: false })).toThrow( + /Step-up authorization requires a TTY on stdin/, + ); + }); + + it("assertStepUpPromptAllowed skips the stdin gate when confirmStepUp is provided", () => { + expect(() => + assertStepUpPromptAllowed({ + isTTY: false, + confirmStepUp: async () => true, + }), + ).not.toThrow(); + }); + + it("step-up fails fast without stdin even when interactive OAuth is force-admitted", async () => { + // MCP_AUTO_OPEN_ENABLED=true admits the interactive-OAuth gate, but the + // [y/N] prompt still needs stdin — otherwise CI hangs forever. + vi.stubEnv("MCP_AUTO_OPEN_ENABLED", "true"); + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); + try { + await expect( + handleCliAuthRecoveryRequired( + { + authenticate: vi.fn(), + beginInteractiveAuthorization: vi.fn(), + completeOAuthFlow: vi.fn(), + checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), + }, + new AuthRecoveryRequiredError( + new URL("https://as.example/authorize"), + { + reason: "insufficient_scope", + requiredScopes: ["weather:read"], + }, + ), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + { isTTY: false }, + ), + ).rejects.toMatchObject({ + exitCode: 3, + message: expect.stringMatching( + /Step-up authorization requires a TTY on stdin/, + ), + }); + expect(runSpy).not.toHaveBeenCalled(); + } finally { + vi.unstubAllEnvs(); + } }); describe("connectInspectorWithOAuth recovery branch", () => { diff --git a/clients/cli/src/cli-oauth-navigation.ts b/clients/cli/src/cli-oauth-navigation.ts index e851061cd..1f5d1111c 100644 --- a/clients/cli/src/cli-oauth-navigation.ts +++ b/clients/cli/src/cli-oauth-navigation.ts @@ -13,7 +13,13 @@ export type CliOAuthAutoOpenControl = { }; export type CliOAuthNavigationOptions = { - /** Override TTY detection (tests). Defaults to stderr.isTTY. */ + /** + * Override stderr TTY detection (tests / programmatic callers). Defaults to + * `process.stderr.isTTY`. Browser auto-open and OSC 8 stay stderr-only on + * purpose — `2>&1 | tee` still prints a plain URL into the tee stream; + * admit-to-flow gating (`stdin || stderr`) lives in + * `assertInteractiveOAuthAllowed`, not here. + */ isTTY?: boolean; /** Override NO_COLOR (tests). */ noColorEnv?: string | undefined; @@ -40,7 +46,8 @@ export type CliOAuthNavigationOptions = { autoOpenEnabled?: boolean; /** * When true, open even if stderr is not a TTY. Independent of - * {@link autoOpenEnabled}. Defaults to {@link isCliAutoOpenForced}. + * {@link autoOpenEnabled}. Defaults to {@link isCliAutoOpenForced} + * (`MCP_AUTO_OPEN_ENABLED=true`). */ forceAutoOpen?: boolean; }; @@ -74,8 +81,11 @@ export function isCliAutoOpenForced( * silent no-ops so SDK-internal `auth()` during connect cannot emit an * uncompletable "Please navigate to:" line. * - * Browser open: off when env disables it; on a TTY when enabled; forced open - * (TTY optional) when `MCP_AUTO_OPEN_ENABLED=true` / {@link forceAutoOpen}. + * Browser open: off when env disables it; on a **stderr** TTY when enabled; + * forced open (TTY optional) when `MCP_AUTO_OPEN_ENABLED=true` / + * {@link forceAutoOpen}. Stderr-only (not `stdin || stderr`): piping stderr + * still gets a clickable plain URL; launching a browser from a redirected + * stderr session would be surprising. */ export function createCliOAuthNavigation( options: CliOAuthNavigationOptions = {}, @@ -85,6 +95,7 @@ export function createCliOAuthNavigation( if (options.disableAutoOpen || !armed) return; const href = url.href; + // stderr-only — do not widen to stdin; see file-level note above. const tty = options.isTTY !== undefined ? options.isTTY diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index ccac5c302..43253b781 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -41,12 +41,17 @@ export type CliOAuthConnectOptions = { */ autoOpenControl?: CliOAuthAutoOpenControl; /** - * Override “human present” detection for interactive-OAuth gating (tests). - * When omitted, production uses `stdin.isTTY || stderr.isTTY` so - * `2>&1 | tee` still works (stdin remains a TTY). + * Override “human present” / interactive-terminal detection (tests and + * programmatic callers). When omitted, production uses + * `stdin.isTTY || stderr.isTTY` so `2>&1 | tee` still works (stdin remains a + * TTY). Also used as the stdin-readiness override for the step-up [y/N] + * prompt when {@link confirmStepUp} is not provided. */ isTTY?: boolean; - /** Override the step-up [y/N] confirmer (tests). */ + /** + * Override the step-up [y/N] confirmer (tests and programmatic callers). + * When omitted, prompts on stderr and reads from stdin. + */ confirmStepUp?: () => Promise; }; @@ -56,15 +61,14 @@ function authRequiredFailure(message: string): never { }); } -function storedAuthOnlyFailure(message: string): never { - authRequiredFailure(message); -} - /** * Interactive OAuth waits up to 15 minutes on the loopback callback. Admit the * flow when a human is present (`stdin` or `stderr` is a TTY — so `2>&1 | tee` * still works), or when `MCP_AUTO_OPEN_ENABLED=true` (explicit non-TTY / * automation opt-in, which also force-opens a browser). + * + * Browser auto-open stays stderr-only — see {@link createCliOAuthNavigation}. + * Step-up [y/N] needs readable stdin — see {@link assertStepUpPromptAllowed}. */ export function assertInteractiveOAuthAllowed( options?: Pick, @@ -79,6 +83,23 @@ export function assertInteractiveOAuthAllowed( ); } +/** + * Standard-OAuth step-up asks [y/N] on stdin. Admit-on-stderr / force-open is + * not enough — without a TTY stdin the readline `question` never settles on + * EOF. Custom {@link CliOAuthConnectOptions.confirmStepUp} skips this gate. + */ +export function assertStepUpPromptAllowed( + options?: Pick, +): void { + if (options?.confirmStepUp) return; + const stdinReady = + options?.isTTY !== undefined ? options.isTTY : process.stdin.isTTY === true; + if (stdinReady) return; + authRequiredFailure( + "Step-up authorization requires a TTY on stdin. For CI/non-interactive runs use --stored-auth-only.", + ); +} + /** Standard-OAuth step-up (not EMA silent re-mint). */ export function isStandardOAuthStepUp( challenge: AuthChallenge, @@ -89,10 +110,27 @@ export function isStandardOAuthStepUp( }); } +/** + * Read [y/N] from stdin. On EOF / interface close (e.g. `< /dev/null`), treat + * as decline so CI never hangs forever on `rl.question`. + */ async function confirmStepUpFromStdin(): Promise { const rl = createInterface({ input: process.stdin, output: process.stderr }); try { - const answer = await rl.question(""); + const answer = await new Promise((resolve) => { + const onClose = () => resolve("n"); + rl.once("close", onClose); + void rl.question("").then( + (line) => { + rl.removeListener("close", onClose); + resolve(line); + }, + () => { + rl.removeListener("close", onClose); + resolve("n"); + }, + ); + }); const normalized = answer.trim().toLowerCase(); return normalized === "y" || normalized === "yes"; } finally { @@ -165,6 +203,7 @@ export async function handleCliAuthRecoveryRequired( return; } assertInteractiveOAuthAllowed(options); + assertStepUpPromptAllowed(options); const proceed = await promptStepUpConfirm( error.authChallenge, confirmStepUp, @@ -212,7 +251,7 @@ export async function connectInspectorWithOAuth( await inspectorClient.connect(); return; } - storedAuthOnlyFailure( + authRequiredFailure( err.message || "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", ); @@ -234,7 +273,7 @@ export async function connectInspectorWithOAuth( if (isUnauthorizedError(err)) { if (options?.storedAuthOnly) { - storedAuthOnlyFailure( + authRequiredFailure( err instanceof Error ? err.message : "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", @@ -288,7 +327,7 @@ export async function withCliAuthRecoveryRetry( ) { return await fn(); } - storedAuthOnlyFailure( + authRequiredFailure( err.message || "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", ); @@ -310,7 +349,7 @@ export async function withCliAuthRecoveryRetry( if (isUnauthorizedError(err)) { if (options?.storedAuthOnly) { - storedAuthOnlyFailure( + authRequiredFailure( err instanceof Error ? err.message : "Authentication required and --stored-auth-only is set; refusing interactive OAuth.", From 25f89335d5f56d6b117ad2e3b18e06c00cb854ec Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 22:21:03 -0700 Subject: [PATCH 11/13] fix(cli): allow piped step-up answers; bound silent pipes (round-9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Drop the hard stdin-TTY gate that broke `echo y | …`. Keep EOF decline and add a short non-TTY timeout so an open pipe that never writes cannot hang. Clarify isTTY as admit-only; extract the readline mock factory. Co-authored-by: Cursor --- clients/cli/README.md | 2 +- clients/cli/__tests__/cliOAuth.test.ts | 137 +++++++++++-------------- clients/cli/src/cliOAuth.ts | 82 ++++++++------- 3 files changed, 106 insertions(+), 115 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 31c643142..8f882359d 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -154,7 +154,7 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 Interactive OAuth (connect-time or mid-RPC) requires a TTY on **stdin or stderr** (so `2>&1 | tee` still works — stdin stays a TTY), or `MCP_AUTO_OPEN_ENABLED=true`. When neither stdin nor stderr is a TTY and that env is unset (typical CI), the CLI fails fast with `auth_required` instead of waiting up to 15 minutes on the loopback callback. Use **`--stored-auth-only`** for non-interactive runs that should only consume the shared store. -**Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues; **N** (or EOF / closed stdin) declines. This prompt requires a **stdin** TTY — admit-on-stderr or `MCP_AUTO_OPEN_ENABLED=true` alone is not enough (avoids an unbounded hang when stdin is `/dev/null`). EMA step-up re-mints silently (no prompt). +**Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues (including piped stdin, e.g. `echo y | …`); **N**, EOF (`< /dev/null` / Ctrl-D), or a non-TTY stdin that never sends a line within a short timeout declines. EMA step-up re-mints silently (no prompt). **Shared OAuth storage:** the CLI **reuses** tokens from `~/.mcp-inspector/storage/oauth.json` when they already exist (same file as other Inspector clients). That is passive file sharing, not launching another app. diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index 03f53b7ed..6d0127773 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -8,7 +8,6 @@ import { isStandardOAuthStepUp, runCliInteractiveOAuth, assertInteractiveOAuthAllowed, - assertStepUpPromptAllowed, withCliAuthRecoveryRetry, } from "../src/cliOAuth.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; @@ -16,17 +15,18 @@ import { createInterface } from "node:readline/promises"; // `confirmStepUpFromStdin` (the default step-up confirmer) reads a line from // stdin via node:readline/promises. Mock the module so the default path can be -// exercised deterministically without real TTY input. Minimal once/removeListener -// surface covers the EOF/close race in the confirmer. -const { mockQuestion, mockClose } = vi.hoisted(() => ({ - mockQuestion: vi.fn(), - mockClose: vi.fn(), -})); -vi.mock("node:readline/promises", () => ({ - createInterface: vi.fn(() => { +// exercised deterministically without real TTY input. +const { mockQuestion, mockClose, createMockReadline } = vi.hoisted(() => { + const mockQuestion = vi.fn(); + const mockClose = vi.fn(); + function createMockReadline(options?: { + question?: () => Promise; + /** When set, emit `close` on the next microtask (EOF). */ + emitClose?: boolean; + }) { const listeners = new Map void>>(); const rl = { - question: mockQuestion, + question: options?.question ?? mockQuestion, close: mockClose, once(event: string, cb: (...args: unknown[]) => void) { let set = listeners.get(event); @@ -41,9 +41,19 @@ vi.mock("node:readline/promises", () => ({ listeners.get(event)?.delete(cb); return rl; }, + emit(event: string) { + for (const cb of listeners.get(event) ?? []) cb(); + }, }; + if (options?.emitClose) { + queueMicrotask(() => rl.emit("close")); + } return rl; - }), + } + return { mockQuestion, mockClose, createMockReadline }; +}); +vi.mock("node:readline/promises", () => ({ + createInterface: vi.fn(() => createMockReadline()), })); const CALLBACK_URL_CONFIG = { @@ -415,30 +425,13 @@ describe("cliOAuth", () => { it("declines when stdin EOF closes the readline interface before an answer", async () => { const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); - vi.mocked(createInterface).mockImplementationOnce(() => { - const listeners = new Map void>>(); - const rl = { - question: vi.fn(() => new Promise(() => {})), - close: mockClose, - once(event: string, cb: (...args: unknown[]) => void) { - let set = listeners.get(event); - if (!set) { - set = new Set(); - listeners.set(event, set); - } - set.add(cb); - return rl; - }, - removeListener(event: string, cb: (...args: unknown[]) => void) { - listeners.get(event)?.delete(cb); - return rl; - }, - }; - queueMicrotask(() => { - for (const cb of listeners.get("close") ?? []) cb(); - }); - return rl as unknown as ReturnType; - }); + vi.mocked(createInterface).mockImplementationOnce( + () => + createMockReadline({ + question: () => new Promise(() => {}), + emitClose: true, + }) as unknown as ReturnType, + ); await expect( handleCliAuthRecoveryRequired( @@ -453,59 +446,47 @@ describe("cliOAuth", () => { expect(runSpy).not.toHaveBeenCalled(); }); - }); - it("assertStepUpPromptAllowed fails without a stdin TTY when using the default confirmer", () => { - expect(() => assertStepUpPromptAllowed({ isTTY: false })).toThrow( - /Step-up authorization requires a TTY on stdin/, - ); - }); + it("accepts a piped y when stdin is non-TTY (echo y | …)", async () => { + // Force-admit interactive OAuth without a TTY; the confirmer still reads + // the piped answer (round-9: do not hard-require stdin.isTTY). + vi.stubEnv("MCP_AUTO_OPEN_ENABLED", "true"); + mockQuestion.mockResolvedValue("y"); + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + try { + await handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + { isTTY: false }, + ); + expect(runSpy).toHaveBeenCalled(); + } finally { + vi.unstubAllEnvs(); + } + }); - it("assertStepUpPromptAllowed skips the stdin gate when confirmStepUp is provided", () => { - expect(() => - assertStepUpPromptAllowed({ - isTTY: false, - confirmStepUp: async () => true, - }), - ).not.toThrow(); - }); + it("declines when a non-TTY stdin never answers (pipe timeout)", async () => { + mockQuestion.mockImplementation(() => new Promise(() => {})); + const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); - it("step-up fails fast without stdin even when interactive OAuth is force-admitted", async () => { - // MCP_AUTO_OPEN_ENABLED=true admits the interactive-OAuth gate, but the - // [y/N] prompt still needs stdin — otherwise CI hangs forever. - vi.stubEnv("MCP_AUTO_OPEN_ENABLED", "true"); - const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); - try { await expect( handleCliAuthRecoveryRequired( - { - authenticate: vi.fn(), - beginInteractiveAuthorization: vi.fn(), - completeOAuthFlow: vi.fn(), - checkAuthChallengeSatisfied: vi.fn().mockResolvedValue(false), - }, - new AuthRecoveryRequiredError( - new URL("https://as.example/authorize"), - { - reason: "insufficient_scope", - requiredScopes: ["weather:read"], - }, - ), + clientNeedingStepUp(), + standardStepUpError(), new MutableRedirectUrlProvider(), CALLBACK_URL_CONFIG, {}, - { isTTY: false }, + { isTTY: true, stepUpPromptTimeoutMs: 20 }, ), - ).rejects.toMatchObject({ - exitCode: 3, - message: expect.stringMatching( - /Step-up authorization requires a TTY on stdin/, - ), - }); + ).rejects.toThrow("Step-up authorization declined."); + expect(runSpy).not.toHaveBeenCalled(); - } finally { - vi.unstubAllEnvs(); - } + }); }); describe("connectInspectorWithOAuth recovery branch", () => { diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index 43253b781..a291691d1 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -29,6 +29,9 @@ export type CliOAuthClient = RunnerInteractiveOAuthClient & { disconnect(): Promise; }; +/** Default bound for a non-TTY step-up [y/N] that never sends a line. */ +export const STEP_UP_PIPE_TIMEOUT_MS = 5_000; + export type CliOAuthConnectOptions = { /** * When true, never open interactive OAuth / step-up prompts. Use the shared @@ -41,11 +44,11 @@ export type CliOAuthConnectOptions = { */ autoOpenControl?: CliOAuthAutoOpenControl; /** - * Override “human present” / interactive-terminal detection (tests and - * programmatic callers). When omitted, production uses + * Override “human present” detection for interactive-OAuth admit gating + * (tests and programmatic callers). When omitted, production uses * `stdin.isTTY || stderr.isTTY` so `2>&1 | tee` still works (stdin remains a - * TTY). Also used as the stdin-readiness override for the step-up [y/N] - * prompt when {@link confirmStepUp} is not provided. + * TTY). Does **not** gate the step-up [y/N] confirmer — that accepts piped + * answers and bounds silent pipes via {@link stepUpPromptTimeoutMs}. */ isTTY?: boolean; /** @@ -53,6 +56,13 @@ export type CliOAuthConnectOptions = { * When omitted, prompts on stderr and reads from stdin. */ confirmStepUp?: () => Promise; + /** + * Bound (ms) for the default step-up confirmer when stdin is not a TTY, so a + * pipe that never writes cannot hang forever. Ignored on a TTY stdin (humans + * may pause). When set explicitly (including in tests), always applied. + * Default: {@link STEP_UP_PIPE_TIMEOUT_MS}. + */ + stepUpPromptTimeoutMs?: number; }; function authRequiredFailure(message: string): never { @@ -68,7 +78,8 @@ function authRequiredFailure(message: string): never { * automation opt-in, which also force-opens a browser). * * Browser auto-open stays stderr-only — see {@link createCliOAuthNavigation}. - * Step-up [y/N] needs readable stdin — see {@link assertStepUpPromptAllowed}. + * Step-up [y/N] accepts piped stdin (`echo y | …`); EOF and silent pipes are + * bounded in the default confirmer (close race + {@link STEP_UP_PIPE_TIMEOUT_MS}). */ export function assertInteractiveOAuthAllowed( options?: Pick, @@ -83,23 +94,6 @@ export function assertInteractiveOAuthAllowed( ); } -/** - * Standard-OAuth step-up asks [y/N] on stdin. Admit-on-stderr / force-open is - * not enough — without a TTY stdin the readline `question` never settles on - * EOF. Custom {@link CliOAuthConnectOptions.confirmStepUp} skips this gate. - */ -export function assertStepUpPromptAllowed( - options?: Pick, -): void { - if (options?.confirmStepUp) return; - const stdinReady = - options?.isTTY !== undefined ? options.isTTY : process.stdin.isTTY === true; - if (stdinReady) return; - authRequiredFailure( - "Step-up authorization requires a TTY on stdin. For CI/non-interactive runs use --stored-auth-only.", - ); -} - /** Standard-OAuth step-up (not EMA silent re-mint). */ export function isStandardOAuthStepUp( challenge: AuthChallenge, @@ -111,24 +105,39 @@ export function isStandardOAuthStepUp( } /** - * Read [y/N] from stdin. On EOF / interface close (e.g. `< /dev/null`), treat - * as decline so CI never hangs forever on `rl.question`. + * Read [y/N] from stdin. Piped answers work (`echo y | …`). On EOF / interface + * close (e.g. `< /dev/null`), or when a non-TTY stdin never sends a line + * within {@link STEP_UP_PIPE_TIMEOUT_MS}, treat as decline so CI never hangs. */ -async function confirmStepUpFromStdin(): Promise { +async function confirmStepUpFromStdin(timeoutMs?: number): Promise { const rl = createInterface({ input: process.stdin, output: process.stderr }); + const applyTimeoutMs = + timeoutMs !== undefined + ? timeoutMs + : process.stdin.isTTY === true + ? undefined + : STEP_UP_PIPE_TIMEOUT_MS; try { const answer = await new Promise((resolve) => { - const onClose = () => resolve("n"); + let settled = false; + let timer: ReturnType | undefined; + function finish(value: string): void { + if (settled) return; + settled = true; + if (timer !== undefined) clearTimeout(timer); + rl.removeListener("close", onClose); + resolve(value); + } + function onClose(): void { + finish("n"); + } rl.once("close", onClose); + if (applyTimeoutMs !== undefined) { + timer = setTimeout(() => finish("n"), applyTimeoutMs); + } void rl.question("").then( - (line) => { - rl.removeListener("close", onClose); - resolve(line); - }, - () => { - rl.removeListener("close", onClose); - resolve("n"); - }, + (line) => finish(line), + () => finish("n"), ); }); const normalized = answer.trim().toLowerCase(); @@ -197,13 +206,14 @@ export async function handleCliAuthRecoveryRequired( serverSettings?: InspectorServerSettings, options?: CliOAuthConnectOptions, ): Promise { - const confirmStepUp = options?.confirmStepUp ?? confirmStepUpFromStdin; + const confirmStepUp = + options?.confirmStepUp ?? + (() => confirmStepUpFromStdin(options?.stepUpPromptTimeoutMs)); if (isStandardOAuthStepUp(error.authChallenge, serverSettings)) { if (await client.checkAuthChallengeSatisfied(error.authChallenge)) { return; } assertInteractiveOAuthAllowed(options); - assertStepUpPromptAllowed(options); const proceed = await promptStepUpConfirm( error.authChallenge, confirmStepUp, From 17a6a3192291d4b9a5d9537ea978d567c3bd8a7f Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 22:49:51 -0700 Subject: [PATCH 12/13] fix(cli): keep partial piped step-up answers (round-10) Capture readline `line` before close so `printf y` without a newline is not treated as decline; timeout is auth_required with a distinct message; document the 5s bound and post-confirm callback wait; pin the TTY no-timeout arm. Co-authored-by: Cursor --- clients/cli/README.md | 2 +- clients/cli/__tests__/cliOAuth.test.ts | 101 +++++++++++++++++++++++-- clients/cli/src/cliOAuth.ts | 44 ++++++++--- 3 files changed, 130 insertions(+), 17 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 8f882359d..fc48bbd55 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -154,7 +154,7 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 Interactive OAuth (connect-time or mid-RPC) requires a TTY on **stdin or stderr** (so `2>&1 | tee` still works — stdin stays a TTY), or `MCP_AUTO_OPEN_ENABLED=true`. When neither stdin nor stderr is a TTY and that env is unset (typical CI), the CLI fails fast with `auth_required` instead of waiting up to 15 minutes on the loopback callback. Use **`--stored-auth-only`** for non-interactive runs that should only consume the shared store. -**Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues (including piped stdin, e.g. `echo y | …`); **N**, EOF (`< /dev/null` / Ctrl-D), or a non-TTY stdin that never sends a line within a short timeout declines. EMA step-up re-mints silently (no prompt). +**Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues (including piped stdin — `echo y | …` or `printf y | …`); **N** or EOF with no answer (`< /dev/null` / Ctrl-D) declines. A non-TTY stdin that never sends a line within **5 seconds** fails with `auth_required` (not the same as an explicit **N**). Answering **y** only confirms step-up — the following browser/loopback OAuth can still wait up to 15 minutes; for headless CI prefer **`--stored-auth-only`** with tokens already in the store. EMA step-up re-mints silently (no prompt). **Shared OAuth storage:** the CLI **reuses** tokens from `~/.mcp-inspector/storage/oauth.json` when they already exist (same file as other Inspector clients). That is passive file sharing, not launching another app. diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index 6d0127773..a3b322058 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -9,6 +9,7 @@ import { runCliInteractiveOAuth, assertInteractiveOAuthAllowed, withCliAuthRecoveryRetry, + STEP_UP_PIPE_TIMEOUT_MS, } from "../src/cliOAuth.js"; import type { MCPServerConfig } from "@inspector/core/mcp/types.js"; import { createInterface } from "node:readline/promises"; @@ -21,14 +22,19 @@ const { mockQuestion, mockClose, createMockReadline } = vi.hoisted(() => { const mockClose = vi.fn(); function createMockReadline(options?: { question?: () => Promise; - /** When set, emit `close` on the next microtask (EOF). */ + /** When set, emit `close` on the next microtask (EOF, no partial line). */ emitClose?: boolean; + /** + * Emit a `line` then `close` on the next microtask — models + * `printf 'y' | …` (partial last line without trailing newline). + */ + emitLineThenClose?: string; }) { const listeners = new Map void>>(); const rl = { question: options?.question ?? mockQuestion, close: mockClose, - once(event: string, cb: (...args: unknown[]) => void) { + on(event: string, cb: (...args: unknown[]) => void) { let set = listeners.get(event); if (!set) { set = new Set(); @@ -37,15 +43,25 @@ const { mockQuestion, mockClose, createMockReadline } = vi.hoisted(() => { set.add(cb); return rl; }, + once(event: string, cb: (...args: unknown[]) => void) { + // Match EventEmitter.removeListener(originalFn) after once(fn). + return rl.on(event, cb); + }, removeListener(event: string, cb: (...args: unknown[]) => void) { listeners.get(event)?.delete(cb); return rl; }, - emit(event: string) { - for (const cb of listeners.get(event) ?? []) cb(); + emit(event: string, ...args: unknown[]) { + for (const cb of [...(listeners.get(event) ?? [])]) cb(...args); }, }; - if (options?.emitClose) { + if (options?.emitLineThenClose !== undefined) { + const line = options.emitLineThenClose; + queueMicrotask(() => { + rl.emit("line", line); + rl.emit("close"); + }); + } else if (options?.emitClose) { queueMicrotask(() => rl.emit("close")); } return rl; @@ -447,6 +463,32 @@ describe("cliOAuth", () => { expect(runSpy).not.toHaveBeenCalled(); }); + it("accepts a partial last line without trailing newline (printf y | …)", async () => { + // On stream end, readline emits the buffered partial line then close + // without settling question() — close must use the captured line. + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + vi.mocked(createInterface).mockImplementationOnce( + () => + createMockReadline({ + question: () => new Promise(() => {}), + emitLineThenClose: "y", + }) as unknown as ReturnType, + ); + + await handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + INTERACTIVE, + ); + + expect(runSpy).toHaveBeenCalled(); + }); + it("accepts a piped y when stdin is non-TTY (echo y | …)", async () => { // Force-admit interactive OAuth without a TTY; the confirmer still reads // the piped answer (round-9: do not hard-require stdin.isTTY). @@ -470,7 +512,7 @@ describe("cliOAuth", () => { } }); - it("declines when a non-TTY stdin never answers (pipe timeout)", async () => { + it("fails with auth_required when a non-TTY stdin never answers (pipe timeout)", async () => { mockQuestion.mockImplementation(() => new Promise(() => {})); const runSpy = vi.spyOn(runnerInteractive, "runRunnerInteractiveOAuth"); @@ -483,10 +525,55 @@ describe("cliOAuth", () => { {}, { isTTY: true, stepUpPromptTimeoutMs: 20 }, ), - ).rejects.toThrow("Step-up authorization declined."); + ).rejects.toMatchObject({ + exitCode: 3, + message: expect.stringMatching(/no answer on stdin within 20ms/), + }); expect(runSpy).not.toHaveBeenCalled(); }); + + it("does not apply the pipe timeout when stdin is a TTY", async () => { + vi.useFakeTimers(); + const stdinDesc = Object.getOwnPropertyDescriptor(process.stdin, "isTTY"); + Object.defineProperty(process.stdin, "isTTY", { + configurable: true, + get: () => true, + }); + let resolveQuestion!: (value: string) => void; + mockQuestion.mockImplementation( + () => + new Promise((resolve) => { + resolveQuestion = resolve; + }), + ); + const runSpy = vi + .spyOn(runnerInteractive, "runRunnerInteractiveOAuth") + .mockResolvedValue({ kind: "success" }); + try { + const pending = handleCliAuthRecoveryRequired( + clientNeedingStepUp(), + standardStepUpError(), + new MutableRedirectUrlProvider(), + CALLBACK_URL_CONFIG, + {}, + INTERACTIVE, + ); + await vi.advanceTimersByTimeAsync(STEP_UP_PIPE_TIMEOUT_MS + 60_000); + const raced = await Promise.race([ + pending.then(() => "done" as const), + Promise.resolve("still-waiting" as const), + ]); + expect(raced).toBe("still-waiting"); + resolveQuestion("y"); + await pending; + expect(runSpy).toHaveBeenCalled(); + } finally { + vi.useRealTimers(); + if (stdinDesc) Object.defineProperty(process.stdin, "isTTY", stdinDesc); + else delete (process.stdin as { isTTY?: boolean }).isTTY; + } + }); }); describe("connectInspectorWithOAuth recovery branch", () => { diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index a291691d1..36d647abf 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -105,9 +105,14 @@ export function isStandardOAuthStepUp( } /** - * Read [y/N] from stdin. Piped answers work (`echo y | …`). On EOF / interface - * close (e.g. `< /dev/null`), or when a non-TTY stdin never sends a line - * within {@link STEP_UP_PIPE_TIMEOUT_MS}, treat as decline so CI never hangs. + * Read [y/N] from stdin. Piped answers work (`echo y | …`, `printf y | …`). + * On EOF with no line (e.g. `< /dev/null`), decline. A non-TTY stdin that + * never sends a line within {@link STEP_UP_PIPE_TIMEOUT_MS} fails with + * `auth_required` (distinct from an explicit **N**). + * + * Partial last lines without a trailing newline are captured via the `line` + * event — on stream end readline emits the buffer then `close` without + * settling `question()`, so close alone would otherwise silently decline. */ async function confirmStepUpFromStdin(timeoutMs?: number): Promise { const rl = createInterface({ input: process.stdin, output: process.stderr }); @@ -118,26 +123,47 @@ async function confirmStepUpFromStdin(timeoutMs?: number): Promise { ? undefined : STEP_UP_PIPE_TIMEOUT_MS; try { - const answer = await new Promise((resolve) => { + const answer = await new Promise((resolve, reject) => { let settled = false; + let lastLine: string | undefined; let timer: ReturnType | undefined; + function cleanup(): void { + if (timer !== undefined) clearTimeout(timer); + rl.removeListener("close", onClose); + rl.removeListener("line", onLine); + } function finish(value: string): void { if (settled) return; settled = true; - if (timer !== undefined) clearTimeout(timer); - rl.removeListener("close", onClose); + cleanup(); resolve(value); } + function finishTimeout(): void { + if (settled) return; + settled = true; + cleanup(); + reject( + new CliExitCodeError( + EXIT_CODES.AUTH_REQUIRED, + `Step-up authorization declined (no answer on stdin within ${String(applyTimeoutMs)}ms). For CI/non-interactive runs use --stored-auth-only.`, + { code: "auth_required" }, + ), + ); + } + function onLine(line: string): void { + lastLine = line; + } function onClose(): void { - finish("n"); + finish(lastLine ?? "n"); } + rl.on("line", onLine); rl.once("close", onClose); if (applyTimeoutMs !== undefined) { - timer = setTimeout(() => finish("n"), applyTimeoutMs); + timer = setTimeout(finishTimeout, applyTimeoutMs); } void rl.question("").then( (line) => finish(line), - () => finish("n"), + () => finish(lastLine ?? "n"), ); }); const normalized = answer.trim().toLowerCase(); From fd1f7b4dfe130389ad586c611c97d3a42da60dba Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Sat, 25 Jul 2026 23:09:56 -0700 Subject: [PATCH 13/13] fix(cli): tighten step-up timeout messaging and TTY test (round-11) Make the TTY no-timeout assertion use a settled flag (the Promise.race check was vacuous), say "timed out" instead of "declined" on the pipe bound, and document that piped answers need a newline or stdin close. Co-authored-by: Cursor --- clients/cli/README.md | 2 +- clients/cli/__tests__/cliOAuth.test.ts | 16 +++++++++++----- clients/cli/src/cliOAuth.ts | 12 +++++++----- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index fc48bbd55..6abb958fe 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -154,7 +154,7 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 Interactive OAuth (connect-time or mid-RPC) requires a TTY on **stdin or stderr** (so `2>&1 | tee` still works — stdin stays a TTY), or `MCP_AUTO_OPEN_ENABLED=true`. When neither stdin nor stderr is a TTY and that env is unset (typical CI), the CLI fails fast with `auth_required` instead of waiting up to 15 minutes on the loopback callback. Use **`--stored-auth-only`** for non-interactive runs that should only consume the shared store. -**Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues (including piped stdin — `echo y | …` or `printf y | …`); **N** or EOF with no answer (`< /dev/null` / Ctrl-D) declines. A non-TTY stdin that never sends a line within **5 seconds** fails with `auth_required` (not the same as an explicit **N**). Answering **y** only confirms step-up — the following browser/loopback OAuth can still wait up to 15 minutes; for headless CI prefer **`--stored-auth-only`** with tokens already in the store. EMA step-up re-mints silently (no prompt). +**Step-up (standard OAuth):** when an RPC needs extra scopes, the CLI prompts on stderr: `Proceed with step-up authorization? [y/N]`. **y** continues (including piped stdin — `echo y | …` or `printf y | …`); **N** or EOF with no answer (`< /dev/null` / Ctrl-D) declines. Piped answers must be **newline-terminated, or stdin must close** — a bare `y` held open without `\n` or EOF is not flushed as a line and times out. A non-TTY stdin that never sends a line within **5 seconds** fails with `auth_required` (`timed out`, not the same as an explicit **N**). Answering **y** only confirms step-up — the following browser/loopback OAuth can still wait up to 15 minutes; for headless CI prefer **`--stored-auth-only`** with tokens already in the store. EMA step-up re-mints silently (no prompt). **Shared OAuth storage:** the CLI **reuses** tokens from `~/.mcp-inspector/storage/oauth.json` when they already exist (same file as other Inspector clients). That is passive file sharing, not launching another app. diff --git a/clients/cli/__tests__/cliOAuth.test.ts b/clients/cli/__tests__/cliOAuth.test.ts index a3b322058..f18367be6 100644 --- a/clients/cli/__tests__/cliOAuth.test.ts +++ b/clients/cli/__tests__/cliOAuth.test.ts @@ -559,12 +559,18 @@ describe("cliOAuth", () => { {}, INTERACTIVE, ); + let settled = false; + void pending.then( + () => { + settled = true; + }, + () => { + settled = true; + }, + ); await vi.advanceTimersByTimeAsync(STEP_UP_PIPE_TIMEOUT_MS + 60_000); - const raced = await Promise.race([ - pending.then(() => "done" as const), - Promise.resolve("still-waiting" as const), - ]); - expect(raced).toBe("still-waiting"); + await Promise.resolve(); // flush microtasks from any premature settle + expect(settled).toBe(false); resolveQuestion("y"); await pending; expect(runSpy).toHaveBeenCalled(); diff --git a/clients/cli/src/cliOAuth.ts b/clients/cli/src/cliOAuth.ts index 36d647abf..665c2d0a7 100644 --- a/clients/cli/src/cliOAuth.ts +++ b/clients/cli/src/cliOAuth.ts @@ -105,14 +105,16 @@ export function isStandardOAuthStepUp( } /** - * Read [y/N] from stdin. Piped answers work (`echo y | …`, `printf y | …`). - * On EOF with no line (e.g. `< /dev/null`), decline. A non-TTY stdin that - * never sends a line within {@link STEP_UP_PIPE_TIMEOUT_MS} fails with - * `auth_required` (distinct from an explicit **N**). + * Read [y/N] from stdin. Piped answers work when newline-terminated or when + * stdin closes (`echo y | …`, `printf y | …`). On EOF with no line + * (`< /dev/null`), decline. A non-TTY stdin that never sends a line within + * {@link STEP_UP_PIPE_TIMEOUT_MS} fails with `auth_required` (timed out — + * distinct from an explicit **N**). * * Partial last lines without a trailing newline are captured via the `line` * event — on stream end readline emits the buffer then `close` without * settling `question()`, so close alone would otherwise silently decline. + * An open pipe that writes `y` without `\n` or EOF never flushes that buffer. */ async function confirmStepUpFromStdin(timeoutMs?: number): Promise { const rl = createInterface({ input: process.stdin, output: process.stderr }); @@ -145,7 +147,7 @@ async function confirmStepUpFromStdin(timeoutMs?: number): Promise { reject( new CliExitCodeError( EXIT_CODES.AUTH_REQUIRED, - `Step-up authorization declined (no answer on stdin within ${String(applyTimeoutMs)}ms). For CI/non-interactive runs use --stored-auth-only.`, + `Step-up authorization timed out (no answer on stdin within ${String(applyTimeoutMs)}ms). For CI/non-interactive runs use --stored-auth-only.`, { code: "auth_required" }, ), );