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 8becc56e..8ab96433 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 @@ -338,6 +338,21 @@ describe("FileSecretStore failure handling", () => { ); }); + it("blames the file, not the passphrase, for a decrypted-but-corrupt payload", async () => { + // GCM has already authenticated by this point, so the passphrase is + // *proven correct* — telling the user to restore it sends them after a + // secret they never lost. `asSecretMap` was already outside the catch for + // this reason; `JSON.parse` was not. + await writeEncryptedFixtureWithPlaintext("{ not json at all", "right-key"); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "right-key", + }); + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( + /The passphrase is correct; the file itself is corrupt/, + ); + }); + it("still blames the passphrase when the envelope is well-formed", async () => { // The other half: structure fine, authentication fails. That *is* a key // problem and must keep the advice that fits it — the round-16 change @@ -755,6 +770,21 @@ describe("readOnDiskEncryption", () => { async function writeEncryptedFixtureWithPayload( payload: unknown, passphrase: string, +): Promise { + return writeEncryptedFixtureWithPlaintext( + JSON.stringify(payload), + passphrase, + ); +} + +/** + * The same, but writing `plaintext` verbatim — so a fixture can carry bytes + * that are authentic under the passphrase and still not valid JSON, which + * `JSON.stringify` can never produce. + */ +async function writeEncryptedFixtureWithPlaintext( + plaintext: string, + passphrase: string, ): Promise { const salt = crypto.randomBytes(16); const kdf = { @@ -776,7 +806,7 @@ async function writeEncryptedFixtureWithPayload( const iv = crypto.randomBytes(12); const cipher = crypto.createCipheriv("aes-256-gcm", key, iv); const body = Buffer.concat([ - cipher.update(JSON.stringify(payload), "utf-8"), + cipher.update(plaintext, "utf-8"), cipher.final(), ]); const data = [iv, cipher.getAuthTag(), body] diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index a3bf9cb5..edff72c1 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -527,25 +527,37 @@ export class FileSecretStore implements SecretStore { above returns a reason for anything `decodeParts` would reject. Kept as the type narrowing `parts` needs. */ if (!parts) throw new SecretFileKeyMismatchError(this.filePath, true); + // Only the *decryption* may be read as a key problem. Once GCM verifies + // the tag the passphrase is proven correct, so anything that fails after + // that point is about the contents — and reporting it as a key mismatch + // sends the user to restore a passphrase that is working. `asSecretMap` + // was already outside this catch for that reason; `JSON.parse` was not, + // so a decrypted-but-corrupt payload still blamed the key. + let plain: string; try { const key = await this.deriveKey(parsed.kdf); const decipher = crypto.createDecipheriv(CIPHER, key, parts.iv); decipher.setAuthTag(parts.tag); - const plain = Buffer.concat([ + plain = Buffer.concat([ decipher.update(parts.body), decipher.final(), ]).toString("utf-8"); - return asSecretMap(JSON.parse(plain), this.filePath); - } catch (err) { - // A shape refusal is about the *contents*, not the key — it decrypted - // fine. Rethrowing it as a key mismatch would send the user to fix a - // passphrase that is working. - if (err instanceof SecretStoreUnavailableError) throw err; + } catch { // GCM authentication failure is what a wrong passphrase looks like, and - // it is indistinguishable from a tampered/corrupt file — both mean "this - // key does not open this file", which is what the message says. + // it is indistinguishable from a tampered file — both mean "this key + // does not open this file", which is what the message says. throw new SecretFileKeyMismatchError(this.filePath, true); } + + let payload: unknown; + try { + payload = JSON.parse(plain); + } catch (err) { + throw new SecretStoreUnavailableError( + `The secrets file at ${this.filePath} decrypted successfully but its contents are not valid JSON: ${err instanceof Error ? err.message : String(err)}. The passphrase is correct; the file itself is corrupt.`, + ); + } + return asSecretMap(payload, this.filePath); } private async writeMap(map: Record): Promise { diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 7a9a4cc8..cb468af9 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -662,18 +662,17 @@ async function recoverOrphanedSnapshots(filePath: string): Promise { * The hand-off body: read the file, copy every entry into the keychain, * then delete the file. * - * **The delete is conditional, because there is no lock.** The store's - * mutations are optimistic (see `FileSecretStore.mutate`), so another - * Inspector can complete a `set` between our read and our delete — and - * removing the file then discards a secret whose write reported success, - * which is the exact loss this migration exists to prevent. So the - * contents are re-read immediately before the delete and compared against - * what was copied; if they differ, the new entries are copied too and the - * comparison is made again. + * **It operates on a snapshot, not on the live file.** The caller claims + * `secrets.json` with an atomic rename before this runs, so nothing else can + * write to what is being read here and a writer that recreates the live path + * is simply untouched. This replaced a re-read-and-compare before the + * delete, which caught writes that had already landed and was blind to the + * one arriving between the check and the `rm`. * - * Bounded, and it declines to delete rather than looping forever: leaving - * the file in place costs nothing (the keychain-wins rule makes the next - * run idempotent) while deleting it on a stale comparison costs a secret. + * Returns whether the snapshot is fully absorbed — the caller deletes it + * only then, and otherwise restores it to the live path. "Nothing to + * migrate" reports `false`: no values moved, and the file's presence is what + * records which encryption mode this install settled on. */ async function handOff( file: FileSecretStore, diff --git a/core/react/useInitialConfig.ts b/core/react/useInitialConfig.ts index 5415ad9c..ab58ab9d 100644 --- a/core/react/useInitialConfig.ts +++ b/core/react/useInitialConfig.ts @@ -255,6 +255,11 @@ export function useInitialConfig( }, [load]); const refresh = useCallback(() => { + // `void` because `load` owns its failures — every fetch/parse error is + // caught inside it and leaves the fields at their defaults — and because + // `refresh` is deliberately synchronous: its callers fire it from + // `refreshingPersist`'s `finally`, where a returned promise would be + // awaited by a caller that has nothing to do with the result. void load(); }, [load]); diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index b0e97d8c..53a7a0a2 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -265,8 +265,8 @@ Each server entry may carry these Inspector-extension fields at the top level: - **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 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. + - **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 source is **claimed atomically** first: the live `secrets.json` is renamed to a unique snapshot (pid plus a per-attempt nonce, so a staging path can never be reused — pid 1 recurs on every container start), the migration reads only that snapshot, and a writer that recreates the live path is untouched and migrated on the next run. The snapshot is deleted only on a complete hand-off; otherwise it is restored with `link` + `unlink` rather than `rename`, since POSIX `rename` silently replaces its destination and would overwrite a newer live file. A snapshot left behind by a process that died mid-migration is adopted at startup — checking only the canonical path would otherwise report "nothing to migrate" while every stored credential quietly disappeared. + - **Surfacing it**: the active store rides `GET /api/config` as a `secretStorage` descriptor and is stated in a permanent footer at the bottom of every dialog that accepts a secret: Client Settings (the enterprise IdP client secret), Server Settings (the per-server OAuth client secret and stdio `env:` values), and Server Config (stdio `env:` values). That third one was missed at first, which made it the one dialog taking secrets with no disclosure at all — so the count here is load-bearing rather than descriptive. 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. - **Hard-cutover legacy behavior (per #1358 decision 4)**: files written by the one pre-#1358 build of v2/main have a nested `settings` block. `normalizeMcpServers` drops the node on read and logs a one-line warn including the server id; the persisted headers / metadata / timeouts / OAuth credentials are intentionally lost on first read. Users re-enter them via the settings form (or hand-edit the file into the flat shape). v2 has not shipped a stable release with the nested shape, so the blast radius is the small set of v2/main dogfooders who edited per-server settings between #1353 merging and this change. - **UI**: `ServerSettingsModal` is opened from the server card's settings affordance. Saving routes through `useServers.updateServerSettings(id, settings)` which issues a settings-only `PUT /api/servers/:id` with `{ id, settings }` — the route preserves the on-disk transport config inside its write lock. Conversely, `useServers.updateServer` (driven by the basic-config modal) issues a config-only PUT with `{ id, config }` and the route preserves the on-disk settings fields. Edits in either modal cannot silently wipe the other half.