diff --git a/README.md b/README.md index d7fc4b71c0..6c4d957082 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ npx @modelcontextprotocol/inspector --tui # TUI ``` > [!WARNING] -> **On a machine with no OS keychain, secrets are saved to a plaintext file by default.** That covers Linux without libsecret or a Secret Service, headless and SSH sessions, Termux, and containers with a mounted volume. OAuth client secrets and stdio `env:` values then go to `~/.mcp-inspector/secrets.json`, unencrypted unless you supply a key. See [Where secrets are stored](./docs/secret-storage.md) for how to get a keychain back, encrypt the file, or keep secrets in memory only. +> **The Inspector manages secrets — OAuth tokens, OAuth client secrets, and stdio `env:` values — and stores them in the OS keychain, if available, by default.** On a machine with no keychain — Linux without libsecret or a Secret Service, headless and SSH sessions, Termux, and containers with a mounted volume — they are saved to `~/.mcp-inspector/secrets.json` instead, unencrypted unless you supply a key. See [Where secrets are stored](./docs/secret-storage.md) for how to get a keychain back, encrypt the file, or keep secrets in memory only. > **Upgrading from v1?** Read the [v1 → v2 migration guide](./docs/v1-to-v2-migration.md) — CLI flags, the new `--config` vs. `--catalog` split, the Node engine bump, and what no longer ships. diff --git a/clients/cli/README.md b/clients/cli/README.md index ce939a25ae..78abe7a46b 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -273,7 +273,7 @@ Interactive OAuth (connect-time or mid-RPC) requires a TTY on **stdin or stderr* **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. +**Shared OAuth storage:** the CLI **reuses** tokens stored by other Inspector clients — indexed by the shared `~/.mcp-inspector/storage/oauth.json`, with the tokens themselves held in the secret store. That is passive storage sharing, not launching another app. **Shared with TUI** (config only, not interactive login): @@ -315,7 +315,7 @@ See [EMA / enterprise-managed auth](../../specification/v2_auth_ema.md) and [OAu #### Stored-auth (web → CLI handoff) -For the common case where OAuth was already completed in the **web inspector on the same machine**, the CLI can reuse the resulting token instead of running its own interactive flow. It reads the shared OAuth state file (the `oauth.json` the web backend writes) directly from disk and injects `Authorization: Bearer ` for `--server-url`. +For the common case where OAuth was already completed in the **web inspector on the same machine**, the CLI can reuse the resulting token instead of running its own interactive flow. It reads the shared OAuth state (the `oauth.json` the web backend writes, joined with the tokens in the secret store) and injects `Authorization: Bearer ` for `--server-url`. | Option | Description | | ----------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | @@ -454,7 +454,7 @@ prose from stderr: | `0` | Success. | | `1` | Usage / unexpected error (the catch-all). | | `2` | No MCP App found on the tool (`--app-info` probe). | -| `3` | Server requires authentication (401/403, `WWW-Authenticate`, OAuth). | +| `3` | Server requires authentication (401/403 or a typed SDK auth error). | | `4` | Server unreachable (DNS, connection refused, timeout, `fetch failed`). | | `5` | Tool error (`tools/call` returned `isError:true`, or the tool was not found). | | `6` | `--strict` found an error-severity tool-schema portability problem (`schema_unportable` — the schema is valid JSON Schema, just not portable). | diff --git a/clients/cli/__tests__/error-handler.test.ts b/clients/cli/__tests__/error-handler.test.ts index 740f44aea5..b2491f88e7 100644 --- a/clients/cli/__tests__/error-handler.test.ts +++ b/clients/cli/__tests__/error-handler.test.ts @@ -6,6 +6,12 @@ import { formatErrorOutput, handleError, } from "../src/error-handler.js"; +import { UnauthorizedError } from "@modelcontextprotocol/client"; +import { + SecretFileLockHeldError, + SecretStoreUnavailableError, +} from "@inspector/core/auth/node/secret-store.js"; +import { OAuthStateFileUnrecognizedError } from "@inspector/core/auth/node/oauth-persist-file.js"; /** * `handleError` is the binary's last-resort error sink (wired up in @@ -111,11 +117,70 @@ describe("classifyError", () => { expect(envelope.code).toBe("schema_unportable"); }); - it("classifies a WWW-Authenticate message as AUTH_REQUIRED without a status", () => { - const { exitCode } = classifyError( - new Error("Dynamic client registration failed: WWW-Authenticate Bearer"), + it("classifies a typed UnauthorizedError as AUTH_REQUIRED without a status", () => { + // Every genuine auth-required condition in the SDK throws typed + // `UnauthorizedError` (or carries a structured 401) — classification is + // by type via isUnauthorizedError, not by sniffing message keywords. + const { exitCode, envelope } = classifyError( + new UnauthorizedError("Failed to authorize"), ); expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(envelope.code).toBe("auth_required"); + }); + + it("classifies a structured 401 buried in the cause chain as AUTH_REQUIRED", () => { + // protocolEra negotiation can wrap the real 401 as a nested cause; + // isUnauthorizedError walks the chain. + const err = new Error("negotiation failed", { + cause: Object.assign(new Error("upstream"), { status: 401 }), + }); + const { exitCode } = classifyError(err); + expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + }); + + it("does not classify prose mentioning OAuth as AUTH_REQUIRED", () => { + // The retired keyword heuristic (/…|OAuth/i) reported errors like this + // one — a programming/usage failure — as "re-authorize", exit 3. It is + // a plain error: exit 1, and the caller reads the message. + const { exitCode, envelope } = classifyError( + new Error("OAuth storage is required for this operation."), + ); + expect(exitCode).toBe(EXIT_CODES.USAGE); + expect(envelope.code).toBe("error"); + }); + + it("classifies SecretStoreUnavailableError as store_unavailable, exit 1", () => { + const { exitCode, envelope } = classifyError( + new SecretStoreUnavailableError("keychain probe failed"), + { url: "https://x.example/mcp" }, + ); + expect(exitCode).toBe(EXIT_CODES.USAGE); + expect(envelope.code).toBe("store_unavailable"); + expect(envelope.url).toBe("https://x.example/mcp"); + }); + + it("does not let OAuth wording in a lock-held store error read as auth_required", () => { + // SecretFileLockHeldError messages mention the OAuth state file. The + // typed operational branch classifies it as store_unavailable — telling + // the user the store is busy, not to re-authorize. + const { exitCode, envelope } = classifyError( + new SecretFileLockHeldError( + "Could not lock the OAuth state file: held by another process", + ), + ); + expect(exitCode).toBe(EXIT_CODES.USAGE); + expect(envelope.code).toBe("store_unavailable"); + }); + + it("classifies an unrecognized OAuth state file as oauth_state_unrecognized", () => { + // Repair-the-file advice, not re-authorize (auth_required) and not + // retry-later (store_unavailable): the error message tells the user + // exactly what to do, and the code lets a script branch on it. + const { exitCode, envelope } = classifyError( + new OAuthStateFileUnrecognizedError("/tmp/oauth.json", "save"), + ); + expect(exitCode).toBe(EXIT_CODES.USAGE); + expect(envelope.code).toBe("oauth_state_unrecognized"); }); it("classifies ENOTFOUND / fetch failed as UNREACHABLE", () => { @@ -379,12 +444,13 @@ describe("envelope URL redaction", () => { }); it("classifies on the unredacted text", () => { - // The only auth signal is inside a parameter value that redaction - // replaces; classifying the redacted copy would fall through to USAGE. + // The only classification signal (an UNREACHABLE_PATTERN match) is + // inside a parameter value that redaction replaces; classifying the + // redacted copy would fall through to USAGE. const { exitCode, envelope } = classifyError( - new Error("Rejected https://srv.example/cb?token=invalid_token"), + new Error("Rejected https://srv.example/cb?token=ECONNREFUSED"), ); - expect(exitCode).toBe(EXIT_CODES.AUTH_REQUIRED); + expect(exitCode).toBe(EXIT_CODES.UNREACHABLE); expect(envelope.message).toBe( "Rejected https://srv.example/cb?token=%5BREDACTED%5D", ); diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index bea1ad4994..0965b093ef 100644 --- a/clients/cli/__tests__/stored-auth.test.ts +++ b/clients/cli/__tests__/stored-auth.test.ts @@ -1,5 +1,13 @@ -import { describe, it, expect, beforeAll, afterAll, vi } from "vitest"; -import { mkdtempSync, writeFileSync, readFileSync, rmSync } from "node:fs"; +import { + describe, + it, + expect, + beforeAll, + afterAll, + afterEach, + vi, +} from "vitest"; +import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; import { createServer, type Server } from "node:http"; import { join, dirname } from "node:path"; import { tmpdir } from "node:os"; @@ -9,7 +17,13 @@ import { normalizeServerUrl, deepLinkTransport, refreshStoredAuthToken, + waitForStoredToken, + type StoredServers, } from "../src/cli.js"; +import { SecretFileLockHeldError } from "@inspector/core/auth/node/secret-store.js"; +import { readOAuthStore } from "@inspector/core/auth/node/oauth-persist-file.js"; +import { oauthSecretServerId } from "@inspector/core/auth/node/oauth-secrets.js"; +import { defaultSecretStore } from "@inspector/core/auth/node/secret-store-selection.js"; import { createTestServerHttp, createEchoTool, @@ -64,6 +78,15 @@ describe("deepLinkTransport", () => { describe("refreshStoredAuthToken", () => { const SERVER = "https://api.example/mcp"; + + // Persisted writes split tokens into the process-wide (in-memory, per + // vitest.config.ts) secret store, and joined reads prefer the store over + // file plaintext — so purge the entry between tests or one test's rotated + // tokens would leak into the next test's fixture. + afterEach(async () => { + await defaultSecretStore().deleteAllForServer(oauthSecretServerId(SERVER)); + }); + const freshTokens = { access_token: "refreshed-access-token", token_type: "Bearer", @@ -100,11 +123,10 @@ describe("refreshStoredAuthToken", () => { client_id: "cid", client_secret: "sec", }); - // Rotation persisted back under the same key. - const persisted = JSON.parse(readFileSync(path, "utf8")) as { - servers: Record; - }; - expect(persisted.servers[SERVER]?.tokens?.refresh_token).toBe( + // Rotation persisted back under the same key — via a joined read, since + // the tokens themselves now live in the secret store, not the file. + const persisted = await readOAuthStore(path); + expect(persisted?.servers[SERVER]?.tokens?.refresh_token).toBe( "rotated-refresh-token", ); } finally { @@ -347,6 +369,15 @@ describe("--use-stored-auth", () => { rmSync(fixturePath, { force: true }); }); + // Same secret-store hygiene as the refreshStoredAuthToken suite: a joined + // read prefers the store, so rotated tokens persisted by one test must not + // leak into the next test's fixture for the same server URL. + afterEach(async () => { + await defaultSecretStore().deleteAllForServer( + oauthSecretServerId(serverUrl), + ); + }); + it("injects the stored token as Authorization: Bearer on the outgoing request", async () => { const result = await runCli( [ @@ -367,6 +398,36 @@ describe("--use-stored-auth", () => { expect(last.headers?.authorization).toBe(`Bearer ${TOKEN}`); }); + it("surfaces an unreadable state path as an error, not no_stored_token", async () => { + // The blanket catch this replaces read *any* failure as an empty + // snapshot, so an unreadable state file (here: a directory) reported + // no_stored_token / exit 3 — "re-authorize" advice for a failure that + // re-authorizing cannot fix. Operational read failures now propagate. + const dir = mkdtempSync(join(tmpdir(), "inspector-cli-eisdir-")); + try { + const result = await runCli( + [ + "--transport", + "http", + "--server-url", + serverUrl, + "--use-stored-auth", + "--method", + "tools/list", + ], + { env: { MCP_INSPECTOR_OAUTH_STATE_PATH: dir } }, + ); + expect(result.exitCode).toBe(1); + const env = JSON.parse(result.stderr.trim()) as { + error: { code: string; message: string }; + }; + expect(env.error.code).toBe("error"); + expect(env.error.message).toContain("EISDIR"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it("merges with --header (explicit headers + stored auth coexist)", async () => { const result = await runCli( [ @@ -538,11 +599,10 @@ describe("--use-stored-auth", () => { 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; - }; - expect(persisted.servers[serverUrl]?.tokens?.refresh_token).toBe( + // Rotation persisted so a subsequent run reuses the new refresh token — + // asserted through a joined read (tokens live in the secret store). + const persisted = await readOAuthStore(fixture); + expect(persisted?.servers[serverUrl]?.tokens?.refresh_token).toBe( "rotated-refresh-token", ); } finally { @@ -872,6 +932,139 @@ describe("--wait-for-auth", () => { expectCliFailure(result); expect(result.stderr).toContain("positive number of seconds"); }); + + it("rethrows a persistent read failure at the deadline instead of masking it as a timeout", async () => { + // Polling races the browser flow *writing* the same state file under the + // same lock, so lock contention is tolerated (see the waitForStoredToken + // unit tests). But a *permanent* operational failure — here EISDIR from + // a state path that is a directory — is something re-authorizing cannot + // fix, so the deadline rethrows it and classifyError maps it to an + // operational envelope (exit 1), not `auth_wait_timeout` (exit 3). + const dir = mkdtempSync(join(tmpdir(), "inspector-cli-wait-eisdir-")); + try { + const result = await runCli( + [ + "--transport", + "http", + "--server-url", + serverUrl, + "--wait-for-auth", + "1", + "--method", + "tools/list", + ], + { env: { MCP_INSPECTOR_OAUTH_STATE_PATH: dir } }, + ); + expect(result.exitCode).toBe(1); + const env = JSON.parse(result.stderr.trim()) as { + error: { code: string; message: string }; + }; + expect(env.error.code).not.toBe("auth_wait_timeout"); + expect(env.error.message).toContain("EISDIR"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); + +/** + * Unit tests for the waitForStoredToken read-failure policy, via the injected + * `readServers` (integration can't exercise lock contention: acquisition + * retries internally for ~10s before throwing, longer than any sane test + * timeout). Policy under test: held locks are the success case in progress + * (never retained); other failures keep polling but are rethrown at the + * deadline; a later successful read clears the retained error. + */ +describe("waitForStoredToken read-failure policy", () => { + const url = "https://wait.example/mcp"; + const withToken: StoredServers = { + [normalizeServerUrl(url)]: { + tokens: { access_token: "healed-tok", token_type: "Bearer" }, + }, + }; + + it("treats a held lock as contention, not an error: deadline reports the ordinary timeout", async () => { + const readServers = vi + .fn<(p: string) => Promise>() + .mockRejectedValue(new SecretFileLockHeldError("state file locked")); + await expect( + waitForStoredToken(url, "/tmp/state.json", 0.3, readServers), + ).rejects.toMatchObject({ + exitCode: 3, + envelope: { code: "auth_wait_timeout" }, + }); + }); + + it("rethrows a retained non-lock failure at the deadline", async () => { + const boom = Object.assign(new Error("EACCES: permission denied"), { + code: "EACCES", + }); + const readServers = vi + .fn<(p: string) => Promise>() + .mockRejectedValue(boom); + await expect( + waitForStoredToken(url, "/tmp/state.json", 0.3, readServers), + ).rejects.toBe(boom); + }); + + it("clears a retained failure once a later read succeeds with the token", async () => { + const readServers = vi + .fn<(p: string) => Promise>() + .mockRejectedValueOnce(new Error("transient outage")) + .mockResolvedValue(withToken); + await expect( + waitForStoredToken(url, "/tmp/state.json", 5, readServers), + ).resolves.toBe("healed-tok"); + }); + + it("honors the deadline while a read is stuck on the state-file lock", async () => { + // A single lock acquisition retries for ~15s; the deadline must abandon + // the in-flight read, not wait it out. + const readServers = vi + .fn<(p: string) => Promise>() + .mockImplementation(() => new Promise(() => {})); + const started = Date.now(); + await expect( + waitForStoredToken(url, "/tmp/state.json", 0.3, readServers), + ).rejects.toMatchObject({ + exitCode: 3, + envelope: { code: "auth_wait_timeout" }, + }); + expect(Date.now() - started).toBeLessThan(2_000); + }); + + it("rethrows the retained failure when the deadline lands mid-read", async () => { + const boom = Object.assign(new Error("EACCES: permission denied"), { + code: "EACCES", + }); + const readServers = vi + .fn<(p: string) => Promise>() + .mockRejectedValueOnce(boom) + .mockImplementation(() => new Promise(() => {})); + await expect( + waitForStoredToken(url, "/tmp/state.json", 0.3, readServers), + ).rejects.toBe(boom); + }); + + it("swallows an abandoned read's late rejection instead of crashing", async () => { + // The abandoned read's promise settles after the wait has already + // thrown; its rejection must not surface as an unhandled rejection. + let rejectLate: ((e: unknown) => void) | undefined; + const readServers = vi + .fn<(p: string) => Promise>() + .mockImplementation( + () => + new Promise((_, reject) => { + rejectLate = reject; + }), + ); + await expect( + waitForStoredToken(url, "/tmp/state.json", 0.3, readServers), + ).rejects.toMatchObject({ envelope: { code: "auth_wait_timeout" } }); + rejectLate?.(new Error("lock acquisition gave up after abandonment")); + // A macrotask tick: an unhandled rejection here would fail the run. + await new Promise((r) => setTimeout(r, 20)); + }); }); /** diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index e6804494e1..293df651f1 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -35,6 +35,7 @@ import { isAllInterfacesHost, } from "@inspector/core/node/hostUrl.js"; import { getStateFilePath } from "@inspector/core/auth/node/storage-node.js"; +import { SecretFileLockHeldError } from "@inspector/core/auth/node/secret-store.js"; import { consumeMethodOutcome } from "./handlers/consume-outcome.js"; import { runMethod } from "./handlers/run-method.js"; import { @@ -45,11 +46,11 @@ import { export type { CliAppInfo } from "./handlers/method-types.js"; export { emitResult } from "./handlers/emit-result.js"; export { collectAppInfo } from "./handlers/collect-app-info.js"; +import { type OAuthPersistSnapshot } from "@inspector/core/auth/oauth-persist.js"; import { - parseOAuthPersistBlob, - serializeOAuthPersistBlob, - type OAuthPersistSnapshot, -} from "@inspector/core/auth/oauth-persist.js"; + readOAuthStore, + writeOAuthSections, +} from "@inspector/core/auth/node/oauth-persist-file.js"; import { discoverAuthorizationServerMetadataFromCandidates, getAuthorizationServerUrl, @@ -57,7 +58,6 @@ import { } from "@inspector/core/auth/discovery.js"; import { withRfc8414OidcCompat } from "@inspector/core/auth/oidcDiscoveryCompat.js"; import { withOAuthRequestTimeout } from "@inspector/core/auth/requestTimeout.js"; -import { writeStoreFile } from "@inspector/core/storage/store-io.js"; import { refreshAuthorization, discoverAuthorizationServerMetadata, @@ -266,28 +266,26 @@ type StoredServerState = { serverMetadata?: OAuthMetadata; }; /** The stored-server map shape the CLI reads out of the OAuth state file. */ -type StoredServers = Record; +export type StoredServers = Record; /** - * Read the OAuth state file directly (bypassing the Zustand store cache) so - * each call sees the current on-disk state — required for `--wait-for-auth` - * polling. Returns the full snapshot, or an empty one when the file is absent - * or unreadable. Uses the shared {@link parseOAuthPersistBlob} so both the - * plain `{servers,idpSessions}` and legacy `{state,version}` layouts are - * accepted, matching whatever the web backend wrote. + * Read the shared OAuth state ({@link OAuthPersistSnapshot}) fresh on every + * call — required for `--wait-for-auth` polling. Returns the full snapshot, + * with tokens and client secrets rejoined from the secret store (where the + * backend now keeps them), or an empty one when the file is absent or not a + * recognized OAuth state shape (both read as `null`). Operational failures + * — the state file locked by another Inspector process, an unreachable or + * unreadable secret store — propagate instead of masquerading as "no stored + * token": the credentials may exist, and `classifyError` maps these to a + * `store_unavailable` envelope rather than `auth_required`. Only the + * `--wait-for-auth` polling loop tolerates them (see + * {@link waitForStoredToken}). */ async function readOAuthSnapshot( statePath: string, ): Promise { - const { readFile } = await import("node:fs/promises"); - try { - const text = await readFile(statePath, "utf8"); - const snapshot = parseOAuthPersistBlob(text); - if (snapshot) return snapshot; - } catch { - // Absent/unreadable/malformed → fall through to the empty snapshot below. - } - return { servers: {}, idpSessions: {} }; + const snapshot = await readOAuthStore(statePath); + return snapshot ?? { servers: {}, idpSessions: {} }; } /** @@ -442,36 +440,95 @@ 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. + // Persist the rotated tokens back under the same key via the shared + // sectioned write: lock → fresh read → overlay just this server's entry → + // atomic write. This generalizes the read-modify-write this function used + // to hand-roll — the merge now happens against the file as it is at write + // time (not the snapshot read before the network round-trip), under the + // same cross-process lock every other writer uses, and keeps the file's + // owner-only `0o600` mode + `mkdir -p` via the shared store IO. servers[found.key] = { ...found.state, tokens }; - await writeStoreFile(statePath, serializeOAuthPersistBlob(snapshot)); + await writeOAuthSections(statePath, snapshot, { servers: [found.key] }); return tokens.access_token; } +/** Sentinel: the wait deadline elapsed while a read was still in flight. */ +const DEADLINE_ELAPSED = Symbol("deadline-elapsed"); + +/** + * Race `promise` against the absolute `deadline` (epoch ms). Resolves with + * {@link DEADLINE_ELAPSED} if the deadline passes first; the abandoned + * promise's eventual rejection is swallowed (a lock-acquisition failure + * landing after abandonment must not become an unhandled rejection). + */ +async function raceDeadline( + promise: Promise, + deadline: number, +): Promise { + promise.catch(() => {}); + const remaining = deadline - Date.now(); + if (remaining <= 0) return DEADLINE_ELAPSED; + let timer: NodeJS.Timeout | undefined; + const elapsed = new Promise((resolve) => { + timer = setTimeout(() => resolve(DEADLINE_ELAPSED), remaining); + }); + try { + return await Promise.race([promise, elapsed]); + } finally { + clearTimeout(timer); + } +} + /** * Poll the OAuth state file until a token for `serverUrl` appears (or the * timeout elapses). Used by `--wait-for-auth` so an automated caller can hand * off to a human for the OAuth dance and resume once the token lands. The * lookup is normalised, so a trailing-slash mismatch between the URL the human * opened and the one the agent passed still resolves. + * + * Read-failure policy: a held lock ({@link SecretFileLockHeldError}) is the + * success case in progress — the browser flow this waits on *writes* the same + * state file under the same lock — so it is never treated as an error here. + * Any other read failure keeps the loop polling (the store may heal mid-wait, + * e.g. a keychain unlocking), but is retained so a deadline hit rethrows the + * real operational problem — `classifyError` maps it to its own envelope — + * instead of masking it as `auth_wait_timeout`, which re-authorizing cannot + * fix. A later successful read clears the retained error. + * + * The deadline bounds the *whole* loop, reads included: a single read can + * block for the state-file lock's full acquisition budget (~15s, see + * `RETRY_BUDGET_MS` in core/auth/node/file-lock.ts), which would let + * `--wait-for-auth 1` run fifteen times past its own deadline. Each read is + * raced against the remaining budget ({@link raceDeadline}) and abandoned + * when it elapses — safe, because the read's only side effect (lazy + * plaintext migration) is atomic under the file lock, and the process is + * about to exit through `handleError` anyway. */ -async function waitForStoredToken( +export async function waitForStoredToken( serverUrl: string, statePath: string, timeoutSec: number, + readServers: (statePath: string) => Promise = readOAuthServers, ): Promise { const key = normalizeServerUrl(serverUrl); const deadline = Date.now() + timeoutSec * 1000; + let servers: StoredServers = {}; + let lastError: unknown; for (;;) { - const servers = await readOAuthServers(statePath); + try { + const read = await raceDeadline(readServers(statePath), deadline); + if (read !== DEADLINE_ELAPSED) { + servers = read; + lastError = undefined; + } + } catch (error) { + if (!(error instanceof SecretFileLockHeldError)) lastError = error; + } const token = findStoredToken(servers, serverUrl); if (token) return token; if (Date.now() >= deadline) { + if (lastError !== undefined) throw lastError; const stored = Object.keys(servers); throw new CliExitCodeError( EXIT_CODES.AUTH_REQUIRED, @@ -482,7 +539,9 @@ async function waitForStoredToken( { code: "auth_wait_timeout", url: serverUrl }, ); } - await new Promise((r) => setTimeout(r, 500)); + await new Promise((r) => + setTimeout(r, Math.min(500, deadline - Date.now())), + ); } } diff --git a/clients/cli/src/error-handler.ts b/clients/cli/src/error-handler.ts index 9a1d286208..7d667084a7 100644 --- a/clients/cli/src/error-handler.ts +++ b/clients/cli/src/error-handler.ts @@ -1,5 +1,8 @@ import { redactUrlQuery } from "@inspector/core/mcp/fetchTracking.js"; import { awaitableError } from "./utils/awaitable-log.js"; +import { isUnauthorizedError } from "@inspector/core/auth/index.js"; +import { SecretStoreUnavailableError } from "@inspector/core/auth/node/secret-store.js"; +import { OAuthStateFileUnrecognizedError } from "@inspector/core/auth/node/oauth-persist-file.js"; /** * Exit-code map. Non-zero codes let an automated caller (CI, an agent) branch @@ -240,14 +243,50 @@ function classifyUnredacted( }; } - // 401 / OAuth-required → AUTH_REQUIRED so the caller can kick the auth flow. - if ( - status === 401 || - status === 403 || - /WWW-Authenticate|Unauthorized|invalid_token|OAuth/i.test( - message + " " + (cause ?? ""), - ) - ) { + // Secret-store / OAuth-state-lock failures are operational, not auth: the + // credentials may well exist but could not be read (keychain unreachable, + // state file locked by another Inspector process, unreadable secrets + // file). Reporting them as auth_required would say "re-authorize" for a + // failure re-authorizing cannot fix. The web path preserves the same + // distinction as a 503. + if (error instanceof SecretStoreUnavailableError) { + return { + exitCode: EXIT_CODES.USAGE, + envelope: { + code: "store_unavailable", + message, + ...(cause !== undefined && { cause }), + ...(url !== undefined && { url }), + }, + }; + } + + // A present-but-unrecognized OAuth state file also is not an auth + // failure — the file must be repaired (or deleted), so it gets a code of + // its own: unlike store_unavailable, retrying will not help. + if (error instanceof OAuthStateFileUnrecognizedError) { + return { + exitCode: EXIT_CODES.USAGE, + envelope: { + code: "oauth_state_unrecognized", + message, + ...(cause !== undefined && { cause }), + ...(url !== undefined && { url }), + }, + }; + } + + // 401/403 or a typed SDK auth error → AUTH_REQUIRED so the caller can kick + // the auth flow. `isUnauthorizedError` is the same detector cliOAuth.ts + // uses: `UnauthorizedError.isInstance`, a structured 401 status/code + // anywhere in the cause chain, and the remote transport's "failed …(401)" + // wording. Every genuine auth-required condition in the SDK throws typed + // `UnauthorizedError` or carries a structured 401 — this replaced a + // keyword sniff (/WWW-Authenticate|Unauthorized|invalid_token|OAuth/i) + // whose terms matched no real thrower while "OAuth" misclassified + // ordinary operational errors ("OAuth storage is required…", "HTTP 500 + // trying to load well-known OAuth metadata") as "re-authorize". + if (status === 401 || status === 403 || isUnauthorizedError(error)) { return { exitCode: EXIT_CODES.AUTH_REQUIRED, envelope: { diff --git a/clients/cli/vitest.config.ts b/clients/cli/vitest.config.ts index 49217c73a8..f77d61d588 100644 --- a/clients/cli/vitest.config.ts +++ b/clients/cli/vitest.config.ts @@ -17,6 +17,11 @@ export default defineConfig({ environment: "node", include: ["__tests__/**/*.test.ts"], setupFiles: ["__tests__/helpers/mock-open-url.ts", NO_RETRY_SETUP], + // OAuth tokens/client secrets are split into the selected secret store by + // the file persistence backend the CLI shares with the web server. Pin the + // in-memory store so stored-auth tests never probe or write the real OS + // keychain on a dev machine. + env: { MCP_INSPECTOR_SECRET_STORE: "memory" }, // Shared budgets (#2323). `testTimeout` was already 15000 here by hand; // the hook and teardown budgets were Vitest's defaults until now. ...TIMEOUTS, diff --git a/clients/web/src/test/core/auth/oauth-persist-file.test.ts b/clients/web/src/test/core/auth/oauth-persist-file.test.ts new file mode 100644 index 0000000000..eab3ea482d --- /dev/null +++ b/clients/web/src/test/core/auth/oauth-persist-file.test.ts @@ -0,0 +1,323 @@ +/** + * Unit tests for the file persist backend's lock-failure handling: when the + * cross-process lock cannot be acquired, `withSecretFileLock` throws a + * `SecretFileLockHeldError` whose message talks about "the secrets file" + * (its other caller) — the OAuth write path must rethrow with OAuth wording + * so the operator looks at the right file, keeping the original as `cause`. + * The lock is mocked because a genuinely stuck lock takes ~15s of retries. + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { + KeychainUnavailableError, + SecretFileLockHeldError, +} from "@inspector/core/auth/node/secret-store.js"; + +vi.mock("@inspector/core/auth/node/file-lock.js", () => ({ + withSecretFileLock: vi.fn(), +})); + +import { withSecretFileLock } from "@inspector/core/auth/node/file-lock.js"; +import { + readOAuthStore, + removeOAuthStore, + writeOAuthSections, +} from "@inspector/core/auth/node/oauth-persist-file.js"; +import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store.js"; +import { oauthSecretServerId } from "@inspector/core/auth/node/oauth-secrets.js"; +import { mkdtemp, readFile } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +const SNAPSHOT = { servers: {}, idpSessions: {} }; + +describe("writeOAuthSections lock failures", () => { + beforeEach(() => { + vi.mocked(withSecretFileLock).mockReset(); + }); + + it("rethrows SecretFileLockHeldError with OAuth wording and cause", async () => { + const original = new SecretFileLockHeldError( + "Could not lock the secrets file", + ); + vi.mocked(withSecretFileLock).mockRejectedValue(original); + + const rejection = writeOAuthSections("/tmp/oauth.json", SNAPSHOT, { + servers: ["s"], + }); + await expect(rejection).rejects.toMatchObject({ + message: expect.stringContaining( + "Could not save OAuth state: the state file at /tmp/oauth.json is locked", + ), + cause: original, + }); + // The subclass must survive the rewording: it is what the HTTP layer + // maps to a retryable 503 — a plain Error would demote it to a 500. + await expect(rejection).rejects.toBeInstanceOf(SecretFileLockHeldError); + }); + + it("passes secret-store failures through untouched", async () => { + // A KeychainUnavailableError thrown inside the locked callback is a + // store failure, not a lock failure — rewrapping it as "the file is + // locked" would lose the type the HTTP layer maps to a 503. + const original = new KeychainUnavailableError(new Error("keychain down")); + vi.mocked(withSecretFileLock).mockRejectedValue(original); + + await expect( + writeOAuthSections("/tmp/oauth.json", SNAPSHOT, { servers: ["s"] }), + ).rejects.toBe(original); + }); + + it("passes other errors through untouched", async () => { + const original = new Error("disk exploded"); + vi.mocked(withSecretFileLock).mockRejectedValue(original); + + await expect( + writeOAuthSections("/tmp/oauth.json", SNAPSHOT, { servers: ["s"] }), + ).rejects.toBe(original); + }); + + it("does not reword a nested secrets-file lock error from inside the callback", async () => { + // The nested FileSecretStore takes its own lock on secrets.json while + // this callback runs. If *that* lock is contended, the error escaping + // here already names the actually-contended file — rewording it as + // "the state file at …oauth.json is locked" would direct the user at + // the wrong file. Only acquisition failures (the mocks above, which + // reject before the callback runs) get the OAuth wording. + vi.mocked(withSecretFileLock).mockImplementation( + async (_path, fn) => fn() as Promise, + ); + const original = new SecretFileLockHeldError( + "Could not lock the secrets file at /home/u/.mcp-inspector/secrets.json", + ); + const store = new InMemorySecretStore(); + store.deleteAllForServer = async () => { + throw original; + }; + + // An empty snapshot with a named section deletes that entry's store + // fields — the first store mutation the callback makes. + await expect( + writeOAuthSections( + "/tmp/does-not-exist-oauth.json", + SNAPSHOT, + { servers: ["https://s.example/mcp"] }, + store, + ), + ).rejects.toBe(original); + }); +}); + +describe("removeOAuthStore lock failures", () => { + beforeEach(() => { + vi.mocked(withSecretFileLock).mockReset(); + }); + + it("runs under the file lock and rethrows lock failures with remove wording", async () => { + const original = new SecretFileLockHeldError( + "Could not lock the secrets file", + ); + vi.mocked(withSecretFileLock).mockRejectedValue(original); + + await expect(removeOAuthStore("/tmp/oauth.json")).rejects.toMatchObject({ + message: expect.stringContaining( + "Could not remove OAuth state: the state file at /tmp/oauth.json is locked", + ), + cause: original, + }); + expect(vi.mocked(withSecretFileLock)).toHaveBeenCalledWith( + "/tmp/oauth.json", + expect.any(Function), + ); + }); +}); + +describe("readOAuthStore locking", () => { + beforeEach(() => { + vi.mocked(withSecretFileLock).mockReset(); + }); + + it("runs the file read and store join under the file lock", async () => { + // The torn-read guard: an unlocked reader could join a writer's old + // residue with its already-committed new secrets. The whole read must + // execute inside the same lock the writers hold. + vi.mocked(withSecretFileLock).mockImplementation( + async (_path, fn) => fn() as Promise, + ); + + const result = await readOAuthStore( + "/tmp/does-not-exist-oauth.json", + new InMemorySecretStore(), + ); + + expect(result).toBeNull(); + expect(vi.mocked(withSecretFileLock)).toHaveBeenCalledWith( + "/tmp/does-not-exist-oauth.json", + expect.any(Function), + ); + }); + + it("rethrows lock failures with read wording, keeping the 503 type", async () => { + const original = new SecretFileLockHeldError( + "Could not lock the secrets file", + ); + vi.mocked(withSecretFileLock).mockRejectedValue(original); + + const rejection = readOAuthStore( + "/tmp/oauth.json", + new InMemorySecretStore(), + ); + await expect(rejection).rejects.toMatchObject({ + message: expect.stringContaining( + "Could not read OAuth state: the state file at /tmp/oauth.json is locked", + ), + cause: original, + }); + await expect(rejection).rejects.toBeInstanceOf(SecretFileLockHeldError); + }); + + it("passes non-lock read failures through untouched", async () => { + const original = new KeychainUnavailableError(new Error("keychain down")); + vi.mocked(withSecretFileLock).mockRejectedValue(original); + + await expect( + readOAuthStore("/tmp/oauth.json", new InMemorySecretStore()), + ).rejects.toBe(original); + }); +}); + +describe("persistEntrySecrets partial-commit compensation", () => { + beforeEach(() => { + vi.mocked(withSecretFileLock).mockReset(); + vi.mocked(withSecretFileLock).mockImplementation( + async (_path, fn) => fn() as Promise, + ); + }); + + const url = "https://s.example/mcp"; + const NEW_STATE = { + tokens: { + access_token: "new-at", + refresh_token: "new-rt", + token_type: "Bearer", + }, + clientInformation: { client_id: "new-cid", client_secret: "new-cs" }, + }; + const SEED_STATE = { + tokens: { access_token: "old-at", token_type: "Bearer" }, + clientInformation: { client_id: "old-cid", client_secret: "old-cs" }, + }; + const OLD_TOKENS = JSON.stringify(SEED_STATE.tokens); + + /** Seed a real prior entry — residue on disk, secrets in the store — then + * make `set` reject per `failWhen`. The bulk-set fallback settles siblings + * before rethrowing, so a selective failure produces a real partial + * commit. */ + const seededStore = async ( + file: string, + failWhen: (field: string, value: string) => boolean, + ) => { + const store = new InMemorySecretStore(); + const serverId = oauthSecretServerId(url); + await writeOAuthSections( + file, + { servers: { [url]: SEED_STATE }, idpSessions: {} }, + { servers: [url] }, + store, + ); + const realSet = store.set.bind(store); + store.set = async (sid: string, field: string, value: string) => { + if (failWhen(field, value)) + throw new KeychainUnavailableError(new Error("keychain flaked")); + return realSet(sid, field, value); + }; + return { store, serverId }; + }; + + it("degrades to the consistent prior pair after a partial bulk-set commit", async () => { + // One sibling set lands ("tokens") while another fails ("client-secret"). + // The store side is restored to the pre-write values — and the file must + // keep the *prior* residue too: committing the new residue over restored + // old secrets would pair the re-registered client_id with the old + // client_secret, a credential pair that never existed. File and store + // change together or not at all; the new credentials stay memory-only. + const dir = await mkdtemp(join(tmpdir(), "oauth-persist-partial-")); + const file = join(dir, "oauth.json"); + const { store, serverId } = await seededStore( + file, + (field, value) => field === "client-secret" && value === "new-cs", + ); + + await writeOAuthSections( + file, + { servers: { [url]: NEW_STATE }, idpSessions: {} }, + { servers: [url] }, + store, + ); + + expect(await store.get(serverId, "tokens")).toBe(OLD_TOKENS); + expect(await store.get(serverId, "client-secret")).toBe("old-cs"); + const written = await readFile(file, "utf8"); + const parsed = JSON.parse(written) as { + servers: Record; + }; + expect(parsed.servers[url]?.clientInformation?.client_id).toBe("old-cid"); + for (const leak of ["new-cid", "new-cs", "new-at", "new-rt"]) { + expect(written).not.toContain(leak); + } + }); + + it("aborts the file write when the compensation cannot be confirmed", async () => { + // The failing field's prior value existed, so its restore goes through + // `set` — which is still down. An unconfirmed restore leaves the store + // in an unknown state; committing anything over it would be a guess, + // so the write must abort and surface the store failure. + const dir = await mkdtemp(join(tmpdir(), "oauth-persist-abort-")); + const file = join(dir, "oauth.json"); + const { store, serverId } = await seededStore( + file, + (field) => field === "client-secret", + ); + const before = await readFile(file, "utf8"); + + await expect( + writeOAuthSections( + file, + { servers: { [url]: NEW_STATE }, idpSessions: {} }, + { servers: [url] }, + store, + ), + ).rejects.toBeInstanceOf(KeychainUnavailableError); + + expect(await readFile(file, "utf8")).toBe(before); + // The sibling that landed was still rolled back before the abort. + expect(await store.get(serverId, "tokens")).toBe(OLD_TOKENS); + expect(await store.get(serverId, "client-secret")).toBe("old-cs"); + }); + + it("drops a brand-new entry from the write when its secrets could not persist", async () => { + // The disk never had this entry, so after the degradation there is no + // prior pair to keep — committing any residue would index secrets the + // store does not hold. The entry is removed from the write entirely + // (restore of never-present fields is a delete, which succeeds, so the + // write itself still goes through). + const dir = await mkdtemp(join(tmpdir(), "oauth-persist-new-entry-")); + const file = join(dir, "oauth.json"); + const store = new InMemorySecretStore(); + store.set = async () => { + throw new KeychainUnavailableError(new Error("keychain down")); + }; + + await writeOAuthSections( + file, + { servers: { [url]: NEW_STATE }, idpSessions: {} }, + { servers: [url] }, + store, + ); + + const parsed = JSON.parse(await readFile(file, "utf8")) as { + servers: Record; + }; + expect(parsed.servers).toEqual({}); + }); +}); diff --git a/clients/web/src/test/core/auth/oauth-persist.test.ts b/clients/web/src/test/core/auth/oauth-persist.test.ts index f55b37b094..14459d7d69 100644 --- a/clients/web/src/test/core/auth/oauth-persist.test.ts +++ b/clients/web/src/test/core/auth/oauth-persist.test.ts @@ -2,6 +2,10 @@ import { describe, it, expect, vi } from "vitest"; import { parseOAuthPersistBlob, serializeOAuthPersistBlob, + mergeOAuthSections, + parseOAuthPersistSections, + parseOAuthStoreWriteBody, + serializeOAuthSectionedWrite, createRemoteOAuthPersistBackend, createSessionOAuthPersistBackend, OAUTH_PERSIST_STORAGE_KEY, @@ -68,6 +72,172 @@ describe("parseOAuthPersistBlob", () => { idpSessions: {}, }); }); + + it("rejects malformed entry maps instead of coercing them", () => { + // `{ servers: ["bad"] }` used to be accepted and then coerced into + // nonsensical entries downstream; a map that is not a record of records + // must reject the whole payload (400 on the route, unreadable on disk). + expect( + parseOAuthPersistBlob({ servers: ["bad"], idpSessions: {} }), + ).toBeNull(); + expect(parseOAuthPersistBlob({ servers: "nope" })).toBeNull(); + expect( + parseOAuthPersistBlob({ idpSessions: { issuer: "scalar" } }), + ).toBeNull(); + expect( + parseOAuthPersistBlob({ state: { servers: ["bad"] }, version: 0 }), + ).toBeNull(); + }); + + it("rejects non-string verbatim secret fields that would poison the store", () => { + // `client_secret` / `registration_access_token` pass through the split + // into the secret store *verbatim* (every other secret is stringified + // first), and one non-string value in secrets.json makes the store + // refuse the entire file — corrupting unrelated servers' credentials. + const entry = (clientInformation: unknown) => ({ + servers: { "http://s": { clientInformation } }, + idpSessions: {}, + }); + expect( + parseOAuthPersistBlob(entry({ client_id: "x", client_secret: 123 })), + ).toBeNull(); + expect( + parseOAuthPersistBlob( + entry({ client_id: "x", registration_access_token: { a: 1 } }), + ), + ).toBeNull(); + // Non-record containers are malformed state, not credentials. + expect(parseOAuthPersistBlob(entry("not a record"))).toBeNull(); + expect( + parseOAuthPersistBlob({ + servers: { + "http://s": { + preregisteredClientInformation: { + client_id: "x", + client_secret: null, + }, + }, + }, + idpSessions: {}, + }), + ).toBeNull(); + expect( + parseOAuthPersistBlob({ + servers: { + "http://s": { + byIssuer: { + "https://as": { + clientInformation: { client_id: "x", client_secret: 5 }, + }, + }, + }, + }, + idpSessions: {}, + }), + ).toBeNull(); + // A byIssuer slot that is not a record rejects too. + expect( + parseOAuthPersistBlob({ + servers: { "http://s": { byIssuer: { "https://as": "scalar" } } }, + idpSessions: {}, + }), + ).toBeNull(); + // String secrets — including under a __proto__ issuer key — stay valid. + const valid = JSON.parse( + '{"servers":{"http://s":{"clientInformation":{"client_id":"x","client_secret":"s3cret"},"byIssuer":{"__proto__":{"clientInformation":{"client_id":"y","client_secret":"also"}}}}},"idpSessions":{}}', + ) as Record; + expect(parseOAuthPersistBlob(valid)).not.toBeNull(); + }); + + it("rejects API write bodies with token payloads the read path would silently drop", () => { + // The split stringifies whatever `tokens` holds into the store, so a + // type-corrupt payload would be accepted with apparent success and then + // dropped when the join validates before serving. An untrusted write + // gets a 400 instead. Partial shapes are legitimate (see the acceptance + // cases below): only present-but-mistyped fields reject. + expect( + parseOAuthStoreWriteBody({ + servers: { "http://s": { tokens: { access_token: 123 } } }, + idpSessions: {}, + }), + ).toBeNull(); + expect( + parseOAuthStoreWriteBody({ + sections: { servers: ["http://s"] }, + snapshot: { + servers: { + "http://s": { + byIssuer: { + "https://as": { tokens: { refresh_token: 42 } }, + }, + }, + }, + idpSessions: {}, + }, + }), + ).toBeNull(); + // IdP session secret fields: the join extracts only string-typed + // `idToken` / `refreshToken`, so a non-string would be dropped the same + // way. + expect( + parseOAuthStoreWriteBody({ + servers: {}, + idpSessions: { "https://idp": { idToken: 42 } }, + }), + ).toBeNull(); + expect( + parseOAuthStoreWriteBody({ + servers: {}, + idpSessions: { "https://idp": { refreshToken: { a: 1 } } }, + }), + ).toBeNull(); + // Valid tokens — including the SEP-2352 issuer stamp the schema strips — + // and string IdP fields stay accepted. So do partial token shapes: a + // legacy plaintext file can hold a refresh-only entry that the join + // serves from the residue, so a GET can return it and a client echoing + // that state back must not be refused. + expect( + parseOAuthStoreWriteBody({ + servers: { + "http://s": { + tokens: { refresh_token: "rt", token_type: "Bearer" }, + }, + }, + idpSessions: {}, + }), + ).not.toBeNull(); + expect( + parseOAuthStoreWriteBody({ + servers: { + "http://s": { + tokens: { + access_token: "at", + token_type: "Bearer", + issuer: "https://as", + }, + byIssuer: { + "https://as": { + tokens: { access_token: "at2", token_type: "Bearer" }, + }, + }, + }, + }, + idpSessions: { + "https://idp": { idToken: "idt", idTokenExpiresAt: 123 }, + }, + }), + ).not.toBeNull(); + // File reads stay tolerant on purpose: a corrupt token entry in + // oauth.json must remain readable so it can be cleared / re-authorized, + // not brick every mutation of the file. (The store is never at risk — + // token payloads are JSON-stringified, unlike verbatim client_secret.) + expect( + parseOAuthPersistBlob({ + servers: { "http://s": { tokens: { access_token: 123 } } }, + idpSessions: { "https://idp": { idToken: 42 } }, + }), + ).not.toBeNull(); + }); }); describe("serializeOAuthPersistBlob", () => { @@ -83,16 +253,219 @@ describe("serializeOAuthPersistBlob", () => { }); }); +describe("mergeOAuthSections", () => { + const disk: OAuthPersistSnapshot = { + servers: { + "http://a": { scope: "a-disk" }, + "http://b": { scope: "b-disk" }, + }, + idpSessions: { "https://idp1": { idToken: "disk-1" } }, + }; + + it("overlays only the named server entries, keeping the rest from disk", () => { + const snapshot: OAuthPersistSnapshot = { + // Stale memory: never saw http://b, has an outdated http://a it did not + // mutate — only the named entry may land. + servers: { "http://c": { scope: "c-mem" }, "http://a": { scope: "old" } }, + idpSessions: {}, + }; + const merged = mergeOAuthSections(disk, snapshot, { + servers: ["http://c"], + }); + expect(merged).toEqual({ + servers: { + "http://a": { scope: "a-disk" }, + "http://b": { scope: "b-disk" }, + "http://c": { scope: "c-mem" }, + }, + idpSessions: { "https://idp1": { idToken: "disk-1" } }, + }); + }); + + it("treats a named key absent from the snapshot as a deletion", () => { + const snapshot: OAuthPersistSnapshot = { servers: {}, idpSessions: {} }; + const merged = mergeOAuthSections(disk, snapshot, { + servers: ["http://a"], + idpSessions: ["https://idp1"], + }); + expect(merged).toEqual({ + servers: { "http://b": { scope: "b-disk" } }, + idpSessions: {}, + }); + }); + + it("overlays named idpSessions independently of servers", () => { + const snapshot: OAuthPersistSnapshot = { + servers: {}, + idpSessions: { + "https://idp1": { idToken: "mem-1" }, + "https://idp2": { idToken: "mem-2" }, + }, + }; + const merged = mergeOAuthSections(disk, snapshot, { + idpSessions: ["https://idp2"], + }); + expect(merged.servers).toEqual(disk.servers); + expect(merged.idpSessions).toEqual({ + "https://idp1": { idToken: "disk-1" }, + "https://idp2": { idToken: "mem-2" }, + }); + }); + + it("starts from an empty store when disk is null (first write)", () => { + const snapshot: OAuthPersistSnapshot = { + servers: { "http://a": { scope: "mem" } }, + idpSessions: {}, + }; + expect( + mergeOAuthSections(null, snapshot, { servers: ["http://a"] }), + ).toEqual({ + servers: { "http://a": { scope: "mem" } }, + idpSessions: {}, + }); + }); + + it("keeps a __proto__ key as an own entry instead of hitting the prototype setter", () => { + // JSON.parse produces "__proto__" as an own key; a plain assignment + // while merging would invoke the inherited setter, silently dropping + // the entry (and orphaning its already-split secrets). + const snapshot: OAuthPersistSnapshot = { + servers: JSON.parse('{"__proto__": {"scope": "evil-name"}}'), + idpSessions: JSON.parse('{"__proto__": {"idToken": "t"}}'), + }; + const merged = mergeOAuthSections(null, snapshot, { + servers: ["__proto__"], + idpSessions: ["__proto__"], + }); + expect(Object.hasOwn(merged.servers, "__proto__")).toBe(true); + expect(Object.hasOwn(merged.idpSessions, "__proto__")).toBe(true); + expect(Object.getPrototypeOf(merged.servers)).toBe(Object.prototype); + // Serialization must carry the entry. + expect(JSON.stringify(merged)).toContain("evil-name"); + }); + + it("propagates a clear of a __proto__ entry instead of resurrecting it", () => { + // After a clear, `snapshot.servers` is `{}` — a plain lookup for + // "__proto__" would return the inherited `Object.prototype`, turning + // the deletion into an update that re-creates an empty entry. + const diskWithProto: OAuthPersistSnapshot = { + servers: JSON.parse('{"__proto__": {"scope": "stale"}}'), + idpSessions: JSON.parse('{"__proto__": {"idToken": "stale"}}'), + }; + const merged = mergeOAuthSections( + diskWithProto, + { servers: {}, idpSessions: {} }, + { servers: ["__proto__"], idpSessions: ["__proto__"] }, + ); + expect(Object.hasOwn(merged.servers, "__proto__")).toBe(false); + expect(Object.hasOwn(merged.idpSessions, "__proto__")).toBe(false); + expect(JSON.stringify(merged)).not.toContain("stale"); + }); +}); + +describe("parseOAuthPersistSections", () => { + it("parses servers and idpSessions string arrays", () => { + expect( + parseOAuthPersistSections({ + servers: ["http://a"], + idpSessions: ["https://i"], + }), + ).toEqual({ servers: ["http://a"], idpSessions: ["https://i"] }); + }); + + it("accepts either key alone or an empty object", () => { + expect(parseOAuthPersistSections({ servers: [] })).toEqual({ + servers: [], + }); + expect(parseOAuthPersistSections({})).toEqual({}); + }); + + it("rejects non-objects and non-string-array values", () => { + expect(parseOAuthPersistSections("a string")).toBeNull(); + expect(parseOAuthPersistSections(null)).toBeNull(); + expect(parseOAuthPersistSections({ servers: "http://a" })).toBeNull(); + expect(parseOAuthPersistSections({ servers: [1] })).toBeNull(); + expect(parseOAuthPersistSections({ idpSessions: {} })).toBeNull(); + }); + + it("rejects unknown keys so a typo cannot become a silent no-op", () => { + expect(parseOAuthPersistSections({ server: ["http://a"] })).toBeNull(); + expect( + parseOAuthPersistSections({ servers: ["http://a"], extra: true }), + ).toBeNull(); + }); +}); + +describe("parseOAuthStoreWriteBody", () => { + const SNAP = { servers: {}, idpSessions: {} }; + + it("treats a bare blob as a full replacement", () => { + expect(parseOAuthStoreWriteBody(SNAP)).toEqual({ snapshot: SNAP }); + }); + + it("parses a { sections, snapshot } envelope", () => { + expect( + parseOAuthStoreWriteBody({ + sections: { servers: ["http://a"] }, + snapshot: SNAP, + }), + ).toEqual({ sections: { servers: ["http://a"] }, snapshot: SNAP }); + }); + + it("round-trips serializeOAuthSectionedWrite", () => { + const sections = { servers: ["http://a"] }; + expect( + parseOAuthStoreWriteBody( + JSON.parse(serializeOAuthSectionedWrite(SNAPSHOT, sections)), + ), + ).toEqual({ sections, snapshot: SNAPSHOT }); + }); + + it("rejects bad envelopes and non-OAuth bodies", () => { + // A `sections` key marks an envelope: a bad descriptor or missing + // snapshot must not fall back to a full replacement. + expect( + parseOAuthStoreWriteBody({ + sections: { servers: "nope" }, + snapshot: SNAP, + }), + ).toBeNull(); + expect(parseOAuthStoreWriteBody({ sections: { servers: [] } })).toBeNull(); + expect(parseOAuthStoreWriteBody({ someOtherStore: true })).toBeNull(); + expect(parseOAuthStoreWriteBody("not an object")).toBeNull(); + // Malformed maps inside either form reject the write, not coerce it. + expect(parseOAuthStoreWriteBody({ servers: ["bad"] })).toBeNull(); + expect( + parseOAuthStoreWriteBody({ + sections: { servers: ["http://a"] }, + snapshot: { servers: ["bad"], idpSessions: {} }, + }), + ).toBeNull(); + }); + + it("rejects an envelope carrying unknown keys", () => { + expect( + parseOAuthStoreWriteBody({ + sections: { servers: ["http://a"] }, + snapshot: SNAP, + extra: 1, + }), + ).toBeNull(); + }); +}); + describe("createRemoteOAuthPersistBackend", () => { const baseUrl = "http://remote.example/"; - const storeId = "oauth"; + // The backend is pinned to the OAuth store: only /api/storage/oauth gives + // the sectioned-write envelope locked-merge semantics; a configurable id + // would let a caller store the envelope verbatim in a generic store, where + // the next read would fail to parse it. const url = "http://remote.example/api/storage/oauth"; it("read() returns the parsed snapshot and sends the auth header", async () => { const fetchFn = vi.fn(async () => jsonResponse(SNAPSHOT)); const backend = createRemoteOAuthPersistBackend({ baseUrl, - storeId, authToken: "tok", fetchFn: fetchFn as unknown as typeof fetch, }); @@ -106,7 +479,6 @@ describe("createRemoteOAuthPersistBackend", () => { it("read() returns null for the empty-object missing-file response", async () => { const backend = createRemoteOAuthPersistBackend({ baseUrl, - storeId, fetchFn: (async () => jsonResponse({})) as unknown as typeof fetch, }); expect(await backend.read()).toBeNull(); @@ -115,7 +487,6 @@ describe("createRemoteOAuthPersistBackend", () => { it("read() returns null on 404 and throws on other errors", async () => { const notFound = createRemoteOAuthPersistBackend({ baseUrl, - storeId, fetchFn: (async () => new Response("", { status: 404 })) as unknown as typeof fetch, }); @@ -123,7 +494,6 @@ describe("createRemoteOAuthPersistBackend", () => { const failing = createRemoteOAuthPersistBackend({ baseUrl, - storeId, fetchFn: (async () => new Response("", { status: 500 })) as unknown as typeof fetch, }); @@ -138,7 +508,6 @@ describe("createRemoteOAuthPersistBackend", () => { }); const backend = createRemoteOAuthPersistBackend({ baseUrl, - storeId, fetchFn: ok, }); await backend.write(SNAPSHOT); @@ -150,7 +519,6 @@ describe("createRemoteOAuthPersistBackend", () => { const failing = createRemoteOAuthPersistBackend({ baseUrl, - storeId, fetchFn: (async () => new Response("", { status: 500 })) as unknown as typeof fetch, }); @@ -159,10 +527,34 @@ describe("createRemoteOAuthPersistBackend", () => { ); }); + it("write() with sections carries them in the body envelope", async () => { + let capturedUrl: string | undefined; + let capturedBody: string | undefined; + const fetchFn = vi.fn(async (input, init) => { + capturedUrl = String(input); + capturedBody = init?.body as string | undefined; + return new Response("", { status: 200 }); + }); + const backend = createRemoteOAuthPersistBackend({ + baseUrl, + fetchFn, + }); + const sections = { servers: ["http://s"] }; + await backend.write(SNAPSHOT, sections); + // In the body, not the URL: a descriptor naming many server URLs + // would otherwise exceed Node's request-target limit. + const parsed = new URL(capturedUrl ?? ""); + expect(parsed.pathname).toBe("/api/storage/oauth"); + expect(parsed.search).toBe(""); + expect(JSON.parse(capturedBody ?? "")).toEqual({ + sections, + snapshot: SNAPSHOT, + }); + }); + it("remove() DELETEs, tolerates 404, and throws on other errors", async () => { const ok = createRemoteOAuthPersistBackend({ baseUrl, - storeId, authToken: "tok", fetchFn: (async () => new Response("", { status: 200 })) as unknown as typeof fetch, @@ -171,7 +563,6 @@ describe("createRemoteOAuthPersistBackend", () => { const gone = createRemoteOAuthPersistBackend({ baseUrl, - storeId, fetchFn: (async () => new Response("", { status: 404 })) as unknown as typeof fetch, }); @@ -179,7 +570,6 @@ describe("createRemoteOAuthPersistBackend", () => { const failing = createRemoteOAuthPersistBackend({ baseUrl, - storeId, fetchFn: (async () => new Response("", { status: 500 })) as unknown as typeof fetch, }); diff --git a/clients/web/src/test/core/auth/oauth-secrets.test.ts b/clients/web/src/test/core/auth/oauth-secrets.test.ts new file mode 100644 index 0000000000..b47ae2d89f --- /dev/null +++ b/clients/web/src/test/core/auth/oauth-secrets.test.ts @@ -0,0 +1,520 @@ +/** + * Unit tests for the pure OAuth secret split/join helpers and the + * MCP_INSPECTOR_PERSIST_TOKENS policy (core/auth/node/oauth-secrets.ts). + */ + +import { describe, it, expect, vi, afterEach } from "vitest"; +import { + PERSIST_TOKENS_ENV, + getPersistTokensPolicy, + resetPersistTokensPolicyWarnings, + oauthSecretServerId, + oauthIdpSecretServerId, + issuerTokensField, + issuerClientSecretField, + issuerRegistrationTokenField, + LEGACY_TOKENS_FIELD, + LEGACY_CLIENT_SECRET_FIELD, + LEGACY_REGISTRATION_TOKEN_FIELD, + PREREG_CLIENT_SECRET_FIELD, + PREREG_REGISTRATION_TOKEN_FIELD, + IDP_SESSION_FIELD, + splitServerOAuthState, + joinServerOAuthState, + splitIdpSession, + joinIdpSession, + serverSecretFields, + snapshotHasPlaintextSecrets, +} from "@inspector/core/auth/node/oauth-secrets.js"; +import type { ServerOAuthState } from "@inspector/core/auth/store.js"; +import type { OAuthTokens } from "@modelcontextprotocol/client"; +import type { OAuthPersistSnapshot } from "@inspector/core/auth/oauth-persist.js"; + +const TOKENS = { + access_token: "at", + token_type: "Bearer", + refresh_token: "rt", +} as const; + +afterEach(() => { + resetPersistTokensPolicyWarnings(); + vi.restoreAllMocks(); +}); + +describe("getPersistTokensPolicy", () => { + it("defaults to 'all' when unset or empty", () => { + expect(getPersistTokensPolicy({})).toBe("all"); + expect(getPersistTokensPolicy({ [PERSIST_TOKENS_ENV]: "" })).toBe("all"); + }); + + it("accepts the three valid values", () => { + for (const v of ["all", "access", "none"] as const) { + expect(getPersistTokensPolicy({ [PERSIST_TOKENS_ENV]: v })).toBe(v); + } + }); + + it("treats an invalid value as 'all' and warns once per value", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const env = { [PERSIST_TOKENS_ENV]: "nope" }; + expect(getPersistTokensPolicy(env)).toBe("all"); + expect(getPersistTokensPolicy(env)).toBe("all"); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0]![0]).toContain(PERSIST_TOKENS_ENV); + // A different invalid value warns again; the reset seam clears the memory. + expect(getPersistTokensPolicy({ [PERSIST_TOKENS_ENV]: "other" })).toBe( + "all", + ); + expect(warn).toHaveBeenCalledTimes(2); + resetPersistTokensPolicyWarnings(); + expect(getPersistTokensPolicy(env)).toBe("all"); + expect(warn).toHaveBeenCalledTimes(3); + }); +}); + +describe("id and field schemes", () => { + it("namespaces store ids and issuer fields", () => { + expect(oauthSecretServerId("https://s.example/mcp")).toBe( + "oauth+https%3A%2F%2Fs.example%2Fmcp", + ); + expect(oauthIdpSecretServerId("https://idp.example")).toBe( + "oauth-idp+https%3A%2F%2Fidp.example", + ); + expect(issuerTokensField("https://as.example")).toBe( + "tokens:https://as.example", + ); + expect(issuerClientSecretField("https://as.example")).toBe( + "client-secret:https://as.example", + ); + }); + + it("store ids are colon-free and prefix-unambiguous", () => { + // Accounts are `serverId:field` and the keyring's deleteAllForServer + // parses at the FIRST colon — a colon inside the id would make purges + // never match (tokens left in the OS keychain forever). + expect(oauthSecretServerId("https://s.example:8443/mcp")).not.toContain( + ":", + ); + expect(oauthIdpSecretServerId("https://idp.example:8443")).not.toContain( + ":", + ); + // A raw-URL id would be a prefix of its port-qualified sibling, letting + // prefix-matching stores purge the wrong server's secrets. + const plain = `${oauthSecretServerId("https://a.example")}:`; + const withPort = `${oauthSecretServerId("https://a.example:8080")}:tokens`; + expect(withPort.startsWith(plain)).toBe(false); + }); +}); + +describe("splitServerOAuthState", () => { + it("moves legacy tokens and client secrets to the store, keeps residue", () => { + const state: ServerOAuthState = { + scope: "read", + codeVerifier: "cv", + tokens: { ...TOKENS }, + clientInformation: { client_id: "cid", client_secret: "cs" }, + preregisteredClientInformation: { + client_id: "pre", + client_secret: "pre-cs", + }, + }; + const { residue, secrets } = splitServerOAuthState(state, "all"); + expect(residue.tokens).toBeUndefined(); + expect(residue.scope).toBe("read"); + expect(residue.codeVerifier).toBe("cv"); + expect(residue.clientInformation).toEqual({ client_id: "cid" }); + expect(residue.preregisteredClientInformation).toEqual({ + client_id: "pre", + }); + expect(JSON.parse(secrets[LEGACY_TOKENS_FIELD]!)).toEqual(TOKENS); + expect(secrets[LEGACY_CLIENT_SECRET_FIELD]).toBe("cs"); + expect(secrets[PREREG_CLIENT_SECRET_FIELD]).toBe("pre-cs"); + // Input untouched. + expect(state.tokens).toEqual(TOKENS); + expect(state.clientInformation!.client_secret).toBe("cs"); + }); + + it("splits per-issuer slots under their issuer-suffixed fields", () => { + const issuer = "https://as.example"; + const state: ServerOAuthState = { + activeIssuer: issuer, + byIssuer: { + [issuer]: { + tokens: { ...TOKENS }, + clientInformation: { client_id: "cid", client_secret: "cs" }, + }, + }, + }; + const { residue, secrets } = splitServerOAuthState(state, "all"); + expect(residue.byIssuer![issuer]!.tokens).toBeUndefined(); + expect(residue.byIssuer![issuer]!.clientInformation).toEqual({ + client_id: "cid", + }); + expect(JSON.parse(secrets[issuerTokensField(issuer)]!)).toEqual(TOKENS); + expect(secrets[issuerClientSecretField(issuer)]).toBe("cs"); + }); + + it("policy 'access' strips refresh tokens; 'none' drops tokens entirely", () => { + const state: ServerOAuthState = { + tokens: { ...TOKENS }, + clientInformation: { client_id: "cid", client_secret: "cs" }, + }; + const access = splitServerOAuthState(state, "access"); + expect(JSON.parse(access.secrets[LEGACY_TOKENS_FIELD]!)).toEqual({ + access_token: "at", + token_type: "Bearer", + }); + const none = splitServerOAuthState(state, "none"); + expect(none.secrets[LEGACY_TOKENS_FIELD]).toBeUndefined(); + // Client secrets are registration credentials, not acquired tokens — + // the policy does not affect them. + expect(none.secrets[LEGACY_CLIENT_SECRET_FIELD]).toBe("cs"); + }); + + it("passes a secretless state through with no secrets", () => { + const state: ServerOAuthState = { + scope: "read", + clientInformation: { client_id: "public-only" }, + }; + const { residue, secrets } = splitServerOAuthState(state, "all"); + expect(residue).toEqual(state); + expect(secrets).toEqual({}); + }); + + it("drops a post-policy token payload with no secret-bearing field", () => { + // Policy "access" applied to a refresh-only entry leaves only + // { token_type } — nothing worth preserving. Keeping it plaintext + // would plant a secretless artifact that lingers in the file forever. + const state: ServerOAuthState = { + tokens: { + refresh_token: "rt-only", + token_type: "Bearer", + } as unknown as OAuthTokens, + }; + const { residue, secrets } = splitServerOAuthState(state, "access"); + expect(secrets[LEGACY_TOKENS_FIELD]).toBeUndefined(); + expect(residue.tokens).toBeUndefined(); + }); + + it("moves a partial-but-legitimate token payload to the store", () => { + // Store write and read share the partial-schema contract, so a + // refresh-only payload belongs in the store like any full token set — + // never as plaintext in the residue. + const partial = { refresh_token: "rt-only", token_type: "Bearer" }; + const state: ServerOAuthState = { + tokens: { ...partial } as unknown as OAuthTokens, + }; + const { residue, secrets } = splitServerOAuthState(state, "all"); + expect(residue.tokens).toBeUndefined(); + expect(JSON.parse(secrets[LEGACY_TOKENS_FIELD]!)).toEqual(partial); + }); + + it("keeps a type-corrupt token payload in the residue, not the store", () => { + // The store must never hold junk the join cannot serve; the corrupt + // entry stays in the file, where it remains visible and clearable. + const corrupt = { access_token: 123, token_type: "Bearer" }; + const state: ServerOAuthState = { + tokens: { ...corrupt } as unknown as OAuthTokens, + }; + const { residue, secrets } = splitServerOAuthState(state, "all"); + expect(secrets[LEGACY_TOKENS_FIELD]).toBeUndefined(); + expect(residue.tokens).toEqual(corrupt); + }); +}); + +describe("joinServerOAuthState", () => { + it("rejoins tokens and client secrets, store wins over plaintext", () => { + const issuer = "https://as.example"; + const residue: ServerOAuthState = { + tokens: { access_token: "stale", token_type: "Bearer" }, + clientInformation: { client_id: "cid" }, + preregisteredClientInformation: { client_id: "pre" }, + byIssuer: { + [issuer]: { clientInformation: { client_id: "icid" } }, + }, + }; + const joined = joinServerOAuthState(residue, { + [LEGACY_TOKENS_FIELD]: JSON.stringify(TOKENS), + [LEGACY_CLIENT_SECRET_FIELD]: "cs", + [PREREG_CLIENT_SECRET_FIELD]: "pre-cs", + [issuerTokensField(issuer)]: JSON.stringify(TOKENS), + [issuerClientSecretField(issuer)]: "ics", + }); + expect(joined.tokens).toEqual(TOKENS); + expect(joined.clientInformation).toEqual({ + client_id: "cid", + client_secret: "cs", + }); + expect(joined.preregisteredClientInformation).toEqual({ + client_id: "pre", + client_secret: "pre-cs", + }); + expect(joined.byIssuer![issuer]).toEqual({ + clientInformation: { client_id: "icid", client_secret: "ics" }, + tokens: TOKENS, + }); + }); + + it("ignores orphaned secrets whose residue slot was cleared", () => { + const joined = joinServerOAuthState( + { scope: "read" }, + { + [LEGACY_CLIENT_SECRET_FIELD]: "orphan", + [PREREG_CLIENT_SECRET_FIELD]: "orphan", + }, + ); + expect(joined).toEqual({ scope: "read" }); + }); + + it("treats a corrupt stored-tokens entry as absent", () => { + const issuer = "https://as.example"; + const joined = joinServerOAuthState( + { byIssuer: { [issuer]: {} } }, + { + [LEGACY_TOKENS_FIELD]: "not json", + [issuerTokensField(issuer)]: JSON.stringify({ no_access_token: 1 }), + }, + ); + expect(joined.tokens).toBeUndefined(); + expect(joined.byIssuer![issuer]!.tokens).toBeUndefined(); + }); +}); + +describe("splitIdpSession / joinIdpSession", () => { + const session = { + idToken: "idt", + refreshToken: "rt", + idTokenExpiresAt: 123, + }; + + it("moves tokens to the store and keeps the expiry in the residue", () => { + const { residue, secrets } = splitIdpSession(session, "all"); + expect(residue).toEqual({ idTokenExpiresAt: 123 }); + expect(JSON.parse(secrets[IDP_SESSION_FIELD]!)).toEqual({ + idToken: "idt", + refreshToken: "rt", + }); + }); + + it("policy 'access' keeps the id token but drops the refresh token", () => { + const { secrets } = splitIdpSession(session, "access"); + expect(JSON.parse(secrets[IDP_SESSION_FIELD]!)).toEqual({ + idToken: "idt", + }); + }); + + it("policy 'none' stores nothing", () => { + expect(splitIdpSession(session, "none").secrets).toEqual({}); + }); + + it("stores nothing for a session with no tokens", () => { + expect(splitIdpSession({ idTokenExpiresAt: 5 }, "all").secrets).toEqual({}); + }); + + it("stringifies only string-typed fields: non-strings never reach the store", () => { + // parseStoredIdpSession extracts only string fields on read, so a + // non-string (corrupt data tolerated by the file parser) would be + // stored with apparent success and yield nothing. It is dropped here. + const corrupt = { + idToken: 42, + refreshToken: "rt", + idTokenExpiresAt: 5, + } as unknown as Parameters[0]; + const { residue, secrets } = splitIdpSession(corrupt, "all"); + expect(JSON.parse(secrets[IDP_SESSION_FIELD]!)).toEqual({ + refreshToken: "rt", + }); + expect(residue).toEqual({ idTokenExpiresAt: 5 }); + // Both fields corrupt: nothing usable, nothing stored. + const junk = { idToken: 42 } as unknown as Parameters< + typeof splitIdpSession + >[0]; + expect(splitIdpSession(junk, "all").secrets).toEqual({}); + }); + + it("rejoins, and tolerates absent or corrupt store entries", () => { + const residue = { idTokenExpiresAt: 123 }; + expect( + joinIdpSession(residue, { + [IDP_SESSION_FIELD]: JSON.stringify({ + idToken: "idt", + refreshToken: "rt", + }), + }), + ).toEqual(session); + expect(joinIdpSession(residue, {})).toEqual(residue); + expect(joinIdpSession(residue, { [IDP_SESSION_FIELD]: "corrupt" })).toEqual( + residue, + ); + expect(joinIdpSession(residue, { [IDP_SESSION_FIELD]: "42" })).toEqual( + residue, + ); + }); +}); + +describe("serverSecretFields", () => { + it("always includes the legacy/prereg fields, plus per-issuer pairs", () => { + expect(serverSecretFields(undefined).sort()).toEqual( + [ + LEGACY_TOKENS_FIELD, + LEGACY_CLIENT_SECRET_FIELD, + LEGACY_REGISTRATION_TOKEN_FIELD, + PREREG_CLIENT_SECRET_FIELD, + PREREG_REGISTRATION_TOKEN_FIELD, + ].sort(), + ); + const issuer = "https://as.example"; + expect(serverSecretFields({ byIssuer: { [issuer]: {} } })).toContain( + issuerTokensField(issuer), + ); + expect(serverSecretFields({ byIssuer: { [issuer]: {} } })).toContain( + issuerClientSecretField(issuer), + ); + expect(serverSecretFields({ byIssuer: { [issuer]: {} } })).toContain( + issuerRegistrationTokenField(issuer), + ); + }); +}); + +describe("snapshotHasPlaintextSecrets", () => { + const empty: OAuthPersistSnapshot = { servers: {}, idpSessions: {} }; + + it("detects each plaintext slot", () => { + const cases: OAuthPersistSnapshot[] = [ + { servers: { s: { tokens: { ...TOKENS } } }, idpSessions: {} }, + { + servers: { + s: { clientInformation: { client_id: "c", client_secret: "x" } }, + }, + idpSessions: {}, + }, + { + servers: { + s: { + preregisteredClientInformation: { + client_id: "c", + client_secret: "x", + }, + }, + }, + idpSessions: {}, + }, + { + servers: { s: { byIssuer: { i: { tokens: { ...TOKENS } } } } }, + idpSessions: {}, + }, + { + servers: { + s: { + byIssuer: { + i: { clientInformation: { client_id: "c", client_secret: "x" } }, + }, + }, + }, + idpSessions: {}, + }, + { servers: {}, idpSessions: { i: { idToken: "t" } } }, + { servers: {}, idpSessions: { i: { refreshToken: "t" } } }, + ]; + for (const snapshot of cases) { + expect(snapshotHasPlaintextSecrets(snapshot)).toBe(true); + } + }); + + it("is false for residue-only snapshots", () => { + expect(snapshotHasPlaintextSecrets(empty)).toBe(false); + expect( + snapshotHasPlaintextSecrets({ + servers: { + s: { + scope: "read", + clientInformation: { client_id: "public" }, + byIssuer: { i: { clientInformation: { client_id: "public" } } }, + }, + }, + idpSessions: { i: { idTokenExpiresAt: 1 } }, + }), + ).toBe(false); + }); +}); + +describe("registration_access_token split (RFC 7592)", () => { + // The DCR management credential rides inside clientInformation because DCR + // responses are saved whole. It is bearer-grade (maskSecrets.ts) and must + // never remain in the oauth.json residue — including when there is no + // client_secret alongside it, the shape that used to slip through. + const issuer = "https://as.example"; + + it("splits and rejoins per-issuer, with and without client_secret", () => { + const state: ServerOAuthState = { + byIssuer: { + [issuer]: { + clientInformation: { + client_id: "cid", + registration_access_token: "rat", + }, + }, + }, + }; + const { residue, secrets } = splitServerOAuthState(state, "all"); + expect(secrets[issuerRegistrationTokenField(issuer)]).toBe("rat"); + expect(residue.byIssuer![issuer]!.clientInformation).toEqual({ + client_id: "cid", + }); + + const joined = joinServerOAuthState(residue, secrets); + expect(joined.byIssuer![issuer]!.clientInformation).toEqual({ + client_id: "cid", + registration_access_token: "rat", + }); + }); + + it("splits both bearer keys from one legacy clientInformation", () => { + const state: ServerOAuthState = { + clientInformation: { + client_id: "cid", + client_secret: "cs", + registration_access_token: "rat", + }, + }; + const { residue, secrets } = splitServerOAuthState(state, "all"); + expect(secrets[LEGACY_CLIENT_SECRET_FIELD]).toBe("cs"); + expect(secrets[LEGACY_REGISTRATION_TOKEN_FIELD]).toBe("rat"); + expect(residue.clientInformation).toEqual({ client_id: "cid" }); + + const joined = joinServerOAuthState(residue, secrets); + expect(joined.clientInformation).toEqual(state.clientInformation); + }); + + it("splits and rejoins the preregistered client's token", () => { + const state: ServerOAuthState = { + preregisteredClientInformation: { + client_id: "cid", + registration_access_token: "rat", + }, + }; + const { residue, secrets } = splitServerOAuthState(state, "all"); + expect(secrets[PREREG_REGISTRATION_TOKEN_FIELD]).toBe("rat"); + expect(residue.preregisteredClientInformation).toEqual({ + client_id: "cid", + }); + const joined = joinServerOAuthState(residue, secrets); + expect(joined.preregisteredClientInformation).toEqual( + state.preregisteredClientInformation, + ); + }); + + it("a plaintext registration token alone marks the snapshot for migration", () => { + const snapshot: OAuthPersistSnapshot = { + servers: { + "https://api.example/mcp": { + clientInformation: { + client_id: "cid", + registration_access_token: "rat", + }, + }, + }, + idpSessions: {}, + }; + expect(snapshotHasPlaintextSecrets(snapshot)).toBe(true); + }); +}); diff --git a/clients/web/src/test/core/auth/oauth-storage-sections.test.ts b/clients/web/src/test/core/auth/oauth-storage-sections.test.ts new file mode 100644 index 0000000000..431814e6a2 --- /dev/null +++ b/clients/web/src/test/core/auth/oauth-storage-sections.test.ts @@ -0,0 +1,307 @@ +/** + * Tests that every `OAuthStorageBase` mutation names the sections it touched + * when persisting (the clobber fix): per-server mutations name their server + * URL, IdP mutations their issuer, and the enterprise-managed sweep the URLs + * it deleted — captured *before* the clear, since the flags are gone after. + * Also pins that the snapshot handed to the backend is taken when the queued + * write actually runs, so a write that waited in the queue carries mutations + * that landed in memory while it waited. + */ + +import { describe, it, expect } from "vitest"; +import { + OAuthStorageBase, + OAuthStorageCoordination, +} from "@inspector/core/auth/oauth-storage.js"; +import { OAuthMemoryStore } from "@inspector/core/auth/store.js"; +import { getOwnEntry } from "@inspector/core/storage/own-entry.js"; +import type { IssuerBoundOAuthState } from "@inspector/core/auth/store.js"; +import type { + OAuthPersistBackend, + OAuthPersistSections, + OAuthPersistSnapshot, +} from "@inspector/core/auth/oauth-persist.js"; +import type { OAuthTokens } from "@modelcontextprotocol/client"; + +interface RecordedWrite { + snapshot: OAuthPersistSnapshot; + sections: OAuthPersistSections | undefined; +} + +function makeRecordingBackend(): { + backend: OAuthPersistBackend; + writes: RecordedWrite[]; +} { + const writes: RecordedWrite[] = []; + return { + writes, + backend: { + async read() { + return null; + }, + async write(snapshot, sections) { + writes.push({ snapshot, sections }); + }, + }, + }; +} + +const TOKENS: OAuthTokens = { access_token: "at", token_type: "Bearer" }; +const SERVER = "http://mcp.example/path"; +const ISSUER = "https://as.example"; + +describe("OAuthStorageBase sectioned persistence", () => { + it("per-server mutations name their server URL", async () => { + const { backend, writes } = makeRecordingBackend(); + const storage = new OAuthStorageBase(new OAuthMemoryStore(), backend); + + await storage.saveTokens(SERVER, TOKENS, { issuer: ISSUER }); + await storage.saveClientInformation( + SERVER, + { client_id: "c1" }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + await storage.saveCodeVerifier(SERVER, "verifier"); + await storage.saveScope(SERVER, "read"); + await storage.saveDiscoveryState(SERVER, { + authorizationServerUrl: ISSUER, + }); + await storage.clearTokens(SERVER); + await storage.clear(SERVER); + + expect(writes).toHaveLength(7); + for (const write of writes) { + expect(write.sections).toEqual({ servers: [SERVER] }); + } + // `clear` deletes the entry — the snapshot no longer carries it, so a + // merging backend propagates the deletion instead of resurrecting it. + expect(writes[6]!.snapshot.servers[SERVER]).toBeUndefined(); + }); + + it("takeRevocationSnapshot names the cleared server", async () => { + const { backend, writes } = makeRecordingBackend(); + const storage = new OAuthStorageBase(new OAuthMemoryStore(), backend); + await storage.saveTokens(SERVER, TOKENS, { issuer: ISSUER }); + + const snapshot = await storage.takeRevocationSnapshot(SERVER); + expect(snapshot.byIssuer[ISSUER]?.tokens).toEqual(TOKENS); + const last = writes.at(-1)!; + expect(last.sections).toEqual({ servers: [SERVER] }); + expect(last.snapshot.servers[SERVER]).toBeUndefined(); + }); + + it("IdP session mutations name their issuer", async () => { + const { backend, writes } = makeRecordingBackend(); + const storage = new OAuthStorageBase(new OAuthMemoryStore(), backend); + + await storage.saveIdpSession(ISSUER, { idToken: "id" }); + await storage.clearIdpSession(ISSUER); + + expect(writes.map((w) => w.sections)).toEqual([ + { idpSessions: [ISSUER] }, + { idpSessions: [ISSUER] }, + ]); + expect(writes[1]!.snapshot.idpSessions[ISSUER]).toBeUndefined(); + }); + + it("clearEnterpriseManagedResourceServers names the URLs it deleted", async () => { + const { backend, writes } = makeRecordingBackend(); + const storage = new OAuthStorageBase(new OAuthMemoryStore(), backend); + + await storage.saveTokens("http://ema-1", TOKENS, { + enterpriseManaged: true, + }); + await storage.saveTokens("http://ema-2", TOKENS, { + enterpriseManaged: true, + }); + await storage.saveTokens("http://plain", TOKENS); + + await storage.clearEnterpriseManagedResourceServers(); + const last = writes.at(-1)!; + // The EMA flags are gone from memory after the clear, so the write must + // have captured the affected URLs beforehand to propagate the deletions. + expect(last.sections).toEqual({ + servers: ["http://ema-1", "http://ema-2"], + }); + expect(last.snapshot.servers["http://plain"]).toBeDefined(); + expect(last.snapshot.servers["http://ema-1"]).toBeUndefined(); + }); + + it("takes the snapshot when the queued write runs, not when it was queued", async () => { + const writes: RecordedWrite[] = []; + let releaseFirst!: () => void; + const firstWriteGate = new Promise((resolve) => { + releaseFirst = resolve; + }); + let call = 0; + const backend: OAuthPersistBackend = { + async read() { + return null; + }, + async write(snapshot, sections) { + call += 1; + if (call === 1) { + await firstWriteGate; + } + writes.push({ snapshot, sections }); + }, + }; + const storage = new OAuthStorageBase(new OAuthMemoryStore(), backend); + + const first = storage.saveScope(SERVER, "first"); + // Queued behind the gated first write; by the time it runs, the code + // verifier below has already landed in memory, and its snapshot must + // carry it (this is what lets a merge write the freshest value). + const second = storage.saveScope(SERVER, "second"); + const third = storage.saveCodeVerifier(SERVER, "cv"); + releaseFirst(); + await Promise.all([first, second, third]); + + expect(writes).toHaveLength(3); + expect(writes[1]!.snapshot.servers[SERVER]).toMatchObject({ + scope: "second", + codeVerifier: "cv", + }); + }); + + it("issuer-agnostic clears keep a __proto__ issuer slot (own-property rebuild)", async () => { + // `mapIssuerSlots` rebuilds `byIssuer`; a plain `byIssuer[key] =` would + // hit the prototype setter for a persisted `__proto__` issuer, dropping + // the slot — clearTokens would then erase that issuer's client + // registration too, not just its tokens. + const { backend } = makeRecordingBackend(); + const memory = new OAuthMemoryStore(); + const storage = new OAuthStorageBase(memory, backend); + const byIssuer = JSON.parse( + '{"__proto__": {"tokens": {"access_token": "at", "token_type": "Bearer"}, "clientInformation": {"client_id": "cid"}}}', + ) as Record; + memory.getState().setServerState(SERVER, { byIssuer }); + + await storage.clearTokens(SERVER); + + const state = memory.getState().getServerState(SERVER); + expect(Object.hasOwn(state.byIssuer!, "__proto__")).toBe(true); + const slot = getOwnEntry(state.byIssuer, "__proto__"); + expect(slot?.tokens).toBeUndefined(); + expect(slot?.clientInformation).toEqual({ client_id: "cid" }); + }); + + it("a failed load blocks mutations and is retried, never cached", async () => { + // A tolerant load (or a permanently cached rejection) would let a store + // outage hydrate empty state — and the next save's sectioned diff would + // delete the credentials the outage hid. The load must fail closed and + // retry once the backend recovers. + let fail = true; + const backend: OAuthPersistBackend = { + async read() { + // deliberately a bare string + if (fail) throw "backend outage"; + return null; + }, + async write() {}, + }; + const storage = new OAuthStorageBase(new OAuthMemoryStore(), backend); + + await expect(storage.saveTokens(SERVER, TOKENS)).rejects.toBe( + "backend outage", + ); + fail = false; + await expect(storage.saveTokens(SERVER, TOKENS)).resolves.toBeUndefined(); + expect(await storage.getTokens(SERVER)).toEqual(TOKENS); + }); +}); + +describe("shared load/persist coordination", () => { + const STALE: OAuthTokens = { access_token: "stale", token_type: "Bearer" }; + + /** A persisted snapshot holding `tokens` for SERVER, built the real way. */ + async function snapshotWithTokens( + tokens: OAuthTokens, + ): Promise { + const memory = new OAuthMemoryStore(); + const scratch = new OAuthStorageBase(memory, { + async read() { + return null; + }, + async write() {}, + }); + await scratch.saveTokens(SERVER, tokens, { issuer: ISSUER }); + return memory.snapshot(); + } + + it("a second instance sharing memory must not replace() a live mutation with stale disk state", async () => { + // The Node storage caches one OAuthMemoryStore per state-file path but + // callers can construct several NodeOAuthStorage instances over it (CLI + // connect + --relogin do). With a per-instance load latch, the second + // instance's first load() re-reads disk and replace()s the shared memory + // — silently reverting a mutation the first instance had already + // reported as saved. Sharing OAuthStorageCoordination pins load-once and + // one persist queue per shared memory. + const disk = await snapshotWithTokens(STALE); + let reads = 0; + const writes: RecordedWrite[] = []; + const backend: OAuthPersistBackend = { + async read() { + reads += 1; + return disk; + }, + async write(snapshot, sections) { + writes.push({ snapshot, sections }); + }, + }; + + const memory = new OAuthMemoryStore(); + const coordination = new OAuthStorageCoordination(); + const first = new OAuthStorageBase(memory, backend, coordination); + await first.saveTokens(SERVER, TOKENS, { issuer: ISSUER }); + + const second = new OAuthStorageBase(memory, backend, coordination); + await second.load(); + + expect(reads).toBe(1); + expect(await second.getTokens(SERVER)).toEqual({ + ...TOKENS, + issuer: ISSUER, + }); + + // The mutation also survives into the next queued persist. + await second.saveScope(SERVER, "s"); + const last = writes[writes.length - 1]!; + expect(JSON.stringify(last.snapshot)).toContain(TOKENS.access_token); + expect(JSON.stringify(last.snapshot)).not.toContain(STALE.access_token); + }); + + it("concurrent first loads on two instances share one backend read", async () => { + let reads = 0; + let release!: (snapshot: OAuthPersistSnapshot | null) => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + const backend: OAuthPersistBackend = { + async read() { + reads += 1; + return gate; + }, + async write() {}, + }; + + const memory = new OAuthMemoryStore(); + const coordination = new OAuthStorageCoordination(); + const first = new OAuthStorageBase(memory, backend, coordination); + const second = new OAuthStorageBase(memory, backend, coordination); + + const loads = Promise.all([first.load(), second.load()]); + release(await snapshotWithTokens(STALE)); + await loads; + + expect(reads).toBe(1); + expect(await first.getTokens(SERVER)).toEqual({ + ...STALE, + issuer: ISSUER, + }); + expect(await second.getTokens(SERVER)).toEqual({ + ...STALE, + issuer: ISSUER, + }); + }); +}); diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index c108b530f2..9f057b60c3 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -1138,7 +1138,8 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { refresh_token: "r-good", }, }, - // Unparseable: no `token_type`. + // Unparseable: type-corrupt `access_token` (partial shapes with + // well-typed fields are readable grants — see the test below). "https://broken.example.com": { tokens: { access_token: 42 } }, }, serverMetadata: { @@ -1171,6 +1172,43 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { ); }); + // The store contract deliberately holds partial payloads (a refresh-only + // grant inherited from a legacy file, say). Revocation must read them with + // the same contract: gating on the full schema here would clear the local + // state and then report the grant unreadable — leaving a live bearer + // refresh token at the AS with no local record of it. + it("revokes a refresh-only grant instead of reporting it unreadable", async () => { + stubSnapshot(storage, { + byIssuer: { + "https://as.example.com": { + tokens: { refresh_token: "r-only", token_type: "Bearer" }, + }, + }, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, + }); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + expect(outcome).toMatchObject({ status: "revoked" }); + expect(fetchFn).toHaveBeenCalledTimes(1); + const body = new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)); + expect(body.get("token")).toBe("r-only"); + expect(body.get("token_type_hint")).toBe("refresh_token"); + }); + // A token is only meaningful to the AS that minted it, so two issuers minting // the same opaque string are two grants. Collapsing them would drop the // second before the issuer-mismatch check could even report it. diff --git a/clients/web/src/test/core/auth/storage-browser.test.ts b/clients/web/src/test/core/auth/storage-browser.test.ts index 89d0cf27c3..82074ab1a2 100644 --- a/clients/web/src/test/core/auth/storage-browser.test.ts +++ b/clients/web/src/test/core/auth/storage-browser.test.ts @@ -167,6 +167,19 @@ describe("BrowserOAuthStorage", () => { expect(result).toEqual(tokens); }); + + it("serves a partial token payload as no tokens, not a throw", async () => { + // A refresh-only entry preserved from a legacy plaintext file (see + // splitTokens in oauth-secrets.ts) is kept at rest for the CLI's + // stored-token refresh, but has no access token to serve. Throwing + // here would brick every flow touching the server (connection state, + // the SDK provider's tokens() callback) instead of re-authorizing. + await storage.saveTokens(testServerUrl, { + refresh_token: "rt-only", + token_type: "Bearer", + } as unknown as OAuthTokens); + await expect(storage.getTokens(testServerUrl)).resolves.toBeUndefined(); + }); }); describe("saveTokens", () => { diff --git a/clients/web/src/test/core/auth/storage-remote.test.ts b/clients/web/src/test/core/auth/storage-remote.test.ts index 7650925f41..b6e3f3893e 100644 --- a/clients/web/src/test/core/auth/storage-remote.test.ts +++ b/clients/web/src/test/core/auth/storage-remote.test.ts @@ -15,7 +15,6 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => { beforeEach(() => { storage = new RemoteOAuthStorage({ baseUrl: "http://remote.example", - storeId: `unit-${Math.random().toString(36).slice(2)}`, fetchFn: NOOP_FETCH, }); }); @@ -112,13 +111,21 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => { expect(await storage.getTokens(serverUrl)).toBeUndefined(); }); - it("default storeId is 'oauth' when omitted", () => { + it("always targets the shared oauth store endpoint", async () => { + const seen: string[] = []; + const recordingFetch = vi.fn(async (input) => { + seen.push(String(input)); + return new Response("{}", { status: 404 }); + }); const s = new RemoteOAuthStorage({ baseUrl: "http://r.example", - fetchFn: NOOP_FETCH, + fetchFn: recordingFetch, }); - // No public accessor; constructing without throwing covers the default-branch. - expect(s).toBeInstanceOf(RemoteOAuthStorage); + await s.getTokens("http://server.example/mcp"); + expect(seen.length).toBeGreaterThan(0); + for (const url of seen) { + expect(url).toContain("/api/storage/oauth"); + } }); it("getCodeVerifier loads remote state automatically when not preloaded", async () => { @@ -141,7 +148,6 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => { const delayedStorage = new RemoteOAuthStorage({ baseUrl: "http://remote.example", - storeId: `delayed-${Math.random().toString(36).slice(2)}`, fetchFn: delayedFetch, }); @@ -158,7 +164,6 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => { const failingStorage = new RemoteOAuthStorage({ baseUrl: "http://remote.example", - storeId: `fail-${Math.random().toString(36).slice(2)}`, fetchFn: failingFetch, }); diff --git a/clients/web/src/test/core/auth/store.test.ts b/clients/web/src/test/core/auth/store.test.ts index 131a5ab8b7..e3fa61c769 100644 --- a/clients/web/src/test/core/auth/store.test.ts +++ b/clients/web/src/test/core/auth/store.test.ts @@ -81,4 +81,20 @@ describe("OAuthMemoryStore", () => { idpSessions: {}, }); }); + + it("answers {} for a missing __proto__ key instead of the inherited prototype", () => { + // Server URLs and issuers are untrusted map keys: a plain lookup for a + // missing "__proto__" returns `Object.prototype`, a truthy non-entry. + const store = new OAuthMemoryStore(); + const state = store.getState(); + expect(state.getServerState("__proto__")).toEqual({}); + expect(state.getIdpSession("__proto__")).toEqual({}); + // And the read-modify-write merge base is the own entry, not the + // prototype: a set for the key round-trips as an own property. + state.setServerState("__proto__", { scope: "read" }); + expect(Object.hasOwn(store.snapshot().servers, "__proto__")).toBe(true); + expect(state.getServerState("__proto__")).toEqual({ scope: "read" }); + state.setIdpSession("__proto__", { idToken: "t" }); + expect(state.getIdpSession("__proto__")).toEqual({ idToken: "t" }); + }); }); diff --git a/clients/web/src/test/core/client/node-persistence.test.ts b/clients/web/src/test/core/client/node-persistence.test.ts index 238e0fc27a..dd0e8e33ae 100644 --- a/clients/web/src/test/core/client/node-persistence.test.ts +++ b/clients/web/src/test/core/client/node-persistence.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; import * as fs from "node:fs/promises"; import { existsSync, readFileSync } from "node:fs"; import * as os from "node:os"; @@ -223,6 +223,321 @@ describe("client node-persistence", () => { await secretStore.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), ).toBeNull(); }); + + it("restores the prior keychain secret when the client.json write fails", async () => { + // Set/delete happens before the file write; without compensation a + // failed write would leave the new secret paired with the old on-disk + // config. Force the write to fail by making the directory read-only. + const filePath = await makeTmpFile( + JSON.stringify({ + enterpriseManagedAuth: { + idp: { issuer: "https://idp.example.com", clientId: "cid" }, + }, + }), + ); + const secretStore = new InMemorySecretStore(); + await secretStore.set( + CLIENT_KEYCHAIN_ID, + SECRET_FIELD_IDP_CLIENT_SECRET, + "old-secret", + ); + + await fs.chmod(tmpDir, 0o555); + try { + await expect( + writeClientConfigStore( + filePath, + { + enterpriseManagedAuth: { + idp: { + issuer: "https://idp.example.com", + clientId: "cid", + clientSecret: "new-secret", + }, + }, + }, + secretStore, + ), + ).rejects.toThrow(); + } finally { + await fs.chmod(tmpDir, 0o755); + } + + expect( + await secretStore.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), + ).toBe("old-secret"); + }); + + it("restores a cleared keychain secret when the client.json write fails", async () => { + const filePath = await makeTmpFile( + JSON.stringify({ + enterpriseManagedAuth: { + idp: { issuer: "https://idp.example.com", clientId: "cid" }, + }, + }), + ); + const secretStore = new InMemorySecretStore(); + await secretStore.set( + CLIENT_KEYCHAIN_ID, + SECRET_FIELD_IDP_CLIENT_SECRET, + "old-secret", + ); + + await fs.chmod(tmpDir, 0o555); + try { + await expect( + writeClientConfigStore( + filePath, + { + cimd: { + enabled: true, + clientMetadataUrl: "https://x.example/c.json", + }, + }, + secretStore, + ), + ).rejects.toThrow(); + } finally { + await fs.chmod(tmpDir, 0o755); + } + + expect( + await secretStore.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), + ).toBe("old-secret"); + }); + + it("warns but rethrows the write failure when the restore itself fails", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const filePath = await makeTmpFile( + JSON.stringify({ + enterpriseManagedAuth: { + idp: { issuer: "https://idp.example.com", clientId: "cid" }, + }, + }), + ); + const store = new InMemorySecretStore(); + await store.set(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET, "old"); + let sets = 0; + const failingRestore: SecretStore = { + get: (id, f) => store.get(id, f), + set: async (id, f, v) => { + sets += 1; + // First set is the write itself; the second is the restore. + if (sets > 1) throw new KeychainUnavailableError(new Error("gone")); + return store.set(id, f, v); + }, + delete: (id, f) => store.delete(id, f), + deleteAllForServer: (id) => store.deleteAllForServer(id), + }; + + await fs.chmod(tmpDir, 0o555); + try { + await expect( + writeClientConfigStore( + filePath, + { + enterpriseManagedAuth: { + idp: { + issuer: "https://idp.example.com", + clientId: "cid", + clientSecret: "new", + }, + }, + }, + failingRestore, + ), + ).rejects.toThrow(/EACCES|EPERM|permission/i); + } finally { + await fs.chmod(tmpDir, 0o755); + warn.mockRestore(); + } + }); + + it("stringifies a non-Error restore failure in the warning", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const filePath = await makeTmpFile( + JSON.stringify({ + enterpriseManagedAuth: { + idp: { issuer: "https://idp.example.com", clientId: "cid" }, + }, + }), + ); + const store = new InMemorySecretStore(); + await store.set(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET, "old"); + let sets = 0; + const failingRestore: SecretStore = { + get: (id, f) => store.get(id, f), + set: async (id, f, v) => { + sets += 1; + if (sets > 1) throw "gone"; // deliberately a bare string + return store.set(id, f, v); + }, + delete: (id, f) => store.delete(id, f), + deleteAllForServer: (id) => store.deleteAllForServer(id), + }; + + await fs.chmod(tmpDir, 0o555); + try { + await expect( + writeClientConfigStore( + filePath, + { + enterpriseManagedAuth: { + idp: { + issuer: "https://idp.example.com", + clientId: "cid", + clientSecret: "new", + }, + }, + }, + failingRestore, + ), + ).rejects.toThrow(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("gone")); + } finally { + await fs.chmod(tmpDir, 0o755); + warn.mockRestore(); + } + }); + + it("deleteClientConfigStore keeps the file when the keychain delete fails", async () => { + // Keychain-first ordering: a failed confirmed delete leaves the file + // (and thus the visible config) untouched, so a retry sees the same + // state instead of a config that looks deleted while its secret lives. + const filePath = await makeTmpFile( + JSON.stringify({ cimd: { enabled: false, clientMetadataUrl: "" } }), + ); + const store = new InMemorySecretStore(); + await store.set(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET, "v"); + const failingDelete: SecretStore = { + get: (id, f) => store.get(id, f), + set: (id, f, v) => store.set(id, f, v), + delete: async () => { + throw new KeychainUnavailableError(new Error("locked")); + }, + deleteAllForServer: async () => { + throw new KeychainUnavailableError(new Error("locked")); + }, + }; + + await expect( + deleteClientConfigStore(filePath, failingDelete), + ).rejects.toThrow(); + expect(existsSync(filePath)).toBe(true); + expect( + await store.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), + ).toBe("v"); + }); + + it("deleteClientConfigStore restores the secret when the delete removes it and then fails", async () => { + // The confirmed-delete contract only promises that a *resolved* delete + // removed the value — a rejected one may have removed it first. The + // compensation must therefore cover the delete itself, not just the + // unlink, or the surviving config loses its indexed secret. + const filePath = await makeTmpFile( + JSON.stringify({ cimd: { enabled: false, clientMetadataUrl: "" } }), + ); + const store = new InMemorySecretStore(); + await store.set(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET, "v"); + const partialDelete: SecretStore = { + get: (id, f) => store.get(id, f), + set: (id, f, v) => store.set(id, f, v), + delete: async (id, f) => { + await store.delete(id, f); + throw new KeychainUnavailableError(new Error("locked")); + }, + deleteAllForServer: async () => { + throw new KeychainUnavailableError(new Error("locked")); + }, + }; + + await expect( + deleteClientConfigStore(filePath, partialDelete), + ).rejects.toThrow(); + expect(existsSync(filePath)).toBe(true); + expect( + await store.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), + ).toBe("v"); + }); + + it("deleteClientConfigStore restores the secret when the file unlink fails", async () => { + // The other half of all-or-nothing: the secret delete succeeded but the + // unlink did not — without the restore, the surviving client.json would + // reload without its credential. + const filePath = await makeTmpFile( + JSON.stringify({ cimd: { enabled: false, clientMetadataUrl: "" } }), + ); + const store = new InMemorySecretStore(); + await store.set(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET, "v"); + + await fs.chmod(tmpDir, 0o555); + try { + await expect(deleteClientConfigStore(filePath, store)).rejects.toThrow(); + } finally { + await fs.chmod(tmpDir, 0o755); + } + + expect(existsSync(filePath)).toBe(true); + expect( + await store.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), + ).toBe("v"); + }); + + it("delete: warns but rethrows the unlink failure when the restore fails", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const filePath = await makeTmpFile( + JSON.stringify({ cimd: { enabled: false, clientMetadataUrl: "" } }), + ); + const store = new InMemorySecretStore(); + await store.set(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET, "v"); + const failingRestore: SecretStore = { + get: (id, f) => store.get(id, f), + set: async () => { + throw new KeychainUnavailableError(new Error("gone")); + }, + delete: (id, f) => store.delete(id, f), + deleteAllForServer: (id) => store.deleteAllForServer(id), + }; + + await fs.chmod(tmpDir, 0o555); + try { + await expect( + deleteClientConfigStore(filePath, failingRestore), + ).rejects.toThrow(/EACCES|EPERM|permission/i); + } finally { + await fs.chmod(tmpDir, 0o755); + warn.mockRestore(); + } + expect(existsSync(filePath)).toBe(true); + }); + + it("delete: stringifies a non-Error restore failure in the warning", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const filePath = await makeTmpFile( + JSON.stringify({ cimd: { enabled: false, clientMetadataUrl: "" } }), + ); + const store = new InMemorySecretStore(); + await store.set(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET, "v"); + const failingRestore: SecretStore = { + get: (id, f) => store.get(id, f), + set: async () => { + throw "gone"; // deliberately a bare string + }, + delete: (id, f) => store.delete(id, f), + deleteAllForServer: (id) => store.deleteAllForServer(id), + }; + + await fs.chmod(tmpDir, 0o555); + try { + await expect( + deleteClientConfigStore(filePath, failingRestore), + ).rejects.toThrow(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("gone")); + } finally { + await fs.chmod(tmpDir, 0o755); + warn.mockRestore(); + } + }); }); describe("session-scoped store keeps client.json durable (#1950 review r19)", () => { @@ -370,3 +685,229 @@ describe("session-scoped store keeps client.json durable (#1950 review r19)", () } }); }); + +describe("client.json writers are serialized per resolved path", () => { + // The compensated snapshot/mutate/write blocks are only sound one at a + // time: two unserialized writers both snapshot the same prior secret, and + // the loser's compensation then overwrites the winner's *committed* value + // with the stale snapshot, leaving client.json describing one client while + // the keychain holds another's secret. The file lock (`withSecretFileLock`, + // the same exclusion oauth.json's writers take) makes the whole block a + // critical section; this test drives the exact interleaving the lock + // exists to close. + it("a failed save's compensation cannot clobber a concurrent save's committed secret", async () => { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-serialize-")); + const file = path.join(dir, "client.json"); + try { + await fs.writeFile( + file, + JSON.stringify({ + enterpriseManagedAuth: { + idp: { issuer: "https://idp.example/", clientId: "cid-old" }, + }, + }), + "utf-8", + ); + const store = new InMemorySecretStore(); + await store.set( + CLIENT_KEYCHAIN_ID, + SECRET_FIELD_IDP_CLIENT_SECRET, + "old", + ); + + // Writer A parks inside its critical section — after its snapshot, + // mid-`set` — until released, then fails, so its compensation restores + // the snapshot. Writer B, started while A is parked, saves a new + // secret and succeeds. + let releaseA!: () => void; + const gateA = new Promise((resolve) => { + releaseA = resolve; + }); + let aReachedSet!: () => void; + const aInsideSet = new Promise((resolve) => { + aReachedSet = resolve; + }); + const gated: SecretStore = { + get: (serverId, field) => store.get(serverId, field), + set: async (serverId, field, value) => { + if (value === "secret-a") { + aReachedSet(); + await gateA; + throw new Error("keychain rejected the write"); + } + return store.set(serverId, field, value); + }, + delete: (serverId, field) => store.delete(serverId, field), + deleteAllForServer: (serverId) => store.deleteAllForServer(serverId), + }; + + const configFor = (suffix: string) => ({ + enterpriseManagedAuth: { + idp: { + issuer: "https://idp.example/", + clientId: `cid-${suffix}`, + clientSecret: `secret-${suffix}`, + }, + }, + }); + + const saveA = writeClientConfigStore(file, configFor("a"), gated); + const rejectedA = saveA.catch((err: unknown) => err); + await aInsideSet; // A holds the lock, parked mid-mutation. + const saveB = writeClientConfigStore(file, configFor("b"), store); + // Give B time to run: under the lock it is parked at acquisition; + // without the lock it would commit here, exposing its secret to A's + // stale compensation below. + await new Promise((resolve) => setTimeout(resolve, 300)); + releaseA(); + expect(await rejectedA).toBeInstanceOf(Error); + await saveB; + + // B's committed state survives A's compensation: the store holds B's + // secret and the file names B's client — the two halves agree. + expect( + await store.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), + ).toBe("secret-b"); + const onDisk = JSON.parse(await fs.readFile(file, "utf-8")) as { + enterpriseManagedAuth: { idp: { clientId: string } }; + }; + expect(onDisk.enterpriseManagedAuth.idp.clientId).toBe("cid-b"); + } finally { + await fs.rm(dir, { recursive: true, force: true }); + } + }); +}); + +describe("withClientConfigLock failure paths", () => { + // These force the lock seam itself to fail, which needs the module graph + // rebuilt around a mocked `file-lock` — class identities (for the + // `instanceof SecretFileLockHeldError` checks) must come from the same + // fresh graph, so everything is imported after `vi.doMock`. + let dir: string; + let file: string; + + async function freshWithLock( + impl: (filePath: string, fn: () => Promise) => Promise, + ) { + vi.resetModules(); + vi.doMock("@inspector/core/auth/node/file-lock.js", () => ({ + withSecretFileLock: impl, + })); + const persistence = + await import("@inspector/core/client/node-persistence.js"); + const stores = await import("@inspector/core/auth/node/secret-store.js"); + return { persistence, stores }; + } + + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-lockfail-")); + file = path.join(dir, "client.json"); + }); + + afterEach(async () => { + vi.doUnmock("@inspector/core/auth/node/file-lock.js"); + vi.resetModules(); + await fs.rm(dir, { recursive: true, force: true }); + }); + + it("rewords a held lock at acquisition to name client.json, keeping type and cause", async () => { + const { persistence, stores } = await freshWithLock(async () => { + throw new stores.SecretFileLockHeldError("Could not lock"); + }); + const rejection = persistence.writeClientConfigStore( + file, + { + enterpriseManagedAuth: { + idp: { issuer: "https://idp.example.com", clientId: "c" }, + }, + }, + new stores.InMemorySecretStore(), + ); + await expect(rejection).rejects.toMatchObject({ + message: expect.stringContaining( + `Could not save the client configuration: the file at ${file} is locked`, + ), + }); + // The subclass survives the rewording — it is what the HTTP layer maps + // to a retryable 503; a plain Error would demote it to a 500. + await expect(rejection).rejects.toBeInstanceOf( + stores.SecretFileLockHeldError, + ); + await expect( + persistence.deleteClientConfigStore( + file, + new stores.InMemorySecretStore(), + ), + ).rejects.toMatchObject({ + message: expect.stringContaining( + "Could not remove the client configuration", + ), + }); + }); + + it("a held lock skips the read-path migration but keeps the read available", async () => { + await fs.writeFile(file, JSON.stringify(configWithPlaintextSecret)); + const { persistence, stores } = await freshWithLock(async () => { + throw new stores.SecretFileLockHeldError("Could not lock"); + }); + const store = new stores.InMemorySecretStore(); + const config = await persistence.readClientConfigStore(file, store); + // The unlocked read's config is served untouched; nothing migrated. + expect( + (config as typeof configWithPlaintextSecret).enterpriseManagedAuth.idp + .clientSecret, + ).toBe("plain"); + expect( + await store.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), + ).toBeNull(); + expect(JSON.parse(await fs.readFile(file, "utf-8"))).toEqual( + configWithPlaintextSecret, + ); + }); + + it("a non-lock acquisition failure propagates untouched", async () => { + await fs.writeFile(file, JSON.stringify(configWithPlaintextSecret)); + const original = new Error("disk exploded"); + const { persistence, stores } = await freshWithLock(async () => { + throw original; + }); + await expect( + persistence.readClientConfigStore(file, new stores.InMemorySecretStore()), + ).rejects.toBe(original); + }); + + it("migration re-reads under the lock: a file deleted meanwhile yields an empty config", async () => { + await fs.writeFile(file, JSON.stringify(configWithPlaintextSecret)); + const { persistence, stores } = await freshWithLock(async (_p, fn) => { + await fs.rm(file, { force: true }); + return fn(); + }); + const store = new stores.InMemorySecretStore(); + expect(await persistence.readClientConfigStore(file, store)).toEqual({}); + expect( + await store.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), + ).toBeNull(); + }); + + it("migration re-reads under the lock: a file already stripped meanwhile migrates nothing", async () => { + await fs.writeFile(file, JSON.stringify(configWithPlaintextSecret)); + const stripped = { + enterpriseManagedAuth: { + idp: { issuer: "https://idp.example.com", clientId: "cid" }, + }, + }; + const { persistence, stores } = await freshWithLock(async (_p, fn) => { + await fs.writeFile(file, JSON.stringify(stripped)); + return fn(); + }); + const store = new stores.InMemorySecretStore(); + // The fresh (already-stripped) file decides: no plaintext left, so the + // store is never written and the fresh shape is served. + expect(await persistence.readClientConfigStore(file, store)).toEqual( + stripped, + ); + expect( + await store.get(CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET), + ).toBeNull(); + }); +}); diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts index fc043d0fad..5dccf460eb 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -397,10 +397,13 @@ describe("withSecretFileLock degrades rather than failing", () => { ); it( - "stays silent per the delete contract when the lock is held", + "rejects a delete rather than removing alongside a live lock holder", async () => { - // `delete` reports nothing by contract — only `set` hard-fails — so the - // refusal above must not turn a delete into a throw. + // The confirmed-delete contract: reporting success for a delete that + // could not happen would let a caller commit state that assumes the + // entry is gone, and the entry would resurface once the lock clears. + // So a live lock holder must make the delete *reject*, with the + // entry left intact. const target = filePath(); const store = new FileSecretStore({ filePath: target }); await store.set("srv", "env:A", "1"); @@ -418,7 +421,9 @@ describe("withSecretFileLock degrades rather than failing", () => { realpath: false, stale: 10_000, }); - await expect(store.delete("srv", "env:A")).resolves.toBeUndefined(); + await expect(store.delete("srv", "env:A")).rejects.toBeInstanceOf( + SecretStoreUnavailableError, + ); await release(); // …and the entry it could not delete is still there, not half-removed. diff --git a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts index bfeeda042d..eb9ec0e321 100644 --- a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts +++ b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts @@ -210,6 +210,30 @@ describe("FileSecretStore failure handling", () => { ); }); + it("refuses a non-string value before touching the file", async () => { + // A cast slipping past the compile-time contract (say a numeric + // client_secret from a malformed payload) must not be written: one + // non-string value makes `asSecretMap` refuse the whole file on every + // later read and write, poisoning unrelated stored credentials. + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("alpha", "keep", "safe"); + await expect( + store.set("alpha", "bad", 123 as unknown as string), + ).rejects.toThrow(/non-string secret value \(number\) for "bad"/); + await expect( + store.setMany("alpha", { + ok: "fine", + worse: { nested: true } as unknown as string, + }), + ).rejects.toThrow(/non-string secret value \(object\) for "worse"/); + // Nothing from the refused batch landed, and the store still works. + expect(await store.get("alpha", "bad")).toBeNull(); + expect(await store.get("alpha", "ok")).toBeNull(); + expect(await store.get("alpha", "keep")).toBe("safe"); + await store.set("alpha", "after", "still-writable"); + expect(await store.get("alpha", "after")).toBe("still-writable"); + }); + it("reads a plaintext file that carries no secrets key as empty", async () => { // A hand-edited (or hand-created) file is the realistic source of this // shape, and it must read as "no secrets yet" rather than throwing: the @@ -256,7 +280,10 @@ describe("FileSecretStore failure handling", () => { expect(raw.version).toBe(2); }); - it("delete stays silent on a file it cannot decrypt", async () => { + it("delete rejects on a file it cannot decrypt", async () => { + // A deletion that cannot be confirmed must escape: committing state + // that assumes the entry is gone would resurrect it once the file + // decrypts again. await writeEncryptedFixture(); const store = new FileSecretStore({ filePath: filePath(), @@ -264,8 +291,10 @@ describe("FileSecretStore failure handling", () => { }); await expect( store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), - ).resolves.toBeUndefined(); - await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); + ).rejects.toBeInstanceOf(SecretStoreUnavailableError); + await expect(store.deleteAllForServer("alpha")).rejects.toBeInstanceOf( + SecretStoreUnavailableError, + ); }); it("set reports a corrupt file rather than silently replacing it", async () => { @@ -491,7 +520,7 @@ describe("FileSecretStore failure handling", () => { }); await expect(store.set("alpha", "env:A", "1")).rejects.toThrow(); expect(await store.get("alpha", "env:A")).toBe(null); - await expect(store.delete("alpha", "env:A")).resolves.toBeUndefined(); + await expect(store.delete("alpha", "env:A")).rejects.toThrow(); await expect(store.set("alpha", "env:B", "2")).rejects.toThrow(); }); @@ -920,6 +949,31 @@ describe("getMany", () => { }); }); + it("returns prototype-named fields and server ids as own entries", async () => { + // A plain `out[serverId] = found` / `found[field] = value` invokes the + // inherited `__proto__` setter instead of creating an entry, so a + // requested field or server id with that name would be silently omitted + // from the result — a violated bulk-read contract, not just a missing + // value. Both maps must be built with own-property writes. + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + await store.set("srv", "__proto__", "field-value"); + await store.set("__proto__", "env:A", "server-value"); + + const out = await store.getMany([ + { serverId: "srv", fields: ["__proto__"] }, + { serverId: "__proto__", fields: ["env:A"] }, + ]); + expect(Object.getOwnPropertyDescriptor(out.srv, "__proto__")?.value).toBe( + "field-value", + ); + expect(Object.getOwnPropertyDescriptor(out, "__proto__")?.value).toEqual({ + "env:A": "server-value", + }); + }); + it("derives the key once for the whole set, not once per field", async () => { // The reason the seam exists: `get` reads and decrypts the *entire* file, // so rehydrating field-by-field cost one scrypt derivation per field, @@ -1189,6 +1243,72 @@ describe("getStrict (round 9)", () => { }); }); +describe("getManyStrict", () => { + it("returns values like getMany when the file is readable", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("srv", "env:A", "1"); + expect( + await store.getManyStrict([ + { serverId: "srv", fields: ["env:A", "env:MISSING"] }, + ]), + ).toEqual({ srv: { "env:A": "1" } }); + }); + + it("answers empty fields for a store file that does not exist yet", async () => { + // Absence is a real answer — only *unreadability* must throw. + const store = new FileSecretStore({ filePath: filePath() }); + expect( + await store.getManyStrict([{ serverId: "srv", fields: ["env:A"] }]), + ).toEqual({ srv: {} }); + }); + + it("returns prototype-named fields and server ids as own entries", async () => { + // Same contract as getMany: a `__proto__`-named request must land as an + // own entry rather than vanish into the inherited setter — this read + // later drives store deletions, so a silently omitted result is a + // deleted secret. + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("srv", "__proto__", "field-value"); + await store.set("__proto__", "env:A", "server-value"); + + const out = await store.getManyStrict([ + { serverId: "srv", fields: ["__proto__"] }, + { serverId: "__proto__", fields: ["env:A"] }, + ]); + expect(Object.getOwnPropertyDescriptor(out.srv, "__proto__")?.value).toBe( + "field-value", + ); + expect(Object.getOwnPropertyDescriptor(out, "__proto__")?.value).toEqual({ + "env:A": "server-value", + }); + }); + + it("wraps a filesystem failure as SecretStoreUnavailableError", async () => { + const blocker = path.join(tmpDir, "blocker"); + await fs.writeFile(blocker, "x", "utf-8"); + const store = new FileSecretStore({ + filePath: path.join(blocker, "secrets.json"), + }); + await expect( + store.getManyStrict([{ serverId: "srv", fields: ["env:A"] }]), + ).rejects.toBeInstanceOf(SecretStoreUnavailableError); + }); + + it("throws where getMany yields no fields, so hydration cannot read an outage as absence", async () => { + // OAuth read hydration feeds the memory state that sectioned writes + // diff against; an unreadable store answering empty maps would make the + // next save delete every credential the outage hid. + await fs.writeFile(filePath(), "{ not json", "utf-8"); + const store = new FileSecretStore({ filePath: filePath() }); + expect( + await store.getMany([{ serverId: "srv", fields: ["env:A"] }]), + ).toEqual({ srv: {} }); + await expect( + store.getManyStrict([{ serverId: "srv", fields: ["env:A"] }]), + ).rejects.toBeInstanceOf(SecretStoreUnavailableError); + }); +}); + describe("readOnDiskEncryption rejects an envelope it could not open", () => { // Naming the cipher is not the same as being openable, and reporting // "encrypted" for a file whose next save is guaranteed to fail is the @@ -1583,9 +1703,14 @@ describe("cross-process convergence (optimistic verify-and-retry)", () => { ); }); - it("a non-convergent delete stays silent, per the interface contract", async () => { - // `delete` reports nothing by contract — only `set` hard-fails — so a - // delete that cannot converge must still resolve rather than throw. + it("resolves a delete once the clobbering writer removes the key itself", async () => { + // The confirmed-delete contract: a delete resolves only when the key's + // absence is confirmed, and throws when it cannot be (unreadable file, + // held lock, non-convergence). Here the clobbering writer replaces the + // file *without* the target key, so the retry finds nothing left to + // delete — absence confirmed by someone else's hand is still absence, + // and the delete resolves. A clobberer that kept the key present would + // exhaust the retries and throw the non-convergence error, same as set. const store = new FileSecretStore({ filePath: filePath() }); await store.set("srv", "env:A", "1"); clobberAfterEveryWrite(store); @@ -1764,7 +1889,9 @@ describe("FileSecretStore with MCP_INSPECTOR_SECRET_KEY_FILE (#2447)", () => { await expect(store.set("alpha", "env:B", "2")).rejects.toThrow( SecretStoreUnavailableError, ); - await expect(store.delete("alpha", "env:A")).resolves.toBeUndefined(); + await expect(store.delete("alpha", "env:A")).rejects.toThrow( + SecretStoreUnavailableError, + ); expect(await fs.readFile(filePath(), "utf-8")).toBe(before); }); diff --git a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts index b31e1c518a..7aca1a2e16 100644 --- a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts +++ b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts @@ -720,6 +720,24 @@ describe("DeferredSecretStore forwards the optional seams", () => { ]), ).toEqual({ srv: { "env:A": "1", "env:B": "2" } }); }); + + it("forwards getManyStrict rather than degrading per-field", async () => { + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(false); + const store = mod.defaultSecretStore(); + expect(typeof store.getManyStrict).toBe("function"); + await store.set("srv", "env:A", "1"); + + const { secretStoreGetManyStrict } = + await import("@inspector/core/auth/node/secret-store.js"); + expect( + await secretStoreGetManyStrict(store, [ + { serverId: "srv", fields: ["env:A"] }, + ]), + ).toEqual({ srv: { "env:A": "1" } }); + }); }); describe("absorbFileSecretsIntoKeyring", () => { diff --git a/clients/web/src/test/integration/auth/node/secret-store.test.ts b/clients/web/src/test/integration/auth/node/secret-store.test.ts index 10cc8aa737..f443b0f1e2 100644 --- a/clients/web/src/test/integration/auth/node/secret-store.test.ts +++ b/clients/web/src/test/integration/auth/node/secret-store.test.ts @@ -103,6 +103,9 @@ import { SECRET_FIELD_OAUTH_CLIENT_SECRET, envSecretField, parseAccount, + secretStoreSetMany, + settleStoreMutations, + type SecretStore, } from "@inspector/core/auth/node/secret-store.js"; // The generic cases live in `secretStoreContract.ts` and are shared with @@ -191,11 +194,14 @@ describe("KeyringSecretStore (mocked native bindings)", () => { ).resolves.toBeUndefined(); }); - it("delete silently no-ops when the keychain is unavailable", async () => { + it("delete rejects with the typed error when the keychain is unavailable", async () => { + // A missing entry is success, but an unconfirmed delete must not be: + // reporting success would let callers commit state that assumes the + // credential is gone, and a later read would resurrect it. keyringMocks.failures.deleteThrows = true; await expect( store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), - ).resolves.toBeUndefined(); + ).rejects.toBeInstanceOf(KeychainUnavailableError); }); it("delete actually removes the value when the keychain is available", async () => { @@ -206,12 +212,14 @@ describe("KeyringSecretStore (mocked native bindings)", () => { ); }); - it("deleteAllForServer no-ops when findCredentialsAsync throws", async () => { - // We don't even know what was written, so there's nothing to sweep. - // Critically, this must not throw — the route's defensive sweep on - // POST and DELETE depends on it. + it("deleteAllForServer rejects when findCredentialsAsync throws", async () => { + // An unenumerable keychain may still hold this server's entries, so + // "success" would be a lie; the routes translate the typed error to + // the same 503 a failed `set` produces. keyringMocks.failures.findThrows = true; - await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); + await expect(store.deleteAllForServer("alpha")).rejects.toBeInstanceOf( + KeychainUnavailableError, + ); }); it("deleteAllForServer removes every entry under the given id", async () => { @@ -361,20 +369,22 @@ describe("KeyringSecretStore (mocked native bindings)", () => { ).rejects.toThrow(/Couldn't access platform storage/); }); - it("delete silently no-ops", async () => { + it("delete rejects with the typed error", async () => { await expect( store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), - ).resolves.toBeUndefined(); + ).rejects.toBeInstanceOf(KeychainUnavailableError); }); - it("deleteAllForServer no-ops even when the credential sweep finds entries", async () => { + it("deleteAllForServer rejects when the credential sweep finds entries it cannot delete", async () => { // findCredentialsAsync can succeed while per-entry construction - // fails; the sweep must still resolve rather than escape. + // fails; an unconfirmed sweep must escape rather than resolve. keyringMocks.failures.constructorThrows = false; await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); keyringMocks.failures.constructorThrows = true; - await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); + await expect(store.deleteAllForServer("alpha")).rejects.toBeInstanceOf( + KeychainUnavailableError, + ); }); }); @@ -444,6 +454,21 @@ describe("KeyringSecretStore (mocked native bindings)", () => { expect((await probeKeyringAvailable()).available).toBe(false); }); + it("reports a partially available keyring (reads work, enumeration doesn't) as unavailable", async () => { + // The exact shape of a headless Linux host or CI runner: keyutils + // serves single entries so `getPassword` works, but enumeration needs + // a Secret Service over D-Bus that isn't there. A get-only probe would + // select a store whose `deleteAllForServer` can never succeed — every + // server add/rename/delete would answer 503 under the confirmed-delete + // contract. Such a keychain must fall back like an unreachable one. + keyringMocks.failures.findThrows = true; + const result = await probeKeyringAvailable(); + expect(result.available).toBe(false); + expect((result as { detail: string }).detail).toContain( + "keychain find unavailable", + ); + }); + it("never writes to the user's keychain", async () => { // Deliberate: a write probe would be a stronger signal but would // deposit a value in someone's login keyring at every startup, for a @@ -535,13 +560,15 @@ describe("@napi-rs/keyring unloadable on this platform (#1905)", () => { } }); - it("delete and deleteAllForServer silently no-op", async () => { + it("delete and deleteAllForServer reject with the typed error", async () => { const mod = await importWithUnloadableKeyring(); const store = new mod.KeyringSecretStore(); await expect( store.delete("alpha", "oauth-client-secret"), - ).resolves.toBeUndefined(); - await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); + ).rejects.toBeInstanceOf(mod.KeychainUnavailableError); + await expect(store.deleteAllForServer("alpha")).rejects.toBeInstanceOf( + mod.KeychainUnavailableError, + ); }); it("the availability probe reports unavailable and names the load error", async () => { @@ -569,7 +596,7 @@ describe("@napi-rs/keyring unloadable on this platform (#1905)", () => { await store.get("alpha", "oauth-client-secret"); await store.get("beta", "oauth-client-secret"); - await store.delete("alpha", "oauth-client-secret"); + await store.delete("alpha", "oauth-client-secret").catch(() => {}); expect(onLoadAttempt).toHaveBeenCalledTimes(1); }); @@ -659,7 +686,9 @@ describe("@napi-rs/keyring loads but exposes the wrong shape", () => { await expect( store.set("alpha", "oauth-client-secret", "v"), ).rejects.toBeInstanceOf(mod.KeychainUnavailableError); - await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); + await expect(store.deleteAllForServer("alpha")).rejects.toBeInstanceOf( + mod.KeychainUnavailableError, + ); }); it("treats a namespace that throws on member access as unavailable", async () => { @@ -685,7 +714,9 @@ describe("@napi-rs/keyring loads but exposes the wrong shape", () => { ).rejects.not.toThrow(/libsecret/); // Absorbed, not escaped: a rejected cached promise would surface here // as the raw access error instead of the typed one. - await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); + await expect(store.deleteAllForServer("alpha")).rejects.toBeInstanceOf( + mod.KeychainUnavailableError, + ); }); it("accepts a well-formed namespace", async () => { @@ -719,3 +750,158 @@ describe("@napi-rs/keyring loads but exposes the wrong shape", () => { expect(await store.get("alpha", "oauth-client-secret")).toBe("shh"); }); }); + +describe("settleStoreMutations (round 8)", () => { + // The point of the helper: a rollback that starts while sibling + // mutations are still in flight can be re-broken by a late-landing + // set or delete. The first failure must not escape until every + // sibling has settled. + it("resolves when every mutation fulfills", async () => { + await expect( + settleStoreMutations([Promise.resolve(1), Promise.resolve(2)]), + ).resolves.toBeUndefined(); + }); + + it("rethrows the first failure only after every sibling settles", async () => { + // Deterministic pending sibling: released explicitly after the settle + // call is already in flight, so the ordering proof does not depend on + // wall-clock timing. + let releaseSlow!: () => void; + let slowSettled = false; + const slow = new Promise((resolve) => { + releaseSlow = () => { + slowSettled = true; + resolve(); + }; + }); + const fast = Promise.reject(new Error("first failure")); + + const settled = settleStoreMutations([fast, slow]); + // Give the helper a microtask turn: with Promise.all semantics the + // rejection would already be observable here, before `slow` settles. + await Promise.resolve(); + releaseSlow(); + + await expect(settled).rejects.toThrow("first failure"); + expect(slowSettled).toBe(true); + }); + + it("secretStoreSetMany's fallback settles in-flight sets before rejecting", async () => { + const landed: string[] = []; + let releaseLate!: () => void; + const late = new Promise((resolve) => { + releaseLate = resolve; + }); + // No `setMany`, so the fallback path runs. One set fails fast, the + // other lands only when explicitly released — a compensating caller + // must not observe the failure while the late set is still in flight. + const store: SecretStore = { + get: async () => null, + set: async (_id, field) => { + if (field === "fails-fast") throw new Error("keychain gone"); + await late; + landed.push(field); + }, + delete: async () => {}, + deleteAllForServer: async () => {}, + }; + + const setMany = secretStoreSetMany(store, "srv", { + "fails-fast": "a", + "lands-late": "b", + }); + await Promise.resolve(); + releaseLate(); + + await expect(setMany).rejects.toThrow("keychain gone"); + expect(landed).toEqual(["lands-late"]); + }); +}); + +describe("secretStoreGetManyStrict", () => { + // The bulk twin of the getStrict seam: a store that cannot be read must + // fail OAuth hydration rather than answer empty maps — an "outage read as + // absence" would make the next sectioned save delete the hidden secrets. + async function secretStoreModule() { + return await import("@inspector/core/auth/node/secret-store.js"); + } + + it("uses the store's getManyStrict when present", async () => { + const { secretStoreGetManyStrict } = await secretStoreModule(); + const store = { + async get() { + return null; + }, + async set() {}, + async delete() {}, + async deleteAllForServer() {}, + async getManyStrict() { + return { srv: { "env:A": "1" } }; + }, + }; + expect( + await secretStoreGetManyStrict(store, [ + { serverId: "srv", fields: ["env:A"] }, + ]), + ).toEqual({ srv: { "env:A": "1" } }); + }); + + it("falls back to per-field strict reads, propagating their failure", async () => { + const { secretStoreGetManyStrict } = await secretStoreModule(); + const store = { + async get() { + // Tolerant read answers null — the strict path must not use it. + return null; + }, + async getStrict(): Promise { + throw new Error("store unreadable"); + }, + async set() {}, + async delete() {}, + async deleteAllForServer() {}, + }; + await expect( + secretStoreGetManyStrict(store, [{ serverId: "srv", fields: ["env:A"] }]), + ).rejects.toThrow("store unreadable"); + }); + + it("collects values and skips absent fields in the fallback", async () => { + const { secretStoreGetManyStrict, InMemorySecretStore } = + await secretStoreModule(); + const store = new InMemorySecretStore(); + await store.set("srv", "env:A", "1"); + expect( + await secretStoreGetManyStrict(store, [ + { serverId: "srv", fields: ["env:A", "env:MISSING"] }, + { serverId: "other", fields: ["env:B"] }, + ]), + ).toEqual({ srv: { "env:A": "1" }, other: {} }); + }); + + it("both fallbacks return prototype-named fields as own entries", async () => { + // The inner field map is built with dynamic keys too: a field named + // "__proto__" written with plain assignment would invoke the inherited + // setter and silently vanish from the result, violating the bulk-read + // contract the same way an unsafe outer `out[serverId]` write does. + const { + secretStoreGetMany, + secretStoreGetManyStrict, + InMemorySecretStore, + } = await secretStoreModule(); + const store = new InMemorySecretStore(); + await store.set("srv", "__proto__", "field-value"); + await store.set("__proto__", "env:A", "server-value"); + for (const read of [secretStoreGetMany, secretStoreGetManyStrict]) { + const out = await read(store, [ + { serverId: "srv", fields: ["__proto__"] }, + { serverId: "__proto__", fields: ["env:A"] }, + ]); + expect(Object.getOwnPropertyDescriptor(out.srv, "__proto__")?.value).toBe( + "field-value", + ); + expect(Object.getOwnPropertyDescriptor(out, "__proto__")?.value).toEqual({ + "env:A": "server-value", + }); + } + }); +}); diff --git a/clients/web/src/test/integration/auth/node/storage.test.ts b/clients/web/src/test/integration/auth/node/storage.test.ts index d78d4a9f92..08d849dd84 100644 --- a/clients/web/src/test/integration/auth/node/storage.test.ts +++ b/clients/web/src/test/integration/auth/node/storage.test.ts @@ -14,6 +14,7 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; import * as os from "node:os"; import { flushStoreFileWrites } from "@inspector/core/storage/store-io.js"; +import { createFileOAuthPersistBackend } from "@inspector/core/auth/node/oauth-persist-file.js"; // Unique path per process so parallel test files don't share the same state file const testStatePath = path.join( @@ -434,6 +435,33 @@ describe("NodeOAuthStorage", () => { expect(await otherView.getTokens(serverUrl)).toEqual(tokens); }); + it("a second instance for the same path does not reload disk state over live memory", async () => { + // Instances for one path share memory AND load/persist coordination + // (storage-node.ts). With a per-instance load latch, the second + // instance's first load() would replace() the shared memory with what is + // on disk — reverting, in memory, a mutation the first instance already + // reported as saved (the deferred persist snapshot would then persist the + // reverted state too). + const serverUrl = "http://localhost:3000"; + const tokens: OAuthTokens = { + access_token: "live-token", + token_type: "Bearer", + }; + await storage.saveTokens(serverUrl, tokens); + await flushStoreFileWrites(testStatePath); + + // Rewrite the state file out-of-band, as another process would. + const backend = createFileOAuthPersistBackend({ filePath: testStatePath }); + const onDisk = await backend.read(); + const mutated = JSON.parse( + JSON.stringify(onDisk).replaceAll("live-token", "disk-token"), + ) as NonNullable; + await backend.write(mutated); + + const second = new NodeOAuthStorage(testStatePath); + expect(await second.getTokens(serverUrl)).toEqual(tokens); + }); + it("persists state to file on save", async () => { const persistTestPath = path.join( os.tmpdir(), @@ -532,9 +560,10 @@ describe("NodeOAuthStorage with custom storagePath", () => { await fs.readFile(customPath, "utf-8"), ) as StateShape; - expect(parsed.servers[testServerUrl]?.tokens?.access_token).toBe( - tokens.access_token, - ); + // The file keeps only the entry's residue — tokens are split into the + // secret store, so they must NOT appear at the custom path. + expect(parsed.servers[testServerUrl]).toBeDefined(); + expect(parsed.servers[testServerUrl]?.tokens).toBeUndefined(); const stored = await storage.getTokens(testServerUrl); expect(stored?.access_token).toBe(tokens.access_token); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-ema-e2e.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-ema-e2e.test.ts index 3f09b39189..bbd13a893b 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-ema-e2e.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-ema-e2e.test.ts @@ -23,6 +23,7 @@ import { NodeOAuthStorage, } from "@inspector/core/auth/node/index.js"; import { flushStoreFileWrites } from "@inspector/core/storage/store-io.js"; +import { readOAuthStore } from "@inspector/core/auth/node/oauth-persist-file.js"; import { TestServerHttp, getDefaultServerConfig, @@ -198,13 +199,20 @@ describe("InspectorClient EMA E2E", () => { await client.connect(); await flushStoreFileWrites(oauthTestStatePath); + // Tokens live in the secret store; the file keeps the residue (including + // the enterpriseManaged tag). Assert via the joined read, plus that the + // file itself carries no plaintext token. const raw = JSON.parse(await fs.readFile(oauthTestStatePath, "utf-8")) as { servers: Record< string, { tokens?: { access_token?: string }; enterpriseManaged?: boolean } >; }; - const entry = raw.servers[mcpUrl]; + expect(raw.servers[mcpUrl]?.tokens).toBeUndefined(); + expect(raw.servers[mcpUrl]?.enterpriseManaged).toBe(true); + + const joined = await readOAuthStore(oauthTestStatePath); + const entry = joined?.servers[mcpUrl]; expect(entry?.tokens?.access_token).toBeDefined(); expect(entry?.enterpriseManaged).toBe(true); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts index dfbedba9d5..86f3b2b678 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-e2e.test.ts @@ -30,6 +30,7 @@ import { } from "@modelcontextprotocol/inspector-test-server"; import { discoverAuthorizationServerMetadata } from "@modelcontextprotocol/client"; import { flushStoreFileWrites } from "@inspector/core/storage/store-io.js"; +import { readOAuthStore } from "@inspector/core/auth/node/oauth-persist-file.js"; import { createOAuthClientConfig, completeOAuthAuthorization, @@ -1204,17 +1205,22 @@ describe("InspectorClient OAuth E2E", () => { ) as StateShape; const servers = parsed.servers ?? {}; expect(Object.keys(servers).length).toBeGreaterThan(0); - // SEP-2352: tokens persist under `byIssuer[issuer].tokens`; accept the - // legacy top-level slot too. - expect( - Object.values(servers).some( - (s) => - !!s?.tokens?.access_token || - Object.values(s?.byIssuer ?? {}).some( + // Tokens are split into the secret store — the file at the custom + // path holds only residue, never a plaintext access token. + const hasPlaintextToken = (s: StateShape["servers"]): boolean => + Object.values(s ?? {}).some( + (entry) => + !!entry?.tokens?.access_token || + Object.values(entry?.byIssuer ?? {}).some( (slot) => !!slot?.tokens?.access_token, ), - ), - ).toBe(true); + ); + expect(hasPlaintextToken(servers)).toBe(false); + // The joined read (residue + secret store) still yields the tokens. + // SEP-2352: tokens persist under `byIssuer[issuer].tokens`; accept the + // legacy top-level slot too. + const joined = await readOAuthStore(customPath); + expect(hasPlaintextToken(joined?.servers)).toBe(true); } finally { try { await fs.unlink(customPath); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-oauth-remote-storage-e2e.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-oauth-remote-storage-e2e.test.ts index 6cbdec2603..792520911f 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-oauth-remote-storage-e2e.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-oauth-remote-storage-e2e.test.ts @@ -208,7 +208,6 @@ describe("InspectorClient OAuth E2E with Remote Storage", () => { }); const remoteStorage = new RemoteOAuthStorage({ baseUrl: remoteBaseUrl!, - storeId: "oauth", authToken: remoteAuthToken!, }); @@ -299,7 +298,6 @@ describe("InspectorClient OAuth E2E with Remote Storage", () => { }); const remoteStorage = new RemoteOAuthStorage({ baseUrl: remoteBaseUrl!, - storeId: "oauth", authToken: remoteAuthToken!, }); @@ -378,7 +376,6 @@ describe("InspectorClient OAuth E2E with Remote Storage", () => { // Second client: should load persisted state const remoteStorage2 = new RemoteOAuthStorage({ baseUrl: remoteBaseUrl!, - storeId: "oauth", authToken: remoteAuthToken!, }); @@ -468,7 +465,6 @@ describe("InspectorClient OAuth E2E with Remote Storage", () => { }); const remoteStorage = new RemoteOAuthStorage({ baseUrl: remoteBaseUrl!, - storeId: "oauth", authToken: remoteAuthToken!, }); diff --git a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts index a4fc1aa935..fb0c0ac2b8 100644 --- a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts +++ b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts @@ -12,6 +12,7 @@ import { rmSync, existsSync, writeFileSync, + chmodSync, } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -1769,6 +1770,49 @@ describe("/api/servers routes", () => { }); expect(res.status).toBe(400); }); + + it("rejects `__proto__` with 400 but keeps other prototype names manageable", async () => { + // `__proto__` is the one prototype name a plain assignment can't + // store, so it is refused with a message that says why (it satisfies + // the stated character-class rule). + const res = await fetch(`${h.baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "__proto__", + config: { type: "streamable-http", url: "https://x.test/mcp" }, + }), + }); + expect(res.status).toBe(400); + const body = (await res.json()) as { error: string }; + expect(body.error).toContain("__proto__"); + + // Other Object.prototype names were valid ids before the `__proto__` + // rejection existed, so they must remain fully manageable: create, + // list, and delete all work (membership checks use `Object.hasOwn`, + // so `constructor` is not a false duplicate on an empty map). + for (const id of ["constructor", "toString", "hasOwnProperty"]) { + const created = await fetch(`${h.baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id, + config: { type: "streamable-http", url: "https://x.test/mcp" }, + }), + }); + expect(created.status, id).toBe(200); + const listed = (await ( + await fetch(`${h.baseUrl}/api/servers`) + ).json()) as { + mcpServers: Record; + }; + expect(Object.hasOwn(listed.mcpServers, id), id).toBe(true); + const deleted = await fetch(`${h.baseUrl}/api/servers/${id}`, { + method: "DELETE", + }); + expect(deleted.status, id).toBe(200); + } + }); }); describe("keychain secrets (#1356)", () => { @@ -2067,6 +2111,154 @@ describe("/api/servers routes", () => { expect(srv.oauth?.scopes).toBe("read"); }); + it("PUT rename sweeps orphaned destination secrets before writing to it", async () => { + // Same reuse safeguard POST has: the destination id has no file entry + // (or the rename would 409), so any store fields under it are orphans + // from a previous failed DELETE. Left in place, fields the rename does + // not overwrite would rehydrate into the renamed server — here an + // OAuth client secret the renamed server never had. + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + "old-name": { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { clientId: "cid" }, + }, + }, + }), + ); + await h.secretStore.set( + "new-name", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "orphaned-secret", + ); + + const res = await fetch(`${h.baseUrl}/api/servers/old-name`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "new-name", + config: { type: "streamable-http", url: "https://x.test/mcp" }, + }), + }); + expect(res.status).toBe(200); + + expect( + await h.secretStore.get("new-name", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe(null); + const cfg = (await ( + await fetch(`${h.baseUrl}/api/servers`) + ).json()) as MCPConfig; + const srv = cfg.mcpServers["new-name"] as { + oauth?: { clientId?: string; clientSecret?: string }; + }; + expect(srv.oauth?.clientId).toBe("cid"); + expect(srv.oauth?.clientSecret).toBeUndefined(); + }); + + it("PUT rename carries secrets via strict reads even when tolerant reads blank out", async () => { + // The rename copies the old id's secrets and then deletes them under + // the old id — a deletion-driving read, so it must come from the + // strict path. The tolerant `get` answers null for an unreadable + // store; if the copy trusted it, a transient blank would commit the + // rename without the secret and the delete would erase the only copy. + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + "old-name": { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { clientId: "cid" }, + }, + }, + }), + ); + await h.secretStore.set( + "old-name", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "keychain-only-secret", + ); + + const store = h.secretStore as InMemorySecretStore & { + getStrict?: (id: string, field: string) => Promise; + }; + const realGet = store.get.bind(store); + store.getStrict = realGet; + store.get = async () => null; + try { + const res = await fetch(`${h.baseUrl}/api/servers/old-name`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "new-name", + config: { type: "streamable-http", url: "https://x.test/mcp" }, + }), + }); + expect(res.status).toBe(200); + } finally { + store.get = realGet; + delete store.getStrict; + } + + expect( + await h.secretStore.get("old-name", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe(null); + expect( + await h.secretStore.get("new-name", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("keychain-only-secret"); + }); + + it("PUT rename aborts before mutating anything when the strict read fails", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + "old-name": { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { clientId: "cid" }, + }, + }, + }), + ); + await h.secretStore.set( + "old-name", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "keychain-only-secret", + ); + + const store = h.secretStore as InMemorySecretStore & { + getStrict?: (id: string, field: string) => Promise; + }; + store.getStrict = async () => { + throw new KeychainUnavailableError(new Error("keychain down")); + }; + try { + const res = await fetch(`${h.baseUrl}/api/servers/old-name`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "new-name", + config: { type: "streamable-http", url: "https://x.test/mcp" }, + }), + }); + expect(res.status).toBe(503); + } finally { + delete store.getStrict; + } + + // Nothing moved: the secret survives under the old id and the disk + // file still names it. + expect( + await h.secretStore.get("old-name", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("keychain-only-secret"); + const cfg = JSON.parse(readFileSync(h.configPath, "utf8")) as MCPConfig; + expect(Object.keys(cfg.mcpServers)).toEqual(["old-name"]); + }); + it("DELETE sweeps every keychain entry for the deleted server", async () => { writeFileSync( h.configPath, @@ -2271,6 +2463,28 @@ describe("/api/servers routes", () => { } }); + it("GET drops a hand-edited __proto__ entry instead of mangling the map", async () => { + const u = await startUnavailableHarness(); + try { + // JSON.parse keeps "__proto__" as an own key, but every downstream + // `mcpServers[id] = …` rebuild would hit the prototype setter. + // The id is reserved: normalize drops it (routes reject it via + // validateStoreId), other entries are untouched. + writeFileSync( + u.configPath, + '{"mcpServers": {"plain": {"type": "stdio", "command": "node"}, "__proto__": {"type": "stdio", "command": "evil"}}}', + ); + const res = await fetch(`${u.baseUrl}/api/servers`); + expect(res.status).toBe(200); + const body = (await res.json()) as MCPConfig; + expect(body.mcpServers.plain).toBeDefined(); + expect(Object.hasOwn(body.mcpServers, "__proto__")).toBe(false); + } finally { + await new Promise((r) => u.server.close(() => r())); + rmSync(u.tempDir, { recursive: true }); + } + }); + it("GET preserves disk plaintext when migration can't write to the keychain", async () => { const u = await startUnavailableHarness(); try { @@ -2997,3 +3211,249 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { expect(readFileSync(configPath, "utf-8")).not.toContain("must-survive"); }); }); + +describe("catalog mutations are all-or-nothing (file/keychain compensation)", () => { + // A store whose destructive operations can be switched to fail, for + // exercising the confirmed-delete contract inside the catalog routes: + // a failure after the disk write must restore the pre-request state + // (disk and keychain), not half-apply the mutation. + class FailingDeleteStore extends InMemorySecretStore { + failFieldDeletes = false; + failPurges = false; + // When set, `deleteAllForServer` removes this one field and then + // throws — modeling the keyring backend, whose purge deletes + // credentials sequentially and is not atomic. + partialPurgeField: string | null = null; + override async delete(serverId: string, field: string): Promise { + if (this.failFieldDeletes) { + throw new KeychainUnavailableError(new Error("keychain locked")); + } + return super.delete(serverId, field); + } + override async deleteAllForServer(serverId: string): Promise { + if (this.partialPurgeField !== null) { + await super.delete(serverId, this.partialPurgeField); + throw new KeychainUnavailableError(new Error("keychain locked")); + } + if (this.failPurges) { + throw new KeychainUnavailableError(new Error("keychain locked")); + } + return super.deleteAllForServer(serverId); + } + } + + let tempDir: string; + let configPath: string; + let store: FailingDeleteStore; + let baseUrl: string; + let server: ServerType; + + beforeEach(async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-catalog-txn-")); + configPath = join(tempDir, "mcp.json"); + store = new FailingDeleteStore(); + ({ baseUrl, server } = await startServer(configPath, store)); + }); + + afterEach(async () => { + await new Promise((resolve) => server.close(() => resolve())); + chmodSync(tempDir, 0o755); + rmSync(tempDir, { recursive: true, force: true }); + }); + + it("PUT in-place: a failed obsolete-field delete restores disk and keychain", async () => { + writeFileSync( + configPath, + JSON.stringify({ + mcpServers: { + srv: { type: "stdio", command: "node", env: { A: "", B: "" } }, + }, + }), + ); + await store.set("srv", envSecretField("A"), "value-A"); + await store.set("srv", envSecretField("B"), "value-B"); + const before = readConfig(configPath); + + // Dropping B makes its keychain entry obsolete; the delete runs after + // the disk write, so its failure must roll the whole request back + // (the restore only *sets* prior values here, so it still works while + // deletes are down). + store.failFieldDeletes = true; + const res = await fetch(`${baseUrl}/api/servers/srv`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + config: { type: "stdio", command: "node", env: { A: "value-A2" } }, + }), + }); + store.failFieldDeletes = false; + + expect(res.status).toBe(503); + expect(readConfig(configPath)).toEqual(before); + expect(await store.get("srv", envSecretField("A"))).toBe("value-A"); + expect(await store.get("srv", envSecretField("B"))).toBe("value-B"); + }); + + it("PUT rename: a failed old-id purge restores disk and keychain", async () => { + writeFileSync( + configPath, + JSON.stringify({ + mcpServers: { + "old-name": { type: "stdio", command: "node", env: { K: "" } }, + }, + }), + ); + await store.set("old-name", envSecretField("K"), "v"); + const before = readConfig(configPath); + + // Fail only the purge (deleteAllForServer): targeted field deletes + // still work, so the compensation can remove `new-name`'s entries. + store.failPurges = true; + const res = await fetch(`${baseUrl}/api/servers/old-name`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "new-name", + config: { type: "stdio", command: "node", env: { K: "" } }, + }), + }); + store.failPurges = false; + + // Without the disk restore, the 503 would leave `new-name` on disk + // while `old-name`'s undeleted secrets are no longer indexed by any + // entry — orphaned where a retry can't find them. + expect(res.status).toBe(503); + expect(readConfig(configPath)).toEqual(before); + expect(await store.get("old-name", envSecretField("K"))).toBe("v"); + expect(await store.get("new-name", envSecretField("K"))).toBe(null); + }); + + it("PUT in-place: a failed disk write rolls the keychain values back", async () => { + writeFileSync( + configPath, + JSON.stringify({ + mcpServers: { + srv: { type: "stdio", command: "node", env: { A: "" } }, + }, + }), + ); + await store.set("srv", envSecretField("A"), "value-A"); + const before = readFileSync(configPath, "utf-8"); + + // The keychain set precedes the disk write: without compensation the + // old on-disk entry would rehydrate with the *new* secret. + chmodSync(tempDir, 0o555); + const res = await fetch(`${baseUrl}/api/servers/srv`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + config: { type: "stdio", command: "node", env: { A: "value-A2" } }, + }), + }); + chmodSync(tempDir, 0o755); + + expect(res.status).toBe(500); + expect(readFileSync(configPath, "utf-8")).toBe(before); + expect(await store.get("srv", envSecretField("A"))).toBe("value-A"); + }); + + it("POST: a failed disk write removes the just-written keychain entries", async () => { + writeFileSync(configPath, JSON.stringify({ mcpServers: {} })); + + chmodSync(tempDir, 0o555); + const res = await fetch(`${baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "newsrv", + config: { type: "stdio", command: "node", env: { A: "secret-A" } }, + }), + }); + chmodSync(tempDir, 0o755); + + expect(res.status).toBe(500); + // No disk entry indexes them, so leaving them would strand credentials; + // a retry POST must also not be trapped by leftovers. + expect(await store.get("newsrv", envSecretField("A"))).toBe(null); + }); + + it("DELETE: a failed keychain purge leaves disk and keychain untouched", async () => { + writeFileSync( + configPath, + JSON.stringify({ + mcpServers: { + srv: { type: "stdio", command: "node", env: { A: "" } }, + }, + }), + ); + await store.set("srv", envSecretField("A"), "value-A"); + const before = readConfig(configPath); + + // The purge now runs before the disk commit, so its failure must + // return a 503 with the entry still on disk and its secret intact — + // not a vanished entry with orphaned credentials. + store.failPurges = true; + const res = await fetch(`${baseUrl}/api/servers/srv`, { + method: "DELETE", + }); + store.failPurges = false; + + expect(res.status).toBe(503); + expect(readConfig(configPath)).toEqual(before); + expect(await store.get("srv", envSecretField("A"))).toBe("value-A"); + }); + + it("DELETE: a purge that fails midway restores the fields it removed", async () => { + writeFileSync( + configPath, + JSON.stringify({ + mcpServers: { + srv: { type: "stdio", command: "node", env: { A: "", B: "" } }, + }, + }), + ); + await store.set("srv", envSecretField("A"), "value-A"); + await store.set("srv", envSecretField("B"), "value-B"); + const before = readConfig(configPath); + + // The keyring purge is not atomic: it can delete some credentials + // and then throw. The compensation must cover the purge itself, not + // only the disk write, or a 503 leaves the surviving entry with part + // of its secrets gone. + store.partialPurgeField = envSecretField("A"); + const res = await fetch(`${baseUrl}/api/servers/srv`, { + method: "DELETE", + }); + store.partialPurgeField = null; + + expect(res.status).toBe(503); + expect(readConfig(configPath)).toEqual(before); + expect(await store.get("srv", envSecretField("A"))).toBe("value-A"); + expect(await store.get("srv", envSecretField("B"))).toBe("value-B"); + }); + + it("DELETE: a failed disk write restores the purged secrets", async () => { + writeFileSync( + configPath, + JSON.stringify({ + mcpServers: { + srv: { type: "stdio", command: "node", env: { A: "" } }, + }, + }), + ); + await store.set("srv", envSecretField("A"), "value-A"); + const before = readFileSync(configPath, "utf-8"); + + // The purge succeeds but the file rewrite fails: without the restore + // the entry would rehydrate with no credential on the next read. + chmodSync(tempDir, 0o555); + const res = await fetch(`${baseUrl}/api/servers/srv`, { + method: "DELETE", + }); + chmodSync(tempDir, 0o755); + + expect(res.status).toBe(500); + expect(readFileSync(configPath, "utf-8")).toBe(before); + expect(await store.get("srv", envSecretField("A"))).toBe("value-A"); + }); +}); diff --git a/clients/web/src/test/integration/mcp/remote/transport.test.ts b/clients/web/src/test/integration/mcp/remote/transport.test.ts index 98feef7ef8..a3e51234a0 100644 --- a/clients/web/src/test/integration/mcp/remote/transport.test.ts +++ b/clients/web/src/test/integration/mcp/remote/transport.test.ts @@ -974,6 +974,139 @@ describe("Remote transport e2e", () => { expect(json.error).toBe("Invalid storeId"); }); + it("sectioned POST merges named entries over the stored file", async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); + const { baseUrl, server, authToken } = await startRemoteServer(0, { + storageDir: tempDir, + }); + remoteServer = server; + const headers = { + "Content-Type": "application/json", + "x-mcp-remote-auth": `Bearer ${authToken}`, + }; + + // Another writer's state lands first (a plain whole-store write). + await fetch(`${baseUrl}/api/storage/oauth`, { + method: "POST", + headers, + body: JSON.stringify({ + servers: { "https://other.example": { scope: "other" } }, + idpSessions: { "https://idp.example": { idToken: "keep" } }, + }), + }); + + // A stale-snapshot writer that never saw the entries above posts a + // sectioned write naming only its own server — the others must survive. + // The descriptor rides in the body envelope, not the URL. + const res = await fetch(`${baseUrl}/api/storage/oauth`, { + method: "POST", + headers, + body: JSON.stringify({ + sections: { servers: ["https://mine.example"] }, + snapshot: { + servers: { "https://mine.example": { scope: "mine" } }, + idpSessions: {}, + }, + }), + }); + expect(res.status).toBe(200); + + const readRes = await fetch(`${baseUrl}/api/storage/oauth`, { + method: "GET", + headers: { "x-mcp-remote-auth": `Bearer ${authToken}` }, + }); + const stored = await readRes.json(); + expect(stored.servers).toEqual({ + "https://other.example": { scope: "other" }, + "https://mine.example": { scope: "mine" }, + }); + expect(stored.idpSessions).toEqual({ + "https://idp.example": { idToken: "keep" }, + }); + }); + + it("rejects sectioned POSTs with a bad descriptor or non-OAuth body", async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); + const { baseUrl, server, authToken } = await startRemoteServer(0, { + storageDir: tempDir, + }); + remoteServer = server; + const headers = { + "Content-Type": "application/json", + "x-mcp-remote-auth": `Bearer ${authToken}`, + }; + + const badSections = await fetch(`${baseUrl}/api/storage/oauth`, { + method: "POST", + headers, + body: JSON.stringify({ + sections: { servers: "nope" }, + snapshot: { servers: {}, idpSessions: {} }, + }), + }); + expect(badSections.status).toBe(400); + expect((await badSections.json()).error).toBe( + "OAuth store writes require an OAuth state body", + ); + + // An envelope whose snapshot is missing must not degrade into a + // full replacement. + const missingSnapshot = await fetch(`${baseUrl}/api/storage/oauth`, { + method: "POST", + headers, + body: JSON.stringify({ sections: { servers: [] } }), + }); + expect(missingSnapshot.status).toBe(400); + + const badBody = await fetch(`${baseUrl}/api/storage/oauth`, { + method: "POST", + headers, + body: JSON.stringify({ someOtherStore: true }), + }); + expect(badBody.status).toBe(400); + expect((await badBody.json()).error).toBe( + "OAuth store writes require an OAuth state body", + ); + + // The legacy query-parameter form is rejected, not ignored: + // silently dropping the descriptor would turn a stale client's + // sectioned merge into a destructive full replacement. + const legacyQuery = await fetch( + `${baseUrl}/api/storage/oauth?sections=${encodeURIComponent('{"servers":[]}')}`, + { + method: "POST", + headers, + body: JSON.stringify({ servers: {}, idpSessions: {} }), + }, + ); + expect(legacyQuery.status).toBe(400); + expect((await legacyQuery.json()).error).toBe( + "The sections descriptor moved from the ?sections query parameter to the request body", + ); + + // A body whose verbatim-extracted secret field is not a string is + // rejected up front: passed through, the split would write the raw + // value into the secret store, and one non-string value there makes + // the store refuse its entire file — corrupting every stored + // credential, not just this entry's. + const poisonSecret = await fetch(`${baseUrl}/api/storage/oauth`, { + method: "POST", + headers, + body: JSON.stringify({ + servers: { + "http://srv.example/mcp": { + clientInformation: { client_id: "cid", client_secret: 123 }, + }, + }, + idpSessions: {}, + }), + }); + expect(poisonSecret.status).toBe(400); + expect((await poisonSecret.json()).error).toBe( + "OAuth store writes require an OAuth state body", + ); + }); + it("rejects requests without auth token", async () => { tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); const { baseUrl, server } = await startRemoteServer(0, { diff --git a/clients/web/src/test/integration/storage/adapters.test.ts b/clients/web/src/test/integration/storage/adapters.test.ts index 998e60e749..2cb50c25db 100644 --- a/clients/web/src/test/integration/storage/adapters.test.ts +++ b/clients/web/src/test/integration/storage/adapters.test.ts @@ -13,6 +13,11 @@ import { NodeOAuthStorage } from "@inspector/core/auth/node/storage-node.js"; import { RemoteOAuthStorage } from "@inspector/core/auth/remote/storage-remote.js"; import { OAuthMemoryStore } from "@inspector/core/auth/store.js"; import { createFileOAuthPersistBackend } from "@inspector/core/auth/node/oauth-persist-file.js"; +import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store.js"; +import { + oauthSecretServerId, + LEGACY_TOKENS_FIELD, +} from "@inspector/core/auth/node/oauth-secrets.js"; import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; import { writeStoreFile, @@ -21,6 +26,7 @@ import { interface StartRemoteServerOptions { storageDir?: string; + secretStore?: InMemorySecretStore; } async function startRemoteServer( @@ -33,6 +39,7 @@ async function startRemoteServer( }> { const { app, authToken } = createRemoteApp({ storageDir: options.storageDir, + secretStore: options.secretStore ?? new InMemorySecretStore(), initialConfig: { defaultEnvironment: {} }, }); return new Promise((resolve, reject) => { @@ -72,7 +79,8 @@ describe("OAuth persistence", () => { it("creates store and persists state", async () => { tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); const filePath = join(tempDir!, "test-store.json"); - const storage = new NodeOAuthStorage(filePath); + const secretStore = new InMemorySecretStore(); + const storage = new NodeOAuthStorage(filePath, secretStore); await storage.saveTokens("https://example.com", { access_token: "test-token", @@ -80,9 +88,25 @@ describe("OAuth persistence", () => { }); await flushStoreFileWrites(filePath); + // Tokens are split into the secret store; the file keeps only the + // non-secret residue for the server entry. const fileContent = readFileSync(filePath, "utf-8"); const parsed = JSON.parse(fileContent); - expect(parsed.servers["https://example.com"].tokens).toEqual({ + expect(parsed.servers["https://example.com"]).toBeDefined(); + expect(parsed.servers["https://example.com"].tokens).toBeUndefined(); + expect( + JSON.parse( + (await secretStore.get( + oauthSecretServerId("https://example.com"), + LEGACY_TOKENS_FIELD, + ))!, + ), + ).toEqual({ access_token: "test-token", token_type: "Bearer" }); + + // A joined read through the backend sees the full state again. + const backend = createFileOAuthPersistBackend({ filePath, secretStore }); + const snapshot = await backend.read(); + expect(snapshot?.servers["https://example.com"].tokens).toEqual({ access_token: "test-token", token_type: "Bearer", }); @@ -91,15 +115,16 @@ describe("OAuth persistence", () => { it("loads persisted state on initialization", async () => { tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); const filePath = join(tempDir!, "test-store.json"); + const secretStore = new InMemorySecretStore(); - const storage1 = new NodeOAuthStorage(filePath); + const storage1 = new NodeOAuthStorage(filePath, secretStore); await storage1.saveTokens("https://example.com", { access_token: "initial-token", token_type: "Bearer", }); await flushStoreFileWrites(filePath); - const backend = createFileOAuthPersistBackend({ filePath }); + const backend = createFileOAuthPersistBackend({ filePath, secretStore }); const snapshot = await backend.read(); const freshMemory = new OAuthMemoryStore(snapshot ?? undefined); const state = freshMemory @@ -111,9 +136,10 @@ describe("OAuth persistence", () => { }); }); - it("reads legacy persist envelope and rewrites as plain JSON on save", async () => { + it("reads legacy persist envelope, migrates secrets, and rewrites as plain JSON on save", async () => { tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); const filePath = join(tempDir!, "test-store.json"); + const secretStore = new InMemorySecretStore(); await writeStoreFile( filePath, JSON.stringify({ @@ -129,17 +155,27 @@ describe("OAuth persistence", () => { }), ); - const storage = new NodeOAuthStorage(filePath); + const storage = new NodeOAuthStorage(filePath, secretStore); expect(await storage.getTokens("https://example.com")).toEqual({ access_token: "legacy", token_type: "Bearer", }); + // The durable store makes the read migrate: plaintext tokens move to + // the secret store and the file is stripped. + expect( + await secretStore.get( + oauthSecretServerId("https://example.com"), + LEGACY_TOKENS_FIELD, + ), + ).not.toBeNull(); + await storage.saveScope("https://example.com", "read"); await flushStoreFileWrites(filePath); const parsed = JSON.parse(readFileSync(filePath, "utf-8")); expect(parsed.servers["https://example.com"].scope).toBe("read"); + expect(parsed.servers["https://example.com"].tokens).toBeUndefined(); expect(parsed.version).toBeUndefined(); expect(parsed.state).toBeUndefined(); }); @@ -179,6 +215,115 @@ describe("OAuth persistence", () => { await backend.remove!(); expect(existsSync(filePath)).toBe(false); }); + + it("write without sections replaces the whole file (legacy path)", async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); + const filePath = join(tempDir!, "oauth.json"); + await writeStoreFile( + filePath, + JSON.stringify({ + servers: { "https://other.example": { scope: "other" } }, + idpSessions: {}, + }), + ); + await flushStoreFileWrites(filePath); + + const backend = createFileOAuthPersistBackend({ filePath }); + await backend.write({ + servers: { "https://mine.example": { scope: "mine" } }, + idpSessions: {}, + }); + await flushStoreFileWrites(filePath); + + const parsed = JSON.parse(readFileSync(filePath, "utf-8")); + expect(parsed.servers).toEqual({ + "https://mine.example": { scope: "mine" }, + }); + }); + + it("sectioned write merges only the named entries over the file", async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); + const filePath = join(tempDir!, "oauth.json"); + // Another process's state already on disk. + await writeStoreFile( + filePath, + JSON.stringify({ + servers: { "https://other.example": { scope: "other" } }, + idpSessions: { "https://idp.example": { idToken: "other-idp" } }, + }), + ); + await flushStoreFileWrites(filePath); + + const backend = createFileOAuthPersistBackend({ filePath }); + // This process's snapshot never saw the other entries — a stale + // whole-file write would erase them; the sectioned write must not. + await backend.write( + { + servers: { "https://mine.example": { scope: "mine" } }, + idpSessions: {}, + }, + { servers: ["https://mine.example"] }, + ); + await flushStoreFileWrites(filePath); + + const parsed = JSON.parse(readFileSync(filePath, "utf-8")); + expect(parsed.servers).toEqual({ + "https://other.example": { scope: "other" }, + "https://mine.example": { scope: "mine" }, + }); + expect(parsed.idpSessions).toEqual({ + "https://idp.example": { idToken: "other-idp" }, + }); + }); + + it("sectioned write propagates deletions of the named entries", async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); + const filePath = join(tempDir!, "oauth.json"); + await writeStoreFile( + filePath, + JSON.stringify({ + servers: { + "https://keep.example": { scope: "keep" }, + "https://cleared.example": { scope: "stale" }, + }, + idpSessions: {}, + }), + ); + await flushStoreFileWrites(filePath); + + const backend = createFileOAuthPersistBackend({ filePath }); + // The named server is absent from the snapshot (it was cleared) — the + // merge must delete it rather than resurrect the disk copy. + await backend.write( + { servers: {}, idpSessions: {} }, + { servers: ["https://cleared.example"] }, + ); + await flushStoreFileWrites(filePath); + + const parsed = JSON.parse(readFileSync(filePath, "utf-8")); + expect(parsed.servers).toEqual({ + "https://keep.example": { scope: "keep" }, + }); + }); + + it("sectioned write against a missing file writes just the named entries", async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); + const filePath = join(tempDir!, "oauth.json"); + const backend = createFileOAuthPersistBackend({ filePath }); + await backend.write( + { + servers: { "https://mine.example": { scope: "mine" } }, + idpSessions: {}, + }, + { servers: ["https://mine.example"] }, + ); + await flushStoreFileWrites(filePath); + const parsed = JSON.parse(readFileSync(filePath, "utf-8")); + expect(parsed).toEqual({ + servers: { "https://mine.example": { scope: "mine" } }, + idpSessions: {}, + }); + }); }); describe("flushStoreFileWrites", () => { @@ -272,7 +417,6 @@ describe("OAuth persistence", () => { const storage = new RemoteOAuthStorage({ baseUrl, - storeId: "test-store", authToken, }); @@ -281,7 +425,7 @@ describe("OAuth persistence", () => { token_type: "Bearer", }); - await waitForRemoteStore(baseUrl, "test-store", authToken, (body) => { + await waitForRemoteStore(baseUrl, "oauth", authToken, (body) => { const d = body as { servers?: Record; }; @@ -291,7 +435,7 @@ describe("OAuth persistence", () => { ); }); - const res = await fetch(`${baseUrl}/api/storage/test-store`, { + const res = await fetch(`${baseUrl}/api/storage/oauth`, { method: "GET", headers: { "x-mcp-remote-auth": `Bearer ${authToken}`, @@ -314,14 +458,13 @@ describe("OAuth persistence", () => { const storage1 = new RemoteOAuthStorage({ baseUrl, - storeId: "test-store", authToken, }); await storage1.saveTokens("https://example.com", { access_token: "initial-token", token_type: "Bearer", }); - await waitForRemoteStore(baseUrl, "test-store", authToken, (body) => { + await waitForRemoteStore(baseUrl, "oauth", authToken, (body) => { const d = body as { servers?: Record; }; @@ -333,7 +476,6 @@ describe("OAuth persistence", () => { const storage2 = new RemoteOAuthStorage({ baseUrl, - storeId: "test-store", authToken, }); @@ -352,7 +494,6 @@ describe("OAuth persistence", () => { const storage = new RemoteOAuthStorage({ baseUrl, - storeId: "test-store", authToken, }); @@ -360,12 +501,12 @@ describe("OAuth persistence", () => { access_token: "test-token", token_type: "Bearer", }); - await waitForRemoteStore(baseUrl, "test-store", authToken, (body) => { + await waitForRemoteStore(baseUrl, "oauth", authToken, (body) => { const d = body as { servers?: Record }; return !!d?.servers && Object.keys(d.servers).length > 0; }); - let res = await fetch(`${baseUrl}/api/storage/test-store`, { + let res = await fetch(`${baseUrl}/api/storage/oauth`, { method: "GET", headers: { "x-mcp-remote-auth": `Bearer ${authToken}`, @@ -376,12 +517,12 @@ describe("OAuth persistence", () => { expect(Object.keys(storeData.servers).length).toBeGreaterThan(0); await storage.clear("https://example.com"); - await waitForRemoteStore(baseUrl, "test-store", authToken, (body) => { + await waitForRemoteStore(baseUrl, "oauth", authToken, (body) => { const d = body as { servers?: Record }; return !d?.servers || Object.keys(d.servers).length === 0; }); - res = await fetch(`${baseUrl}/api/storage/test-store`, { + res = await fetch(`${baseUrl}/api/storage/oauth`, { method: "GET", headers: { "x-mcp-remote-auth": `Bearer ${authToken}`, @@ -391,5 +532,127 @@ describe("OAuth persistence", () => { const emptyStore = await res.json(); expect(Object.keys(emptyStore.servers).length).toBe(0); }); + + it("plain POST fully replaces the store and splits secrets on disk", async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); + const secretStore = new InMemorySecretStore(); + const { baseUrl, server, authToken } = await startRemoteServer(0, { + storageDir: tempDir, + secretStore, + }); + remoteServer = server; + const headers = { + "Content-Type": "application/json", + "x-mcp-remote-auth": `Bearer ${authToken}`, + }; + + const post = await fetch(`${baseUrl}/api/storage/oauth`, { + method: "POST", + headers, + body: JSON.stringify({ + servers: { + "https://example.com": { + scope: "read", + tokens: { access_token: "posted", token_type: "Bearer" }, + }, + }, + idpSessions: {}, + }), + }); + expect(post.status).toBe(200); + + // On disk: residue only. Via the store: the secret. Via GET: rejoined. + const raw = JSON.parse( + readFileSync(join(tempDir, "oauth.json"), "utf-8"), + ); + expect(raw.servers["https://example.com"].scope).toBe("read"); + expect(raw.servers["https://example.com"].tokens).toBeUndefined(); + expect( + await secretStore.get( + oauthSecretServerId("https://example.com"), + LEGACY_TOKENS_FIELD, + ), + ).not.toBeNull(); + const got = await fetch(`${baseUrl}/api/storage/oauth`, { headers }); + expect((await got.json()).servers["https://example.com"].tokens).toEqual({ + access_token: "posted", + token_type: "Bearer", + }); + }); + + it("POST over the body cap is refused with 413 before parsing", async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); + const secretStore = new InMemorySecretStore(); + const { baseUrl, server, authToken } = await startRemoteServer(0, { + storageDir: tempDir, + secretStore, + }); + remoteServer = server; + const headers = { + "Content-Type": "application/json", + "x-mcp-remote-auth": `Bearer ${authToken}`, + }; + + // Just over MAX_STORAGE_BODY_BYTES (4 MiB): the bodyLimit middleware + // must reject before c.req.json() buffers it, and nothing may land on + // disk. + const oversized = `{"servers":{},"idpSessions":{},"pad":"${"x".repeat( + 4 * 1024 * 1024, + )}"}`; + const res = await fetch(`${baseUrl}/api/storage/oauth`, { + method: "POST", + headers, + body: oversized, + }); + expect(res.status).toBe(413); + expect((await res.json()).error).toBe("Storage payload too large"); + expect(existsSync(join(tempDir, "oauth.json"))).toBe(false); + }); + + it("DELETE purges the file and its secret-store entries", async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-storage-test-")); + const secretStore = new InMemorySecretStore(); + const { baseUrl, server, authToken } = await startRemoteServer(0, { + storageDir: tempDir, + secretStore, + }); + remoteServer = server; + const headers = { + "Content-Type": "application/json", + "x-mcp-remote-auth": `Bearer ${authToken}`, + }; + + await fetch(`${baseUrl}/api/storage/oauth`, { + method: "POST", + headers, + body: JSON.stringify({ + servers: { + "https://example.com": { + tokens: { access_token: "doomed", token_type: "Bearer" }, + }, + }, + idpSessions: {}, + }), + }); + expect( + await secretStore.get( + oauthSecretServerId("https://example.com"), + LEGACY_TOKENS_FIELD, + ), + ).not.toBeNull(); + + const del = await fetch(`${baseUrl}/api/storage/oauth`, { + method: "DELETE", + headers, + }); + expect(del.status).toBe(200); + expect(existsSync(join(tempDir, "oauth.json"))).toBe(false); + expect( + await secretStore.get( + oauthSecretServerId("https://example.com"), + LEGACY_TOKENS_FIELD, + ), + ).toBeNull(); + }); }); }); diff --git a/clients/web/src/test/integration/storage/oauth-secret-split.test.ts b/clients/web/src/test/integration/storage/oauth-secret-split.test.ts new file mode 100644 index 0000000000..24afda57d1 --- /dev/null +++ b/clients/web/src/test/integration/storage/oauth-secret-split.test.ts @@ -0,0 +1,1395 @@ +/** + * Integration tests for the OAuth secret split at the file boundary + * (core/auth/node/oauth-persist-file.ts): write-side split + store cleanup, + * joined reads, lazy migration of pre-split plaintext files, policy + * enforcement, and store-failure degradation. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { + mkdtempSync, + readFileSync, + rmSync, + existsSync, + writeFileSync, + chmodSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + writeOAuthSections, + readOAuthStore, + removeOAuthStore, + resetOAuthSecretStoreWarnings, + OAuthStateFileUnrecognizedError, +} from "@inspector/core/auth/node/oauth-persist-file.js"; +import { + InMemorySecretStore, + SessionSecretStore, + type SecretStore, +} from "@inspector/core/auth/node/secret-store.js"; +import { + PERSIST_TOKENS_ENV, + oauthSecretServerId, + oauthIdpSecretServerId, + issuerTokensField, + issuerClientSecretField, + LEGACY_TOKENS_FIELD, + LEGACY_CLIENT_SECRET_FIELD, + LEGACY_REGISTRATION_TOKEN_FIELD, + IDP_SESSION_FIELD, + isUsableStoredSecret, + resetPersistTokensPolicyWarnings, +} from "@inspector/core/auth/node/oauth-secrets.js"; +import { + writeStoreFile, + flushStoreFileWrites, +} from "@inspector/core/storage/store-io.js"; +import type { OAuthPersistSnapshot } from "@inspector/core/auth/oauth-persist.js"; + +const SERVER = "https://api.example/mcp"; +const ISSUER = "https://as.example"; +const TOKENS = { + access_token: "at", + token_type: "Bearer", + refresh_token: "rt", +}; + +function snapshotWith( + overrides: Partial = {}, +): OAuthPersistSnapshot { + return { + servers: { + [SERVER]: { + scope: "read", + tokens: { ...TOKENS }, + clientInformation: { client_id: "cid", client_secret: "cs" }, + }, + }, + idpSessions: {}, + ...overrides, + }; +} + +let tempDir: string; +let filePath: string; +let savedPolicy: string | undefined; + +beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-oauth-split-")); + filePath = join(tempDir, "oauth.json"); + savedPolicy = process.env[PERSIST_TOKENS_ENV]; + delete process.env[PERSIST_TOKENS_ENV]; +}); + +afterEach(() => { + if (savedPolicy === undefined) delete process.env[PERSIST_TOKENS_ENV]; + else process.env[PERSIST_TOKENS_ENV] = savedPolicy; + resetPersistTokensPolicyWarnings(); + resetOAuthSecretStoreWarnings(); + vi.restoreAllMocks(); + rmSync(tempDir, { recursive: true, force: true }); +}); + +function readRawFile(): OAuthPersistSnapshot { + return JSON.parse(readFileSync(filePath, "utf8")) as OAuthPersistSnapshot; +} + +describe("writeOAuthSections secret split", () => { + it("writes only residue to the file and secrets to the store", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await flushStoreFileWrites(filePath); + + const raw = readRawFile(); + expect(raw.servers[SERVER]!.scope).toBe("read"); + expect(raw.servers[SERVER]!.tokens).toBeUndefined(); + expect(raw.servers[SERVER]!.clientInformation).toEqual({ + client_id: "cid", + }); + const id = oauthSecretServerId(SERVER); + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( + TOKENS, + ); + expect(await store.get(id, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs"); + + const joined = await readOAuthStore(filePath, store); + expect(joined?.servers[SERVER]).toEqual(snapshotWith().servers[SERVER]); + }); + + it("splits IdP sessions and rejoins them on read", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections( + filePath, + { + servers: {}, + idpSessions: { + [ISSUER]: { idToken: "idt", refreshToken: "rt", idTokenExpiresAt: 9 }, + }, + }, + undefined, + store, + ); + await flushStoreFileWrites(filePath); + + const raw = readRawFile(); + expect(raw.idpSessions[ISSUER]).toEqual({ idTokenExpiresAt: 9 }); + expect( + await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), + ).not.toBeNull(); + + const joined = await readOAuthStore(filePath, store); + expect(joined?.idpSessions[ISSUER]).toEqual({ + idToken: "idt", + refreshToken: "rt", + idTokenExpiresAt: 9, + }); + }); + + it("deletes store entries when a sectioned write clears the entry", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await writeOAuthSections( + filePath, + { servers: {}, idpSessions: {} }, + { servers: [SERVER] }, + store, + ); + await flushStoreFileWrites(filePath); + + const id = oauthSecretServerId(SERVER); + expect(await store.get(id, LEGACY_TOKENS_FIELD)).toBeNull(); + expect(await store.get(id, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); + expect(readRawFile().servers[SERVER]).toBeUndefined(); + }); + + it("deletes a removed issuer's store fields (candidates span old and new shapes)", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections( + filePath, + { + servers: { + [SERVER]: { + byIssuer: { + [ISSUER]: { + tokens: { ...TOKENS }, + clientInformation: { client_id: "c", client_secret: "s" }, + }, + }, + }, + }, + idpSessions: {}, + }, + { servers: [SERVER] }, + store, + ); + const id = oauthSecretServerId(SERVER); + expect(await store.get(id, issuerTokensField(ISSUER))).not.toBeNull(); + + await writeOAuthSections( + filePath, + { servers: { [SERVER]: { scope: "read" } }, idpSessions: {} }, + { servers: [SERVER] }, + store, + ); + expect(await store.get(id, issuerTokensField(ISSUER))).toBeNull(); + expect(await store.get(id, issuerClientSecretField(ISSUER))).toBeNull(); + }); + + it("enforces the persist-tokens policy and self-cleans on downgrade", async () => { + const store = new InMemorySecretStore(); + const id = oauthSecretServerId(SERVER); + + process.env[PERSIST_TOKENS_ENV] = "access"; + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual({ + access_token: "at", + token_type: "Bearer", + }); + + process.env[PERSIST_TOKENS_ENV] = "none"; + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + expect(await store.get(id, LEGACY_TOKENS_FIELD)).toBeNull(); + // Client secrets are registration credentials, not acquired tokens. + expect(await store.get(id, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs"); + }); + + it("deletes an IdP session's store entry when a sectioned write clears it", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections( + filePath, + { servers: {}, idpSessions: { [ISSUER]: { idToken: "idt" } } }, + undefined, + store, + ); + const id = oauthIdpSecretServerId(ISSUER); + expect(await store.get(id, IDP_SESSION_FIELD)).not.toBeNull(); + + // Sections naming only idpSessions also exercises the servers-omitted + // side of a partial descriptor. + await writeOAuthSections( + filePath, + { servers: {}, idpSessions: {} }, + { idpSessions: [ISSUER] }, + store, + ); + await flushStoreFileWrites(filePath); + expect(await store.get(id, IDP_SESSION_FIELD)).toBeNull(); + expect(readRawFile().idpSessions[ISSUER]).toBeUndefined(); + }); + + it("stringifies a non-Error store failure in the warning", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const failing: SecretStore = { + get: async () => null, + set: async () => { + throw "not an Error object"; + }, + delete: async () => {}, + deleteAllForServer: async () => {}, + }; + await writeOAuthSections(filePath, snapshotWith(), undefined, failing); + expect( + warn.mock.calls.some(([msg]) => + String(msg).includes("not an Error object"), + ), + ).toBe(true); + }); + + it("degrades to memory-only with one warning when the store write fails", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const failing: SecretStore = { + get: async () => null, + set: async () => { + throw new Error("keychain says no"); + }, + delete: async () => {}, + deleteAllForServer: async () => {}, + }; + await writeOAuthSections(filePath, snapshotWith(), undefined, failing); + await writeOAuthSections(filePath, snapshotWith(), undefined, failing); + await flushStoreFileWrites(filePath); + + // A brand-new entry that fails to persist its secrets is dropped from + // the file entirely (file and store change together, or not at all) — + // its credentials stay memory-only for the session. + const raw = readRawFile(); + expect(raw.servers[SERVER]).toBeUndefined(); + const failures = warn.mock.calls.filter(([msg]) => + String(msg).includes("keychain says no"), + ); + expect(failures).toHaveLength(1); + + resetOAuthSecretStoreWarnings(); + await writeOAuthSections(filePath, snapshotWith(), undefined, failing); + expect( + warn.mock.calls.filter(([msg]) => + String(msg).includes("keychain says no"), + ), + ).toHaveLength(2); + }); + + it("rolls back a new entry's store secrets when the file write fails", async () => { + // The file is the only index of the store's entries: if the residue + // write fails after the store writes committed, a brand-new server's + // secrets would be stranded where removeOAuthStore can never find + // them. Force the write to fail by making the parent path a file. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + void warn; // silence the unlocked-write warning for the blocked path + const blocker = join(tempDir, "blocker"); + writeFileSync(blocker, "not a directory"); + const blockedPath = join(blocker, "oauth.json"); + const store = new InMemorySecretStore(); + + await expect( + writeOAuthSections(blockedPath, snapshotWith(), undefined, store), + ).rejects.toThrow(); + + // The store writes were rolled back — nothing stranded. + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD), + ).toBeNull(); + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_CLIENT_SECRET_FIELD), + ).toBeNull(); + }); + + it("warns that the store may be inconsistent when the rollback itself fails", async () => { + // A failed compensation is not a failed save: the store already changed + // and could not be put back, so the save-path warning ("tokens kept in + // memory for this session") would be false. The message must say the + // store may disagree with the file and point at re-authorization. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const store = new InMemorySecretStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await flushStoreFileWrites(filePath); + + const updated = snapshotWith(); + updated.servers[SERVER]!.tokens = { ...TOKENS, access_token: "at2" }; + // The update's own writes ("at2", unchanged "cs") succeed; only the + // rollback's attempt to put the *prior* token value back fails. + const realSet = store.set.bind(store); + store.set = async (id, field, value) => { + if (value.includes(`"access_token":"${TOKENS.access_token}"`)) { + throw new Error("store refused the restore"); + } + return realSet(id, field, value); + }; + + chmodSync(tempDir, 0o555); + try { + await expect( + writeOAuthSections(filePath, updated, undefined, store), + ).rejects.toThrow(); + } finally { + chmodSync(tempDir, 0o755); + } + + expect( + warn.mock.calls.some(([msg]) => + String(msg).includes( + "Could not restore secret-store entries after a failed OAuth state write", + ), + ), + ).toBe(true); + // The save-path wording must not appear: nothing here is "kept in + // memory" — the store diverged from the file and could not be put back. + expect( + warn.mock.calls.some(([msg]) => + String(msg).includes("kept in memory for this session"), + ), + ).toBe(false); + }); + + it("restores an indexed entry's prior store secrets when the file write fails", async () => { + // An already-indexed entry is not rolled back by deletion — its old + // residue is still on disk, so the store must be restored to the *old* + // values or the next read joins the old residue (e.g. the previous + // client_id) with the new secrets. Force the second write to fail by + // making the directory read-only after the first commit. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + void warn; // silence the unlocked-write warning for the read-only dir + const store = new InMemorySecretStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await flushStoreFileWrites(filePath); + + const updated = snapshotWith(); + updated.servers[SERVER]!.tokens = { + ...TOKENS, + access_token: "at2", + refresh_token: "rt2", + }; + updated.servers[SERVER]!.clientInformation = { + client_id: "cid2", + client_secret: "cs2", + }; + + chmodSync(tempDir, 0o555); + try { + await expect( + writeOAuthSections(filePath, updated, undefined, store), + ).rejects.toThrow(); + } finally { + chmodSync(tempDir, 0o755); + } + + // The store holds the *old* secrets again, matching the old residue + // still on disk — no cid/cs2 mismatch on the next joined read. + const id = oauthSecretServerId(SERVER); + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( + TOKENS, + ); + expect(await store.get(id, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs"); + const joined = await readOAuthStore(filePath, store); + expect(joined?.servers[SERVER]).toEqual(snapshotWith().servers[SERVER]); + }); + + it("deduplicates sections: rollback restores the true prior value", async () => { + // A duplicated URL would make the second pass snapshot the value the + // first pass just wrote, and a rollback would then finish by + // "restoring" that intermediate value over the real prior one. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + void warn; // silence the unlocked-write warning for the read-only dir + const store = new InMemorySecretStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await flushStoreFileWrites(filePath); + + const updated = snapshotWith(); + updated.servers[SERVER]!.clientInformation = { + client_id: "cid", + client_secret: "cs2", + }; + + chmodSync(tempDir, 0o555); + try { + await expect( + writeOAuthSections( + filePath, + updated, + { servers: [SERVER, SERVER], idpSessions: [] }, + store, + ), + ).rejects.toThrow(); + } finally { + chmodSync(tempDir, 0o755); + } + + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_CLIENT_SECRET_FIELD), + ).toBe("cs"); + }); + + it("aborts the write when a store delete fails, keeping the old residue", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await flushStoreFileWrites(filePath); + + // Same store contents, but deletes now fail (keychain went away). + const failingDelete: SecretStore = { + get: (id, f) => store.get(id, f), + set: (id, f, v) => store.set(id, f, v), + delete: async () => { + throw new Error("keychain unavailable"); + }, + deleteAllForServer: async () => { + throw new Error("keychain unavailable"); + }, + }; + + // Clear the tokens: the split produces no `tokens` secret, so the + // write must delete the store copy — if that fails, committing the + // residue would let the next read resurrect the cleared tokens. + const cleared: OAuthPersistSnapshot = { + servers: { + [SERVER]: { + scope: "read", + clientInformation: { client_id: "cid", client_secret: "cs" }, + }, + }, + idpSessions: {}, + }; + await expect( + writeOAuthSections( + filePath, + cleared, + { servers: [SERVER] }, + failingDelete, + ), + ).rejects.toThrow("keychain unavailable"); + + // Entry removal (purge) failures abort too, for the same reason. + await expect( + writeOAuthSections( + filePath, + { servers: {}, idpSessions: {} }, + { servers: [SERVER] }, + failingDelete, + ), + ).rejects.toThrow("keychain unavailable"); + }); + + it("persists and rejoins entries keyed __proto__ instead of dropping them", async () => { + // Server URLs and issuers are attacker-influenceable map keys. A plain + // assignment while building residue would hit the prototype setter: + // the write reports success, the secrets land in the store, but the + // file serializes no entry — unindexed credentials. + const store = new InMemorySecretStore(); + const snapshot: OAuthPersistSnapshot = { + servers: JSON.parse( + JSON.stringify({ + x: { + scope: "read", + tokens: { ...TOKENS }, + clientInformation: { client_id: "cid", client_secret: "cs" }, + }, + }).replace('"x"', '"__proto__"'), + ), + idpSessions: JSON.parse('{"__proto__": {"idToken": "idt"}}'), + }; + await writeOAuthSections( + filePath, + snapshot, + { servers: ["__proto__"], idpSessions: ["__proto__"] }, + store, + ); + await flushStoreFileWrites(filePath); + + const raw = readRawFile(); + expect(Object.hasOwn(raw.servers, "__proto__")).toBe(true); + expect(Object.hasOwn(raw.idpSessions, "__proto__")).toBe(true); + + const joined = await readOAuthStore(filePath, store); + const entry = Object.entries(joined!.servers).find( + ([url]) => url === "__proto__", + )?.[1]; + expect(entry?.tokens).toEqual(TOKENS); + expect(entry?.clientInformation?.client_secret).toBe("cs"); + + // And a clear must propagate: with the entry gone from the snapshot, a + // plain lookup for "__proto__" would return the inherited prototype and + // process the clear as an update — leaving an empty residue entry on + // disk and the secrets alive in the store. + await writeOAuthSections( + filePath, + { servers: {}, idpSessions: {} }, + { servers: ["__proto__"], idpSessions: ["__proto__"] }, + store, + ); + await flushStoreFileWrites(filePath); + const cleared = readRawFile(); + expect(Object.hasOwn(cleared.servers, "__proto__")).toBe(false); + expect(Object.hasOwn(cleared.idpSessions, "__proto__")).toBe(false); + const rejoined = await readOAuthStore(filePath, store); + expect(Object.hasOwn(rejoined?.servers ?? {}, "__proto__")).toBe(false); + // The store's secret fields were purged, not orphaned. + expect(await store.get("oauth+__proto__", "client-secret")).toBeNull(); + expect(await store.get("oauth+__proto__", "tokens")).toBeNull(); + }); +}); + +describe("readOAuthStore migration", () => { + it("migrates with policy `all`: existing tokens are moved, not destroyed", async () => { + // The persist-tokens policy is write-side. Migration relocates + // already-persisted credentials; under `none` it must not silently + // destroy them on the first read (the next save applies the policy). + process.env[PERSIST_TOKENS_ENV] = "none"; + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + + const snapshot = await readOAuthStore(filePath, store); + expect(snapshot?.servers[SERVER]!.tokens).toEqual(TOKENS); + expect(readRawFile().servers[SERVER]!.tokens).toBeUndefined(); + const raw = await store.get( + oauthSecretServerId(SERVER), + LEGACY_TOKENS_FIELD, + ); + expect(JSON.parse(raw!)).toEqual(TOKENS); + }); + + it("migration keeps refresh tokens under policy `access`", async () => { + process.env[PERSIST_TOKENS_ENV] = "access"; + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + + const snapshot = await readOAuthStore(filePath, store); + expect(snapshot?.servers[SERVER]!.tokens).toEqual(TOKENS); + }); + + it("migration is store-wins: an existing store value is not overwritten", async () => { + // The store can legitimately be ahead of a plaintext file (a newer + // write whose residue commit failed, a restored file backup) — copying + // the plaintext over it would roll credentials back. Mirror the + // mcp.json/client.json migrations: copy only where the store is empty. + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + const id = oauthSecretServerId(SERVER); + const newerTokens = { ...TOKENS, access_token: "newer-at" }; + await store.set(id, LEGACY_TOKENS_FIELD, JSON.stringify(newerTokens)); + + const snapshot = await readOAuthStore(filePath, store); + + // The newer store tokens survive; the plaintext client secret (absent + // from the store) is still migrated; the file is stripped either way. + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( + newerTokens, + ); + expect(await store.get(id, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs"); + expect(readRawFile().servers[SERVER]!.tokens).toBeUndefined(); + expect(snapshot?.servers[SERVER]!.tokens).toEqual(newerTokens); + }); + + it("store-wins requires a usable value: corrupt store tokens are replaced", async () => { + // A malformed store value would be discarded by the read-side join, so + // treating it as authoritative would strip the valid plaintext and lose + // the token entirely. Migration must replace it from the plaintext. + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + const id = oauthSecretServerId(SERVER); + await store.set(id, LEGACY_TOKENS_FIELD, "corrupt {not json"); + + const snapshot = await readOAuthStore(filePath, store); + + expect(snapshot?.servers[SERVER]!.tokens).toEqual(TOKENS); + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( + TOKENS, + ); + expect(readRawFile().servers[SERVER]!.tokens).toBeUndefined(); + }); + + it("partial stored tokens are honored over stale plaintext; junk is replaced", async () => { + // `{ access_token }` without `token_type` is a legitimate store value + // under the shared partial-schema contract — a newer save may have + // written it while its residue commit failed, leaving stale plaintext + // behind. Store-wins applies to it like any full token set. Only a + // value the join rejects (type-corrupt junk) is replaced by migration. + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + const id = oauthSecretServerId(SERVER); + const partial = { access_token: "incomplete" }; + await store.set(id, LEGACY_TOKENS_FIELD, JSON.stringify(partial)); + + const snapshot = await readOAuthStore(filePath, store); + + expect(snapshot?.servers[SERVER]!.tokens).toEqual(partial); + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( + partial, + ); + + // Type-corrupt junk in the store is not usable: migration replaces it + // with the valid plaintext instead of honoring it. + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + await store.set( + id, + LEGACY_TOKENS_FIELD, + JSON.stringify({ access_token: 123 }), + ); + const replaced = await readOAuthStore(filePath, store); + expect(replaced?.servers[SERVER]!.tokens).toEqual(TOKENS); + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( + TOKENS, + ); + }); + + it("read fails when the store cannot be read, instead of joining empty", async () => { + // A tolerant bulk read during a store outage would hydrate memory with + // every credential absent — and the next sectioned save would *delete* + // them from the store. The read must fail, not masquerade as empty. + const store = new InMemorySecretStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await flushStoreFileWrites(filePath); + const outage: SecretStore = { + // Tolerant read still answers null — hydration must not use it. + get: async () => null, + getStrict: async () => { + throw new Error("store outage"); + }, + set: (...args) => store.set(...args), + delete: (...args) => store.delete(...args), + deleteAllForServer: (id) => store.deleteAllForServer(id), + }; + + await expect(readOAuthStore(filePath, outage)).rejects.toThrow( + "store outage", + ); + }); + + it("migrates a plaintext file into a durable store on read", async () => { + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + + const snapshot = await readOAuthStore(filePath, store); + expect(snapshot?.servers[SERVER]).toEqual(snapshotWith().servers[SERVER]); + + const raw = readRawFile(); + expect(raw.servers[SERVER]!.tokens).toBeUndefined(); + expect(raw.servers[SERVER]!.clientInformation).toEqual({ + client_id: "cid", + }); + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD), + ).not.toBeNull(); + }); + + it("migrates a plaintext registration_access_token with no client_secret", async () => { + // The RFC 7592 management credential alone must trigger the migration + // sweep — key-by-key detection used to leave it plaintext when no + // client_secret sat beside it. + await writeStoreFile( + filePath, + JSON.stringify({ + servers: { + [SERVER]: { + clientInformation: { + client_id: "cid", + registration_access_token: "rat", + }, + }, + }, + idpSessions: {}, + }), + ); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + + const snapshot = await readOAuthStore(filePath, store); + expect(snapshot?.servers[SERVER]!.clientInformation).toEqual({ + client_id: "cid", + registration_access_token: "rat", + }); + + expect(readRawFile().servers[SERVER]!.clientInformation).toEqual({ + client_id: "cid", + }); + expect( + await store.get( + oauthSecretServerId(SERVER), + LEGACY_REGISTRATION_TOKEN_FIELD, + ), + ).toBe("rat"); + }); + + it("migrates plaintext IdP sessions too", async () => { + await writeStoreFile( + filePath, + JSON.stringify({ + servers: {}, + idpSessions: { [ISSUER]: { idToken: "idt", idTokenExpiresAt: 3 } }, + }), + ); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + + const snapshot = await readOAuthStore(filePath, store); + expect(snapshot?.idpSessions[ISSUER]).toEqual({ + idToken: "idt", + idTokenExpiresAt: 3, + }); + expect(readRawFile().idpSessions[ISSUER]).toEqual({ idTokenExpiresAt: 3 }); + }); + + it("leaves a plaintext file untouched when the store is not durable", async () => { + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new SessionSecretStore(); + + const snapshot = await readOAuthStore(filePath, store); + expect(snapshot?.servers[SERVER]!.tokens).toEqual(TOKENS); + expect(readRawFile().servers[SERVER]!.tokens).toEqual(TOKENS); + }); + + it("keeps unchanged plaintext secrets durable when a non-durable store writes the entry", async () => { + // The read-side guard alone is not enough: a mutation of an unrelated + // field (here: scope) flows the joined entry back through the write + // split, and an unconditional strip would demote the file's only + // durable token copy to memory-only. + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new SessionSecretStore(); + + const joined = await readOAuthStore(filePath, store); + const mutated: OAuthPersistSnapshot = { + servers: { + [SERVER]: { ...joined!.servers[SERVER]!, scope: "read write" }, + }, + idpSessions: {}, + }; + await writeOAuthSections(filePath, mutated, { servers: [SERVER] }, store); + await flushStoreFileWrites(filePath); + + const raw = readRawFile(); + expect(raw.servers[SERVER]!.scope).toBe("read write"); + // Unchanged secrets stay in the file — still the only durable copy. + expect(raw.servers[SERVER]!.tokens).toEqual(TOKENS); + expect(raw.servers[SERVER]!.clientInformation).toEqual({ + client_id: "cid", + client_secret: "cs", + }); + }); + + it("keeps new or changed secrets session-only under a non-durable store", async () => { + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new SessionSecretStore(); + + const reauthed: OAuthPersistSnapshot = { + servers: { + [SERVER]: { + scope: "read", + tokens: { access_token: "at2", token_type: "Bearer" }, + clientInformation: { client_id: "cid", client_secret: "cs" }, + }, + }, + idpSessions: {}, + }; + await writeOAuthSections(filePath, reauthed, { servers: [SERVER] }, store); + await flushStoreFileWrites(filePath); + + const raw = readRawFile(); + // The changed tokens are session-only (memory-store contract) … + expect(raw.servers[SERVER]!.tokens).toBeUndefined(); + // … while the unchanged client secret stays durable in the file. + expect(raw.servers[SERVER]!.clientInformation).toEqual({ + client_id: "cid", + client_secret: "cs", + }); + const joined = await readOAuthStore(filePath, store); + expect(joined?.servers[SERVER]!.tokens).toEqual({ + access_token: "at2", + token_type: "Bearer", + }); + }); + + it("compares with the active policy: `access` keeps the unchanged access token durable", async () => { + process.env[PERSIST_TOKENS_ENV] = "access"; + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new SessionSecretStore(); + + const joined = await readOAuthStore(filePath, store); + const mutated: OAuthPersistSnapshot = { + servers: { + [SERVER]: { ...joined!.servers[SERVER]!, scope: "read write" }, + }, + idpSessions: {}, + }; + await writeOAuthSections(filePath, mutated, { servers: [SERVER] }, store); + await flushStoreFileWrites(filePath); + + // The raw disk blob still carried its refresh token while the split + // never does under `access` — the compare must be policy-to-policy or + // the unchanged access token would be wrongly treated as changed and + // stripped from the only durable copy. + const raw = readRawFile(); + expect(raw.servers[SERVER]!.tokens).toEqual({ + access_token: "at", + token_type: "Bearer", + }); + }); + + it("preserves an unchanged plaintext IdP session under a non-durable store", async () => { + const session = { idToken: "idt", refreshToken: "idprt" }; + await writeStoreFile( + filePath, + JSON.stringify({ + servers: {}, + idpSessions: { [ISSUER]: { ...session, idTokenExpiresAt: 1 } }, + }), + ); + await flushStoreFileWrites(filePath); + const store = new SessionSecretStore(); + + const joined = await readOAuthStore(filePath, store); + const mutated: OAuthPersistSnapshot = { + servers: {}, + idpSessions: { + [ISSUER]: { ...joined!.idpSessions[ISSUER]!, idTokenExpiresAt: 2 }, + }, + }; + await writeOAuthSections( + filePath, + mutated, + { idpSessions: [ISSUER] }, + store, + ); + await flushStoreFileWrites(filePath); + + const raw = readRawFile(); + expect(raw.idpSessions[ISSUER]).toMatchObject({ + ...session, + idTokenExpiresAt: 2, + }); + }); + + it("aborts the strip when the store write fails, keeping the plaintext usable", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const failing: SecretStore = { + get: async () => null, + set: async () => { + throw new Error("store down"); + }, + delete: async () => {}, + deleteAllForServer: async () => {}, + }; + + const snapshot = await readOAuthStore(filePath, failing); + expect(snapshot?.servers[SERVER]!.tokens).toEqual(TOKENS); + expect(readRawFile().servers[SERVER]!.tokens).toEqual(TOKENS); + expect( + warn.mock.calls.some(([msg]) => String(msg).includes("store down")), + ).toBe(true); + // The migration warning must not claim the tokens went memory-only — + // the plaintext file was kept and keeps working. + expect( + warn.mock.calls.some(([msg]) => + String(msg).includes("plaintext copy in oauth.json was kept"), + ), + ).toBe(true); + }); + + it("warns once per reason for repeated migration failures, non-Error included", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const failing: SecretStore = { + get: async () => null, + set: async () => { + // deliberately a bare string + throw "string failure"; + }, + delete: async () => {}, + deleteAllForServer: async () => {}, + }; + + await readOAuthStore(filePath, failing); + await readOAuthStore(filePath, failing); + const migrationWarnings = warn.mock.calls.filter(([msg]) => + String(msg).includes("string failure"), + ); + expect(migrationWarnings).toHaveLength(1); + }); + + it("returns null for a missing file", async () => { + expect(await readOAuthStore(filePath, new InMemorySecretStore())).toBe( + null, + ); + }); + + it("uses the selected default store when none is injected", async () => { + // The vitest config pins MCP_INSPECTOR_SECRET_STORE=memory, so the + // default-parameter paths resolve to the in-process memory store — this + // covers the write/read/remove signatures the CLI uses. + await writeOAuthSections(filePath, snapshotWith()); + const snapshot = await readOAuthStore(filePath); + expect(snapshot?.servers[SERVER]!.tokens).toEqual(TOKENS); + await removeOAuthStore(filePath); + expect(existsSync(filePath)).toBe(false); + }); + + it("tolerates a getMany that omits a requested server id", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections( + filePath, + snapshotWith({ idpSessions: { [ISSUER]: { idToken: "idt" } } }), + undefined, + store, + ); + const withEmptyGetMany: SecretStore = { + get: async () => null, + getMany: async () => ({}), + set: async () => {}, + delete: async () => {}, + deleteAllForServer: async () => {}, + }; + const snapshot = await readOAuthStore(filePath, withEmptyGetMany); + expect(snapshot?.servers[SERVER]).toEqual({ + scope: "read", + clientInformation: { client_id: "cid" }, + }); + expect(snapshot?.idpSessions[ISSUER]).toEqual({}); + }); + + it("skips the strip when the locked re-read no longer has plaintext", async () => { + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + // A store whose durability probe strips the file first — standing in for + // a concurrent process winning the migration race between the unlocked + // plaintext check and the locked re-read. + const inner = new InMemorySecretStore(); + const racing: SecretStore = { + isDurable: async () => { + const residue = snapshotWith(); + delete residue.servers[SERVER]!.tokens; + delete residue.servers[SERVER]!.clientInformation; + await writeStoreFile(filePath, JSON.stringify(residue)); + await flushStoreFileWrites(filePath); + return true; + }, + get: inner.get.bind(inner), + set: inner.set.bind(inner), + delete: inner.delete.bind(inner), + deleteAllForServer: inner.deleteAllForServer.bind(inner), + }; + const snapshot = await readOAuthStore(filePath, racing); + // Nothing was migrated by *this* read; the residue is served as-is. + expect(snapshot?.servers[SERVER]).toEqual({ scope: "read" }); + }); +}); + +describe("removeOAuthStore", () => { + it("purges every store entry the file indexes, then deletes the file", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections( + filePath, + snapshotWith({ + idpSessions: { [ISSUER]: { idToken: "idt" } }, + }), + undefined, + store, + ); + await flushStoreFileWrites(filePath); + + await removeOAuthStore(filePath, store); + expect(existsSync(filePath)).toBe(false); + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD), + ).toBeNull(); + expect( + await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), + ).toBeNull(); + }); + + it("propagates a failed purge and leaves the file as the index", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await flushStoreFileWrites(filePath); + + const failingPurge: SecretStore = { + get: async () => null, + set: async () => {}, + delete: async () => {}, + deleteAllForServer: async () => { + throw new Error("purge failed"); + }, + }; + await expect(removeOAuthStore(filePath, failingPurge)).rejects.toThrow( + "purge failed", + ); + // The file is the only index of the store entries — deleting it after + // a failed purge would strand credentials the next attempt can't find. + expect(existsSync(filePath)).toBe(true); + }); + + it("restores already-purged entries when a later purge fails", async () => { + const store = new InMemorySecretStore(); + await writeOAuthSections( + filePath, + snapshotWith({ idpSessions: { [ISSUER]: { idToken: "idt" } } }), + undefined, + store, + ); + await flushStoreFileWrites(filePath); + + // Servers are purged first, IdP sessions second: fail the second purge. + let purges = 0; + const failingSecond: SecretStore = { + get: (id, f) => store.get(id, f), + set: (id, f, v) => store.set(id, f, v), + delete: (id, f) => store.delete(id, f), + deleteAllForServer: async (id) => { + purges += 1; + if (purges === 2) throw new Error("keychain went away"); + await store.deleteAllForServer(id); + }, + }; + + await expect(removeOAuthStore(filePath, failingSecond)).rejects.toThrow( + "keychain went away", + ); + expect(existsSync(filePath)).toBe(true); + // The first target's purged secrets were restored — a retry of the + // removal (or a plain read) still finds everything the file indexes. + expect( + JSON.parse( + (await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD))!, + ), + ).toEqual(TOKENS); + expect( + await store.get(oauthIdpSecretServerId(ISSUER), IDP_SESSION_FIELD), + ).not.toBeNull(); + }); + + it("restores purged secrets when the file delete fails", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + void warn; // silence the unlocked-write warning for the read-only dir + const store = new InMemorySecretStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await flushStoreFileWrites(filePath); + + chmodSync(tempDir, 0o555); + try { + await expect(removeOAuthStore(filePath, store)).rejects.toThrow(); + } finally { + chmodSync(tempDir, 0o755); + } + + // The file survives as the index and the store matches it again. + expect(existsSync(filePath)).toBe(true); + expect( + JSON.parse( + (await store.get(oauthSecretServerId(SERVER), LEGACY_TOKENS_FIELD))!, + ), + ).toEqual(TOKENS); + expect( + await store.get(oauthSecretServerId(SERVER), LEGACY_CLIENT_SECRET_FIELD), + ).toBe("cs"); + }); + + it("is a no-op purge for a missing file", async () => { + await expect( + removeOAuthStore(filePath, new InMemorySecretStore()), + ).resolves.toBeUndefined(); + expect(existsSync(filePath)).toBe(false); + }); +}); + +describe("isUsableStoredSecret", () => { + it("validates structured fields with the same checks the join applies", () => { + const tokens = JSON.stringify({ access_token: "at", token_type: "Bearer" }); + expect(isUsableStoredSecret(LEGACY_TOKENS_FIELD, tokens)).toBe(true); + expect(isUsableStoredSecret(issuerTokensField(ISSUER), tokens)).toBe(true); + // Parseable JSON but not a usable tokens shape. + expect(isUsableStoredSecret(LEGACY_TOKENS_FIELD, "{}")).toBe(false); + // A partial-but-legitimate payload is usable: the store's write and + // read gates share the partial-schema contract, and `getTokens` + // withholds a partial set from the SDK on its own. + expect( + isUsableStoredSecret( + LEGACY_TOKENS_FIELD, + JSON.stringify({ access_token: "x" }), + ), + ).toBe(true); + // Type-corrupt junk the join would reject is not usable. + expect( + isUsableStoredSecret( + LEGACY_TOKENS_FIELD, + JSON.stringify({ access_token: 123 }), + ), + ).toBe(false); + expect(isUsableStoredSecret(issuerTokensField(ISSUER), "not json")).toBe( + false, + ); + expect( + isUsableStoredSecret(IDP_SESSION_FIELD, JSON.stringify({ idToken: "i" })), + ).toBe(true); + // `null` parses but the join requires a non-null object. + expect(isUsableStoredSecret(IDP_SESSION_FIELD, "null")).toBe(false); + expect(isUsableStoredSecret(IDP_SESSION_FIELD, "not json")).toBe(false); + // An object the join would extract nothing from is not usable either: + // the split only ever stores a value with at least one string field. + expect(isUsableStoredSecret(IDP_SESSION_FIELD, "{}")).toBe(false); + expect( + isUsableStoredSecret(IDP_SESSION_FIELD, JSON.stringify({ idToken: 42 })), + ).toBe(false); + // Opaque secrets (client secrets) have no structure to validate. + expect(isUsableStoredSecret(LEGACY_CLIENT_SECRET_FIELD, "anything")).toBe( + true, + ); + }); + + it("schema validation does not strip the SEP-2352 issuer stamp on join", async () => { + // parseStoredTokens validates with OAuthTokensSchema but must return the + // *original* object: the schema strips unknown fields, and the issuer + // stamp rides on the stored value. + const stamped = { ...TOKENS, issuer: ISSUER }; + await writeStoreFile(filePath, JSON.stringify(snapshotWith())); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + const id = oauthSecretServerId(SERVER); + await store.set(id, LEGACY_TOKENS_FIELD, JSON.stringify(stamped)); + + const snapshot = await readOAuthStore(filePath, store); + expect(snapshot?.servers[SERVER]!.tokens).toEqual(stamped); + }); +}); + +describe("unrecognized oauth.json refuses mutations", () => { + // The file's keys are the only index of secret-store entries. A present + // but unrecognized file (valid JSON of the wrong shape, or empty — + // malformed JSON already throws from JSON.parse) must refuse mutations: + // treating it as empty would let a sectioned write replace it with only + // the named entries, or let removal skip the store purge, orphaning + // every other entry's credentials. + const CORRUPT = JSON.stringify({ servers: ["not", "a", "map"] }); + + it("writeOAuthSections refuses and leaves file and store untouched", async () => { + await writeStoreFile(filePath, CORRUPT); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + const otherId = oauthSecretServerId("https://other.example/mcp"); + await store.set(otherId, LEGACY_TOKENS_FIELD, JSON.stringify(TOKENS)); + + await expect( + writeOAuthSections( + filePath, + snapshotWith(), + { servers: [SERVER] }, + store, + ), + ).rejects.toThrow(OAuthStateFileUnrecognizedError); + + expect(readFileSync(filePath, "utf-8")).toBe(CORRUPT); + expect(await store.get(otherId, LEGACY_TOKENS_FIELD)).toBe( + JSON.stringify(TOKENS), + ); + }); + + it("writeOAuthSections refuses an empty (truncated) file", async () => { + await writeStoreFile(filePath, ""); + await flushStoreFileWrites(filePath); + + await expect( + writeOAuthSections( + filePath, + snapshotWith(), + undefined, + new InMemorySecretStore(), + ), + ).rejects.toThrow(OAuthStateFileUnrecognizedError); + + expect(readFileSync(filePath, "utf-8")).toBe(""); + }); + + it("removeOAuthStore refuses and leaves file and store entries in place", async () => { + await writeStoreFile(filePath, CORRUPT); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + const otherId = oauthSecretServerId("https://other.example/mcp"); + await store.set(otherId, LEGACY_TOKENS_FIELD, JSON.stringify(TOKENS)); + + await expect(removeOAuthStore(filePath, store)).rejects.toThrow( + OAuthStateFileUnrecognizedError, + ); + + expect(existsSync(filePath)).toBe(true); + expect(await store.get(otherId, LEGACY_TOKENS_FIELD)).toBe( + JSON.stringify(TOKENS), + ); + }); + + it("readOAuthStore stays tolerant: unrecognized file reads as no stored state", async () => { + await writeStoreFile(filePath, CORRUPT); + await flushStoreFileWrites(filePath); + expect( + await readOAuthStore(filePath, new InMemorySecretStore()), + ).toBeNull(); + }); +}); + +describe("partial token payloads round-trip through the store", () => { + // The store's write gate (`splitTokens`) and read gate + // (`parseStoredTokens`) share one contract: every present field + // well-typed, none required. A partial-but-legitimate payload — e.g. a + // refresh-only entry inherited from a legacy plaintext file — therefore + // moves to the store like any full token set and is served back by the + // join (the CLI's stored-token refresh depends on that), never left as + // plaintext in `oauth.json`. Only a type-corrupt payload stays in the + // file, where it remains clearable. + const PARTIAL = { refresh_token: "rt-only", token_type: "Bearer" }; + const CORRUPT = { access_token: 123, token_type: "Bearer" }; + + it("a save moves a partial token payload to the store and serves it back", async () => { + const store = new InMemorySecretStore(); + const id = oauthSecretServerId(SERVER); + const snapshot = snapshotWith(); + snapshot.servers[SERVER]!.tokens = { ...PARTIAL } as never; + + await writeOAuthSections(filePath, snapshot, { servers: [SERVER] }, store); + await flushStoreFileWrites(filePath); + + // The bearer-grade refresh token is in the store, not the file. + expect(readRawFile().servers[SERVER]!.tokens).toBeUndefined(); + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( + PARTIAL, + ); + + const read = await readOAuthStore(filePath, store); + expect(read?.servers[SERVER]?.tokens).toEqual(PARTIAL); + expect(read?.servers[SERVER]?.clientInformation?.client_secret).toBe("cs"); + }); + + it("migration moves partial plaintext tokens into the store without loss", async () => { + const legacy = snapshotWith(); + legacy.servers[SERVER]!.tokens = { ...PARTIAL } as never; + await writeStoreFile(filePath, JSON.stringify(legacy)); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + const id = oauthSecretServerId(SERVER); + + const snapshot = await readOAuthStore(filePath, store); + + // Served on the first read — the loss scenario was migration storing a + // payload the old full-schema read gate then refused to serve. + expect(snapshot?.servers[SERVER]?.tokens).toEqual(PARTIAL); + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( + PARTIAL, + ); + const raw = readRawFile(); + expect(raw.servers[SERVER]!.tokens).toBeUndefined(); + expect( + raw.servers[SERVER]!.clientInformation?.client_secret, + ).toBeUndefined(); + + // Fully stripped, so the file must stay byte-stable across reads. + const bytesAfterFirstRead = readFileSync(filePath, "utf-8"); + const again = await readOAuthStore(filePath, store); + expect(again?.servers[SERVER]?.tokens).toEqual(PARTIAL); + expect(readFileSync(filePath, "utf-8")).toBe(bytesAfterFirstRead); + }); + + it("a type-corrupt token payload stays in the file, clearable and byte-stable", async () => { + const legacy = snapshotWith(); + legacy.servers[SERVER]!.tokens = { ...CORRUPT } as never; + await writeStoreFile(filePath, JSON.stringify(legacy)); + await flushStoreFileWrites(filePath); + const store = new InMemorySecretStore(); + const id = oauthSecretServerId(SERVER); + + // Migration must not copy junk into the store (the join would reject + // it) and must not destroy it either — the entry stays clearable. + const snapshot = await readOAuthStore(filePath, store); + expect(snapshot?.servers[SERVER]?.tokens).toEqual(CORRUPT); + expect(await store.get(id, LEGACY_TOKENS_FIELD)).toBeNull(); + expect(readRawFile().servers[SERVER]!.tokens).toEqual(CORRUPT); + + // Such a file re-enters migration on every read; the skipped rewrite + // keeps it byte-stable. + const bytesAfterFirstRead = readFileSync(filePath, "utf-8"); + await readOAuthStore(filePath, store); + expect(readFileSync(filePath, "utf-8")).toBe(bytesAfterFirstRead); + + // Clearing the entry still works and removes the junk from the file. + const cleared = snapshotWith(); + delete cleared.servers[SERVER]!.tokens; + await writeOAuthSections(filePath, cleared, { servers: [SERVER] }, store); + await flushStoreFileWrites(filePath); + expect(readRawFile().servers[SERVER]!.tokens).toBeUndefined(); + }); +}); + +describe("saves only touch changed store fields", () => { + // `persistEntrySecrets` writes and deletes only deltas against the + // strict per-field snapshot. Without the filter, every save rewrites the + // unchanged credential batch and issues deletes for the always-candidate + // legacy fields the store never held — so a store that turned read-only + // between saves would degrade (or abort) a save that only changed + // non-secret state, silently losing it from the file even though the + // store already held exactly the desired values. + class ReadOnlyableStore extends InMemorySecretStore { + readOnly = false; + override async set( + serverId: string, + field: string, + value: string, + ): Promise { + if (this.readOnly) throw new Error("keychain is read-only"); + return super.set(serverId, field, value); + } + override async delete(serverId: string, field: string): Promise { + if (this.readOnly) throw new Error("keychain is read-only"); + return super.delete(serverId, field); + } + } + + it("a scope-only save persists against a store that turned read-only", async () => { + const store = new ReadOnlyableStore(); + await writeOAuthSections(filePath, snapshotWith(), undefined, store); + await flushStoreFileWrites(filePath); + store.readOnly = true; + + const next = snapshotWith(); + next.servers[SERVER]!.scope = "write"; + await writeOAuthSections(filePath, next, { servers: [SERVER] }, store); + await flushStoreFileWrites(filePath); + + // The non-secret update reached the file; the entry was not reverted. + const raw = readRawFile(); + expect(raw.servers[SERVER]!.scope).toBe("write"); + expect(raw.servers[SERVER]!.tokens).toBeUndefined(); + + // The store still holds the unchanged credentials, untouched. + const id = oauthSecretServerId(SERVER); + expect(JSON.parse((await store.get(id, LEGACY_TOKENS_FIELD))!)).toEqual( + TOKENS, + ); + expect(await store.get(id, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs"); + }); +}); diff --git a/clients/web/src/test/integration/storage/oauth-write-convergence.test.ts b/clients/web/src/test/integration/storage/oauth-write-convergence.test.ts new file mode 100644 index 0000000000..22b7985c09 --- /dev/null +++ b/clients/web/src/test/integration/storage/oauth-write-convergence.test.ts @@ -0,0 +1,495 @@ +/** + * Convergence verification in `writeOAuthSections` + * (core/auth/node/oauth-persist-file.ts): `withSecretFileLock` deliberately + * degrades to an unlocked run when its lock directory cannot be created, so + * the sectioned read-merge-write re-reads the file after writing and + * re-applies itself when another writer landed in between — the same + * verify/re-apply pattern as `FileSecretStore.mutateLocked`. These tests + * simulate the racing writer with a hook that rewrites the file immediately + * after each `writeStoreFile`. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + writeOAuthSections, + readOAuthStore, +} from "@inspector/core/auth/node/oauth-persist-file.js"; +import { + InMemorySecretStore, + SecretStoreUnavailableError, +} from "@inspector/core/auth/node/secret-store.js"; +import { + oauthSecretServerId, + LEGACY_TOKENS_FIELD, + LEGACY_CLIENT_SECRET_FIELD, +} from "@inspector/core/auth/node/oauth-secrets.js"; +import { writeStoreFile } from "@inspector/core/storage/store-io.js"; +import type { OAuthPersistSnapshot } from "@inspector/core/auth/oauth-persist.js"; + +const hook = vi.hoisted(() => ({ + beforeWrite: undefined as ((path: string, data: string) => void) | undefined, + afterWrite: undefined as + | ((path: string, data: string) => void | Promise) + | undefined, + beforeRead: undefined as ((path: string) => void) | undefined, +})); + +vi.mock("@inspector/core/storage/store-io.js", async (importOriginal) => { + const actual = + await importOriginal< + typeof import("@inspector/core/storage/store-io.js") + >(); + return { + ...actual, + writeStoreFile: vi.fn(async (filePath: string, data: string) => { + hook.beforeWrite?.(filePath, data); + await actual.writeStoreFile(filePath, data); + await hook.afterWrite?.(filePath, data); + }), + readStoreFile: vi.fn(async (filePath: string) => { + hook.beforeRead?.(filePath); + return actual.readStoreFile(filePath); + }), + }; +}); + +const SERVER_A = "https://a.example/mcp"; +const SERVER_B = "https://b.example/mcp"; + +function serverState(tag: string) { + return { + scope: "read", + tokens: { + access_token: `at-${tag}`, + token_type: "Bearer", + refresh_token: `rt-${tag}`, + }, + clientInformation: { client_id: `cid-${tag}`, client_secret: `cs-${tag}` }, + }; +} + +function snapshotOf( + servers: OAuthPersistSnapshot["servers"], +): OAuthPersistSnapshot { + return { servers, idpSessions: {} }; +} + +let tempDir: string; +let filePath: string; +let store: InMemorySecretStore; +/** File bytes holding only server A, as the racing writer would leave them. */ +let onlyA: string; + +beforeEach(async () => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-oauth-converge-")); + filePath = join(tempDir, "oauth.json"); + store = new InMemorySecretStore(); + hook.beforeWrite = undefined; + hook.beforeRead = undefined; + hook.afterWrite = undefined; + vi.mocked(writeStoreFile).mockClear(); + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_A]: serverState("a") }), + { servers: [SERVER_A] }, + store, + ); + onlyA = readFileSync(filePath, "utf-8"); +}); + +afterEach(() => { + hook.beforeWrite = undefined; + hook.beforeRead = undefined; + hook.afterWrite = undefined; + rmSync(tempDir, { recursive: true, force: true }); +}); + +describe("writeOAuthSections convergence verification", () => { + it("re-applies its sections when another writer lands between write and read-back", async () => { + let clobbers = 0; + hook.afterWrite = (path) => { + // The racing writer's result: a full state file that lacks server B — + // exactly what an unlocked concurrent read-merge-write would leave. + if (clobbers++ === 0) writeFileSync(path, onlyA); + }; + + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ); + + // Seed + first (clobbered) attempt + converging retry. + expect(vi.mocked(writeStoreFile)).toHaveBeenCalledTimes(3); + const read = await readOAuthStore(filePath, store); + expect(read?.servers[SERVER_A]?.tokens?.access_token).toBe("at-a"); + expect(read?.servers[SERVER_B]?.tokens?.access_token).toBe("at-b"); + }); + + it("gives up with a typed, retryable error when the file keeps changing", async () => { + hook.afterWrite = (path) => writeFileSync(path, onlyA); + + await expect( + writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ), + ).rejects.toThrow(SecretStoreUnavailableError); + + // Seed + five attempts, then the bounded loop reports instead of spinning. + expect(vi.mocked(writeStoreFile)).toHaveBeenCalledTimes(6); + }); + + it("unwinds a new entry's store secrets when it gives up, so nothing is stranded without a file index", async () => { + hook.afterWrite = (path) => writeFileSync(path, onlyA); + + await expect( + writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ), + ).rejects.toThrow(/kept overwriting/); + + // Server B never made it into the file, so its secrets must not linger + // in the store (they would have no index for removeOAuthStore to find). + const idB = oauthSecretServerId(SERVER_B); + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBeNull(); + expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); + // Server A's stored secrets are untouched. + const idA = oauthSecretServerId(SERVER_A); + expect(await store.get(idA, LEGACY_TOKENS_FIELD)).not.toBeNull(); + expect(await store.get(idA, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-a"); + }); + + it("restores pre-operation values when a retry attempt itself fails", async () => { + // Attempt 1 succeeds but is clobbered; attempt 2's file write fails hard. + // The rollback must not treat attempt 1 as committed: its priors would + // "restore" the values attempt 1 itself wrote, stranding server B's + // secrets in the store while the surviving file has no index for them. + let writes = 0; + hook.beforeWrite = () => { + writes += 1; + if (writes === 2) throw new Error("disk full"); + }; + hook.afterWrite = (path) => { + if (writes === 1) writeFileSync(path, onlyA); + }; + + await expect( + writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ), + ).rejects.toThrow(/disk full/); + + const idB = oauthSecretServerId(SERVER_B); + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBeNull(); + expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); + const idA = oauthSecretServerId(SERVER_A); + expect(await store.get(idA, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-a"); + expect(readFileSync(filePath, "utf-8")).toBe(onlyA); + }); + + it("rolls back when a clobbering writer leaves an unrecognized file", async () => { + // The retry's disk read throws on unrecognized content; that exit must + // restore the store like any other failure, or the earlier attempt's + // writes are stranded. + hook.afterWrite = (path) => + writeFileSync(path, JSON.stringify({ hello: "world" })); + + await expect( + writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ), + ).rejects.toThrow(/refusing/i); + + const idB = oauthSecretServerId(SERVER_B); + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBeNull(); + expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); + const idA = oauthSecretServerId(SERVER_A); + expect(await store.get(idA, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-a"); + }); + + it("keeps a concurrent writer's newer value when rolling back", async () => { + // Blind pre-operation restore would be wrong too: a value a concurrent + // writer stored between attempts is newer state this call did not write, + // and rolling it back to the pre-operation value would clobber that + // writer. The rollback baseline folds each attempt's priors, telling our + // own earlier attempt's writes (equal to what this call writes — they + // are constant across attempts) apart from foreign values. + const idB = oauthSecretServerId(SERVER_B); + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ); + const withOldB = readFileSync(filePath, "utf-8"); + const foreignTokens = JSON.stringify({ + access_token: "at-bF", + token_type: "Bearer", + }); + + let writes = 0; + hook.beforeWrite = () => { + writes += 1; + if (writes === 2) throw new Error("disk full"); + }; + hook.afterWrite = async (path) => { + if (writes !== 1) return; + // The concurrent writer lands after our first attempt: its own store + // write for server B, and a file replacing ours. + await store.set(idB, LEGACY_TOKENS_FIELD, foreignTokens); + writeFileSync(path, withOldB); + }; + + await expect( + writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b2") }), + { servers: [SERVER_B] }, + store, + ), + ).rejects.toThrow(/disk full/); + + // The foreign value survives the rollback; fields the foreign writer + // did not touch return to their pre-operation values. + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBe(foreignTokens); + expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-b"); + }); + + it("escalates a retry store failure instead of degrading, restoring pre-operation secrets", async () => { + // Attempt 1 lands fully but is clobbered by a writer restoring the old + // file; attempt 2's store write fails. Degrading here would be unsound — + // the disk entry is no longer the pre-call state the degrade contract + // pairs with — so the failure escalates into the reconciling exit, which + // finds the file changed and restores the pre-operation secrets. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const idB = oauthSecretServerId(SERVER_B); + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ); + const withOldB = readFileSync(filePath, "utf-8"); + const oldTokens = await store.get(idB, LEGACY_TOKENS_FIELD); + expect(oldTokens).toContain("at-b"); + + const realSet = store.set.bind(store); + hook.afterWrite = async (path) => { + writeFileSync(path, withOldB); + // The foreign writer restored the store too: only-delta persistence + // means the retry issues a store write at all only when the store + // does not already hold the desired values. + await realSet(idB, LEGACY_TOKENS_FIELD, oldTokens!); + await realSet(idB, LEGACY_CLIENT_SECRET_FIELD, "cs-b"); + hook.afterWrite = undefined; + let failed = false; + store.set = async (serverId, field, value) => { + // Fail exactly one set: a degrade's compensating restore would + // succeed, so only escalation reaches the reconciling exit. + if (!failed && serverId === idB) { + failed = true; + throw new Error("keychain says no"); + } + return realSet(serverId, field, value); + }; + }; + + await expect( + writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b2") }), + { servers: [SERVER_B] }, + store, + ), + ).rejects.toThrow(/keychain says no/); + + store.set = realSet; + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBe(oldTokens); + expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-b"); + const read = await readOAuthStore(filePath, store); + expect(read?.servers[SERVER_B]?.tokens?.access_token).toBe("at-b"); + expect(read?.servers[SERVER_B]?.clientInformation?.client_id).toBe("cid-b"); + warn.mockRestore(); + }); + + it("restores the baseline for fields the committed attempt degraded, not a later attempt's writes", async () => { + // Attempt 1's store write fails, degrading server B back to its old + // residue; that file write lands but its read-back fails. Attempt 2's + // store writes succeed, but its file write fails, escalating into the + // reconciling exit — which confirms the file still holds attempt 1's + // blob. That blob pairs with the *old* secrets (attempt 1 degraded B), + // so attempt 2's store writes must be rolled back to the baseline, not + // left in place under the old residue. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const idB = oauthSecretServerId(SERVER_B); + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ); + const oldTokens = await store.get(idB, LEGACY_TOKENS_FIELD); + expect(oldTokens).toContain("at-b"); + + const realSet = store.set.bind(store); + let failNewSets = true; + store.set = async (serverId, field, value) => { + // The degrade's own compensating restore (old values) must succeed. + if (failNewSets && serverId === idB && value.includes("b2")) + throw new Error("keychain says no"); + return realSet(serverId, field, value); + }; + let reads = 0; + hook.beforeRead = () => { + reads += 1; + // Read 1: attempt 1's disk read. Read 2: its failing read-back. + // Read 3: attempt 2's disk read — the store has recovered by now. + // Read 4: the reconciling exit's confirmation read. + if (reads === 2) throw new Error("EIO: read failed"); + if (reads === 3) failNewSets = false; + }; + let writes = 0; + hook.beforeWrite = () => { + writes += 1; + if (writes === 2) throw new Error("disk full"); + }; + + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b2") }), + { servers: [SERVER_B] }, + store, + ); + + store.set = realSet; + // The committed file holds the old residue; the store must pair with + // it — attempt 2's b2 values must not survive. + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBe(oldTokens); + expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-b"); + const read = await readOAuthStore(filePath, store); + expect(read?.servers[SERVER_B]?.tokens?.access_token).toBe("at-b"); + warn.mockRestore(); + }); + + it("re-applies a committed attempt's writes when a retry's store failure escalates", async () => { + // Attempt 1 lands fully but its read-back fails; attempt 2's store + // write fails outright (no degrade on retries) and escalates into the + // reconciling exit. The file is confirmed to still hold attempt 1's + // write, so the save is committed: the store is re-pointed at attempt + // 1's values and the call reports success. + const idB = oauthSecretServerId(SERVER_B); + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ); + + const realSet = store.set.bind(store); + let failSets = false; + store.set = async (serverId, field, value) => { + if (failSets) throw new Error("keychain flake"); + return realSet(serverId, field, value); + }; + let reads = 0; + hook.beforeRead = () => { + reads += 1; + // Read 1: attempt 1's disk read. Read 2: its failing read-back. + // Read 3: attempt 2's disk read — the store starts flaking here. + // Read 4: the confirmation read — the flake has passed. + if (reads === 2) throw new Error("EIO: read failed"); + if (reads === 3) failSets = true; + if (reads === 4) failSets = false; + }; + + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b2") }), + { servers: [SERVER_B] }, + store, + ); + + store.set = realSet; + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toContain("at-b2"); + expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-b2"); + const read = await readOAuthStore(filePath, store); + expect(read?.servers[SERVER_B]?.tokens?.access_token).toBe("at-b2"); + }); + + it("reports success when the file is confirmed to still hold an unverified write", async () => { + // Attempt 1's write lands but its read-back fails; attempt 2's disk read + // fails too (same sick filesystem). The file still holds attempt 1's + // write, so rolling back only the store would pair committed residue + // with restored old secrets. The reconciling exit re-reads the file, + // finds the write, and reports the save as what it is: committed. + let reads = 0; + hook.beforeRead = () => { + reads += 1; + // Read 1: attempt 1's disk read. Reads 2-3: attempt 1's verifying + // read-back and attempt 2's disk read, both failing. Read 4: the + // reconciling exit's confirmation read, which succeeds. + if (reads === 2 || reads === 3) throw new Error("EIO: read failed"); + }; + + await writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ); + + const idB = oauthSecretServerId(SERVER_B); + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toContain("at-b"); + expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBe("cs-b"); + const read = await readOAuthStore(filePath, store); + expect(read?.servers[SERVER_B]?.tokens?.access_token).toBe("at-b"); + }); + + it("restores and warns when the unverified write cannot be confirmed either way", async () => { + // Same as above, but the confirmation read fails too. Nothing can say + // whether the file holds the write; the store is restored (the bias + // that cannot strand secrets) and the warning says the file may still + // hold the interrupted save. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + let reads = 0; + hook.beforeRead = () => { + reads += 1; + if (reads >= 2) throw new Error("EIO: read failed"); + }; + + await expect( + writeOAuthSections( + filePath, + snapshotOf({ [SERVER_B]: serverState("b") }), + { servers: [SERVER_B] }, + store, + ), + ).rejects.toThrow(/EIO/); + + const idB = oauthSecretServerId(SERVER_B); + expect(await store.get(idB, LEGACY_TOKENS_FIELD)).toBeNull(); + expect(await store.get(idB, LEGACY_CLIENT_SECRET_FIELD)).toBeNull(); + expect( + warn.mock.calls.some(([msg]) => + String(msg).includes("Could not re-read"), + ), + ).toBe(true); + warn.mockRestore(); + }); +}); diff --git a/clients/web/src/test/integration/storage/own-entry.test.ts b/clients/web/src/test/integration/storage/own-entry.test.ts new file mode 100644 index 0000000000..616dabad97 --- /dev/null +++ b/clients/web/src/test/integration/storage/own-entry.test.ts @@ -0,0 +1,39 @@ +/** + * Direct tests of the own-property map helpers. The OAuth persistence and + * catalog paths exercise these through their own suites; this file pins the + * helper contract itself — every branch, with the `__proto__` key that + * motivates the module. + */ +import { describe, it, expect } from "vitest"; +import { setOwnEntry, getOwnEntry } from "@inspector/core/storage/own-entry.js"; + +describe("setOwnEntry", () => { + it("creates an own, enumerable, writable entry for __proto__", () => { + const map: Record = {}; + setOwnEntry(map, "__proto__", "value"); + expect(Object.hasOwn(map, "__proto__")).toBe(true); + expect(Object.getPrototypeOf(map)).toBe(Object.prototype); + expect(JSON.stringify(map)).toContain("value"); + // Writable + configurable: a second write and a delete both work. + setOwnEntry(map, "__proto__", "next"); + expect(getOwnEntry(map, "__proto__")).toBe("next"); + delete map["__proto__"]; + expect(Object.hasOwn(map, "__proto__")).toBe(false); + }); +}); + +describe("getOwnEntry", () => { + it("returns an own entry", () => { + expect(getOwnEntry({ a: 1 }, "a")).toBe(1); + }); + + it("returns undefined for a missing inherited name instead of the prototype member", () => { + expect(getOwnEntry({}, "__proto__")).toBeUndefined(); + expect(getOwnEntry({}, "constructor")).toBeUndefined(); + expect(getOwnEntry({}, "toString")).toBeUndefined(); + }); + + it("returns undefined for an undefined map", () => { + expect(getOwnEntry(undefined, "a")).toBeUndefined(); + }); +}); diff --git a/clients/web/src/test/integration/storage/store-id.test.ts b/clients/web/src/test/integration/storage/store-id.test.ts index 9bac81a5b5..f459cb2859 100644 --- a/clients/web/src/test/integration/storage/store-id.test.ts +++ b/clients/web/src/test/integration/storage/store-id.test.ts @@ -15,6 +15,24 @@ describe("validateStoreId", () => { expect(validateStoreId("a/b")).toBe(false); }); + it("rejects `__proto__` but keeps other Object.prototype names valid", () => { + // A plain `map[id] = …` assignment with `__proto__` would invoke the + // prototype setter and silently drop the entry, so it alone is refused. + // Other inherited names (`constructor`, `toString`, …) were valid ids + // before this check existed and must stay valid — pre-existing mcp.json + // entries would otherwise list in GET but be refused by PUT/DELETE. + // They are safe because all dynamic-key map access is own-property + // based (`Object.hasOwn`, `getOwnEntry`/`setOwnEntry`). + expect(validateStoreId("__proto__")).toBe(false); + for (const name of Object.getOwnPropertyNames(Object.prototype)) { + if (name === "__proto__") continue; + if (!/^[a-zA-Z0-9_-]+$/.test(name)) continue; // out of charset anyway + expect(validateStoreId(name), name).toBe(true); + } + // Ordinary underscore names stay valid too. + expect(validateStoreId("__internal__")).toBe(true); + }); + it("is re-exported from store-io for back-compat", () => { expect(reexported).toBe(validateStoreId); }); diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index ac7f541b49..0799c9bd77 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -293,6 +293,13 @@ export default defineConfig(({ command }) => { test: { name: "unit", environment: "happy-dom", + // OAuth tokens and client secrets now live in the OS secret store + // (core/auth/node/oauth-secrets.ts). Without this, any test + // that touches the file OAuth backend without injecting a store + // double would probe — and on a dev machine, write to — the real + // keychain. Tests that exercise selection behavior stash/delete + // this var themselves, so the pin doesn't constrain them. + env: { MCP_INSPECTOR_SECRET_STORE: "memory" }, // Don't let happy-dom actually navigate child frames. Components like // the MCP Apps sandbox render an