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

fix(auth): address Copilot review round 5, and land the impl its tests need

Browse source 
Repairs origin as well as answering the review. 2d2c787e committed the
`absorbFileSecretsIntoKeyring` tests without the implementation they
import, so the branch has been failing `tsc` since; this commit brings the
source across.

Round 5's four findings (two inline, two suppressed):

- **The stale-lock takeover was still racy.** Round 4 made *stealing*
  liveness-aware but left the takeover as remove-then-recreate, which is
  not an election: two waiters can both judge the same lock stale, the
  first removes it and takes a fresh one, and the second then removes
  *that* — both inside the section, which is the lost update the takeover
  exists to prevent. Claiming is now an atomic `rename` to a
  token-unique name, so exactly one waiter can take a given stale lock and
  the losers get ENOENT. A failed claim also falls through to the deadline
  instead of `continue`ing, so a takeover that can never succeed times out
  rather than spinning forever on a swallowed error.

- **Every `stat` failure was read as "no file yet".** An existing file we
  cannot inspect (EACCES on the directory, a mode-less filesystem) then
  produced no warning at all, and the footer went on stating owner-only
  permissions having verified nothing. There is now a third state,
  `unknown`, carried end to end: `permissionsUnknown` on the descriptor, a
  caveat that says the permissions could not be checked, a `warn` tone, and
  its own branch in the footer sentence — which otherwise would have
  claimed "Owner-only permissions." by omission, the same bug one layer up.

- **mountinfo decoded only `\040`.** The kernel escapes tab, newline and
  backslash too (`\011`, `\012`, `\134`), so a mounted path containing any
  of them was compared in its escaped form, matched nothing, and read as
  the container's writable layer — `memory` selected and persistence
  silently lost on a box that had arranged for it. Decoded as octal in
  general now, since the rule is octal-in-general and a future addition
  would otherwise reintroduce exactly this.

- **File-backed secrets vanished once a keychain appeared.** Save secrets
  without libsecret, install libsecret, and the next run selects the
  keychain and stops seeing them: still on disk, read by nothing, and
  nothing looks broken. `absorbFileSecretsIntoKeyring` hands them over on
  the run that finds a keychain, under the rules the two existing plaintext
  migrations already use — keychain wins on conflict, the file is removed
  only on complete success, and a file that cannot be read is never
  deleted, because that is precisely the case where the values would be
  lost.

Tests: a two-waiter race over one stale lock asserting a single holder
throughout; `unknown` from `readSecretFilePermissions` through the
descriptor to the footer sentence and tone; the hand-off's happy path,
keychain-wins, write-failure, undecryptable-file, empty-file,
malformed-key, and explicitly-configured-keyring cases; and six more
rejected descriptor shapes, including each file-only field on a non-file
kind. Full `npm run ci` green — 5,732 tests under the coverage gate.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-22 14:15:35 -04:00
commit decc673a5d
9 changed files with 458 additions and 26 deletions
Download patch file Download diff file Expand all files Collapse all files

22
clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx
View file

@ -112,4 +112,26 @@ describe("SecretStorageFooter", () => {
renderWithMantine(<SecretStorageFooter info={undefined} />);
expect(screen.queryByTestId("secret-storage-footer")).toBeNull();
});
it("says the permissions could not be checked rather than claiming owner-only", async () => {
// The third permission state. Falling into the verified-0600 branch here
// would have the band assert owner-only having read nothing — the same
// bug as claiming 0600 in the caveat, one layer up.
renderWithMantine(
<SecretStorageFooter
info={{
kind: "file",
reason: "fallback",
durable: true,
plaintext: true,
path: "/home/node/.mcp-inspector/secrets.json",
permissionsUnknown: "EACCES",
}}
/>,
);
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent("Permissions could not be checked.");
expect(band).not.toHaveTextContent("Owner-only");
expect(band).toHaveAttribute("data-tone", "warn");
});
});

31
clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx
View file

@ -127,6 +127,28 @@ const formatMode = (mode: number): string => mode.toString(8).padStart(4, "0");
* and the underlying facts still come from the shared helpers, which is
* what keeps them from disagreeing about anything that matters.
*/
/**
* How the band describes the file's permissions.
*
* Three states, not two, and the third is the point: "Owner-only
* permissions." is a claim of *fact*, so it may only be made when the mode
* was actually read and found to be 0600. `looseMode` says we looked and it
* was wrong; `permissionsUnknown` says we could not look at all. Treating
* the second like the first — which a plain `looseMode === undefined` test
* does — makes the footer assert owner-only having verified nothing, which
* is the failure this band exists to prevent, arrived at by omission rather
* than by error.
*/
function filePermissionsSentence(info: SecretStorageInfo): string {
if (info.looseMode !== undefined) {
return `Mode ${formatMode(info.looseMode)} — not owner-only.`;
}
if (info.permissionsUnknown !== undefined) {
return "Permissions could not be checked.";
}
return "Owner-only permissions.";
}
function footerMessage(info: SecretStorageInfo): string {
if (info.kind === "keyring") return secretStorageLabel(info);
if (info.kind === "memory") {
@ -143,10 +165,7 @@ function footerMessage(info: SecretStorageInfo): string {
// is something else and could not be tightened, and saying "owner-only"
// there would be the footer asserting the opposite of what is true — the
// one failure this whole band exists to prevent.
const permissions =
info.looseMode === undefined
? "Owner-only permissions."
: `Mode ${formatMode(info.looseMode)} — not owner-only.`;
const permissions = filePermissionsSentence(info);
return `${encryption} file. ${permissions}`;
}
@ -173,6 +192,10 @@ function footerTooltip(info: SecretStorageInfo): string | undefined {
parts.push(
`Mode ${formatMode(info.looseMode)} could not be tightened to 0600 — anyone who can read the file can read the secrets in it.`,
);
} else if (info.permissionsUnknown !== undefined) {
parts.push(
`The file's permissions could not be read (${info.permissionsUnknown}), so it is not known whether others can read it.`,
);
}
parts.push("Click to copy file path");
return parts.join(" ");

45
clients/web/src/test/core/react/useInitialConfig.test.tsx
View file

@ -284,6 +284,7 @@ describe("useInitialConfig", () => {
path: "/home/node/.mcp-inspector/secrets.json",
pendingEncryption: true,
looseMode: 0o644,
permissionsUnknown: "EACCES",
};
const fetchFn = vi
.fn()
@ -354,6 +355,50 @@ describe("useInitialConfig", () => {
looseMode: "644",
},
],
[
"a non-string permissionsUnknown",
{
kind: "file",
reason: "fallback",
durable: true,
plaintext: true,
path: "/x",
permissionsUnknown: true,
},
],
[
// Each file-only field is rejected on its own, so no single one can
// slip through on the back of the others being absent.
"permissionsUnknown on a keyring descriptor",
{
kind: "keyring",
reason: "default",
durable: true,
permissionsUnknown: "EACCES",
},
],
[
"looseMode on a keyring descriptor",
{ kind: "keyring", reason: "default", durable: true, looseMode: 0o644 },
],
[
"pendingEncryption on a memory descriptor",
{
kind: "memory",
reason: "fallback",
durable: false,
pendingEncryption: true,
},
],
[
"a path on a memory descriptor",
{
kind: "memory",
reason: "fallback",
durable: false,
path: "/x/secrets.json",
},
],
[
// File-only fields on a non-file kind mean the payload was not built
// by a backend we understand; rendering a mixture of two stores'

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

@ -452,6 +452,38 @@ describe("tightenSecretFilePermissions", () => {
expect((await fs.stat(filePath())).mode & 0o777).toBe(0o600);
});
it("reports unknown, not absent, when an existing file cannot be inspected", async () => {
// Collapsing every stat failure into "absent" is what made it dangerous:
// the descriptor then omits the warning and the footer states mode 0600
// as fact, having verified nothing.
vi.resetModules();
vi.doMock("node:fs/promises", async () => {
const actual =
await vi.importActual<typeof import("node:fs/promises")>(
"node:fs/promises",
);
const denied = Object.assign(new Error("EACCES: permission denied"), {
code: "EACCES",
});
return {
...actual,
default: actual,
stat: vi.fn(() => Promise.reject(denied)),
};
});
try {
const mod =
await import("@inspector/core/auth/node/file-secret-store.js");
expect(await mod.readSecretFilePermissions(filePath())).toEqual({
state: "unknown",
detail: "EACCES",
});
} finally {
vi.doUnmock("node:fs/promises");
vi.resetModules();
}
});
it("readSecretFilePermissions reports the mode without changing it", async () => {
// The descriptor is rebuilt on every GET /api/config, so the function it
// calls must not chmod — once per page load, on a file another process
@ -905,6 +937,40 @@ describe("acquireLock", () => {
await fs.rm(lockDir, { recursive: true, force: true });
});
it("elects a single winner when two waiters find the same stale lock", async () => {
// `rm`-then-`mkdir` is not an election: both waiters judge the lock
// stale, the first removes it and takes a fresh one, and the second then
// removes *that* — putting both inside the section at once, which is the
// lost update the takeover exists to prevent. The rename is atomic, so
// exactly one waiter can claim a given stale lock.
const target = path.join(tmpDir, "elected.json");
const lockDir = `${target}.lock`;
await fs.mkdir(lockDir, { recursive: true });
await fs.writeFile(
path.join(lockDir, "owner"),
// A pid that cannot be running, so both waiters agree it is stale.
JSON.stringify({ token: "dead", pid: 2 ** 31 - 1, host: os.hostname() }),
"utf-8",
);
const old = new Date(Date.now() - 60_000);
await fs.utimes(lockDir, old, old);
let inside = 0;
let maxInside = 0;
const timings = { staleMs: 50, timeoutMs: 10_000, pollMs: 5 };
const run = async () => {
const release = await acquireLock(target, timings);
inside += 1;
maxInside = Math.max(maxInside, inside);
await new Promise((r) => setTimeout(r, 40));
inside -= 1;
await release();
};
await Promise.all([run(), run()]);
// Never two holders at once.
expect(maxInside).toBe(1);
});
it("cleans up and reports when it cannot stamp ownership", async () => {
// mkdir succeeded, the owner write did not. Returning a no-op release
// here would enter the critical section unlocked *and* leave the

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

@ -612,29 +612,71 @@ describe("absorbFileSecretsIntoKeyring", () => {
it("does not delete a file it cannot read", async () => {
// An undecryptable file is not an empty one. Deleting it here would lose
// exactly the values this whole hand-off exists to preserve.
// exactly the values this hand-off exists to preserve.
const filePath = path.join(tmpDir, "secrets.json");
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
process.env.MCP_INSPECTOR_SECRET_KEY = "right-key";
const seeded = await loadWithProbe(false);
vi.spyOn(console, "warn").mockImplementation(() => {});
await new (
await import("@inspector/core/auth/node/file-secret-store.js")
).FileSecretStore({ filePath, passphrase: "right-key" }).set(
const { FileSecretStore } =
await import("@inspector/core/auth/node/file-secret-store.js");
await new FileSecretStore({ filePath, passphrase: "right-key" }).set(
"srv",
"oauthClientSecret",
"encrypted",
);
void seeded;
delete process.env.MCP_INSPECTOR_SECRET_KEY;
// The passphrase is gone by the time the keychain reappears.
const mod = await loadWithProbe(true);
const keyring = new InMemorySecretStore();
await mod.absorbFileSecretsIntoKeyring(keyring);
expect(existsSync(filePath)).toBe(true);
expect(await keyring.get("srv", "oauthClientSecret")).toBe(null);
});
it("is a no-op for a file that holds no entries", async () => {
// An empty map is not a hand-off. Deleting the file here would be
// harmless but pointless; the interesting part is that it does not warn
// about a migration that never happened.
const filePath = await seedFile({});
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
const mod = await loadWithProbe(true);
await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore());
expect(existsSync(filePath)).toBe(true);
expect(warn).not.toHaveBeenCalled();
});
it("skips an entry whose account key is malformed", async () => {
// A hand-edited file can carry a key that is not `serverId:field`. It
// cannot be addressed through the store API, so it is passed over rather
// than guessed at — but it must not abort the entries around it.
const filePath = await seedFile({
"no-separator": "orphan",
"srv:oauthClientSecret": "from-file",
});
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
vi.spyOn(console, "warn").mockImplementation(() => {});
const mod = await loadWithProbe(true);
const keyring = new InMemorySecretStore();
await mod.absorbFileSecretsIntoKeyring(keyring);
expect(existsSync(filePath)).toBe(true);
expect(await keyring.get("srv", "oauthClientSecret")).toBe(null);
expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file");
});
it("runs the hand-off for an explicitly configured keyring too", async () => {
// `MCP_INSPECTOR_SECRET_STORE=keyring` reaches the keychain by a
// different path than the probe does, and a leftover file is just as
// invisible either way.
const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
process.env.MCP_INSPECTOR_SECRET_STORE = "keyring";
vi.spyOn(console, "warn").mockImplementation(() => {});
const mod = await loadWithProbe(false);
// Resolving is what triggers it, via the configured branch.
await mod.resolveSecretStore();
expect(existsSync(filePath)).toBe(false);
});
it("is a no-op when there is no file", async () => {
@ -646,6 +688,59 @@ describe("absorbFileSecretsIntoKeyring", () => {
});
});
describe("the descriptor reports a mode it could not verify", () => {
it("carries permissionsUnknown when the file exists but cannot be inspected", async () => {
// The third state. Without it an unverifiable mode fell into the same
// branch as a verified 0600, and the footer claimed owner-only
// permissions having read nothing.
const filePath = path.join(tmpDir, "secrets.json");
process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
process.env.MCP_INSPECTOR_SECRET_STORE = "file";
await fs.writeFile(
filePath,
JSON.stringify({ version: 1, encryption: "none", secrets: {} }),
"utf-8",
);
vi.spyOn(console, "warn").mockImplementation(() => {});
vi.resetModules();
vi.doMock("node:fs/promises", async () => {
const actual =
await vi.importActual<typeof import("node:fs/promises")>(
"node:fs/promises",
);
const denied = Object.assign(new Error("EACCES: permission denied"), {
code: "EACCES",
});
return {
...actual,
default: actual,
stat: vi.fn(() => Promise.reject(denied)),
};
});
vi.doMock("@inspector/core/auth/node/secret-store.js", async () => {
const actual = await vi.importActual<
typeof import("@inspector/core/auth/node/secret-store.js")
>("@inspector/core/auth/node/secret-store.js");
return {
...actual,
probeKeyringAvailable: vi.fn(async () => ({ available: true })),
};
});
try {
const mod =
await import("@inspector/core/auth/node/secret-store-selection.js");
const info = await mod.getSecretStorageInfo();
expect(info.permissionsUnknown).toBe("EACCES");
expect(info.looseMode).toBeUndefined();
} finally {
vi.doUnmock("node:fs/promises");
vi.doUnmock("@inspector/core/auth/node/secret-store.js");
vi.resetModules();
}
});
});
describe("the descriptor reports a mode it could not tighten", () => {
it("carries looseMode into the descriptor and warns once at selection", async () => {
// The warning alone was not enough: the caveat under the secret field

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

@ -382,8 +382,29 @@ export async function acquireLock(
}
if (code !== "EEXIST") return noop; // cannot lock here at all — see above
if (await canSteal(lockDir, ownerFile, staleMs)) {
await fs.rm(lockDir, { recursive: true, force: true }).catch(() => {});
continue;
// Claim the stale lock by *renaming* it, not by removing it.
//
// `rm` is not an election: two waiters can both judge the same lock
// stale, and by the time the second one runs, the first may already
// have removed it and created a fresh lock of its own — which the
// second then deletes, and both enter the section together. That is
// the lost update, reached through the takeover meant to prevent it.
//
// `rename` is atomic and single-winner: exactly one waiter moves the
// directory aside, and the losers get ENOENT and fall through to
// retry against whatever the winner does next. The unique
// destination (our token) also means two winners of *different*
// stale locks cannot collide.
const claimed = `${lockDir}.stale-${token}`;
try {
await fs.rename(lockDir, claimed);
await fs.rm(claimed, { recursive: true, force: true });
continue;
} catch {
// Lost the election, or the rename failed outright. Either way,
// fall through to the deadline check rather than looping — a
// takeover that can never succeed must time out, not spin.
}
}
if (Date.now() >= deadline)
throw new SecretFileLockTimeoutError(filePath);
@ -566,6 +587,25 @@ export class FileSecretStore implements SecretStore {
* throw for "there is a file and we cannot read it" — which must never be
* flattened into an empty map, or `set` would cheerfully overwrite it.
*/
/**
* Every secret in the file, keyed by `${serverId}:${field}` — `null` when
* there is no file.
*
* Exists for one caller: the hand-off performed when a keychain becomes
* available on a box that had been falling back to a file (see
* `absorbFileSecretsIntoKeyring`). Nothing in the normal request path
* enumerates secrets, and deliberately so — this is the widest possible
* read of the store and it should stay a one-purpose seam rather than a
* general convenience.
*
* Throws for the same reasons `get` swallows: an unreadable or
* undecryptable file must not present as "no secrets", because the caller
* would conclude there was nothing to migrate and move on.
*/
async readAll(): Promise<Record<string, string> | null> {
return this.serialize(() => this.readMap());
}
private async readMap(): Promise<Record<string, string> | null> {
const raw = await readStoreFile(this.filePath);
if (raw === null) return null;
@ -830,10 +870,19 @@ export async function readSecretFilePermissions(
const mode = (await fs.stat(filePath)).mode & 0o777;
if (mode === 0o600) return { state: "ok" };
return { state: "loose", mode };
} catch {
// No file yet — the common first run. `writeStoreFile` creates it 0600,
// so there is nothing to warn about.
return { state: "absent" };
} catch (err) {
const code = (err as NodeJS.ErrnoException).code;
if (code === "ENOENT") {
// No file yet — the common first run. `writeStoreFile` creates it
// 0600, so there is nothing to warn about.
return { state: "absent" };
}
// The file exists and we could not inspect it (EACCES on the directory,
// a permission-less filesystem). Collapsing this into "absent" is what
// makes it dangerous: the descriptor then omits the warning and the
// footer goes on stating mode 0600 as fact, having verified nothing.
// "Unknown" is the honest third answer.
return { state: "unknown", detail: code ?? "unknown error" };
}
}
@ -846,4 +895,6 @@ export async function readSecretFilePermissions(
export type SecretFilePermissionState =
| { state: "ok" }
| { state: "absent" }
| { state: "loose"; mode: number };
| { state: "loose"; mode: number }
/** The file is there and its mode could not be read — see above. */
| { state: "unknown"; detail: string };

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

@ -40,6 +40,7 @@
*/
import * as fsSync from "node:fs";
import * as fs from "node:fs/promises";
import * as path from "node:path";
import {
type SecretStorageInfo,
@ -54,6 +55,7 @@ import {
} from "./file-secret-store.js";
import {
KeyringSecretStore,
parseAccount,
probeKeyringAvailable,
secretStoreIsDurable,
SessionSecretStore,
@ -186,10 +188,31 @@ function nearestExisting(dir: string): string | null {
* `null` where that file cannot be read (macOS, Windows, a `/proc`-less
* container).
*
* Field 5 of each line is the mount point, with spaces escaped as `\040`.
* `/` is dropped: in a container that is the writable layer, which is
* exactly what the durability question is asking *against*.
* Field 5 of each line is the mount point, octal-escaped. `/` is dropped:
* in a container that is the writable layer, which is exactly what the
* durability question is asking *against*.
*/
/**
* Undo the kernel's octal escaping of a mountinfo path.
*
* The field is space-delimited, so `show_mountinfo` escapes anything that
* would break parsing — space (`\040`), tab (`\011`), newline (`\012`) and
* the backslash itself (`\134`). Decoding only the space left a mounted
* path containing any of the others compared in its escaped form, so it
* matched nothing, and a genuinely mounted volume read as the container's
* writable layer: `memory` selected, persistence silently lost on a box
* that had arranged for it.
*
* Handled as one general `\ooo` pass rather than four literals, since the
* escaping rule is octal-in-general and a future addition would otherwise
* reintroduce exactly this bug.
*/
function unescapeMountPoint(point: string): string {
return point.replace(/\\(\d{3})/g, (_match, octal: string) =>
String.fromCharCode(parseInt(octal, 8)),
);
}
function readMountPoints(): string[] | null {
try {
return fsSync
@ -197,7 +220,7 @@ function readMountPoints(): string[] | null {
.split("\n")
.map((line) => line.split(" ")[4])
.filter((point): point is string => Boolean(point))
.map((point) => point.replace(/\\040/g, " "))
.map(unescapeMountPoint)
.filter((point) => point !== "/");
} catch {
return null;
@ -325,6 +348,7 @@ async function describeFileStore(
path: filePath,
plaintext,
...(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 } : {}),
@ -385,11 +409,18 @@ export function resolveSecretStore(): Promise<ResolvedSecretStore> {
const configured = parseSecretStoreEnv(process.env[SECRET_STORE_ENV]);
if (configured) {
const result = await buildStore(configured, "configured");
if (configured === "keyring") {
await absorbFileSecretsIntoKeyring(result.store);
}
warnAboutSecretStorage(result.info);
return result;
}
const probe = await probeKeyringAvailable();
if (probe.available) return buildStore("keyring", "default");
if (probe.available) {
const result = await buildStore("keyring", "default");
await absorbFileSecretsIntoKeyring(result.store);
return result;
}
const kind = chooseFallbackKind({
container: isContainer(),
@ -402,6 +433,78 @@ export function resolveSecretStore(): Promise<ResolvedSecretStore> {
return resolved;
}
/**
* Hand a leftover secrets file over to the keychain, once one exists.
*
* The transition this closes: a box with no libsecret falls back to
* `secrets.json` and the user saves an OAuth client secret there. They
* install libsecret. The next run probes successfully, selects the keychain
* — and every value they saved becomes invisible, because nothing reads the
* file any more. The data is still on disk, which makes it worse rather
* than better: nothing looks broken, the secrets have simply gone, and the
* file sits there as an unexplained leftover.
*
* The rules are the ones the two existing plaintext migrations already use,
* for consistency rather than novelty:
*
* - **The keychain wins on conflict.** An account already present there is
* left alone; the file's value is treated as the older copy.
* - **The file is removed only on complete success.** A partial hand-off
* leaves it exactly as it was, so the next run retries and the
* keychain-wins rule silently absorbs whatever already made it across.
* - **An unreadable file is not an empty one.** If it cannot be decrypted
* (the passphrase changed, or is now unset) this reports and returns
* rather than deleting it — the whole point is not to lose the values.
*
* Never throws. A keychain that fails mid-hand-off leaves the file intact,
* which is the safe state, and the user still gets a working session.
*/
export async function absorbFileSecretsIntoKeyring(
keyring: SecretStore,
): Promise<void> {
const filePath = defaultSecretFilePath();
if (!fsSync.existsSync(filePath)) return;
const file = new FileSecretStore({ filePath });
let entries: Record<string, string> | null;
try {
entries = await file.readAll();
} catch (err) {
console.warn(
`\n[mcp-inspector] The OS keychain is available again, but the secrets file at ${filePath} could not be read, so its contents were left there: ${err instanceof Error ? err.message : String(err)}`,
);
return;
}
if (!entries || Object.keys(entries).length === 0) return;
try {
for (const [account, value] of Object.entries(entries)) {
const parsed = parseAccount(account);
if (!parsed) continue;
const existing = await keyring.get(parsed.serverId, parsed.field);
if (existing === null) {
await keyring.set(parsed.serverId, parsed.field, value);
}
}
} catch (err) {
console.warn(
`\n[mcp-inspector] Could not move the secrets file at ${filePath} into the OS keychain, so it has been left in place: ${err instanceof Error ? err.message : String(err)}`,
);
return;
}
try {
await fs.rm(filePath, { force: true });
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 file remains.
// Not worth failing startup over, and the keychain-wins rule makes the
// next attempt harmless.
}
}
/**
* The active store's descriptor — for the banner and `GET /api/config`.
*

20
core/auth/secret-storage-info.ts
View file

@ -72,6 +72,17 @@ export interface SecretStorageInfo {
* and the browser had no way to know.
*/
looseMode?: number;
/**
* Set when the secrets file exists and its mode could not be read at all
* — `EACCES` on the containing directory, or a filesystem with no POSIX
* modes. Carries the errno so the message can say why.
*
* Distinct from {@link looseMode}, and the distinction matters: that one
* says "we looked and it is wrong", this one says "we could not look".
* Collapsing the second into silence is what let the footer keep stating
* mode 0600 as fact having verified nothing.
*/
permissionsUnknown?: string;
/**
* True when secrets outlive the process. False only for `memory`, and
* it is the *promise* being made rather than an implementation detail:
@ -116,6 +127,9 @@ export function secretStorageTone(info: SecretStorageInfo): SecretStorageTone {
// An encrypted file that anyone can read is still worth flagging: the
// passphrase is the only thing standing between a reader and the values.
if (info.looseMode !== undefined) return "warn";
// Unverified is not the same as fine. The quiet tone here would read as a
// confirmation we never made.
if (info.permissionsUnknown !== undefined) return "warn";
return "neutral";
}
@ -137,6 +151,12 @@ export function secretStorageCaveat(
const mode = info.looseMode.toString(8).padStart(4, "0");
return `The secrets file is mode ${mode}, not 0600, and could not be tightened — anyone who can read it can read the secrets in it.`;
}
// Ordered with the permission problems, above encryption, for the same
// reason: not knowing whether others can read the file is a question about
// exposure, and it should not be answered by a reassuring silence.
if (info.permissionsUnknown !== undefined) {
return `The secrets file's permissions could not be checked (${info.permissionsUnknown}), so it is not known whether others can read it.`;
}
if (info.plaintext) {
return info.pendingEncryption
? "Existing secrets in this file are still unencrypted (file mode 0600). They are re-encrypted the next time a secret is saved."

9
core/react/useInitialConfig.ts
View file

@ -111,6 +111,12 @@ function usableSecretStorage(value: unknown): SecretStorageInfo | undefined {
if (v.looseMode !== undefined && typeof v.looseMode !== "number") {
return undefined;
}
if (
v.permissionsUnknown !== undefined &&
typeof v.permissionsUnknown !== "string"
) {
return undefined;
}
} else if (
// File-only fields on a keychain or memory descriptor mean the payload
// was not built by a backend we understand. Rejecting is cheap and the
@ -118,7 +124,8 @@ function usableSecretStorage(value: unknown): SecretStorageInfo | undefined {
v.path !== undefined ||
v.plaintext !== undefined ||
v.pendingEncryption !== undefined ||
v.looseMode !== undefined
v.looseMode !== undefined ||
v.permissionsUnknown !== undefined
) {
return undefined;
}