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

fix(auth): stop losing preserved plaintext, and claim the hand-off file atomically

Browse source 
Round 19 found real data loss rather than process noise, in two places.

**An unrelated settings edit destroyed the secret the durability gate had
just preserved.** The GET migration withholds its strip for a session-scoped
store, so `mcp.json` stays the durable copy — but the POST/PUT paths always
wrote the stripped shape. The GET also returns the rehydrated secret to the
settings form, which resends the whole object when the user changes anything,
so editing a timeout wrote the stripped entry, moved the only durable copy
into RAM, and lost it at exit. Every operation reported success. The client
config had the identical shape: the read-path guard was there, the write path
had none, and saving a CIMD URL did the same thing to the IdP client secret.

Both write paths now agree with their read paths: while the store cannot
outlive the process, the file stays the durable copy.

**The hand-off's delete is no longer a check-to-delete race.** Re-reading
immediately before `rm` caught writes that had already landed and was blind to
the one arriving between the check and the delete — destroying a *later*,
already-successful write, which is worse than the optimistic-write residual
and, unlike it, fixable without a lock. The file is claimed by an atomic
rename before anything reads it, so the migration operates on a snapshot
nobody else can reach and a writer that recreates `secrets.json` is simply
untouched.

Claiming introduced a failure mode the existing tests caught immediately: a
migration that aborts must not leave the secrets stranded at the staging path.
The snapshot is restored on any incomplete outcome, and when the live path has
been recreated in the meantime it is left in place with its location logged —
an operator-visible orphan is recoverable; clobbering someone's write is not.

Also:

- **`setMany`**, the write-side twin of `getMany`. `set` costs a whole-file
  read-decrypt-encrypt-write-verify cycle per call, and the settings form
  resends a server's full `env` map on any edit, so a 10-variable stdio server
  spent ~30 scrypt derivations saving an unrelated change. One cycle now; the
  keychain keeps its parallel individual writes.
- **A doc that contradicted its function.** `readOnDiskEncryption` still said
  it "never decrypts, so it needs no passphrase" — round 12 made it
  authenticate precisely because structure is not access.

Full `npm run ci` green, staged tree verified before running it.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-22 21:34:34 -04:00
commit 5ef3572578
4 changed files with 374 additions and 81 deletions
Download patch file Download diff file Expand all files Collapse all files

57
clients/web/src/test/core/client/node-persistence.test.ts
View file

@ -224,3 +224,60 @@ describe("client node-persistence", () => {
).toBeNull();
});
});
describe("session-scoped store keeps client.json durable (#1950 review r19)", () => {
it("does not strip the IdP secret when the store cannot outlive the process", async () => {
// The read-path migration already withheld its strip for a session
// store, but the write path did not — and `readClientConfigStore` hands
// the rehydrated secret to the form, which resends the whole object when
// an unrelated field changes. Saving a CIMD URL therefore moved the only
// durable copy into RAM, to be lost at exit.
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-durable-"));
const file = path.join(dir, "client.json");
try {
await writeClientConfigStore(
file,
{
enterpriseManagedAuth: {
enabled: true,
idp: {
issuer: "https://idp.example/",
clientId: "cid",
clientSecret: "must-survive",
},
},
},
new SessionSecretStore(),
);
expect(await fs.readFile(file, "utf-8")).toContain("must-survive");
} finally {
await fs.rm(dir, { recursive: true, force: true });
}
});
it("still strips against a durable store, so the guard is the difference", async () => {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-durable-"));
const file = path.join(dir, "client.json");
try {
await writeClientConfigStore(
file,
{
enterpriseManagedAuth: {
enabled: true,
idp: {
issuer: "https://idp.example/",
clientId: "cid",
clientSecret: "goes-to-the-store",
},
},
},
new InMemorySecretStore(),
);
expect(await fs.readFile(file, "utf-8")).not.toContain(
"goes-to-the-store",
);
} finally {
await fs.rm(dir, { recursive: true, force: true });
}
});
});

58
clients/web/src/test/integration/auth/node/file-secret-store.test.ts
View file

@ -10,6 +10,7 @@
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
import * as crypto from "node:crypto";
import * as fs from "node:fs/promises";
import { existsSync as existsSyncFile } from "node:fs";
import * as os from "node:os";
import * as path from "node:path";
import {
@ -914,6 +915,63 @@ describe("getMany", () => {
}
});
it("setMany derives the key once, not three times per field", async () => {
// `set` is a read-decrypt-encrypt-write-verify cycle per call — roughly
// three derivations each — and the settings form resends a server's full
// env map on any edit, so a 10-variable server paid ~30 to save an
// unrelated timeout change.
vi.resetModules();
let derivations = 0;
vi.doMock("node:crypto", async () => {
const actual =
await vi.importActual<typeof import("node:crypto")>("node:crypto");
return {
...actual,
default: actual,
scrypt: (...args: Parameters<typeof actual.scrypt>) => {
derivations++;
return actual.scrypt(...args);
},
};
});
try {
const mod =
await import("@inspector/core/auth/node/file-secret-store.js");
const store = new mod.FileSecretStore({
filePath: filePath(),
passphrase: "hunter2",
});
await store.set("srv", "env:SEED", "0");
derivations = 0;
await store.setMany("srv", {
"env:A": "1",
"env:B": "2",
"env:C": "3",
});
const bulk = derivations;
derivations = 0;
await store.set("srv", "env:D", "4");
const single = derivations;
// Three fields in one cycle must not cost more than one field does.
expect(bulk).toBeLessThanOrEqual(single);
expect(await store.get("srv", "env:A")).toBe("1");
expect(await store.get("srv", "env:C")).toBe("3");
expect(await store.get("srv", "env:SEED")).toBe("0");
} finally {
vi.doUnmock("node:crypto");
vi.resetModules();
}
});
it("setMany with nothing to write does not touch the file", async () => {
const store = new FileSecretStore({ filePath: filePath() });
await store.setMany("srv", {});
expect(existsSyncFile(filePath())).toBe(false);
});
it("is tolerant like get: an unreadable store yields no fields", async () => {
await fs.writeFile(filePath(), "{ not json", "utf-8");
const store = new FileSecretStore({ filePath: filePath() });

33
clients/web/src/test/integration/auth/node/secret-store-selection.test.ts
View file

@ -857,23 +857,26 @@ describe("absorbFileSecretsIntoKeyring", () => {
);
});
it("declines to delete a file that changed while the hand-off ran", async () => {
// Read → copy → delete is no longer one transaction: the store dropped
// its cross-process lock in favour of optimistic writes, so another
// Inspector can complete a `set` between the read and the delete. That
// entry has not been copied, so deleting the file would discard a write
// that reported success. The hand-off re-reads immediately before the
// delete and backs off when the contents moved.
it("claims the file atomically, so a later write is not deleted", async () => {
// Round 19: the pre-delete comparison was a check-to-delete race. A
// writer completing a `set` *after* the comparison and before the `rm`
// had its write verified, reported success, and then lost it — a later,
// already-successful write destroyed, which is worse than the
// optimistic-write residual and, unlike that one, fixable.
//
// The file is now claimed by an atomic rename before anything reads it,
// so a writer that recreates `secrets.json` afterwards is untouched: its
// file is a different one and the next run migrates it.
const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
vi.spyOn(console, "warn").mockImplementation(() => {});
const mod = await loadWithProbe(true);
// Stand in for the other Inspector: write to the file *during* the copy,
// which is exactly the window the old lock covered.
const keyring = new InMemorySecretStore();
const { FileSecretStore } =
await import("@inspector/core/auth/node/file-secret-store.js");
// Stand in for the other Inspector: write to the live path *during* the
// copy — i.e. after the claim, in the window the old comparison missed.
const original = keyring.set.bind(keyring);
let intruded = false;
keyring.set = async (id: string, field: string, value: string) => {
@ -886,14 +889,18 @@ describe("absorbFileSecretsIntoKeyring", () => {
await mod.absorbFileSecretsIntoKeyring(keyring);
// The file survives, and so does the entry that arrived late.
// The late write survives at the live path...
expect(existsSync(filePath)).toBe(true);
expect(
await new FileSecretStore({ filePath }).get("other", "env:LATE"),
).toBe("z");
expect(warn.mock.calls.flat().join("\n")).toMatch(
/changed while that ran, so it has been left in place/,
// ...the migrated values reached the keychain...
expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file");
// ...and no staging file is left behind.
const leftovers = (await fs.readdir(path.dirname(filePath))).filter((f) =>
f.includes(".migrating-"),
);
expect(leftovers).toEqual([]);
});
it("deletes the file when nothing changed under it", async () => {

57
clients/web/src/test/integration/mcp/remote/servers-route.test.ts
View file

@ -2727,6 +2727,63 @@ describe("plaintext migration against a session-scoped store (#1950)", () => {
expect(saidSessionWarning()).toHaveLength(1);
});
it("keeps the plaintext on disk through a later unrelated settings edit", async () => {
// The gap round 19 found. The GET migration withheld the strip for a
// session store, but the *write* paths did not — and the GET returns the
// rehydrated secret to the settings form, which resends the whole object
// on any edit. So changing a timeout wrote the stripped shape, moving the
// only durable copy into RAM to be lost at exit. The user changed a
// timeout and lost a client secret, with everything reporting success.
const { app } = createRemoteApp({
dangerouslyOmitAuth: true,
mcpConfigPath: configPath,
initialConfig: { defaultEnvironment: {} },
secretStore: new SessionSecretStore(),
});
// Read it the way the UI does — this is what fills the form.
const listed = (await (
await app.request(new Request("http://test/api/servers"))
).json()) as MCPConfig;
expect(
(listed.mcpServers.srv as { oauth?: { clientSecret?: string } }).oauth
?.clientSecret,
).toBe("must-survive");
// Now save an unrelated change, resending the settings verbatim.
const res = await app.request(
new Request("http://test/api/servers/srv", {
method: "PUT",
headers: { "content-type": "application/json" },
body: JSON.stringify({
id: "srv",
config: { type: "streamable-http", url: "https://x.test/mcp" },
settings: {
headers: [],
env: [],
metadata: [],
connectionTimeout: 45000,
requestTimeout: 60000,
taskTtl: 60000,
maxFetchRequests: 1000,
roots: [],
oauthClientId: "cid",
oauthClientSecret: "must-survive",
},
}),
}),
);
expect(res.status).toBe(200);
// The durable copy is still on disk. Exiting now loses nothing.
const onDisk = JSON.parse(readFileSync(configPath, "utf-8")) as {
mcpServers: Record<string, { oauth?: { clientSecret?: string } }>;
};
expect(onDisk.mcpServers.srv?.oauth?.clientSecret).toBe("must-survive");
// And the unrelated edit did land.
expect(readFileSync(configPath, "utf-8")).toContain("45000");
});
it("aborts the migration when the keychain read fails, preserving mcp.json", async () => {
// The keychain-wins lookup used the tolerant `get`, so an unreadable
// keychain read as "nothing there" — and the write that followed would