From 59f3bd92a4df7df23b650711167654e08dfd8f1a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 11 Jul 2026 23:21:59 -0400 Subject: [PATCH] =?UTF-8?q?cli:=20address=20#1673=20review=20=E2=80=94=20r?= =?UTF-8?q?efresh=20fallback,=200600=20write,=20distinct=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Refresh failure now falls back to injecting a still-present stored access token instead of hard-exiting 3, so a transient auth-server hiccup doesn't regress a previously-working invocation (review #1). Only a failure with no access token to fall back on surfaces as exit 3. - Persist rotated tokens via the shared writeStoreFile (0o600 + mkdir -p) instead of a raw writeFile, so the secrets file keeps owner-only perms identical to the web backend (review #2). - Missing clientInformation now throws the distinct no_client_information envelope code rather than reusing no_stored_token (review #5a). - Docs: --wait-for-auth no longer claims parity with --use-stored-auth (it injects the freshly-landed token without refreshing); noted the always-refresh rotation/concurrency caveat (review #3, #4). - Tests: discovery-returns-undefined metadata path, no_client_information code, and an e2e refresh-failure → access-token fallback. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw --- clients/cli/README.md | 6 +- clients/cli/__tests__/stored-auth.test.ts | 90 +++++++++++++++++++++++ clients/cli/src/cli.ts | 26 ++++++- 3 files changed, 116 insertions(+), 6 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 6e9126ab..83dbe53d 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -194,13 +194,15 @@ For the common case where OAuth was already completed in the **web inspector on | Option | Description | | ----------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `--use-stored-auth` | Read the stored auth for `--server-url` and inject `Authorization: Bearer`. When a `refresh_token` is stored, the CLI runs the OAuth refresh grant first and injects the **fresh** access token (persisting the rotation); otherwise it injects the stored access token. Exits `3` (`no_stored_token`) — listing the stored keys — when nothing matches. Requires `--server-url`. | -| `--wait-for-auth ` | Poll the OAuth state file (500 ms interval) until a token for `--server-url` appears, then proceed as if `--use-stored-auth` were set. Times out at `` with exit `3` (`auth_wait_timeout`). Use after handing off to a human to complete OAuth in a browser. | +| `--wait-for-auth ` | Poll the OAuth state file (500 ms interval) until an access token for `--server-url` appears, then inject it. Times out at `` with exit `3` (`auth_wait_timeout`). Use after handing off to a human to complete OAuth in a browser. Unlike `--use-stored-auth`, this injects the freshly-landed access token directly (a token that just completed the browser flow is not expired), so it does **not** run the refresh grant. | | `--list-stored-auth` | Print `{ oauthStatePath, storedServerUrls }` (the server keys that currently have a token) and exit. No server connection is made. | | `--print-handoff` | Print a JSON handoff block (`deepLink`, `portForwardCmd`, `oauthStatePath`, `apiToken`) for `--server-url` and exit — everything a script/remote VM needs to drive the browser-side OAuth dance. Requires `--server-url`. | **State-file resolution** follows `MCP_INSPECTOR_OAUTH_STATE_PATH` → `/oauth.json` → `~/.mcp-inspector/storage/oauth.json` — the same precedence the rest of the Inspector uses, so the CLI and web backend agree on the file. Server keys are canonicalised with `new URL().href` (the scheme the web store writes), so a trailing-slash or case mismatch between the URL a human opened and the one the agent passed still resolves. -**Token refresh (#1665).** When the stored server state carries a `refresh_token` (plus the `clientInformation` the web inspector persists after a completed flow), `--use-stored-auth` runs the SDK's OAuth `refresh_token` grant to mint a fresh access token before connecting, then writes the rotated tokens back to the state file so web and CLI stay consistent. The auth-server metadata is reused from the stored state, or discovered from `--server-url` when absent. This covers both an absent and an expired stored access token — the persisted blob carries no expiry, so the refresh token is treated as the durable credential. A failed refresh (revoked/expired refresh token) exits `3` (`auth_required`, envelope `refresh_failed`); re-complete the flow in the web inspector and retry. Without a stored `refresh_token` the access token is injected as-is, and a stale one surfaces as an HTTP `401` → exit `3`. +**Token refresh (#1665).** When the stored server state carries a `refresh_token` (plus the `clientInformation` the web inspector persists after a completed flow), `--use-stored-auth` runs the SDK's OAuth `refresh_token` grant to mint a fresh access token before connecting, then writes the rotated tokens back to the state file (owner-only `0o600`, via the shared store writer) so web and CLI stay consistent. The auth-server metadata is reused from the stored state, or discovered from `--server-url` when absent. This covers both an absent and an expired stored access token — the persisted blob carries no expiry, so the refresh token is treated as the durable credential. If the refresh fails (revoked token, transient auth-server error) **and** a stored access token is also present, the CLI falls back to injecting that token rather than hard-failing; only when there is nothing to fall back on does it exit `3` (`auth_required`, envelope `refresh_failed`). Without a stored `refresh_token` the access token is injected as-is, and a stale one surfaces as an HTTP `401` → exit `3`. + +Because there is no stored expiry, a `refresh_token` is refreshed on every `--use-stored-auth` run. Two consequences with rotating refresh tokens: two concurrent invocations against the same state file race the single-use token (one wins), and a crash between a successful grant and the write-back leaves the rotated token unsaved. Both are narrow; re-authorize in the web inspector to recover. **Short-circuit modes.** `--list-stored-auth` and `--print-handoff` each print their output and exit without connecting to a server; they ignore the method/target flags. They are mutually exclusive — if both are passed, `--list-stored-auth` takes precedence. diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index 8e83f95d..0661ac06 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/clients/cli/__tests__/stored-auth.test.ts @@ -111,6 +111,46 @@ describe("refreshStoredAuthToken", () => { } }); + it("passes metadata=undefined to the refresh grant when discovery returns nothing", async () => { + const path = writeOAuthFixture({ + [SERVER]: { + tokens: { refresh_token: "old-refresh", token_type: "Bearer" }, + clientInformation: { client_id: "cid" }, + }, + }); + try { + const refresh = vi.fn().mockResolvedValue(freshTokens); + const discover = vi.fn().mockResolvedValue(undefined); + const token = await refreshStoredAuthToken(SERVER, path, { + refresh, + discover, + }); + expect(token).toBe("refreshed-access-token"); + const [, opts] = refresh.mock.calls[0]!; + expect(opts.metadata).toBeUndefined(); + } finally { + rmSync(path, { force: true }); + } + }); + + it("uses the distinct no_client_information code when client info is missing", async () => { + const path = writeOAuthFixture({ + [SERVER]: { + tokens: { refresh_token: "old-refresh", token_type: "Bearer" }, + }, + }); + try { + await expect( + refreshStoredAuthToken(SERVER, path, { refresh: vi.fn() }), + ).rejects.toMatchObject({ + exitCode: 3, + envelope: { code: "no_client_information" }, + }); + } finally { + rmSync(path, { force: true }); + } + }); + it("throws AUTH_REQUIRED when no refresh token is stored", async () => { const path = writeOAuthFixture({ [SERVER]: { @@ -394,6 +434,56 @@ describe("--use-stored-auth", () => { await new Promise((resolve) => tokenServer.close(() => resolve())); } }); + + it("falls back to the stored access token when the refresh grant fails (#1665 review #1)", async () => { + // Token endpoint that rejects the refresh grant, so the CLI must fall back + // to the still-present stored access token instead of hard-failing. + const tokenServer: Server = createServer((_req, res) => { + res.writeHead(400, { "content-type": "application/json" }); + res.end(JSON.stringify({ error: "invalid_grant" })); + }); + await new Promise((resolve) => tokenServer.listen(0, resolve)); + const addr = tokenServer.address(); + const tokenBase = + typeof addr === "object" && addr ? `http://127.0.0.1:${addr.port}` : ""; + const fixture = writeOAuthFixture({ + [serverUrl]: { + tokens: { + access_token: "still-usable-access", + refresh_token: "old-refresh", + token_type: "Bearer", + }, + clientInformation: { client_id: "cid", client_secret: "sec" }, + serverMetadata: { + issuer: tokenBase, + token_endpoint: `${tokenBase}/token`, + response_types_supported: ["code"], + grant_types_supported: ["authorization_code", "refresh_token"], + token_endpoint_auth_methods_supported: ["client_secret_post"], + }, + }, + }); + try { + const result = await runCli( + [ + "--transport", + "http", + "--server-url", + serverUrl, + "--use-stored-auth", + "--method", + "tools/list", + ], + { env: { MCP_INSPECTOR_OAUTH_STATE_PATH: fixture } }, + ); + expectCliSuccess(result); + const last = server.getRecordedRequests().at(-1)!; + expect(last.headers?.authorization).toBe("Bearer still-usable-access"); + } finally { + rmSync(fixture, { force: true }); + await new Promise((resolve) => tokenServer.close(() => resolve())); + } + }); }); describe("--list-stored-auth", () => { diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 3e8f244a..f9bafd5e 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -34,6 +34,7 @@ import { type OAuthPersistSnapshot, } from "@inspector/core/auth/oauth-persist.js"; import { getAuthorizationServerUrl } from "@inspector/core/auth/discovery.js"; +import { writeStoreFile } from "@inspector/core/storage/store-io.js"; import { refreshAuthorization, discoverAuthorizationServerMetadata, @@ -595,7 +596,7 @@ export async function refreshStoredAuthToken( throw new CliExitCodeError( EXIT_CODES.AUTH_REQUIRED, `Stored auth for ${normalizeServerUrl(serverUrl)} has a refresh token but no client information; cannot refresh. Re-authorize in the web inspector.`, - { code: "no_stored_token", url: serverUrl }, + { code: "no_client_information", url: serverUrl }, ); } @@ -624,9 +625,11 @@ export async function refreshStoredAuthToken( // Persist the rotated tokens back under the same key, preserving every other // server entry and the idpSessions block, so web and CLI stay consistent. + // Route through the shared `writeStoreFile` (not a raw `writeFile`) so the + // secrets file keeps its owner-only `0o600` mode + `mkdir -p`, identical to + // how the web backend's OAuth persist backend writes it. servers[found.key] = { ...found.state, tokens }; - const { writeFile } = await import("node:fs/promises"); - await writeFile(statePath, serializeOAuthPersistBlob(snapshot), "utf8"); + await writeStoreFile(statePath, serializeOAuthPersistBlob(snapshot)); return tokens.access_token; } @@ -1077,7 +1080,22 @@ async function parseArgs(argv?: string[]): Promise { const servers = await readOAuthServers(oauthStatePath); const stored = findStoredServerState(servers, options.serverUrl); if (stored?.state.tokens?.refresh_token) { - token = await refreshStoredAuthToken(options.serverUrl, oauthStatePath); + const storedAccess = stored.state.tokens.access_token; + try { + token = await refreshStoredAuthToken( + options.serverUrl, + oauthStatePath, + ); + } catch (err) { + // A failed refresh (transient auth-server hiccup, missing client + // info) shouldn't turn a previously-working invocation into a hard + // failure when a still-usable access token is also on disk — fall + // back to injecting it (a genuinely stale one surfaces as HTTP 401 → + // exit 3, the same as without a refresh token). With no stored access + // token to fall back on, the refresh error stands. + if (!storedAccess) throw err; + token = storedAccess; + } } else { const found = findStoredToken(servers, options.serverUrl); if (!found) {