fix(auth): stop blaming the passphrase for a corrupt payload, and refresh the stale docs

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 <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-22 23:08:52 -04:00
commit de84b67046
2 changed files with 70 additions and 24 deletions

View file

@ -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<string, string>): Promise<void> {

View file

@ -662,18 +662,17 @@ async function recoverOrphanedSnapshots(filePath: string): Promise<void> {
* 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,