From cd4ccf91ee77254d0959cc3fbc9acae7d455b2db Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Wed, 23 Sep 2026 17:03:29 -0700 Subject: [PATCH 01/45] fix(auth): stop stale-snapshot writers from clobbering oauth.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every process (web backend, daemon, CLI) that persists OAuth state used to flush its whole in-memory snapshot over the shared oauth.json — so a writer holding a stale snapshot erased entries other processes wrote after it last read the file (observed live: a background EMA flow wiping a fresh login). Every write already enters through a mutation scoped to named entries, so persistence now names what changed and merges only that: - oauth-persist.ts: OAuthPersistSections + pure mergeOAuthSections (named servers/idpSessions keys overlaid onto a fresh read; absent = deletion), parseOAuthPersistSections for the wire form; backends accept an optional sections arg; the remote backend forwards it as a ?sections= query param. - oauth-storage.ts: persist(sections) snapshots inside the queued closure (fresh at write time); every mutation passes its sections, with the enterprise-managed sweep capturing its URLs before clearing them. - oauth-persist-file.ts: shared writeOAuthSections = cross-process file lock -> fresh read -> merge -> atomic write; lock failures rethrown with OAuth wording and the original as cause. - remote server storage route: sectioned POSTs apply the same shared locked merge (400 on bad descriptors or non-OAuth bodies); plain POSTs and the client store are unchanged. - cli.ts refreshStoredAuthToken: its hand-rolled read-modify-write now persists through writeOAuthSections — same lock, same merge, merged against the file at write time. Memory is deliberately not refreshed from the merged result: overwriting it could revert concurrent in-process mutations, and reads staying cached is fine — correctness comes from the per-mutation read-modify-write. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- clients/cli/src/cli.ts | 17 +- .../test/core/auth/oauth-persist-file.test.ts | 51 ++++++ .../src/test/core/auth/oauth-persist.test.ts | 120 +++++++++++++ .../core/auth/oauth-storage-sections.test.ts | 161 ++++++++++++++++++ .../integration/mcp/remote/transport.test.ts | 91 ++++++++++ .../test/integration/storage/adapters.test.ts | 109 ++++++++++++ core/auth/node/oauth-persist-file.ts | 49 +++++- core/auth/oauth-persist.ts | 130 +++++++++++++- core/auth/oauth-storage.ts | 62 ++++--- core/mcp/remote/node/server.ts | 27 +++ 10 files changed, 778 insertions(+), 39 deletions(-) create mode 100644 clients/web/src/test/core/auth/oauth-persist-file.test.ts create mode 100644 clients/web/src/test/core/auth/oauth-storage-sections.test.ts diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 43a5c7da14..95c5770fa6 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -46,9 +46,9 @@ export { emitResult } from "./handlers/emit-result.js"; export { collectAppInfo } from "./handlers/collect-app-info.js"; import { parseOAuthPersistBlob, - serializeOAuthPersistBlob, type OAuthPersistSnapshot, } from "@inspector/core/auth/oauth-persist.js"; +import { writeOAuthSections } from "@inspector/core/auth/node/oauth-persist-file.js"; import { discoverAuthorizationServerMetadataFromCandidates, getAuthorizationServerUrl, @@ -56,7 +56,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, @@ -434,13 +433,15 @@ 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; } 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..55aa0c4cd0 --- /dev/null +++ b/clients/web/src/test/core/auth/oauth-persist-file.test.ts @@ -0,0 +1,51 @@ +/** + * Unit tests for the file persist backend's lock-failure handling: when the + * cross-process lock cannot be acquired, `withSecretFileLock` throws a + * `SecretStoreUnavailableError` 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 { SecretStoreUnavailableError } 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 { writeOAuthSections } from "@inspector/core/auth/node/oauth-persist-file.js"; + +const SNAPSHOT = { servers: {}, idpSessions: {} }; + +describe("writeOAuthSections lock failures", () => { + beforeEach(() => { + vi.mocked(withSecretFileLock).mockReset(); + }); + + it("rethrows SecretStoreUnavailableError with OAuth wording and cause", async () => { + const original = new SecretStoreUnavailableError( + "Could not lock the secrets file", + ); + vi.mocked(withSecretFileLock).mockRejectedValue(original); + + await expect( + writeOAuthSections("/tmp/oauth.json", SNAPSHOT, { servers: ["s"] }), + ).rejects.toMatchObject({ + message: expect.stringContaining( + "Could not save OAuth state: the state file at /tmp/oauth.json is locked", + ), + cause: 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); + }); +}); 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..c55588e2e9 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,8 @@ import { describe, it, expect, vi } from "vitest"; import { parseOAuthPersistBlob, serializeOAuthPersistBlob, + mergeOAuthSections, + parseOAuthPersistSections, createRemoteOAuthPersistBackend, createSessionOAuthPersistBackend, OAUTH_PERSIST_STORAGE_KEY, @@ -83,6 +85,104 @@ 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: {}, + }); + }); +}); + +describe("parseOAuthPersistSections", () => { + it("parses servers and idpSessions string arrays", () => { + expect( + parseOAuthPersistSections( + JSON.stringify({ 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 malformed JSON, non-objects, and non-string-array values", () => { + expect(parseOAuthPersistSections("not json")).toBeNull(); + expect(parseOAuthPersistSections('"a string"')).toBeNull(); + expect(parseOAuthPersistSections('{"servers":"http://a"}')).toBeNull(); + expect(parseOAuthPersistSections('{"servers":[1]}')).toBeNull(); + expect(parseOAuthPersistSections('{"idpSessions":{}}')).toBeNull(); + }); +}); + describe("createRemoteOAuthPersistBackend", () => { const baseUrl = "http://remote.example/"; const storeId = "oauth"; @@ -159,6 +259,26 @@ describe("createRemoteOAuthPersistBackend", () => { ); }); + it("write() with sections carries them as a query parameter", async () => { + let capturedUrl: string | undefined; + const fetchFn = vi.fn(async (input) => { + capturedUrl = String(input); + return new Response("", { status: 200 }); + }); + const backend = createRemoteOAuthPersistBackend({ + baseUrl, + storeId, + fetchFn, + }); + const sections = { servers: ["http://s"] }; + await backend.write(SNAPSHOT, sections); + const parsed = new URL(capturedUrl ?? ""); + expect(parsed.pathname).toBe(`/api/storage/${storeId}`); + expect(JSON.parse(parsed.searchParams.get("sections") ?? "")).toEqual( + sections, + ); + }); + it("remove() DELETEs, tolerates 404, and throws on other errors", async () => { const ok = createRemoteOAuthPersistBackend({ baseUrl, 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..2c9c4d6612 --- /dev/null +++ b/clients/web/src/test/core/auth/oauth-storage-sections.test.ts @@ -0,0 +1,161 @@ +/** + * 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 } from "@inspector/core/auth/oauth-storage.js"; +import { OAuthMemoryStore } 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", + }); + }); +}); 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..7e4ca85109 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,97 @@ 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. + const sections = encodeURIComponent( + JSON.stringify({ servers: ["https://mine.example"] }), + ); + const res = await fetch( + `${baseUrl}/api/storage/oauth?sections=${sections}`, + { + method: "POST", + headers, + body: JSON.stringify({ + 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?sections=${encodeURIComponent('{"servers":"nope"}')}`, + { + method: "POST", + headers, + body: JSON.stringify({ servers: {}, idpSessions: {} }), + }, + ); + expect(badSections.status).toBe(400); + expect((await badSections.json()).error).toBe( + "Invalid sections parameter", + ); + + const badBody = await fetch( + `${baseUrl}/api/storage/oauth?sections=${encodeURIComponent('{"servers":[]}')}`, + { + method: "POST", + headers, + body: JSON.stringify({ someOtherStore: true }), + }, + ); + expect(badBody.status).toBe(400); + expect((await badBody.json()).error).toBe( + "Sectioned write requires 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..28e60391de 100644 --- a/clients/web/src/test/integration/storage/adapters.test.ts +++ b/clients/web/src/test/integration/storage/adapters.test.ts @@ -179,6 +179,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", () => { diff --git a/core/auth/node/oauth-persist-file.ts b/core/auth/node/oauth-persist-file.ts index dc0714fb06..84f94fd5e9 100644 --- a/core/auth/node/oauth-persist-file.ts +++ b/core/auth/node/oauth-persist-file.ts @@ -4,6 +4,12 @@ * `node:fs`/`atomically`); the browser must never load that. Node consumers * (e.g. `NodeOAuthStorage`) import the file backend from here, while the * shared blob (de)serialization and browser/remote backends stay isomorphic. + * + * Sectioned writes (`OAuthPersistSections`) are applied here as a locked + * read-modify-write: only the named entries are overlaid onto a fresh read of + * the file, so several processes (web backend, daemon, CLI) sharing one + * `oauth.json` can each persist their own mutations without erasing entries + * the others wrote after this process last read the file. */ import { @@ -12,15 +18,52 @@ import { deleteStoreFile, } from "../../storage/store-io.js"; import { + mergeOAuthSections, parseOAuthPersistBlob, serializeOAuthPersistBlob, type OAuthPersistBackend, + type OAuthPersistSections, + type OAuthPersistSnapshot, } from "../oauth-persist.js"; +import { withSecretFileLock } from "./file-lock.js"; +import { SecretStoreUnavailableError } from "./secret-store.js"; export interface FileOAuthPersistBackendOptions { filePath: string; } +/** + * Overlay the named sections of `snapshot` onto the OAuth state file under + * the cross-process file lock: lock → fresh read → merge → atomic write. + * Shared by the file backend and the remote server's storage route so both + * writers use the identical locked merge. + * + * The lock's own errors talk about "the secrets file" (its other caller); + * they are rethrown with OAuth wording so an operator seeing the message + * looks at `oauth.json`, with the original attached as `cause`. + */ +export async function writeOAuthSections( + filePath: string, + snapshot: OAuthPersistSnapshot, + sections: OAuthPersistSections, +): Promise { + try { + await withSecretFileLock(filePath, async () => { + const disk = parseOAuthPersistBlob(await readStoreFile(filePath)); + const merged = mergeOAuthSections(disk, snapshot, sections); + await writeStoreFile(filePath, serializeOAuthPersistBlob(merged)); + }); + } catch (error) { + if (error instanceof SecretStoreUnavailableError) { + throw new Error( + `Could not save OAuth state: the state file at ${filePath} is locked by another Inspector process and did not become available.`, + { cause: error }, + ); + } + throw error; + } +} + export function createFileOAuthPersistBackend( options: FileOAuthPersistBackendOptions, ): OAuthPersistBackend { @@ -29,7 +72,11 @@ export function createFileOAuthPersistBackend( const raw = await readStoreFile(options.filePath); return parseOAuthPersistBlob(raw); }, - async write(snapshot) { + async write(snapshot, sections) { + if (sections) { + await writeOAuthSections(options.filePath, snapshot, sections); + return; + } await writeStoreFile( options.filePath, serializeOAuthPersistBlob(snapshot), diff --git a/core/auth/oauth-persist.ts b/core/auth/oauth-persist.ts index 133dc4198e..be135037d0 100644 --- a/core/auth/oauth-persist.ts +++ b/core/auth/oauth-persist.ts @@ -4,6 +4,13 @@ * accepts legacy persist envelopes `{ state: { servers, idpSessions }, * version }` and promotes the inner payload. * + * Also defines sectioned writes (`OAuthPersistSections` + + * `mergeOAuthSections`): a mutation names the `servers`/`idpSessions` + * entries it touched, and shared-store backends overlay only those entries + * onto a fresh read — so one process's stale snapshot can't clobber entries + * another process wrote (see `./node/oauth-persist-file.ts` and the remote + * server's storage route for the two merge sites). + * * This module must stay browser-safe: it imports only the Node-free * `store-serialize` helpers, never `store-io` (which pulls `node:fs`). The * Node-only file backend lives in `./node/oauth-persist-file.ts`. @@ -20,6 +27,94 @@ export interface OAuthPersistSnapshot { idpSessions: Record; } +/** + * Names the sections one mutation touched, at the granularity persistence + * merges on: whole `servers[url]` / `idpSessions[issuer]` entries. + * + * Every mutation on `OAuthStorageBase` is scoped to named entries (a token + * save touches one server, an IdP login one issuer), and each one already + * persists individually — so the write path can know exactly what changed at + * the moment it changes. Backends that share their store with other writers + * (the file backend; the remote backend via the server route) use this to + * write **only** the named entries over a fresh read of the store, so a + * process holding stale memory can no longer erase entries it never mutated + * by flushing its whole snapshot (the last-writer-wins clobber this replaces). + */ +export interface OAuthPersistSections { + /** Server URLs (keys of {@link OAuthPersistSnapshot.servers}) to write. */ + servers?: string[]; + /** IdP issuers (keys of {@link OAuthPersistSnapshot.idpSessions}) to write. */ + idpSessions?: string[]; +} + +/** + * Overlay the named sections of `snapshot` onto `disk`, leaving every other + * entry as the store currently has it. A named key absent from `snapshot` is + * a deletion (the mutation was a clear), so clears propagate rather than + * resurrect. Pure — shared by the Node file backend and the remote server + * route so the two merge implementations cannot drift. + */ +export function mergeOAuthSections( + disk: OAuthPersistSnapshot | null, + snapshot: OAuthPersistSnapshot, + sections: OAuthPersistSections, +): OAuthPersistSnapshot { + const merged: OAuthPersistSnapshot = { + servers: { ...disk?.servers }, + idpSessions: { ...disk?.idpSessions }, + }; + for (const url of sections.servers ?? []) { + const value = snapshot.servers[url]; + if (value === undefined) { + delete merged.servers[url]; + } else { + merged.servers[url] = value; + } + } + for (const issuer of sections.idpSessions ?? []) { + const value = snapshot.idpSessions[issuer]; + if (value === undefined) { + delete merged.idpSessions[issuer]; + } else { + merged.idpSessions[issuer] = value; + } + } + return merged; +} + +function isStringArray(value: unknown): value is string[] { + return Array.isArray(value) && value.every((v) => typeof v === "string"); +} + +/** + * Parse a serialized {@link OAuthPersistSections} (the remote backend sends it + * as a query parameter). Returns `null` on anything that is not the exact + * shape — the server route must not merge on an attacker-shaped descriptor. + */ +export function parseOAuthPersistSections( + raw: string, +): OAuthPersistSections | null { + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return null; + } + if (!isRecord(parsed)) { + return null; + } + const sections: OAuthPersistSections = {}; + if ("servers" in parsed) { + if (!isStringArray(parsed.servers)) return null; + sections.servers = parsed.servers; + } + if ("idpSessions" in parsed) { + if (!isStringArray(parsed.idpSessions)) return null; + sections.idpSessions = parsed.idpSessions; + } + return sections; +} + function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } @@ -70,7 +165,17 @@ export function serializeOAuthPersistBlob( export interface OAuthPersistBackend { read(): Promise; - write(snapshot: OAuthPersistSnapshot): Promise; + /** + * Persist `snapshot`. When `sections` is given, a backend whose store is + * shared with other writers must write only the named entries over a fresh + * read of the store (see {@link mergeOAuthSections}); a backend whose store + * has a single owner (sessionStorage) may ignore it and write the snapshot + * whole. + */ + write( + snapshot: OAuthPersistSnapshot, + sections?: OAuthPersistSections, + ): Promise; remove?(): Promise; } @@ -112,7 +217,7 @@ export function createRemoteOAuthPersistBackend( const store = await res.json(); return parseOAuthPersistBlob(store); }, - async write(snapshot) { + async write(snapshot, sections) { const headers: Record = { "Content-Type": "application/json", }; @@ -120,11 +225,22 @@ export function createRemoteOAuthPersistBackend( headers["x-mcp-remote-auth"] = `Bearer ${options.authToken}`; } - const res = await fetchFn(`${baseUrl}/api/storage/${options.storeId}`, { - method: "POST", - headers, - body: serializeOAuthPersistBlob(snapshot), - }); + // The server holds the shared store, so the merge happens there: the + // sections descriptor rides a query parameter and the route overlays + // only the named entries onto the file, under its cross-process lock. + // Posting the whole snapshot bare would overwrite entries other + // processes wrote since this browser tab loaded. + const sectionsQuery = sections + ? `?sections=${encodeURIComponent(JSON.stringify(sections))}` + : ""; + const res = await fetchFn( + `${baseUrl}/api/storage/${options.storeId}${sectionsQuery}`, + { + method: "POST", + headers, + body: serializeOAuthPersistBlob(snapshot), + }, + ); if (!res.ok) { throw new Error(`Failed to write store: ${res.status}`); diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index 68b7810c2f..8312c4c782 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -14,7 +14,10 @@ import { type ServerOAuthState, type IssuerBoundOAuthState, } from "./store.js"; -import type { OAuthPersistBackend } from "./oauth-persist.js"; +import type { + OAuthPersistBackend, + OAuthPersistSections, +} from "./oauth-persist.js"; import type { IdpSessionState, OAuthClientRegistrationKind, @@ -71,12 +74,19 @@ export class OAuthStorageBase implements OAuthStorage { await this.load(); } - private async persist(): Promise { - const snapshot = this.memory.snapshot(); + /** + * Flush memory to the backend, naming the sections the calling mutation + * touched so shared-store backends can merge just those entries over a + * fresh read (the clobber fix — see {@link OAuthPersistSections}). The + * snapshot is taken *inside* the queued closure, so a write that waited in + * the queue carries the state as of when it actually runs, coalescing with + * any mutations that landed in memory while it waited. + */ + private async persist(sections: OAuthPersistSections): Promise { const prior = this.persistQueue; const tracked = prior .catch(() => {}) - .then(() => this.backend.write(snapshot)); + .then(() => this.backend.write(this.memory.snapshot(), sections)); this.persistQueue = tracked; await tracked; } @@ -210,7 +220,7 @@ export class OAuthStorageBase implements OAuthStorage { clientRegistrationKind: options.registrationKind, }); } - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async savePreregisteredClientInformation( @@ -222,7 +232,7 @@ export class OAuthStorageBase implements OAuthStorage { preregisteredClientInformation: clientInformation, clientRegistrationKind: "static", }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async clearClientInformation( @@ -236,7 +246,7 @@ export class OAuthStorageBase implements OAuthStorage { this.memory.getState().setServerState(serverUrl, { preregisteredClientInformation: undefined, }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); return; } @@ -261,7 +271,7 @@ export class OAuthStorageBase implements OAuthStorage { clientRegistrationKind: undefined, }); } - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async getTokens( @@ -309,7 +319,7 @@ export class OAuthStorageBase implements OAuthStorage { ...(options?.enterpriseManaged === true && { enterpriseManaged: true }), }); } - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async clearTokens(serverUrl: string, issuer?: string): Promise { @@ -331,7 +341,7 @@ export class OAuthStorageBase implements OAuthStorage { .getState() .setServerState(serverUrl, { byIssuer, tokens: undefined }); } - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async getCodeVerifier(serverUrl: string): Promise { @@ -346,7 +356,7 @@ export class OAuthStorageBase implements OAuthStorage { ): Promise { await this.ensureLoaded(); this.memory.getState().setServerState(serverUrl, { codeVerifier }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async clearCodeVerifier(serverUrl: string): Promise { @@ -354,7 +364,7 @@ export class OAuthStorageBase implements OAuthStorage { this.memory .getState() .setServerState(serverUrl, { codeVerifier: undefined }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async getScope(serverUrl: string): Promise { @@ -366,13 +376,13 @@ export class OAuthStorageBase implements OAuthStorage { async saveScope(serverUrl: string, scope: string | undefined): Promise { await this.ensureLoaded(); this.memory.getState().setServerState(serverUrl, { scope }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async clearScope(serverUrl: string): Promise { await this.ensureLoaded(); this.memory.getState().setServerState(serverUrl, { scope: undefined }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async getServerMetadata(serverUrl: string): Promise { @@ -389,7 +399,7 @@ export class OAuthStorageBase implements OAuthStorage { this.memory .getState() .setServerState(serverUrl, { serverMetadata: metadata }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async clearServerMetadata(serverUrl: string): Promise { @@ -397,7 +407,7 @@ export class OAuthStorageBase implements OAuthStorage { this.memory .getState() .setServerState(serverUrl, { serverMetadata: undefined }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async getDiscoveryState( @@ -413,7 +423,7 @@ export class OAuthStorageBase implements OAuthStorage { ): Promise { await this.ensureLoaded(); this.memory.getState().setServerState(serverUrl, { discoveryState: state }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async clearDiscoveryState(serverUrl: string): Promise { @@ -421,7 +431,7 @@ export class OAuthStorageBase implements OAuthStorage { this.memory .getState() .setServerState(serverUrl, { discoveryState: undefined }); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async takeRevocationSnapshot(serverUrl: string): Promise { @@ -443,14 +453,14 @@ export class OAuthStorageBase implements OAuthStorage { serverMetadata: state.serverMetadata, }; this.memory.getState().clearServerState(serverUrl); - await this.persist(); + await this.persist({ servers: [serverUrl] }); return snapshot; } async clear(serverUrl: string): Promise { await this.ensureLoaded(); this.memory.getState().clearServerState(serverUrl); - await this.persist(); + await this.persist({ servers: [serverUrl] }); } async getIdpSession(issuer: string): Promise { @@ -472,18 +482,24 @@ export class OAuthStorageBase implements OAuthStorage { ): Promise { await this.ensureLoaded(); this.memory.getState().setIdpSession(issuer, session); - await this.persist(); + await this.persist({ idpSessions: [issuer] }); } async clearIdpSession(issuer: string): Promise { await this.ensureLoaded(); this.memory.getState().clearIdpSession(issuer); - await this.persist(); + await this.persist({ idpSessions: [issuer] }); } async clearEnterpriseManagedResourceServers(): Promise { await this.ensureLoaded(); + // Capture the affected URLs *before* the clear — afterwards the + // enterpriseManaged flags are gone from memory, and the persist needs to + // name each deleted entry so the merge propagates the deletions. + const servers = Object.entries(this.memory.getState().servers) + .filter(([, state]) => state.enterpriseManaged === true) + .map(([url]) => url); this.memory.getState().clearEnterpriseManagedResourceServers(); - await this.persist(); + await this.persist({ servers }); } } diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index dbeb87ce65..39e3aa7f70 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -37,6 +37,11 @@ import type { } from "../types.js"; import type { JSONRPCMessage } from "@modelcontextprotocol/client"; import { AuthChallengeError } from "../../../auth/challenge.js"; +import { + parseOAuthPersistBlob, + parseOAuthPersistSections, +} from "../../../auth/oauth-persist.js"; +import { writeOAuthSections } from "../../../auth/node/oauth-persist-file.js"; import { MCP_PARAM_HEADER_PREFIX } from "../../../json/xMcpHeader.js"; import { DEFAULT_MAX_FETCH_REQUESTS, @@ -1421,6 +1426,28 @@ export function createRemoteApp( return c.json({ ok: true }); } + // Sectioned OAuth write (the remote OAuth persist backend opts in via + // `?sections=`): merge only the named entries over a fresh read of the + // file, under the cross-process lock. Without this, a browser tab + // holding a stale snapshot would overwrite entries other processes + // (daemon, CLI) wrote since the tab loaded. + const sectionsRaw = c.req.query("sections"); + if (sectionsRaw !== undefined) { + const sections = parseOAuthPersistSections(sectionsRaw); + if (!sections) { + return c.json({ error: "Invalid sections parameter" }, 400); + } + const snapshot = parseOAuthPersistBlob(body); + if (!snapshot) { + return c.json( + { error: "Sectioned write requires an OAuth state body" }, + 400, + ); + } + await writeOAuthSections(filePath, snapshot, sections); + return c.json({ ok: true }); + } + const jsonData = serializeStore(body); await writeStoreFile(filePath, jsonData); return c.json({ ok: true }); From 8549ef0586f937286b2d51ba64e1016c226366aa Mon Sep 17 00:00:00 2001 From: Bob Dickinson Date: Wed, 23 Sep 2026 18:42:56 -0700 Subject: [PATCH 02/45] feat(auth): move OAuth tokens and client secrets into the secret store Split oauth.json persistence so secret material (acquired tokens, client secrets, IdP session tokens) is written to the OS secret store while non-secret residue stays in the file: - New core/auth/node/oauth-secrets.ts: pure split/join/policy module mapping server entries to oauth: fields (per-issuer and legacy tokens/client-secret/prereg-client-secret) and IdP sessions to oauth-idp:. - Rewrite oauth-persist-file.ts: writeOAuthSections splits secrets to the store, readOAuthStore joins them back (store wins over file plaintext) and lazily migrates plaintext secrets when the store is durable, removeOAuthStore purges store entries. Store write failures degrade to memory-only with a once-per-reason warning; secrets are never written back to the file. - New MCP_INSPECTOR_PERSIST_TOKENS=all|access|none knob controlling which acquired tokens persist (write-side; registration client secrets always persist). Invalid values warn and default to all. - Remote storage routes special-case the oauth store (sectioned and full-replace writes via locked merge+split, purge on DELETE); sectioned writes on other stores are rejected. - CLI reads stored auth via the joined readOAuthStore so migrated tokens remain visible to --wait-for-auth and refresh. - Pin MCP_INSPECTOR_SECRET_STORE=memory in web/cli test configs so tests never touch the real OS keychain. - Docs: environment-variables.md and secret-storage.md updated. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Dickinson --- clients/cli/__tests__/stored-auth.test.ts | 51 +- clients/cli/src/cli.ts | 23 +- clients/cli/vitest.config.ts | 5 + .../src/test/core/auth/oauth-secrets.test.ts | 351 +++++++++++++ .../integration/auth/node/storage.test.ts | 7 +- .../mcp/inspectorClient-ema-e2e.test.ts | 10 +- .../mcp/inspectorClient-oauth-e2e.test.ts | 24 +- .../integration/mcp/remote/transport.test.ts | 16 +- .../test/integration/storage/adapters.test.ts | 163 +++++- .../storage/oauth-secret-split.test.ts | 468 ++++++++++++++++++ clients/web/vite.config.ts | 11 + core/auth/node/oauth-persist-file.ts | 348 +++++++++++-- core/auth/node/oauth-secrets.ts | 320 ++++++++++++ core/auth/node/storage-node.ts | 8 +- core/auth/oauth-persist.ts | 7 + core/auth/remote/storage-remote.ts | 9 +- core/mcp/remote/node/server.ts | 52 +- docs/environment-variables.md | 1 + docs/secret-storage.md | 11 +- 19 files changed, 1780 insertions(+), 105 deletions(-) create mode 100644 clients/web/src/test/core/auth/oauth-secrets.test.ts create mode 100644 clients/web/src/test/integration/storage/oauth-secret-split.test.ts create mode 100644 core/auth/node/oauth-secrets.ts diff --git a/clients/cli/__tests__/stored-auth.test.ts b/clients/cli/__tests__/stored-auth.test.ts index bea1ad4994..059f8d5e25 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"; @@ -10,6 +18,9 @@ import { deepLinkTransport, refreshStoredAuthToken, } from "../src/cli.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 +75,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 +120,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 +366,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( [ @@ -538,11 +566,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 { diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 95c5770fa6..b452b4154e 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -44,11 +44,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, - type OAuthPersistSnapshot, -} from "@inspector/core/auth/oauth-persist.js"; -import { writeOAuthSections } from "@inspector/core/auth/node/oauth-persist-file.js"; + readOAuthStore, + writeOAuthSections, +} from "@inspector/core/auth/node/oauth-persist-file.js"; import { discoverAuthorizationServerMetadataFromCandidates, getAuthorizationServerUrl, @@ -260,20 +260,17 @@ type StoredServerState = { 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 + * unreadable. */ async function readOAuthSnapshot( statePath: string, ): Promise { - const { readFile } = await import("node:fs/promises"); try { - const text = await readFile(statePath, "utf8"); - const snapshot = parseOAuthPersistBlob(text); + const snapshot = await readOAuthStore(statePath); if (snapshot) return snapshot; } catch { // Absent/unreadable/malformed → fall through to the empty snapshot below. 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-secrets.test.ts b/clients/web/src/test/core/auth/oauth-secrets.test.ts new file mode 100644 index 0000000000..0168853567 --- /dev/null +++ b/clients/web/src/test/core/auth/oauth-secrets.test.ts @@ -0,0 +1,351 @@ +/** + * 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, + LEGACY_TOKENS_FIELD, + LEGACY_CLIENT_SECRET_FIELD, + PREREG_CLIENT_SECRET_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 { 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://s.example/mcp", + ); + expect(oauthIdpSecretServerId("https://idp.example")).toBe( + "oauth-idp:https://idp.example", + ); + expect(issuerTokensField("https://as.example")).toBe( + "tokens:https://as.example", + ); + expect(issuerClientSecretField("https://as.example")).toBe( + "client-secret:https://as.example", + ); + }); +}); + +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({}); + }); +}); + +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("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, + PREREG_CLIENT_SECRET_FIELD, + ].sort(), + ); + const issuer = "https://as.example"; + expect(serverSecretFields({ byIssuer: { [issuer]: {} } })).toContain( + issuerTokensField(issuer), + ); + expect(serverSecretFields({ byIssuer: { [issuer]: {} } })).toContain( + issuerClientSecretField(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); + }); +}); 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..40774b4668 100644 --- a/clients/web/src/test/integration/auth/node/storage.test.ts +++ b/clients/web/src/test/integration/auth/node/storage.test.ts @@ -532,9 +532,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/remote/transport.test.ts b/clients/web/src/test/integration/mcp/remote/transport.test.ts index 7e4ca85109..d3b979516b 100644 --- a/clients/web/src/test/integration/mcp/remote/transport.test.ts +++ b/clients/web/src/test/integration/mcp/remote/transport.test.ts @@ -1061,7 +1061,21 @@ describe("Remote transport e2e", () => { ); expect(badBody.status).toBe(400); expect((await badBody.json()).error).toBe( - "Sectioned write requires an OAuth state body", + "OAuth store writes require an OAuth state body", + ); + + // Sectioned writes are an OAuth-store contract; other stores are raw KV. + const wrongStore = await fetch( + `${baseUrl}/api/storage/other-store?sections=${encodeURIComponent('{"servers":[]}')}`, + { + method: "POST", + headers, + body: JSON.stringify({ some: "data" }), + }, + ); + expect(wrongStore.status).toBe(400); + expect((await wrongStore.json()).error).toBe( + "Sectioned writes are only supported for the oauth store", ); }); diff --git a/clients/web/src/test/integration/storage/adapters.test.ts b/clients/web/src/test/integration/storage/adapters.test.ts index 28e60391de..5131f09d15 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(); }); @@ -381,7 +417,7 @@ describe("OAuth persistence", () => { const storage = new RemoteOAuthStorage({ baseUrl, - storeId: "test-store", + storeId: "oauth", authToken, }); @@ -390,7 +426,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; }; @@ -400,7 +436,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}`, @@ -423,14 +459,14 @@ describe("OAuth persistence", () => { const storage1 = new RemoteOAuthStorage({ baseUrl, - storeId: "test-store", + storeId: "oauth", 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; }; @@ -442,7 +478,7 @@ describe("OAuth persistence", () => { const storage2 = new RemoteOAuthStorage({ baseUrl, - storeId: "test-store", + storeId: "oauth", authToken, }); @@ -461,7 +497,7 @@ describe("OAuth persistence", () => { const storage = new RemoteOAuthStorage({ baseUrl, - storeId: "test-store", + storeId: "oauth", authToken, }); @@ -469,12 +505,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}`, @@ -485,12 +521,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}`, @@ -500,5 +536,98 @@ 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("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..386622beeb --- /dev/null +++ b/clients/web/src/test/integration/storage/oauth-secret-split.test.ts @@ -0,0 +1,468 @@ +/** + * 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 } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + writeOAuthSections, + readOAuthStore, + removeOAuthStore, + resetOAuthSecretStoreWarnings, +} 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, + IDP_SESSION_FIELD, + 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); + + // The residue file is still written — never with the secrets in it. + const raw = readRawFile(); + expect(raw.servers[SERVER]!.scope).toBe("read"); + expect(raw.servers[SERVER]!.tokens).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); + }); +}); + +describe("readOAuthStore migration", () => { + 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 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("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); + }); + + 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("warns but still deletes the file when the store purge fails", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + 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 removeOAuthStore(filePath, failingPurge); + expect(existsSync(filePath)).toBe(false); + expect( + warn.mock.calls.some(([msg]) => String(msg).includes("purge failed")), + ).toBe(true); + }); + + it("is a no-op purge for a missing file", async () => { + await expect( + removeOAuthStore(filePath, new InMemorySecretStore()), + ).resolves.toBeUndefined(); + expect(existsSync(filePath)).toBe(false); + }); +}); 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