diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx index 2962d601..2c274993 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx @@ -52,7 +52,7 @@ export const EncryptedFile: Story = { // and stays readable in the button's accessible name. await expect( canvas.getByRole("button", { - name: "Copy secrets file path: /home/node/.mcp-inspector/secrets.json", + name: "Secrets: Encrypted file. Owner-only permissions. Copy secrets file path: /home/node/.mcp-inspector/secrets.json", }), ).toBeInTheDocument(); }, diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx index 995da7d4..84717d96 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx @@ -45,9 +45,12 @@ describe("SecretStorageFooter", () => { "Secrets: Encrypted file. Owner-only permissions.", ); expect(band).toHaveAttribute("data-tone", "neutral"); + // The accessible name must carry the *status* as well as the affordance: + // `aria-label` replaces descendant text, so labelling it only "Copy…" + // would announce the button and discard the warning the band exists for. expect( screen.getByRole("button", { - name: "Copy secrets file path: /home/node/.mcp-inspector/secrets.json", + name: "Secrets: Encrypted file. Owner-only permissions. Copy secrets file path: /home/node/.mcp-inspector/secrets.json", }), ).toBeInTheDocument(); }); @@ -136,6 +139,19 @@ describe("SecretStorageFooter", () => { ); }); + it("announces the security status, not just the copy affordance", () => { + // The regression this pins: `aria-label` overrides the button's + // descendant text, so a screen-reader user heard "Copy secrets file + // path …" and lost "Plaintext file" entirely — the one sentence that + // should change their mind about typing a secret. + renderWithMantine(); + const button = screen.getByRole("button"); + expect(button).toHaveAccessibleName( + /^Secrets: Plaintext file\. Owner-only permissions\./, + ); + expect(button).toHaveAccessibleName(/Copy secrets file path/); + }); + it("renders nothing when the backend didn't report a store", () => { // A guessed answer under a secret field is worse than no answer, so the // unknown case is silence rather than a default. diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx index 18dcfa83..66415f88 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx @@ -291,12 +291,19 @@ export function SecretStorageFooter({ info }: SecretStorageFooterProps) { {({ copied, copy }) => band( - {/* The accessible name carries the path itself: a screen-reader - user gets no benefit from a clipboard they cannot inspect, so - the one place the location is still *readable* is here. */} + {/* The accessible name has to carry **both** halves. `aria-label` + replaces the button's descendant text, so labelling it only + "Copy secrets file path …" announced the affordance and threw + away the security status — "Plaintext file", "Unreadable + file", "not owner-only" — which is the entire reason this band + exists. The status leads, because it is the part a user needs + before deciding to type a secret; the path follows, since a + clipboard is no use to someone who cannot inspect it. Leading + with the visible text also keeps the accessible name a + superset of it (WCAG 2.5.3). */} {row} diff --git a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts index 750e8fa9..0df11689 100644 --- a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts +++ b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts @@ -680,17 +680,32 @@ describe("readOnDiskEncryption", () => { ); }); - it("needs no passphrase — it reads the envelope, never the payload", async () => { - // A store whose key is wrong still has to be describable in the UI. + it("reports a file the current passphrase cannot open as unreadable, not encrypted", async () => { + // This asserted the opposite until round 12, and the reviewer was right + // to call it: structure is not access. A store with the wrong key — or + // none — reads `null` from every `get` and refuses every `set`, so + // describing the file as "encrypted" gives the quiet, healthy footer to + // precisely the state the unreadable one exists to warn about. The user + // then discovers it when their save fails. const writer = new FileSecretStore({ filePath: filePath(), passphrase: "right-key", }); await writer.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); - const keyless = new FileSecretStore({ filePath: filePath() }); - expect(await keyless.readOnDiskEncryption()).toEqual({ - state: "encrypted", - }); + + for (const wrong of [ + new FileSecretStore({ filePath: filePath() }), + new FileSecretStore({ filePath: filePath(), passphrase: "wrong-key" }), + ]) { + const state = await wrong.readOnDiskEncryption(); + expect(state.state).toBe("unreadable"); + expect(state.state === "unreadable" ? state.detail : "").toMatch( + /cannot be decrypted with the current MCP_INSPECTOR_SECRET_KEY/, + ); + } + + // And the store that *can* open it is still described as encrypted. + expect(await writer.readOnDiskEncryption()).toEqual({ state: "encrypted" }); }); }); @@ -970,6 +985,36 @@ describe("readOnDiskEncryption rejects an envelope it could not open", () => { }, "kdf N is 3, which is not a power of two above 1", ], + [ + "an empty ciphertext", + { + version: 1, + encryption: "aes-256-gcm", + kdf: { algorithm: "scrypt", salt: "AAAA", N: 16384, r: 8, p: 1 }, + data: `${Buffer.alloc(12).toString("base64")}.${Buffer.alloc(16).toString("base64")}.`, + }, + "ciphertext is empty", + ], + [ + "a salt that is not base64", + { + version: 1, + encryption: "aes-256-gcm", + kdf: { algorithm: "scrypt", salt: "!!!", N: 16384, r: 8, p: 1 }, + data: `${Buffer.alloc(12).toString("base64")}.${Buffer.alloc(16).toString("base64")}.${Buffer.alloc(8).toString("base64")}`, + }, + "kdf salt is missing or not base64", + ], + [ + "a non-positive KDF r", + { + version: 1, + encryption: "aes-256-gcm", + kdf: { algorithm: "scrypt", salt: "AAAA", N: 16384, r: 0, p: 1 }, + data: `${Buffer.alloc(12).toString("base64")}.${Buffer.alloc(16).toString("base64")}.${Buffer.alloc(8).toString("base64")}`, + }, + "kdf r/p are 0/1, which are not positive integers", + ], [ "an unsupported KDF", { @@ -1003,18 +1048,100 @@ describe("readOnDiskEncryption accepts a real envelope", () => { }); }); -describe("concurrent writers (no lock, optimistic verify-and-retry)", () => { - // The lock these replace was removed after three review rounds each found - // a real race in it; the last needed a compare-and-swap on a directory - // entry that Node does not expose. The store now lets writers collide and - // makes the loser notice — so these assert convergence, which is the - // property that actually matters, rather than mutual exclusion, which it - // no longer claims. +/** + * Write an entirely different map straight to disk, bypassing the store. + * + * This is the *only* way to model a second process here. `serialize` keys + * its queue on the resolved path, process-wide, so two `FileSecretStore` + * instances pointing at one file do **not** race — they take turns. An + * earlier version of these tests used two instances and believed it was + * exercising the retry; it was exercising the queue, and passed for a + * reason unrelated to the code under test. + */ +async function writeAsAnotherProcess( + target: string, + secrets: Record, +): Promise { + await fs.writeFile( + target, + JSON.stringify({ version: 1, encryption: "none", secrets }), + "utf-8", + ); +} - it("two stores racing set on one file keep both entries", async () => { - // The classic lost update: both read the same map, both write. Whichever - // lands second clobbers the other's entry — and the clobbered writer's - // verifying read is what catches it and re-applies. +/** Inject one external write between the store's write and its verify. */ +function injectOneExternalWrite( + store: FileSecretStore, + target: string, + secrets: Record, +): void { + // Double cast: `writeMap` is private, so no single cast reaches it. Safe + // because the asserted shape is its real signature — a rename or + // signature change fails this line rather than silently detaching the + // stub. See the note on `clobberAfterEveryWrite` below. + const inner = store as unknown as { + writeMap: (m: Record) => Promise; + }; + const realWrite = inner.writeMap.bind(store); + let done = false; + inner.writeMap = async (map: Record) => { + await realWrite(map); + if (done) return; + done = true; + await writeAsAnotherProcess(target, secrets); + }; +} + +describe("descriptor and retry edge cases", () => { + it("reports a decrypt that succeeds but yields the wrong shape", async () => { + // Authentic ciphertext, right passphrase, wrong payload — so `readMap` + // throws something that is *not* a key mismatch, and the descriptor must + // surface that reason rather than the passphrase advice. + await writeEncryptedFixtureWithPayload(["not", "a", "map"], "right-key"); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "right-key", + }); + const state = await store.readOnDiskEncryption(); + expect(state.state).toBe("unreadable"); + expect(state.state === "unreadable" ? state.detail : "").toMatch( + /does not hold a secret map/, + ); + }); + + it("retries when the verifying read itself fails, then reports the unreadable file", async () => { + // The verify can fail rather than mismatch — another writer replacing the + // file with something unparseable. That is not a clobber to re-apply + // onto, so the attempt is abandoned and the next one reports the real + // problem instead of looping silently. + const store = new FileSecretStore({ filePath: filePath() }); + const inner = store as unknown as { + writeMap: (m: Record) => Promise; + }; + const realWrite = inner.writeMap.bind(store); + inner.writeMap = async (map: Record) => { + await realWrite(map); + await fs.writeFile(filePath(), "{ not json", "utf-8"); + }; + + await expect(store.set("srv", "env:A", "1")).rejects.toBeInstanceOf( + SecretStoreUnavailableError, + ); + }); + + it("getMany answers empty for a file that does not exist yet", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + expect(await store.getMany("srv", ["env:A", "env:B"])).toEqual({}); + }); +}); + +describe("in-process mutations are serialized per path", () => { + // Not a race, and the tests must not claim to be one. `serialize` is keyed + // on the resolved path process-wide, so two instances on one file take + // turns — which is a real property worth pinning, because it is what makes + // the in-process case correct without relying on the optimistic retry. + + it("two stores writing one file keep both entries", async () => { const a = new FileSecretStore({ filePath: filePath() }); const b = new FileSecretStore({ filePath: filePath() }); await Promise.all([a.set("srv", "env:A", "1"), b.set("srv", "env:B", "2")]); @@ -1025,8 +1152,6 @@ describe("concurrent writers (no lock, optimistic verify-and-retry)", () => { }); it("keeps every entry when many independent stores write at once", async () => { - // Independent instances, so the in-process queue does not serialize them - // for us — each is its own writer with its own view of the file. const stores = Array.from( { length: 8 }, () => new FileSecretStore({ filePath: filePath() }), @@ -1039,20 +1164,7 @@ describe("concurrent writers (no lock, optimistic verify-and-retry)", () => { } }); - it("converges when the racing writers are encrypted", async () => { - // Same property with a scrypt derivation inside every attempt, which is - // where the window is widest. - const opts = { filePath: filePath(), passphrase: "hunter2" }; - const a = new FileSecretStore(opts); - const b = new FileSecretStore(opts); - await Promise.all([a.set("srv", "env:A", "1"), b.set("srv", "env:B", "2")]); - - const reader = new FileSecretStore(opts); - expect(await reader.get("srv", "env:A")).toBe("1"); - expect(await reader.get("srv", "env:B")).toBe("2"); - }); - - it("a racing delete and set both land", async () => { + it("orders a delete and a set on the same file", async () => { const store = new FileSecretStore({ filePath: filePath() }); await store.set("srv", "env:OLD", "x"); @@ -1067,6 +1179,62 @@ describe("concurrent writers (no lock, optimistic verify-and-retry)", () => { expect(await reader.get("srv", "env:OLD")).toBe(null); expect(await reader.get("srv", "env:NEW")).toBe("y"); }); +}); + +describe("cross-process convergence (optimistic verify-and-retry)", () => { + // These drive the path the in-process queue cannot reach: a write landing + // between our write and our verifying read, as a second Inspector would + // produce it. + + it("re-applies onto the other writer's result when the verify sees a mismatch", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("srv", "env:EXISTING", "0"); + injectOneExternalWrite(store, filePath(), { + "srv:env:OTHER": "from-another-process", + }); + + await store.set("srv", "env:MINE", "1"); + + // Our value survived *and* landed on top of theirs rather than replacing + // it — which is the whole claim: the clobbered writer repairs it. + const reader = new FileSecretStore({ filePath: filePath() }); + expect(await reader.get("srv", "env:MINE")).toBe("1"); + expect(await reader.get("srv", "env:OTHER")).toBe("from-another-process"); + }); + + it("re-applies a delete against a concurrently rewritten file", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("srv", "env:DOOMED", "x"); + injectOneExternalWrite(store, filePath(), { + "srv:env:DOOMED": "x", + "srv:env:OTHER": "from-another-process", + }); + + await store.delete("srv", "env:DOOMED"); + + const reader = new FileSecretStore({ filePath: filePath() }); + expect(await reader.get("srv", "env:DOOMED")).toBe(null); + expect(await reader.get("srv", "env:OTHER")).toBe("from-another-process"); + }); + + it("converges through a scrypt derivation on every attempt", async () => { + // The encrypted path re-derives per attempt, which is where the retry is + // most expensive and the window widest. + const opts = { filePath: filePath(), passphrase: "hunter2" }; + const store = new FileSecretStore(opts); + await store.set("srv", "env:EXISTING", "0"); + // The intruder writes a *plaintext* envelope, as a differently + // configured process would; the store must still read it and re-apply. + injectOneExternalWrite(store, filePath(), { + "srv:env:OTHER": "from-another-process", + }); + + await store.set("srv", "env:MINE", "1"); + + const reader = new FileSecretStore(opts); + expect(await reader.get("srv", "env:MINE")).toBe("1"); + expect(await reader.get("srv", "env:OTHER")).toBe("from-another-process"); + }); // A writer that never wins. Simulated by replacing the file *behind* the // store immediately after each write — with a raw `fs` write rather than a diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index 4a1cb148..dbfadae0 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -365,6 +365,28 @@ export class FileSecretStore implements SecretStore { // fail, which is the one thing this state exists to prevent. const problem = encryptedEnvelopeProblem(parsed); if (problem) return { state: "unreadable", detail: problem }; + // Structure is not access. A well-formed envelope this passphrase + // cannot open reads as `null` from every `get` and refuses every + // `set` — so classifying it "encrypted" hands the quiet, healthy + // footer to the exact state the unreadable one promises to warn + // about, and the user finds out when their save fails. Authenticate + // it: decrypt and discard. Costs one scrypt derivation (~23ms) per + // descriptor build, which is once per `/api/config`, and buys the + // difference between describing the file and merely describing its + // shape. + try { + await this.readMap(); + } catch (err) { + return { + state: "unreadable", + detail: + err instanceof SecretFileKeyMismatchError + ? `it cannot be decrypted with the current ${SECRET_KEY_ENV}` + : err instanceof Error + ? err.message + : String(err), + }; + } return { state: "encrypted" }; } return { diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index cc51c295..893ff435 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -250,9 +250,11 @@ Each server entry may carry these Inspector-extension fields at the top level: - `oauth.clientId` / `oauth.clientSecret` / `oauth.scopes` → pre-seeded OAuth client credentials via `InspectorClientOptions.oauth` (the disk-side `oauth` object is lifted into the flat `oauthClientId` / etc. fields on `InspectorServerSettings` for the form). - **First-connect contract**: settings apply on the _first_ outbound request after the entry loads from disk — no need to open the settings form. The browser sends `settings` to the backend in the `/api/mcp/connect` body; the backend reads it from `RemoteConnectRequest` and threads it into `createTransportNode`. - **Secret storage (#1356)**: `oauth.clientSecret` and stdio `env` values are persisted in the OS keychain (macOS Keychain Services / Windows Credential Manager / Linux libsecret via `@napi-rs/keyring`), keyed by `(serverId, field)` under the service name `mcp-inspector`. Field names: `oauth-client-secret`, `env:` (one per stdio env variable). The on-disk `mcp.json` is stripped of these values — `oauth.clientSecret` is omitted entirely, stdio env keys are preserved with empty-string placeholders (`"env": { "API_KEY": "" }`) so the file still documents the env interface the server expects. The wire shape returned by `GET /api/servers` is unchanged from before #1356: the handler rehydrates values from the keychain so browser code sees the same JSON it has always seen. **TUI/CLI rehydration:** the shared `loadServerEntries()` path (`core/mcp/node/servers.ts`) calls `rehydrateMcpConfigFromKeychain()` (`core/mcp/node/server-secrets.ts`) after reading `mcp.json`, so runner clients see the same effective OAuth client secrets and stdio env values as the web catalog list. This path is read-only — it does not migrate plaintext secrets into the keychain (that remains the web `GET /api/servers` migration sweep). The keychain interactions live in `core/auth/node/secret-store.ts` behind a `SecretStore` interface. As of #1950 there are **three** implementations — `KeyringSecretStore`, `FileSecretStore`, `InMemorySecretStore` — chosen per run by `core/auth/node/secret-store-selection.ts`; the integration suite still injects one directly via `RemoteServerOptions.secretStore`. - - **Migration**: on every `GET /api/servers`, the handler walks the freshly-read config and, for any entry that still carries plaintext secrets (older Inspector builds, hand-edited files, files imported from another tool), lifts each value into the keychain and rewrites the file with the stripped shape. The migration is idempotent — when the keychain already holds a value for `(serverId, field)`, the keychain wins and the disk plaintext is dropped unread. After the rewrite the disk file no longer contains the secret material. + - **Migration**: on every `GET /api/servers`, the handler walks the freshly-read config and, for any entry that still carries plaintext secrets (older Inspector builds, hand-edited files, files imported from another tool), lifts each value into **the selected secret store** and rewrites the file with the stripped shape. The migration is idempotent — when the store already holds a value for `(serverId, field)`, the store wins and the disk plaintext is dropped unread. + - **Post-#1950 the rewrite is gated on durability.** The disk copy is stripped only once the value is somewhere that outlives the process (`secretStoreIsDurable`). Against the session-scoped in-memory store — the container fallback — stripping would trade a secret that survives restarts for one that dies with the process, and because this runs on an ordinary `GET`, merely opening the app would destroy it. The values are still loaded into the store, so the session behaves normally; only the delete is withheld, and a one-line warning says so. + - **The "keychain wins" lookup uses the strict read.** `get` is tolerant by contract, so a transient store failure answers `null` — and this branch _writes_ on `null` and then strips the disk copy, which would let an older on-disk value replace a newer stored one. `secretStoreGetStrict` throws instead, and the migration abandons rather than writing on an answer it cannot trust. - **Linux without libsecret**: every `SecretStore` shares one _tolerance contract_ — `get` returns `null`, the destructive operations silently no-op, and only `set` hard-fails (with a `SecretStoreUnavailableError`, translated to a `503`). So no-secret flows (creating a stdio server with no env values, deleting an entry, reading the list, the defensive sweep on POST) work normally on a box with no reachable keychain. **Since #1950 that is no longer the end of the story**: a host without a keychain now falls back to a file or in-memory store rather than being unable to persist a secret at all — see [Secret store selection (#1950)](#secret-store-selection-1950). - - **Migration tolerance**: when migration encounters `KeychainUnavailableError`, the GET handler logs a warning, leaves the on-disk plaintext untouched, and serves the (still-plaintext) response. Subsequent reads retry — installing libsecret later lifts the secrets on the next GET without any user action. + - **Migration tolerance**: when migration encounters a `SecretStoreUnavailableError` — `KeychainUnavailableError` is one subclass, and the file store raises its own for an unreadable file, a changed passphrase or a write failure — the GET handler logs a warning, leaves the on-disk plaintext untouched, and serves the (still-plaintext) response. Subsequent reads retry, so installing libsecret (or restoring the passphrase) lifts the secrets on the next GET with no user action. Partial-migration semantics are deliberate: if `set` threw partway through the loop the handler returns the _original_ config rather than the partially-rewritten one, so the disk file stays intact and the idempotent store-wins branch absorbs the already-written entries on retry. - **Write ordering on POST/PUT**: keychain writes happen before the disk write, and obsolete-field deletions happen after. The intent is that a `set` failure (the only hard-fail path) leaves both stores in their pre-write state — no half-applied entry on disk that would trap a retry POST at `409`, and no premature deletion of an obsolete field whose disk write later fails. - **Id rename (`PUT` with new `id`)**: secrets are keyed by **server id** (the `mcpServers` map key), not URL. On rename the handler reads existing keychain entries for the **original** id (`expectedSecretFields` + `readKeychainEntriesFor`), merges them with any secrets in the PUT body (body wins on conflict), filters to fields the **new** on-disk entry still expects (so removed stdio env keys are not carried over), writes under the **new** id, updates disk, then `deleteAllForServer(originalId)`. This matters for config-only renames (e.g. Server Config modal) that do not re-send OAuth client secrets or stdio env values — those live only in the keychain after #1356. Covered by `servers-route.test.ts` (`PUT rename moves … when it exists only in the keychain` for OAuth and stdio env). - **Out of scope for this PR**: the OAuth handshake itself still runs in the browser via the MCP SDK, so during the token exchange the secret transits the wire (browser → MCP SDK → OAuth provider's token endpoint). The on-disk win this PR delivers is that the secret is no longer in the shareable / symlinked `mcp.json` and is no longer the source-of-truth on the filesystem. Moving the token exchange to the Node side is tracked separately.