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 6 on the secret-store PR

Browse source 
Four findings, all suppressed, all real. Three are the same mistake in
different places: a state that was never established being reported as a
confident answer.

- **`readOnDiskEncryption` conflated "no file" with "unreadable file".**
  Both answered `null`, and the descriptor falls back to the configured
  write policy on `null` — so a passphrase-configured install reported
  "File (encrypted)" about a file whose envelope had never parsed, and
  which `set` was in fact about to refuse rather than overwrite. It now
  returns a four-state result, and only `absent` falls back to the policy
  (honestly: the first write really will create it that way). `unreadable`
  produces `encryptionUnknown` on the descriptor, `plaintext` is omitted
  entirely so nothing can read a default out of it, and the label, caveat,
  tone and footer all carry "unreadable" rather than guessing a mode.

- **The loose-mode warning overstated exposure for an encrypted file.**
  "Anyone who can read it can read the secrets in it" is false when the
  file is ciphertext — a reader gets the bytes and still needs the
  passphrase. Both the caveat and the footer tooltip now branch on
  encryption: the passphrase becomes the only protection, which is the
  real consequence. Overstating it is not harmless, because a warning that
  cries wolf is discounted on the occasion it is literally true.

- **The keychain hand-off could invert its own keychain-wins rule.**
  `SecretStore.get` answers `null` both for a missing entry and for a
  keychain it could not read, and the migration *writes* on `null` — so a
  transient read failure would overwrite a newer keychain value with the
  older file copy. Added an optional `getStrict` (throws instead of
  swallowing) implemented by `KeyringSecretStore`, with
  `secretStoreGetStrict` falling back to `get` for stores whose reads
  cannot fail. A throw now aborts the hand-off with the file left in
  place, so the next run retries.

Writing the tests turned up one more, in code the review had passed:
`parseOwner` recorded a garbled pid as `-1`, and `process.kill(-1, 0)` is
a *wildcard* asking about every process this user can signal — it answers
"alive" almost always, which would have made a lock with a corrupt stamp
permanently unstealable. Only a positive pid is usable now; anything else
is treated as no pid at all and the mtime decides.

Tests: the four envelope states kept distinct; the descriptor refusing to
guess, with the genuine no-file case as the control; strict-read abort
without a write and with the file preserved, plus a store whose `get` and
`getStrict` disagree so the test proves which one the migration consults;
encryption-aware wording asserted in both directions in the caveat and the
band; three unusable owner stamps and a wildcard pid; and the descriptor
shapes that must now be rejected — neither half of the encryption question
answered, or both. Full `npm run ci` green — 5,750 tests under the
coverage gate.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-22 14:40:29 -04:00
commit fdeffa1f00
3 changed files with 489 additions and 29 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -456,7 +456,7 @@ async function canSteal(
} catch {
// No stamp to consult; fall through to the time-only answer.
}
if (!owner || owner.host !== os.hostname() || typeof owner.pid !== "number") {
if (!owner || owner.host !== os.hostname() || owner.pid === undefined) {
return true;
}
return !processIsAlive(owner.pid);
@ -475,7 +475,8 @@ function processIsAlive(pid: number): boolean {
interface LockOwner {
token: string;
pid: number;
/** Absent when the stamp carried no usable (positive) pid. */
pid?: number;
host: string;
}
@ -487,8 +488,13 @@ function parseOwner(raw: string): LockOwner | null {
const { token, pid, host } = parsed as Partial<LockOwner>;
if (typeof token !== "string") return null;
return {
// Only a *positive* pid is usable. `-1` and `0` are wildcards to
// `process.kill` — `kill(-1, 0)` asks about every process this user
// can signal and answers "alive" for almost any host, which would make
// a garbled stamp permanently unstealable. Anything else is recorded
// as absent so the caller falls back to the mtime.
pid: typeof pid === "number" && pid > 0 ? pid : undefined,
token,
pid: typeof pid === "number" ? pid : -1,
host: String(host),
};
} catch {
@ -496,6 +502,17 @@ function parseOwner(raw: string): LockOwner | null {
}
}
/**
* What the secrets file's envelope says, or why it could not be read.
* `absent` is "nothing written yet"; `unreadable` is "there is a file and
* we cannot tell" — two very different things to say to a user.
*/
export type SecretFileEncryptionState =
| { state: "absent" }
| { state: "plaintext" }
| { state: "encrypted" }
| { state: "unreadable"; detail: string };
export interface FileSecretStoreOptions {
/** Absolute path of the secrets file. */
filePath: string;
@ -550,20 +567,36 @@ export class FileSecretStore implements SecretStore {
* Reads the envelope only. It never decrypts, so it needs no passphrase
* and answers correctly even when the key is wrong or missing — which is
* also a state the UI has to describe rather than throw on.
*
* `absent` and `unreadable` are kept apart deliberately. Collapsing them
* (as a bare `null` did) means an existing file that is corrupt, or
* written in an envelope this build does not know, is reported as "no
* file yet" — and the descriptor then falls back to the configured write
* policy, so a passphrase-configured install claims "Encrypted file"
* about a file whose encryption state was never established and which
* `set` is in fact about to refuse.
*/
async readOnDiskEncryption(): Promise<SecretFile["encryption"] | null> {
async readOnDiskEncryption(): Promise<SecretFileEncryptionState> {
let raw: string | null;
try {
raw = await readStoreFile(this.filePath);
} catch (err) {
return {
state: "unreadable",
detail: err instanceof Error ? err.message : String(err),
};
}
if (raw === null) return { state: "absent" };
try {
const raw = await readStoreFile(this.filePath);
if (raw === null) return null;
const parsed = JSON.parse(raw) as SecretFile;
return parsed.encryption === "none" || parsed.encryption === CIPHER
? parsed.encryption
: null;
if (parsed.encryption === "none") return { state: "plaintext" };
if (parsed.encryption === CIPHER) return { state: "encrypted" };
return {
state: "unreadable",
detail: `unsupported envelope (encryption="${String(parsed.encryption)}", version=${String(parsed.version)})`,
};
} catch {
// Absent, unreadable, or not JSON. "Unknown" is the honest answer, and
// the caller falls back to reporting the configured policy — the only
// thing that is knowable about a file nobody can read.
return null;
return { state: "unreadable", detail: "not valid JSON" };
}
}

31
core/auth/node/secret-store-selection.ts
View file

@ -57,6 +57,7 @@ import {
KeyringSecretStore,
parseAccount,
probeKeyringAvailable,
secretStoreGetStrict,
secretStoreIsDurable,
SessionSecretStore,
type SecretStore,
@ -334,7 +335,14 @@ async function describeFileStore(
detail?: string,
): Promise<SecretStorageInfo> {
const onDisk = await store.readOnDiskEncryption();
const plaintext = onDisk === null ? !store.encrypted : onDisk === "none";
// `absent` is the only state where falling back to the configured write
// policy is honest — the first write will create the file in exactly that
// mode. `unreadable` must not: the encryption state was never established,
// and reporting the policy there would claim "Encrypted file" about a file
// `set` is about to refuse.
const unreadable = onDisk.state === "unreadable" ? onDisk.detail : undefined;
const plaintext =
onDisk.state === "absent" ? !store.encrypted : onDisk.state === "plaintext";
// Re-checked here rather than captured at selection, for the same reason
// the encryption state is: the mode can change under a running process,
// and this descriptor is what the browser renders as fact. The *read-only*
@ -346,12 +354,17 @@ async function describeFileStore(
kind,
reason,
path: filePath,
plaintext,
// Omitted entirely when the envelope could not be read, so no consumer
// can read a default out of it.
...(unreadable === undefined ? { plaintext } : {}),
...(unreadable !== undefined ? { encryptionUnknown: unreadable } : {}),
...(perms.state === "loose" ? { looseMode: perms.mode } : {}),
...(perms.state === "unknown" ? { permissionsUnknown: perms.detail } : {}),
// Only meaningful while the two disagree; omitted otherwise so the
// payload does not carry a flag that says nothing.
...(plaintext && store.encrypted ? { pendingEncryption: true } : {}),
...(unreadable === undefined && plaintext && store.encrypted
? { pendingEncryption: true }
: {}),
durable: true,
detail,
};
@ -481,7 +494,17 @@ export async function absorbFileSecretsIntoKeyring(
for (const [account, value] of Object.entries(entries)) {
const parsed = parseAccount(account);
if (!parsed) continue;
const existing = await keyring.get(parsed.serverId, parsed.field);
// Strict: `get` answers `null` for an unreadable keychain as well as
// for a missing entry, and this branch *writes* on `null`. A transient
// read failure would therefore overwrite a newer keychain value with
// the older file copy — the exact inversion of the keychain-wins rule
// this migration is built on. A throw here aborts the hand-off and
// leaves the file in place, so the next run retries.
const existing = await secretStoreGetStrict(
keyring,
parsed.serverId,
parsed.field,
);
if (existing === null) {
await keyring.set(parsed.serverId, parsed.field, value);
}

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

@ -320,6 +320,23 @@ export interface SecretStore {
*/
isDurable?(): Promise<boolean>;
get(serverId: string, field: string): Promise<string | null>;
/**
* Like {@link get}, but **throws** when the store cannot be read instead
* of answering `null`.
*
* `get` is deliberately tolerant — an unreachable keychain and an absent
* entry are the same answer to a caller that just wants to render a form.
* 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 overwrite a newer
* keychain value with an older file copy — silently inverting the
* keychain-wins rule the migration exists to honor.
*
* Optional, so existing implementations (and test doubles) keep
* compiling; {@link secretStoreGetStrict} falls back to `get` when it is
* absent, which is correct for stores whose reads cannot fail.
*/
getStrict?(serverId: string, field: string): Promise<string | null>;
set(serverId: string, field: string, value: string): Promise<void>;
/** No-op if no entry exists. */
delete(serverId: string, field: string): Promise<void>;
@ -370,6 +387,21 @@ export interface SecretStore {
* one the first version of this contract actually handled.
*/
export class KeyringSecretStore implements SecretStore {
/**
* The intolerant read. Same lookup as {@link get}, minus the catch — a
* keychain that cannot answer must not be reported as an empty one to a
* caller that is about to write based on the answer.
*/
async getStrict(serverId: string, field: string): Promise<string | null> {
const keyring = await loadKeyring();
if (!keyring.ok) throw new KeychainUnavailableError(keyring.err);
const entry = new keyring.mod.AsyncEntry(
SERVICE_NAME,
buildAccount(serverId, field),
);
return (await entry.getPassword()) ?? null;
}
async get(serverId: string, field: string): Promise<string | null> {
try {
const keyring = await loadKeyring();
@ -466,6 +498,22 @@ export class KeyringSecretStore implements SecretStore {
* other way would make every double silently non-durable and quietly
* disable the migrations they exist to exercise.
*/
/**
* 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.
*/
export async function secretStoreGetStrict(
store: SecretStore,
serverId: string,
field: string,
): Promise<string | null> {
return store.getStrict
? store.getStrict(serverId, field)
: store.get(serverId, field);
}
export async function secretStoreIsDurable(
store: SecretStore,
): Promise<boolean> {