From de84b67046e07142a136199fba43ff9688215eb5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 23:08:52 -0400 Subject: [PATCH] fix(auth): stop blaming the passphrase for a corrupt payload, and refresh the stale docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 23 produced **no inline comments** — nothing wrong in the code it had not already seen. The five suppressed ones are all in unchanged code: one real misdiagnosis and four pieces of drift. **A decrypted-but-corrupt payload was reported as a wrong passphrase.** By the time `JSON.parse` runs, GCM has verified the tag — so the passphrase is *proven correct*, and a `SecretFileKeyMismatchError` there tells the user to restore a secret they never lost. `asSecretMap` was already outside that catch for exactly this reason; `JSON.parse` was not, so the same class of mistake survived one line above the fix for it. The decrypt now has its own narrow catch and everything after it reports a corrupt file. The test fixture could not express this case — it `JSON.stringify`s its payload, so it can only produce valid JSON. Split into a raw-plaintext variant so a fixture can carry bytes that are authentic under the passphrase and still unparseable, which is the only shape that reaches the branch. The rest is documentation that had drifted from the code it describes: - `handOff`'s doc still described the re-read-and-compare protocol that the atomic claim replaced two rounds ago — an obsolete concurrency invariant is worse than none, because the next reader takes it as the contract. - The spec still described that same hand-off, and still called Client and Server Settings "the only two dialogs that accept a secret". Both now match the code, and the dialog count is called out as load-bearing: the one that was missing from it was the one taking secrets with no disclosure at all. - The `void load()` in `refresh` carries the justification the repo's no-floating-promises rule requires — `load` owns its failures, and `refresh` is deliberately synchronous because `refreshingPersist` calls it from a `finally`. Full `npm run ci` green, staged tree verified first. Signed-off-by: cliffhall --- .../auth/node/file-secret-store.test.ts | 32 ++++++++++++++++++- core/auth/node/file-secret-store.ts | 30 +++++++++++------ core/auth/node/secret-store-selection.ts | 21 ++++++------ core/react/useInitialConfig.ts | 5 +++ specification/v2_servers_file.md | 4 +-- 5 files changed, 69 insertions(+), 23 deletions(-) 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.