diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index bae88517..ba7dca8b 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -742,6 +742,7 @@ function App() { writable: serverListWritable, version: inspectorVersion, secretStorage, + refresh: refreshInitialConfig, } = useInitialConfig({ baseUrl: configBaseUrl, authToken: getAuthToken(), @@ -3994,7 +3995,16 @@ function App() { targetId: settingsModalTargetId, resolveInitial: (id) => servers.find((s) => s.id === id)?.settings ?? EMPTY_SETTINGS, - onPersist: updateServerSettings, + onPersist: async (id: string, settings: InspectorServerSettings) => { + await updateServerSettings(id, settings); + // The saved settings may include an OAuth client secret or a stdio + // `env:` value, and writing one can change what the secrets file *is* + // — the first save under a newly-set passphrase re-encrypts a + // pre-existing plaintext file. The backend re-derives the descriptor + // per request, so ask again; otherwise the footer keeps reporting the + // state from page load for the rest of the session (#1950 review r14). + refreshInitialConfig(); + }, // Surface failures via toast — the modal usually closes // immediately on user dismiss, so a silent fail-on-flush would // leave the user thinking their last edits saved when they @@ -4026,6 +4036,10 @@ function App() { authToken: getAuthToken(), }); setClientConfig(next); + // Same reason as the server-settings persist: this can carry the + // enterprise IdP client secret, and that write is what flips a + // pending-encryption file to encrypted. + refreshInitialConfig(); }, onError: (err) => { notifications.show({ diff --git a/clients/web/src/test/core/react/useInitialConfig.test.tsx b/clients/web/src/test/core/react/useInitialConfig.test.tsx index a94980df..17a1de66 100644 --- a/clients/web/src/test/core/react/useInitialConfig.test.tsx +++ b/clients/web/src/test/core/react/useInitialConfig.test.tsx @@ -8,7 +8,7 @@ */ import { describe, it, expect, vi } from "vitest"; -import { renderHook, waitFor } from "@testing-library/react"; +import { act, renderHook, waitFor } from "@testing-library/react"; import { useInitialConfig } from "@inspector/core/react/useInitialConfig"; function jsonResponse(body: unknown, ok = true): Response { @@ -469,4 +469,53 @@ describe("useInitialConfig", () => { expect(result.current.secretStorage).toBeUndefined(); }); }); + + describe("refresh (#1950 review r14)", () => { + it("re-fetches on demand so a descriptor this app changed is not stale", async () => { + // The lazy encryption upgrade: the first save under a newly-set + // passphrase re-encrypts a pre-existing plaintext file. `/api/config` + // re-derives per request, but without a refresh the page keeps the + // descriptor it fetched at mount and the footer says "Plaintext file" + // for the rest of the session about a file that no longer is. + let plaintext = true; + const fetchFn = vi.fn().mockImplementation(() => + Promise.resolve( + jsonResponse({ + secretStorage: { + kind: "file", + reason: "fallback", + durable: true, + plaintext, + path: "/tmp/secrets.json", + }, + }), + ), + ); + + const { result } = renderHook(() => + useInitialConfig({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.secretStorage?.plaintext).toBe(true); + + plaintext = false; // the upgrading write happens + act(() => result.current.refresh()); + await waitFor(() => + expect(result.current.secretStorage?.plaintext).toBe(false), + ); + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + + it("drops a refresh that resolves after unmount", async () => { + const fetchFn = vi.fn().mockResolvedValue(jsonResponse({})); + const { result, unmount } = renderHook(() => + useInitialConfig({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + const refresh = result.current.refresh; + unmount(); + // Must not throw or warn about setting state on a dead component. + expect(() => refresh()).not.toThrow(); + }); + }); }); 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 e9081852..f6cc0c66 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 @@ -1094,6 +1094,32 @@ describe("readOnDiskEncryption rejects an envelope it could not open", () => { }); }); +describe("readOnDiskEncryption validates the plaintext payload too", () => { + it("reports unreadable for a plaintext file whose secrets are not a map", async () => { + // The encrypted branch was tightened in round 12 and this one was left + // behind, so `{ "encryption": "none", "secrets": [] }` — trivial to + // hand-write — described a healthy plaintext store whose next save + // `readMap` refuses. + await fs.writeFile( + filePath(), + JSON.stringify({ version: 1, encryption: "none", secrets: [] }), + "utf-8", + ); + const store = new FileSecretStore({ filePath: filePath() }); + const state = await store.readOnDiskEncryption(); + expect(state.state).toBe("unreadable"); + expect(state.state === "unreadable" ? state.detail : "").toMatch( + /does not hold a secret map/, + ); + }); + + it("still reports plaintext for a file this build actually wrote", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("srv", "env:A", "1"); + expect(await store.readOnDiskEncryption()).toEqual({ state: "plaintext" }); + }); +}); + describe("readOnDiskEncryption accepts a real envelope", () => { it("still reports encrypted for a file this build actually wrote", async () => { // The negative cases above are only meaningful next to this: tightening diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index 95994b72..d8b2c8f2 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -356,7 +356,23 @@ export class FileSecretStore implements SecretStore { detail: `format version ${String(parsed.version)}, expected ${FORMAT_VERSION}`, }; } - if (parsed.encryption === "none") return { state: "plaintext" }; + if (parsed.encryption === "none") { + // Symmetry with the encrypted branch: a `secrets` that is not a + // string map is refused by `readMap`, so returning "plaintext" here + // describes a healthy store whose next save is guaranteed to fail — + // `{ "encryption": "none", "secrets": [] }` being the easy case to + // hand-write. The encrypted branch was tightened in round 12 and this + // one was left behind. + try { + asSecretMap(parsed.secrets ?? {}, this.filePath); + } catch (err) { + return { + state: "unreadable", + detail: err instanceof Error ? err.message : String(err), + }; + } + return { state: "plaintext" }; + } if (parsed.encryption === CIPHER) { // Naming the cipher is not the same as being openable, and a partial // check is its own trap: three dot-separated strings satisfy diff --git a/core/auth/node/secret-store.ts b/core/auth/node/secret-store.ts index 611418dd..f80d2f7a 100644 --- a/core/auth/node/secret-store.ts +++ b/core/auth/node/secret-store.ts @@ -540,9 +540,16 @@ export class KeyringSecretStore implements SecretStore { */ /** * Read `store` intolerantly: throw rather than answer `null` when the store - * itself cannot be read. Falls back to `get` for a store that does not - * distinguish the two, which is right for the in-memory and file stores — - * neither can fail in a way that masquerades as absence. + * itself cannot be read. + * + * Falls back to `get` only for stores that genuinely cannot fail that way — + * in practice `InMemorySecretStore`, whose reads are a `Map` lookup. It is + * **not** a safe fallback for the file store: `FileSecretStore.get` + * deliberately converts filesystem, format and key failures to `null`, which + * is exactly the masquerade this helper exists to prevent, and is why that + * store implements `getStrict` itself. The fallback stays because the seam is + * optional and third-party/test doubles must keep compiling — not because + * `get` is equivalent for anything that can fail. */ export async function secretStoreGetStrict( store: SecretStore, diff --git a/core/react/useInitialConfig.ts b/core/react/useInitialConfig.ts index 475e39f3..cbc5ddef 100644 --- a/core/react/useInitialConfig.ts +++ b/core/react/useInitialConfig.ts @@ -33,7 +33,7 @@ * after unmount or a re-fetch is dropped rather than overwriting current state. */ -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import type { SecretStorageInfo } from "../auth/secret-storage-info.js"; export interface UseInitialConfigOptions { @@ -56,6 +56,21 @@ export interface UseInitialConfigResult { secretStorage: SecretStorageInfo | undefined; /** True while the initial fetch is in flight. */ loading: boolean; + /** + * Re-fetch the payload. + * + * Exists for `secretStorage`, which is the one field here that describes + * state *this app changes*. Saving a secret under a newly-set passphrase + * re-encrypts the file, so the descriptor the backend would serve now + * differs from the one fetched at mount — and without a way to ask again, + * the footer keeps saying "Plaintext file" for the rest of the session, + * about a file that is no longer plaintext. The backend already re-derives + * the descriptor per request; this is the client half of that. + * + * Safe to call at any time: the GET is idempotent, and a response that + * arrives after unmount is dropped. + */ + refresh: () => void; } /** Minimal shape we read from the `/api/config` payload. */ @@ -207,5 +222,21 @@ export function useInitialConfig( }; }, [load]); - return { version, sandboxUrl, writable, secretStorage, loading }; + // A manual re-fetch shares `load`'s cancellation contract, but has no + // effect teardown to key off — so it tracks its own liveness through a ref + // that the unmount effect flips. Without that, a refresh fired just before + // unmount would still call `setState` on a dead component. + const alive = useRef(true); + useEffect(() => { + alive.current = true; + return () => { + alive.current = false; + }; + }, []); + + const refresh = useCallback(() => { + void load(() => !alive.current); + }, [load]); + + return { version, sandboxUrl, writable, secretStorage, loading, refresh }; } diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index 893ff435..b0e97d8c 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -263,7 +263,7 @@ Each server entry may carry these Inspector-extension fields at the top level: - **`FileSecretStore` at rest**: one JSON document at `~/.mcp-inspector/secrets.json` (or `MCP_INSPECTOR_SECRET_FILE`, else under `MCP_STORAGE_DIR`), written `0600` and re-tightened at startup. With `MCP_INSPECTOR_SECRET_KEY` set it is AES-256-GCM with a scrypt-derived key and a per-write random salt; without it the values are in the clear, and _that_ is what the loud banner and the warning-toned modal footer are about. The **whole map is encrypted as a unit**, not value-by-value, so the account names (`serverId:field`) are hidden too — a per-value scheme would leave a readable index of which servers you hold a client secret for. Upgrading is lazy: setting the passphrase later re-encrypts on the next write rather than rewriting the file during a run that may never touch a secret, and the descriptor reports `pendingEncryption` until it does. A file that can no longer be decrypted reads as empty but **refuses to be written**, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - **Concurrency, and what it does not promise**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes there is **no lock**. An earlier iteration had a `mkdir` election with an owner stamp, heartbeat and stale-takeover; three review rounds each found a real race, and the last is not closable with what Node exposes — claiming a stale lock needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`). It was replaced with optimistic concurrency: read `M0`, apply, write `M1`, read back `M2`, and re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. **This is not mutual exclusion.** The verify only catches a clobber that has already landed, so ordering the two writers write-A / verify-A / write-B / verify-B leaves both reporting success with A's entry gone — A's verify ran before there was anything to see. A crash between write and verify is one instance of that shape, not the whole of it. The window is narrower than the lock's (which lost updates across a wider set of interleavings, with every participant alive) and needs no primitive Node lacks, but it is a real residual and is documented as one. If a deployment needs the guarantee, the answer is an OS-backed lock from a dedicated library, not another hand-rolled election. - **Strict reads (`getStrict`)**: `get` is tolerant by contract, and that tolerance is wrong for exactly one caller — a migration that treats `null` as proof of absence and then _writes_. A transient read failure would look like "nothing there", and the write would replace a newer stored value with an older on-disk copy, inverting the keychain-wins rule the migration is built on. `getStrict` throws instead, and both plaintext migrations plus the keychain hand-off use it. Two traps this hit on the way in, both worth remembering: the seam must be forwarded by `DeferredSecretStore` (the store production actually uses, so without forwarding the strictness existed only in tests that injected a concrete store), and it must be implemented by _every_ store rather than falling back to `get` for the one it was introduced for. - - **Bulk reads (`getMany`)**: rehydration asks field by field, and `FileSecretStore.get` reads and decrypts the entire file per call. scrypt at `N=16384` measures ~23ms, so a 20-secret encrypted catalog spent ~450ms of pure key derivation on every `GET /api/servers` — a visible stall rather than a micro-optimization. `getMany` decrypts once per server; stores for which per-field reads are already cheap fall back to parallel `get`. + - **Bulk reads (`getMany`)**: rehydration asked field by field, and `FileSecretStore.get` reads and decrypts the entire file per call. scrypt at `N=16384` measures ~23ms, so an encrypted catalog spent ~450ms of pure key derivation on every `GET /api/servers` — a visible stall rather than a micro-optimization. The seam takes a **list of `{ serverId, fields }` across servers**, and both rehydration callers pass the whole catalog in one request, so `FileSecretStore` decrypts once per rehydration; stores for which per-field reads are already cheap (the keychain) fall back to parallel `get`. The cross-server shape is the point: a per-server version shipped first and left a 20-server catalog paying 20 serialized derivations, because both callers iterate servers — the same stall reached one server at a time instead of one field at a time. - **Durability gate on migration**: the plaintext-stripping migrations only delete the disk copy once the value is somewhere that outlives the process (`isDurable`). Against a session-scoped store, stripping would trade a secret that survives restarts for one that dies with the process — and it runs on an ordinary `GET`, so merely opening the app would destroy it. - **Hand-off when a keychain appears**: install libsecret after storing secrets in a file, and the next run selects the keychain and stops seeing them — still on disk, read by nothing, nothing visibly broken. `absorbFileSecretsIntoKeyring` copies them over on that run, under the same keychain-wins rule, and deletes the file **only** on complete success. Since the store takes no lock, the delete is conditional: the file is re-read immediately beforehand and compared against what was copied, and the delete is declined when they differ, because another process may have added an entry that was never copied. - **Surfacing it**: the active store rides `GET /api/config` as a `secretStorage` descriptor and is stated in a permanent footer at the bottom of the Client Settings and Server Settings modals — the only two dialogs that accept a secret. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is by design the thing a user dismisses before doing the work it describes. The descriptor is re-derived per request rather than cached, because `plaintext`/`pendingEncryption` describe bytes this very process changes.