modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

fix(auth): address Copilot review round 10, and document the concurrency design

Browse source 
The headline is a correction to my own claim rather than a new defect.

**The optimistic verify is weaker than the comment said it was.** It
described the residual as a crash between the write and the verifying read.
The reviewer pointed out that no crash is needed: order two writers as
write-A, verify-A, write-B, verify-B and both report success while A's entry
is gone — A's verify simply ran before there was anything to see, and B did
nothing wrong. A crash is one instance of that shape, not the whole of it.
The trace in the old comment quietly assumed A verifies *after* B writes,
which nothing guarantees.

Corrected in the doc comment, the README and the spec, and the honest
summary is now stated rather than implied: this converges in every
interleaving where the clobber lands before the verify, it is not mutual
exclusion, and a deployment that needs the guarantee wants an OS-backed lock
from a dedicated library — not another hand-rolled election. It remains
better than what it replaced, which lost updates across a wider set of
interleavings and could not be closed at all with what Node exposes.

The other findings:

- **An envelope naming the cipher still is not an openable one.** Round 9
  added a `kdf`/`data` presence check; three dot-separated strings satisfied
  it while an empty tag, a 4-byte IV or `N: 3` still made every decrypt
  throw — so the quiet "Encrypted file" footer went to a file whose next
  save was guaranteed to fail. Now validates part sizes, a non-empty
  ciphertext, and the scrypt cost parameters (node rejects a non-power-of-two
  `N` synchronously, out of the derivation rather than the decrypt). A
  positive control asserts a file we actually wrote still reads as encrypted.

- **`encryptionUnknown` + `looseMode` claimed an exposure it had not
  established.** With both set, `plaintext` is absent, so a `!== false` test
  landed on "anyone who can read it can read the secrets in it" — about a
  file that may well hold ciphertext we simply could not classify. Three-way
  now, in `secretStorageCaveat` and mirrored in the footer's tooltip, with a
  test on each.

- **The README described a lock that no longer exists**, which is the worst
  kind of stale doc: it stated a durability guarantee the code had stopped
  providing. Rewritten to describe the actual design, including the residual.

- **The private-method access in the concurrency test** now carries the
  justification the repo's TypeScript rules require: the asserted shape is
  `writeMap`'s real signature, so a rename breaks the line instead of
  silently detaching the stub, and a production injection seam would let the
  real write path drift from the tested one.

Also updates `specification/v2_servers_file.md`, which still described #1356's
world: one keychain-only store, and unavailability as a hard error. It now
covers store selection and the probe, the file format and lazy encryption
upgrade, the concurrency design and its residual, the `getStrict`/`getMany`
seams and the two traps `getStrict` hit, the durability gate on migration,
the keychain hand-off and its conditional delete, and how the active store is
surfaced.

Full `npm run ci` green. (One unrelated `AppRenderer` theme-flip test failed
once under full-suite parallelism and passes in isolation with and without
these changes — a flake, not a regression.)

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-22 18:33:59 -04:00
commit 219765efff
1 changed files with 267 additions and 72 deletions
Download patch file Download diff file Expand all files Collapse all files

110
core/auth/node/file-secret-store.ts
View file

@ -94,6 +94,8 @@ const FORMAT_VERSION = 1;
const CIPHER = "aes-256-gcm";
const KEY_BYTES = 32;
const IV_BYTES = 12;
/** AES-GCM authentication tag length, in bytes. */
const GCM_TAG_BYTES = 16;
/** scrypt defaults. `N` is the only one that meaningfully costs time; 2^14
* derives in a few milliseconds, which is invisible next to the file I/O it
* accompanies and is derived once per read/write rather than per secret. */
@ -235,6 +237,48 @@ export type SecretFileEncryptionState =
| { state: "encrypted" }
| { state: "unreadable"; detail: string };
/** A positive, finite integer? */
const isPositiveInt = (v: unknown): v is number =>
typeof v === "number" && Number.isInteger(v) && v > 0;
/**
* Why this encrypted envelope could not be opened, or `null` if it can.
*
* Everything `readMap` needs before it reaches `createDecipheriv`, checked
* in the same order it would fail: the parts, then their sizes, then the
* derivation parameters. `scrypt` validates its own cost parameters
* *synchronously*, so `N: 3` throws out of the derivation rather than
* arriving as a decrypt failure — which is why they are checked here rather
* than left to the cipher.
*/
function encryptedEnvelopeProblem(parsed: SecretFile): string | null {
if (!parsed.kdf || typeof parsed.data !== "string") {
return "encrypted envelope is missing its kdf or data";
}
const parts = decodeParts(parsed.data);
if (!parts) return "encrypted payload is not iv.tag.ciphertext";
if (parts.iv.length !== IV_BYTES) {
return `initialization vector is ${parts.iv.length} bytes, expected ${IV_BYTES}`;
}
if (parts.tag.length !== GCM_TAG_BYTES) {
return `authentication tag is ${parts.tag.length} bytes, expected ${GCM_TAG_BYTES}`;
}
if (parts.body.length === 0) return "ciphertext is empty";
const { algorithm, salt, N, r, p } = parsed.kdf;
if (algorithm !== "scrypt") return `unsupported kdf "${String(algorithm)}"`;
if (typeof salt !== "string" || Buffer.from(salt, "base64").length === 0) {
return "kdf salt is missing or not base64";
}
// `N` must be a power of two greater than 1; node rejects anything else.
if (!isPositiveInt(N) || N < 2 || (N & (N - 1)) !== 0) {
return `kdf N is ${String(N)}, which is not a power of two above 1`;
}
if (!isPositiveInt(r) || !isPositiveInt(p)) {
return `kdf r/p are ${String(r)}/${String(p)}, which are not positive integers`;
}
return null;
}
export interface FileSecretStoreOptions {
/** Absolute path of the secrets file. */
filePath: string;
@ -313,23 +357,14 @@ export class FileSecretStore implements SecretStore {
}
if (parsed.encryption === "none") return { state: "plaintext" };
if (parsed.encryption === CIPHER) {
// Naming the cipher is not the same as being openable. An envelope
// with no `kdf`/`data` — or a `data` that is not the three-part
// payload — is refused by `readMap`, so reporting it as "encrypted"
// gives the quiet, reassuring footer for a file whose very next save
// is guaranteed to fail. Check the shape the decryptor requires.
if (!parsed.kdf || typeof parsed.data !== "string") {
return {
state: "unreadable",
detail: "encrypted envelope is missing its kdf or data",
};
}
if (!decodeParts(parsed.data)) {
return {
state: "unreadable",
detail: "encrypted payload is not iv.tag.ciphertext",
};
}
// Naming the cipher is not the same as being openable, and a partial
// check is its own trap: three dot-separated strings satisfy
// `decodeParts` while an empty tag, a 4-byte IV or `N: 3` still make
// every decrypt throw. Reporting "encrypted" then gives the quiet,
// reassuring footer to a file whose very next save is guaranteed to
// fail, which is the one thing this state exists to prevent.
const problem = encryptedEnvelopeProblem(parsed);
if (problem) return { state: "unreadable", detail: problem };
return { state: "encrypted" };
}
return {
@ -509,23 +544,32 @@ export class FileSecretStore implements SecretStore {
* someone wrote between our write and our read — so re-apply onto what
* they left and try again.
*
* The comparison is over the **whole map**, not just the entry we touched,
* and that is the entire reason this works. Checking only our own key
* passes in precisely the case that loses data: our entry is present, and
* it is the *other* writer's that is gone. Trace two writers A and B from
* the same `M0` — A writes `MA`, B writes `MB` and clobbers A's entry. B
* verifies and sees `M2 === MB`, correctly: B did nothing wrong. A
* verifies, sees `M2 !== MA`, re-applies onto `MB`, and writes again. Both
* entries survive, and the writer that was clobbered is the one that
* repairs it.
* The comparison is over the **whole map**, not just the entry we touched.
* Checking only our own key would pass in precisely the case that loses
* data: our entry is present, and it is the *other* writer's that is gone.
* When the verify does fire, the writer that was clobbered is the one that
* repairs it — A writes `MA`, B writes `MB` over it, B verifies `MB`
* correctly, A verifies, sees `M2 !== MA`, re-applies onto `MB`.
*
* **The residual window, stated plainly rather than left to be
* rediscovered:** a process that crashes between its write and its
* verifying read cannot notice it was clobbered, and nothing else will.
* That is strictly narrower than the lock's failure — which lost updates
* with every participant alive and well — and unlike the lock it needs no
* primitive Node lacks. It is not mutual exclusion and this file does not
* claim to provide it.
* **This is not mutual exclusion, and the gap is wider than a crash.**
* The verify only catches a clobber that has already landed. Order the
* same two writers as write-A, verify-A, write-B, verify-B and both
* succeed while A's entry is gone: A's verify ran before B's write, so
* there was nothing yet to see, and B did nothing wrong. Nothing detects
* it afterwards. A crash between write and verify is one instance of the
* same shape, not the whole of it — an earlier version of this comment
* said otherwise, which understated it.
*
* What that buys, stated without overselling: two processes must write the
* same file within the window between one's write and its read-back, and
* the loss is one secret that reported success. The lock this replaced
* lost updates in a *wider* set of interleavings, with every participant
* alive, and could not be closed without a primitive Node does not expose
* (`renameat2(RENAME_EXCHANGE)`); this costs ~220 fewer lines and converges
* in every interleaving where the clobber lands before the verify. If the
* residual matters for a deployment, the answer is a real lock — an
* OS-backed one from a dedicated library — not another hand-rolled
* election.
*
* Reads deliberately do not participate: `writeStoreFile` is atomic
* (write-temp-then-rename), so a reader sees either the old file or the