cli: stored-auth injection should honor refresh_token, not just access_token (#1665)

clients/cli/README.md

@@ -193,14 +193,16 @@ For the common case where OAuth was already completed in the **web inspector on
 
 | Option                  | Description                                                                                                                                                                                    |
 | ----------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| `--use-stored-auth`     | Read the stored access token for `--server-url` and inject it as `Authorization: Bearer`. Exits `3` (`no_stored_token`) — listing the stored keys — when no token matches. Requires `--server-url`. |
-| `--wait-for-auth <sec>` | 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 `<sec>` with exit `3` (`auth_wait_timeout`). Use after handing off to a human to complete OAuth in a browser. |
+| `--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 <sec>` | Poll the OAuth state file (500 ms interval) until an access token for `--server-url` appears, then inject it. Times out at `<sec>` 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` → `<MCP_STORAGE_DIR>/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.
 
-**Blind injection.** The token is injected as-is; the CLI does not validate or refresh it (a stored `refresh_token` is not yet used — a natural follow-up). A stale/expired access token surfaces as an HTTP `401` on the first request → exit `3` (`auth_required`). Re-complete the flow in the web inspector (or use `--wait-for-auth`) and retry.
+**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.
 

clients/cli/__tests__/stored-auth.test.ts

@@ -1,10 +1,11 @@
-import { describe, it, expect, beforeAll, afterAll } from "vitest";
-import { mkdtempSync, writeFileSync, rmSync } from "node:fs";
+import { describe, it, expect, beforeAll, afterAll, vi } from "vitest";
+import { mkdtempSync, writeFileSync, readFileSync, rmSync } from "node:fs";
+import { createServer, type Server } from "node:http";
 import { join, dirname } from "node:path";
 import { tmpdir } from "node:os";
 import { runCli } from "./helpers/cli-runner.js";
 import { expectCliFailure, expectCliSuccess } from "./helpers/assertions.js";
-import { normalizeServerUrl } from "../src/cli.js";
+import { normalizeServerUrl, refreshStoredAuthToken } from "../src/cli.js";
 import {
   createTestServerHttp,
   createEchoTool,
@@ -36,6 +37,177 @@ describe("normalizeServerUrl", () => {
   });
 });
 
+describe("refreshStoredAuthToken", () => {
+  const SERVER = "https://api.example/mcp";
+  const freshTokens = {
+    access_token: "refreshed-access-token",
+    token_type: "Bearer",
+    refresh_token: "rotated-refresh-token",
+    expires_in: 3600,
+  };
+
+  it("runs the refresh grant, injects the fresh token, and persists the rotation", async () => {
+    const path = writeOAuthFixture({
+      [SERVER]: {
+        tokens: { refresh_token: "old-refresh", token_type: "Bearer" },
+        clientInformation: { client_id: "cid", client_secret: "sec" },
+        serverMetadata: {
+          issuer: "https://auth.example",
+          token_endpoint: "https://auth.example/token",
+        },
+      },
+    });
+    try {
+      const refresh = vi.fn().mockResolvedValue(freshTokens);
+      const discover = vi.fn();
+      const token = await refreshStoredAuthToken(SERVER, path, {
+        refresh,
+        discover,
+      });
+      expect(token).toBe("refreshed-access-token");
+      // Stored metadata present → no discovery needed.
+      expect(discover).not.toHaveBeenCalled();
+      // Refresh called with the stored refresh token + client information.
+      expect(refresh).toHaveBeenCalledTimes(1);
+      const [, opts] = refresh.mock.calls[0]!;
+      expect(opts.refreshToken).toBe("old-refresh");
+      expect(opts.clientInformation).toEqual({
+        client_id: "cid",
+        client_secret: "sec",
+      });
+      // Rotation persisted back under the same key.
+      const persisted = JSON.parse(readFileSync(path, "utf8")) as {
+        servers: Record<string, { tokens?: { refresh_token?: string } }>;
+      };
+      expect(persisted.servers[SERVER]?.tokens?.refresh_token).toBe(
+        "rotated-refresh-token",
+      );
+    } finally {
+      rmSync(path, { force: true });
+    }
+  });
+
+  it("discovers the auth-server metadata when it was not persisted", 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({
+        issuer: "https://api.example",
+        token_endpoint: "https://api.example/token",
+      });
+      const token = await refreshStoredAuthToken(SERVER, path, {
+        refresh,
+        discover,
+      });
+      expect(token).toBe("refreshed-access-token");
+      expect(discover).toHaveBeenCalledTimes(1);
+    } finally {
+      rmSync(path, { force: true });
+    }
+  });
+
+  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]: {
+        tokens: { access_token: "only-access", token_type: "Bearer" },
+      },
+    });
+    try {
+      await expect(
+        refreshStoredAuthToken(SERVER, path, { refresh: vi.fn() }),
+      ).rejects.toMatchObject({ exitCode: 3 });
+    } finally {
+      rmSync(path, { force: true });
+    }
+  });
+
+  it("throws AUTH_REQUIRED when a refresh token is present but client information 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 });
+    } finally {
+      rmSync(path, { force: true });
+    }
+  });
+
+  it("surfaces a refresh-grant failure as AUTH_REQUIRED (refresh_failed)", async () => {
+    const path = writeOAuthFixture({
+      [SERVER]: {
+        tokens: { refresh_token: "old-refresh", token_type: "Bearer" },
+        clientInformation: { client_id: "cid" },
+        serverMetadata: {
+          issuer: "https://auth.example",
+          token_endpoint: "https://auth.example/token",
+        },
+      },
+    });
+    try {
+      const refresh = vi
+        .fn()
+        .mockRejectedValue(new Error("invalid_grant: token revoked"));
+      await expect(
+        refreshStoredAuthToken(SERVER, path, { refresh }),
+      ).rejects.toMatchObject({
+        exitCode: 3,
+        envelope: { code: "refresh_failed" },
+      });
+    } finally {
+      rmSync(path, { force: true });
+    }
+  });
+});
+
 describe("--use-stored-auth", () => {
   let server: ReturnType<typeof createTestServerHttp>;
   let serverUrl: string;
@@ -197,6 +369,121 @@ describe("--use-stored-auth", () => {
     const last = server.getRecordedRequests().at(-1)!;
     expect(last.headers?.authorization).toBe(`Bearer ${TOKEN}`);
   });
+
+  it("refreshes a stored refresh_token end-to-end and injects the fresh access token (#1665)", async () => {
+    // Minimal OAuth token endpoint: honors the refresh_token grant with a
+    // rotated token pair, so the CLI's real SDK refresh path has something to
+    // talk to (the MCP test server accepts any bearer, so a successful connect
+    // proves the refreshed token was injected).
+    let tokenRequests = 0;
+    const tokenServer: Server = createServer((req, res) => {
+      tokenRequests += 1;
+      res.writeHead(200, { "content-type": "application/json" });
+      res.end(
+        JSON.stringify({
+          access_token: "refreshed-access-token",
+          token_type: "Bearer",
+          refresh_token: "rotated-refresh-token",
+          expires_in: 3600,
+        }),
+      );
+    });
+    await new Promise<void>((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: { 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);
+      expect(tokenRequests).toBeGreaterThan(0);
+      const last = server.getRecordedRequests().at(-1)!;
+      expect(last.headers?.authorization).toBe("Bearer refreshed-access-token");
+      // Rotation persisted so a subsequent run reuses the new refresh token.
+      const persisted = JSON.parse(readFileSync(fixture, "utf8")) as {
+        servers: Record<string, { tokens?: { refresh_token?: string } }>;
+      };
+      expect(persisted.servers[serverUrl]?.tokens?.refresh_token).toBe(
+        "rotated-refresh-token",
+      );
+    } finally {
+      rmSync(fixture, { force: true });
+      await new Promise<void>((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<void>((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<void>((resolve) => tokenServer.close(() => resolve()));
+    }
+  });
 });
 
 describe("--list-stored-auth", () => {