fix(auth): make the durability guard provenance-aware, and harden the claim
Round 20, and almost all of it is fallout from round 19's two changes. Both were right in direction and wrong in the detail, in the same way: each solved its case by treating a broad thing as a proxy for a narrow one. **The durability guard wrote newly entered secrets to disk in plaintext.** Preserving legacy plaintext for a session-scoped store is correct — stripping it moves the only durable copy into RAM. Using *the whole submitted entry* as the proxy for "legacy" was not: it turned `MCP_INSPECTOR_SECRET_STORE=memory` into "write every new secret to `mcp.json` in the clear", while the footer for that very store says secrets are written nowhere. A security regression introduced by a data-loss fix, and the same overshoot in `client.json`. Provenance decides now. A secret field is kept on disk only when it was already there, unchanged, in the file being overwritten; a newly entered or changed value is stripped and lives in the session store, which is exactly what the footer promises. Where provenance cannot be established — an unparseable prior file — the value is treated as new, because writing a secret to disk on a guess is the failure that matters. **The atomic claim was not as safe as its comments said.** Four ways: - A crash between the claim and the copy left only `secrets.json.migrating-<pid>`. The next run checked the canonical path, found nothing, selected the keychain, and every stored credential vanished with no warning. Orphans are now adopted at startup. - Restoring used `rename`, which on POSIX **replaces** its destination — so the restore path could overwrite a `secrets.json` a concurrent writer had recreated, destroying the later successful write that claiming exists to protect. The comment beside it asserted the opposite. `link` + `unlink` is the no-clobber primitive this needed. - Every claim failure was read as a lost race. `EACCES`/`EROFS` leave the file exactly where it is, and returning quietly selected the keychain while file-backed secrets sat there invisible. Only `ENOENT` is benign now. - A failed cleanup left a plaintext-capable snapshot on disk with a silent catch, while the app reported the keychain as the only store. It names the path now. **A third secret-entry surface had no disclosure at all.** `ServerConfigModal` takes stdio `env` values, which are extracted into the store exactly as Server Settings' are — and the footer's own doc comment claimed there were only two such dialogs. It has the footer now, and every server write that can carry a secret (config submit, both import flows) refreshes the descriptor, not just the settings save. Full `npm run ci` green, staged tree verified first. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
5ef3572578
commit
15e9260484
3 changed files with 426 additions and 17 deletions
|
|
@ -530,6 +530,14 @@ export async function absorbFileSecretsIntoKeyring(
|
|||
keyring: SecretStore,
|
||||
): Promise<void> {
|
||||
const filePath = defaultSecretFilePath();
|
||||
|
||||
// A crash between the claim and the delete leaves only
|
||||
// `secrets.json.migrating-<pid>`. Checking the canonical path alone then
|
||||
// reports "nothing to migrate", the keychain is selected, and every stored
|
||||
// credential silently disappears — the claim protecting the delete having
|
||||
// introduced a way to lose everything. Adopt any orphan first.
|
||||
await recoverOrphanedSnapshots(filePath);
|
||||
|
||||
if (!fsSync.existsSync(filePath)) return;
|
||||
|
||||
// **Claim the file atomically before reading it.** Comparing its contents
|
||||
|
|
@ -550,9 +558,17 @@ export async function absorbFileSecretsIntoKeyring(
|
|||
const staged = `${filePath}.migrating-${process.pid}`;
|
||||
try {
|
||||
await fs.rename(filePath, staged);
|
||||
} catch {
|
||||
// Someone else claimed it first, or it vanished. Either way there is
|
||||
// nothing here to migrate and no file of ours to clean up.
|
||||
} catch (err) {
|
||||
const code = (err as NodeJS.ErrnoException).code;
|
||||
if (code !== "ENOENT") {
|
||||
// Only ENOENT means someone else claimed it. Anything else — EACCES on
|
||||
// the directory, EROFS — leaves the file exactly where it is, and
|
||||
// returning quietly would select the keychain while file-backed
|
||||
// secrets sit there unreadable by anything, with nothing said.
|
||||
console.warn(
|
||||
`\n[mcp-inspector] Could not claim the secrets file at ${filePath} for migration into the OS keychain (${code ?? "unknown error"}), so it has been left in place. Its secrets are not visible to this session.`,
|
||||
);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -567,10 +583,14 @@ export async function absorbFileSecretsIntoKeyring(
|
|||
console.warn(
|
||||
`\n[mcp-inspector] The OS keychain is available again. Secrets kept in ${filePath} while it was not have been moved into the keychain, and the file has been removed.`,
|
||||
);
|
||||
} catch {
|
||||
// Every value is in the keychain; only the now-redundant snapshot
|
||||
// remains. Not worth failing startup over, and keychain-wins makes the
|
||||
// next attempt harmless.
|
||||
} catch (err) {
|
||||
// Every value is in the keychain, so this is not worth failing startup
|
||||
// over — but the snapshot is a *plaintext-capable copy of the secrets*
|
||||
// and the app is about to report the keychain as the only store. A
|
||||
// silent catch leaves a file the user cannot know to delete.
|
||||
console.warn(
|
||||
`\n[mcp-inspector] Secrets were moved into the OS keychain, but the temporary copy at ${staged} could not be removed (${err instanceof Error ? err.message : String(err)}). Delete it once you have confirmed the keychain has your secrets.`,
|
||||
);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
|
@ -580,18 +600,58 @@ export async function absorbFileSecretsIntoKeyring(
|
|||
// *failed* migration lose the file, which is what the claim exists to
|
||||
// prevent one line further down.
|
||||
try {
|
||||
await fs.rename(staged, filePath);
|
||||
// `link` and not `rename`: POSIX `rename` **replaces** its destination
|
||||
// silently, so restoring with it would overwrite a `secrets.json` that a
|
||||
// concurrent writer recreated while we held the snapshot — destroying a
|
||||
// later successful write, which is the exact loss the claim exists to
|
||||
// prevent. `link` fails with EEXIST instead, which is the no-clobber
|
||||
// primitive this needs; the snapshot is unlinked only once the live path
|
||||
// holds the same inode.
|
||||
await fs.link(staged, filePath);
|
||||
await fs.rm(staged, { force: true });
|
||||
} catch {
|
||||
// The live path is occupied — a writer recreated it while we held the
|
||||
// snapshot — so restoring would overwrite a newer file. Leave the
|
||||
// snapshot and say exactly where it is: an operator-visible orphan is
|
||||
// recoverable, and silently clobbering someone's write is not.
|
||||
// The live path is occupied, or hard links are unavailable here. Leave
|
||||
// the snapshot and say exactly where it is: an operator-visible orphan
|
||||
// is recoverable, and clobbering someone's write is not.
|
||||
console.warn(
|
||||
`\n[mcp-inspector] Could not restore ${staged} to ${filePath} because a new secrets file was written while the migration ran. Both files exist; the snapshot holds the secrets that were not migrated.`,
|
||||
`\n[mcp-inspector] Could not restore ${staged} to ${filePath}; both files exist and the snapshot holds the secrets that were not migrated. Merge it by hand, or delete it once you have confirmed nothing is missing.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Adopt a snapshot left behind by a process that died mid-migration.
|
||||
*
|
||||
* Renamed back only when the canonical path is free — a live `secrets.json`
|
||||
* is newer than any orphan and must not be replaced. Otherwise the orphan is
|
||||
* reported rather than removed: it may hold values the live file does not,
|
||||
* and this is not the place to decide a merge.
|
||||
*/
|
||||
async function recoverOrphanedSnapshots(filePath: string): Promise<void> {
|
||||
const dir = path.dirname(filePath);
|
||||
const prefix = `${path.basename(filePath)}.migrating-`;
|
||||
let names: string[];
|
||||
try {
|
||||
names = (await fs.readdir(dir)).filter((n) => n.startsWith(prefix));
|
||||
} catch {
|
||||
return; // No directory yet, or unreadable — nothing to recover.
|
||||
}
|
||||
for (const name of names) {
|
||||
const orphan = path.join(dir, name);
|
||||
try {
|
||||
await fs.link(orphan, filePath);
|
||||
await fs.rm(orphan, { force: true });
|
||||
console.warn(
|
||||
`\n[mcp-inspector] Recovered secrets left behind by an interrupted migration (${orphan} → ${filePath}).`,
|
||||
);
|
||||
} catch {
|
||||
console.warn(
|
||||
`\n[mcp-inspector] Found secrets from an interrupted migration at ${orphan}, and a current ${filePath} already exists, so it was left alone. It may hold values the current file does not.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The hand-off body: read the file, copy every entry into the keychain,
|
||||
* then delete the file.
|
||||
|
|
|
|||
|
|
@ -131,10 +131,54 @@ export async function writeClientConfigStore(
|
|||
const durable = await secretStoreIsDurable(secretStore);
|
||||
await writeStoreFile(
|
||||
filePath,
|
||||
serializeStore(durable ? stripped : validated),
|
||||
serializeStore(
|
||||
durable
|
||||
? stripped
|
||||
: await preserveLegacyPlaintext(
|
||||
filePath,
|
||||
validated,
|
||||
stripped,
|
||||
idpSecret,
|
||||
),
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* For a session-scoped store: keep the IdP secret on disk only when it was
|
||||
* **already there, unchanged**.
|
||||
*
|
||||
* Stripping legacy plaintext would move the only durable copy into RAM (the
|
||||
* read-path migration withholds its strip for exactly this reason, and the
|
||||
* write path has to agree). But writing a *newly entered* secret to disk
|
||||
* would contradict the footer, which promises a session store writes secrets
|
||||
* nowhere — so provenance decides, not the presence of a value in the
|
||||
* submitted body.
|
||||
*/
|
||||
async function preserveLegacyPlaintext(
|
||||
filePath: string,
|
||||
validated: ClientConfig,
|
||||
stripped: ClientConfig,
|
||||
idpSecret: string | undefined,
|
||||
): Promise<ClientConfig> {
|
||||
if (!idpSecret) return stripped;
|
||||
try {
|
||||
const raw = await readStoreFile(filePath);
|
||||
if (raw === null) return stripped;
|
||||
const prior = parseClientConfig(parseStore(raw));
|
||||
const priorSecret =
|
||||
extractSecretsFromClientConfig(prior).secrets[
|
||||
SECRET_FIELD_IDP_CLIENT_SECRET
|
||||
];
|
||||
return priorSecret === idpSecret ? validated : stripped;
|
||||
} catch {
|
||||
// Unreadable or unparseable prior file: treat the value as new, which is
|
||||
// the conservative direction — it keeps the secret off disk rather than
|
||||
// writing it there on a guess.
|
||||
return stripped;
|
||||
}
|
||||
}
|
||||
|
||||
/** Remove client.json and the install-level IdP secret from the keychain. */
|
||||
export async function deleteClientConfigStore(
|
||||
filePath: string,
|
||||
|
|
|
|||
|
|
@ -2116,8 +2116,30 @@ export function createRemoteApp(
|
|||
const entryForDisk = async (
|
||||
built: StoredMCPServer,
|
||||
stripped: StoredMCPServer,
|
||||
): Promise<StoredMCPServer> =>
|
||||
(await secretStoreIsDurable(secretStore)) ? stripped : built;
|
||||
previous?: StoredMCPServer,
|
||||
): Promise<StoredMCPServer> => {
|
||||
if (await secretStoreIsDurable(secretStore)) return stripped;
|
||||
// Non-durable, and the distinction is the whole of it: **legacy plaintext
|
||||
// that was already on disk** is preserved, because stripping it would
|
||||
// move the only durable copy into RAM. A **newly entered** secret is not,
|
||||
// because the footer promises that a session store writes secrets
|
||||
// nowhere — and the first version of this used the whole submitted entry
|
||||
// as a proxy for provenance, which turned `MCP_INSPECTOR_SECRET_STORE=
|
||||
// memory` into "write every new secret to mcp.json in the clear" while
|
||||
// the UI said the opposite.
|
||||
//
|
||||
// A changed value is new: the old plaintext goes, the new value lives in
|
||||
// the session store, and the footer's promise holds for it.
|
||||
const prior = previous ? extractSecretsFromStored(previous).secrets : {};
|
||||
const submitted = extractSecretsFromStored(built).secrets;
|
||||
const carry: Record<string, string> = {};
|
||||
for (const [field, value] of Object.entries(submitted)) {
|
||||
if (prior[field] === value) carry[field] = value;
|
||||
}
|
||||
return Object.keys(carry).length > 0
|
||||
? mergeSecretsIntoStored(stripped, carry)
|
||||
: stripped;
|
||||
};
|
||||
|
||||
const keychainErrorResponse = (
|
||||
c: Context,
|
||||
|
|
@ -2534,7 +2556,7 @@ export function createRemoteApp(
|
|||
}
|
||||
}
|
||||
const { stripped, secrets } = extractSecretsFromStored(built);
|
||||
const onDisk = await entryForDisk(built, stripped);
|
||||
const onDisk = await entryForDisk(built, stripped, existing);
|
||||
const next: MCPConfig = { mcpServers: {} };
|
||||
for (const [key, val] of Object.entries(current.mcpServers)) {
|
||||
if (key === originalId) {
|
||||
|
|
|
|||
Loading…
Reference in a new issue