diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx
index 3eec7f5a..a639d91f 100644
--- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx
+++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx
@@ -134,4 +134,43 @@ describe("SecretStorageFooter", () => {
expect(band).not.toHaveTextContent("Owner-only");
expect(band).toHaveAttribute("data-tone", "warn");
});
+
+ it("says an unreadable file is unreadable, not encrypted", async () => {
+ // The descriptor omits `plaintext` here, and the band must not read a
+ // default out of that absence.
+ renderWithMantine(
+ ,
+ );
+ const band = screen.getByTestId("secret-storage-footer");
+ expect(band).toHaveTextContent("Unreadable file.");
+ expect(band).toHaveTextContent("Saving a secret will fail.");
+ expect(band).not.toHaveTextContent("Encrypted file");
+ expect(band).toHaveAttribute("data-tone", "warn");
+ });
+
+ it("does not claim a loose-mode encrypted file exposes its secrets", async () => {
+ renderWithMantine(
+ ,
+ );
+ const band = screen.getByTestId("secret-storage-footer");
+ expect(band).toHaveTextContent("Mode 0644 — not owner-only.");
+ expect(band).toHaveAttribute("data-tone", "warn");
+ });
});
diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx
index 901ddf2b..8df1234e 100644
--- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx
+++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx
@@ -159,6 +159,11 @@ function footerMessage(info: SecretStorageInfo): string {
? `${secretStorageLabel(info)}: ${caveat}`
: secretStorageLabel(info);
}
+ // "Unreadable" rather than guessing a mode: the envelope could not be
+ // parsed, so neither "Plaintext" nor "Encrypted" is a claim we can make.
+ if (info.encryptionUnknown !== undefined) {
+ return "Unreadable file. Saving a secret will fail.";
+ }
const encryption = info.plaintext ? "Plaintext" : "Encrypted";
const permissions = filePermissionsSentence(info);
return `${encryption} file. ${permissions}`;
@@ -174,7 +179,11 @@ function footerMessage(info: SecretStorageInfo): string {
function footerTooltip(info: SecretStorageInfo): string | undefined {
if (info.kind !== "file" || !info.path) return undefined;
const parts: string[] = [];
- if (info.plaintext) {
+ if (info.encryptionUnknown !== undefined) {
+ parts.push(
+ `The file could not be read (${info.encryptionUnknown}), so its contents are unknown and saving a secret will fail rather than overwrite it.`,
+ );
+ } else if (info.plaintext) {
// Advice that can actually clear the condition: someone who has already
// set the passphrase is waiting on the next write, not on themselves.
parts.push(
@@ -184,8 +193,13 @@ function footerTooltip(info: SecretStorageInfo): string | undefined {
);
}
if (info.looseMode !== undefined) {
+ // Encryption-aware: a reader of an encrypted file gets ciphertext, not
+ // secrets. Claiming otherwise overstates the exposure, and a warning
+ // that cries wolf is discounted on the occasion it is literally true.
parts.push(
- `Mode ${formatMode(info.looseMode)} could not be tightened to 0600 — anyone who can read the file can read the secrets in it.`,
+ info.plaintext === false
+ ? `Mode ${formatMode(info.looseMode)} could not be tightened to 0600 — others can copy the file, and the passphrase is then the only thing protecting its contents.`
+ : `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(
diff --git a/clients/web/src/test/core/auth/secret-storage-info.test.ts b/clients/web/src/test/core/auth/secret-storage-info.test.ts
index 12e9c397..486b03dd 100644
--- a/clients/web/src/test/core/auth/secret-storage-info.test.ts
+++ b/clients/web/src/test/core/auth/secret-storage-info.test.ts
@@ -121,6 +121,38 @@ describe("secretStorageCaveat", () => {
expect(secretStorageCaveat(loose)).toContain("0640");
});
+ it("does not overstate exposure for an encrypted file with a loose mode", () => {
+ // A reader of an encrypted file gets ciphertext, not secrets. Saying
+ // otherwise trains people to discount the warning on the occasion it is
+ // literally true.
+ const loose: SecretStorageInfo = { ...encryptedFile, looseMode: 0o644 };
+ const caveat = secretStorageCaveat(loose);
+ expect(caveat).toContain("0644");
+ expect(caveat).toContain("passphrase is then the only thing");
+ expect(caveat).not.toContain("can read the secrets in it");
+ });
+
+ it("keeps the blunt wording for a plaintext file with a loose mode", () => {
+ const loose: SecretStorageInfo = { ...plaintextFile, looseMode: 0o644 };
+ expect(secretStorageCaveat(loose)).toContain("can read the secrets in it");
+ });
+
+ it("says a file it could not read is unreadable, and warns about the next save", () => {
+ const unreadable: SecretStorageInfo = {
+ kind: "file",
+ reason: "fallback",
+ durable: true,
+ path: "/x/secrets.json",
+ encryptionUnknown: "not valid JSON",
+ };
+ // Neither "encrypted" nor "unencrypted" is a claim we can make here.
+ expect(secretStorageLabel(unreadable)).toBe("File (unreadable)");
+ expect(secretStorageTone(unreadable)).toBe("warn");
+ expect(secretStorageCaveat(unreadable)).toMatch(
+ /could not be read \(not valid JSON\)\. Saving a secret will fail/,
+ );
+ });
+
it("prefers the memory caveat when a memory store somehow carries the flag", () => {
// Defensive ordering: `plaintext` is meaningless for memory, and the
// durability loss is the more consequential of the two statements.
diff --git a/clients/web/src/test/core/react/useInitialConfig.test.tsx b/clients/web/src/test/core/react/useInitialConfig.test.tsx
index 799cb39f..a94980df 100644
--- a/clients/web/src/test/core/react/useInitialConfig.test.tsx
+++ b/clients/web/src/test/core/react/useInitialConfig.test.tsx
@@ -296,6 +296,26 @@ describe("useInitialConfig", () => {
expect(result.current.secretStorage).toEqual(info);
});
+ it("passes through a descriptor that reports an unreadable file", async () => {
+ // `plaintext` is legitimately absent here; the guard must accept that
+ // shape rather than requiring a field the backend cannot honestly set.
+ const info = {
+ kind: "file",
+ reason: "fallback",
+ durable: true,
+ path: "/home/node/.mcp-inspector/secrets.json",
+ encryptionUnknown: "not valid JSON",
+ };
+ const fetchFn = vi
+ .fn()
+ .mockResolvedValue(jsonResponse({ secretStorage: info }));
+ const { result } = renderHook(() =>
+ useInitialConfig({ baseUrl: "http://test.local", fetchFn }),
+ );
+ await waitFor(() => expect(result.current.loading).toBe(false));
+ expect(result.current.secretStorage).toEqual(info);
+ });
+
it("is undefined on a backend that omits the field", async () => {
const fetchFn = vi.fn().mockResolvedValue(jsonResponse({}));
const { result } = renderHook(() =>
@@ -355,6 +375,33 @@ describe("useInitialConfig", () => {
looseMode: "644",
},
],
+ [
+ // Neither half of the encryption question answered.
+ "a file descriptor with neither plaintext nor encryptionUnknown",
+ { kind: "file", reason: "fallback", durable: true, path: "/x" },
+ ],
+ [
+ // Both answered, which is contradictory — the state was either read
+ // or it wasn't.
+ "a file descriptor with both plaintext and encryptionUnknown",
+ {
+ kind: "file",
+ reason: "fallback",
+ durable: true,
+ path: "/x",
+ plaintext: true,
+ encryptionUnknown: "not valid JSON",
+ },
+ ],
+ [
+ "encryptionUnknown on a memory descriptor",
+ {
+ kind: "memory",
+ reason: "fallback",
+ durable: false,
+ encryptionUnknown: "not valid JSON",
+ },
+ ],
[
"a non-string permissionsUnknown",
{
diff --git a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts
index 744c300e..69f6df4b 100644
--- a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts
+++ b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts
@@ -484,6 +484,33 @@ describe("tightenSecretFilePermissions", () => {
}
});
+ it("names an errno-less stat failure rather than printing undefined", async () => {
+ vi.resetModules();
+ vi.doMock("node:fs/promises", async () => {
+ const actual =
+ await vi.importActual(
+ "node:fs/promises",
+ );
+ // No `code` property — a rejection shaped unlike an errno error.
+ return {
+ ...actual,
+ default: actual,
+ stat: vi.fn(() => Promise.reject(new Error("something odd"))),
+ };
+ });
+ try {
+ const mod =
+ await import("@inspector/core/auth/node/file-secret-store.js");
+ expect(await mod.readSecretFilePermissions(filePath())).toEqual({
+ state: "unknown",
+ detail: "unknown error",
+ });
+ } 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
@@ -560,25 +587,40 @@ describe("readOnDiskEncryption", () => {
passphrase: "hunter2",
});
expect(withKey.encrypted).toBe(true);
- expect(await withKey.readOnDiskEncryption()).toBe("none");
+ expect(await withKey.readOnDiskEncryption()).toEqual({
+ state: "plaintext",
+ });
await withKey.set("alpha", "env:A", "1");
- expect(await withKey.readOnDiskEncryption()).toBe("aes-256-gcm");
+ expect(await withKey.readOnDiskEncryption()).toEqual({
+ state: "encrypted",
+ });
});
- it("answers null for an absent, unparseable, or unrecognized file", async () => {
+ it("keeps 'no file yet' apart from 'there is a file and we cannot read it'", async () => {
+ // Collapsing these is what let a passphrase-configured install claim
+ // "Encrypted file" about a corrupt file `set` was about to refuse: the
+ // descriptor falls back to the write policy on `absent`, which is only
+ // honest when the first write really will create the file that way.
const store = new FileSecretStore({ filePath: filePath() });
- expect(await store.readOnDiskEncryption()).toBe(null);
+ expect(await store.readOnDiskEncryption()).toEqual({ state: "absent" });
await fs.writeFile(filePath(), "{not json", "utf-8");
- expect(await store.readOnDiskEncryption()).toBe(null);
+ expect(await store.readOnDiskEncryption()).toEqual({
+ state: "unreadable",
+ detail: "not valid JSON",
+ });
await fs.writeFile(
filePath(),
JSON.stringify({ version: 1, encryption: "rot13" }),
"utf-8",
);
- expect(await store.readOnDiskEncryption()).toBe(null);
+ const unsupported = await store.readOnDiskEncryption();
+ expect(unsupported.state).toBe("unreadable");
+ expect(
+ unsupported.state === "unreadable" ? unsupported.detail : "",
+ ).toMatch(/unsupported envelope/);
});
it("needs no passphrase — it reads the envelope, never the payload", async () => {
@@ -589,7 +631,9 @@ describe("readOnDiskEncryption", () => {
});
await writer.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh");
const keyless = new FileSecretStore({ filePath: filePath() });
- expect(await keyless.readOnDiskEncryption()).toBe("aes-256-gcm");
+ expect(await keyless.readOnDiskEncryption()).toEqual({
+ state: "encrypted",
+ });
});
});
@@ -884,6 +928,51 @@ describe("acquireLock", () => {
await release();
});
+ it.each([
+ ["not JSON at all", "{ not json"],
+ ["JSON that is not an object", '"just-a-string"'],
+ ["an object with no token", JSON.stringify({ pid: 1, host: "h" })],
+ ])("steals a stale lock whose owner stamp is %s", async (_label, stamp) => {
+ // An unusable stamp cannot answer the liveness question, so the mtime is
+ // all there is — and stealing on it is right, because the alternative is
+ // a lock nothing can ever break.
+ const target = path.join(tmpDir, `stamp-${_label.length}.json`);
+ const lockDir = `${target}.lock`;
+ await fs.mkdir(lockDir, { recursive: true });
+ await fs.writeFile(path.join(lockDir, "owner"), stamp, "utf-8");
+ const old = new Date(Date.now() - 60_000);
+ await fs.utimes(lockDir, old, old);
+
+ const release = await acquireLock(target, {
+ staleMs: 50,
+ timeoutMs: 2_000,
+ pollMs: 10,
+ });
+ await release();
+ });
+
+ it("steals a stale lock whose stamp carries no usable pid", async () => {
+ // A stamp with a non-numeric pid names no process to ask about, so it
+ // cannot vouch for the holder being alive.
+ const target = path.join(tmpDir, "nopid.json");
+ const lockDir = `${target}.lock`;
+ await fs.mkdir(lockDir, { recursive: true });
+ await fs.writeFile(
+ path.join(lockDir, "owner"),
+ JSON.stringify({ token: "t", pid: "not-a-number", host: os.hostname() }),
+ "utf-8",
+ );
+ const old = new Date(Date.now() - 60_000);
+ await fs.utimes(lockDir, old, old);
+
+ const release = await acquireLock(target, {
+ staleMs: 50,
+ timeoutMs: 2_000,
+ pollMs: 10,
+ });
+ await release();
+ });
+
it("steals a stale lock stamped by another machine", async () => {
// A shared filesystem: we cannot ask this host about that host's pid, so
// the mtime is all there is. Refusing would make a lock nothing can ever
diff --git a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts
index 04059d3b..a18a1a8f 100644
--- a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts
+++ b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts
@@ -634,6 +634,63 @@ describe("absorbFileSecretsIntoKeyring", () => {
expect(await keyring.get("srv", "oauthClientSecret")).toBe(null);
});
+ it("aborts rather than overwriting when the keychain read fails", async () => {
+ // `get` answers null for an unreachable keychain as well as a missing
+ // entry, and the hand-off *writes* on null — so a transient read failure
+ // would replace a newer keychain value with the older file copy, exactly
+ // inverting the keychain-wins rule. The strict read turns that into a
+ // throw, which leaves the file in place for the next run.
+ const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
+ process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
+ const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
+ const mod = await loadWithProbe(true);
+
+ let wrote = false;
+ const flaky: SecretStore = {
+ get: async () => null,
+ getStrict: async () => {
+ throw new Error("keychain temporarily unavailable");
+ },
+ set: async () => {
+ wrote = true;
+ },
+ delete: async () => {},
+ deleteAllForServer: async () => {},
+ };
+
+ await mod.absorbFileSecretsIntoKeyring(flaky);
+
+ expect(wrote).toBe(false);
+ expect(existsSync(filePath)).toBe(true);
+ expect(warn).toHaveBeenCalledWith(expect.stringContaining("left in place"));
+ });
+
+ it("uses the strict read when the store offers one", async () => {
+ // And when it answers "present", the keychain still wins.
+ const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
+ process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
+ vi.spyOn(console, "warn").mockImplementation(() => {});
+ const mod = await loadWithProbe(true);
+
+ let wrote = false;
+ const store: SecretStore = {
+ // Deliberately disagrees with `getStrict` so the test proves which one
+ // the migration consults.
+ get: async () => null,
+ getStrict: async () => "already-here",
+ set: async () => {
+ wrote = true;
+ },
+ delete: async () => {},
+ deleteAllForServer: async () => {},
+ };
+
+ await mod.absorbFileSecretsIntoKeyring(store);
+
+ expect(wrote).toBe(false);
+ expect(existsSync(filePath)).toBe(false);
+ });
+
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
@@ -688,6 +745,47 @@ describe("absorbFileSecretsIntoKeyring", () => {
});
});
+describe("the descriptor refuses to guess an unreadable file's encryption", () => {
+ it("reports encryptionUnknown instead of falling back to the write policy", async () => {
+ // With a passphrase configured, the old fallback claimed "File
+ // (encrypted)" about a file whose envelope had never been parsed — and
+ // which `set` was about to refuse rather than overwrite.
+ const filePath = path.join(tmpDir, "secrets.json");
+ process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
+ process.env.MCP_INSPECTOR_SECRET_STORE = "file";
+ process.env.MCP_INSPECTOR_SECRET_KEY = "hunter2";
+ await fs.writeFile(filePath, "{ not json", "utf-8");
+ vi.spyOn(console, "warn").mockImplementation(() => {});
+
+ const mod = await loadWithProbe(false);
+ const info = await mod.getSecretStorageInfo();
+
+ expect(info.encryptionUnknown).toBe("not valid JSON");
+ // No half-answer left for a consumer to read a default out of.
+ expect(info.plaintext).toBeUndefined();
+ expect(info.pendingEncryption).toBeUndefined();
+
+ const { secretStorageLabel, secretStorageCaveat, secretStorageTone } =
+ await import("@inspector/core/auth/secret-storage-info.js");
+ expect(secretStorageLabel(info)).toBe("File (unreadable)");
+ expect(secretStorageTone(info)).toBe("warn");
+ expect(secretStorageCaveat(info)).toMatch(/Saving a secret will fail/);
+ });
+
+ it("still uses the write policy when there is genuinely no file", async () => {
+ // The control: `absent` is the one state where reporting the policy is
+ // honest, because the first write really will create it that way.
+ process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "none.json");
+ process.env.MCP_INSPECTOR_SECRET_STORE = "file";
+ process.env.MCP_INSPECTOR_SECRET_KEY = "hunter2";
+ vi.spyOn(console, "warn").mockImplementation(() => {});
+ const mod = await loadWithProbe(false);
+ const info = await mod.getSecretStorageInfo();
+ expect(info.encryptionUnknown).toBeUndefined();
+ expect(info.plaintext).toBe(false);
+ });
+});
+
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
diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts
index 06e7a4bb..35bf92bc 100644
--- a/core/auth/node/file-secret-store.ts
+++ b/core/auth/node/file-secret-store.ts
@@ -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;
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 {
+ async readOnDiskEncryption(): Promise {
+ 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" };
}
}
diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts
index bcb061a9..536e0761 100644
--- a/core/auth/node/secret-store-selection.ts
+++ b/core/auth/node/secret-store-selection.ts
@@ -57,6 +57,7 @@ import {
KeyringSecretStore,
parseAccount,
probeKeyringAvailable,
+ secretStoreGetStrict,
secretStoreIsDurable,
SessionSecretStore,
type SecretStore,
@@ -334,7 +335,14 @@ async function describeFileStore(
detail?: string,
): Promise {
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);
}
diff --git a/core/auth/node/secret-store.ts b/core/auth/node/secret-store.ts
index 5bce41e4..b59b4e34 100644
--- a/core/auth/node/secret-store.ts
+++ b/core/auth/node/secret-store.ts
@@ -320,6 +320,23 @@ export interface SecretStore {
*/
isDurable?(): Promise;
get(serverId: string, field: string): Promise;
+ /**
+ * 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;
set(serverId: string, field: string, value: string): Promise;
/** No-op if no entry exists. */
delete(serverId: string, field: string): Promise;
@@ -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 {
+ 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 {
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 {
+ return store.getStrict
+ ? store.getStrict(serverId, field)
+ : store.get(serverId, field);
+}
+
export async function secretStoreIsDurable(
store: SecretStore,
): Promise {
diff --git a/core/auth/secret-storage-info.ts b/core/auth/secret-storage-info.ts
index cfb28756..847d547f 100644
--- a/core/auth/secret-storage-info.ts
+++ b/core/auth/secret-storage-info.ts
@@ -83,6 +83,18 @@ export interface SecretStorageInfo {
* mode 0600 as fact having verified nothing.
*/
permissionsUnknown?: string;
+ /**
+ * Set when the secrets file exists but its envelope could not be read —
+ * corrupt JSON, or a format written by a newer Inspector. Carries the
+ * reason.
+ *
+ * Mutually exclusive with a meaningful {@link plaintext}: the whole point
+ * is that the encryption state was never established, so neither "File
+ * (encrypted)" nor "File (unencrypted)" may be claimed. `set` will refuse
+ * such a file rather than overwrite it, so this is also a live warning
+ * about writes, not only a description.
+ */
+ encryptionUnknown?: string;
/**
* True when secrets outlive the process. False only for `memory`, and
* it is the *promise* being made rather than an implementation detail:
@@ -106,6 +118,7 @@ export function secretStorageLabel(info: SecretStorageInfo): string {
case "keyring":
return "OS keychain";
case "file":
+ if (info.encryptionUnknown !== undefined) return "File (unreadable)";
return info.plaintext ? "File (unencrypted)" : "File (encrypted)";
case "memory":
return "Memory (this session only)";
@@ -130,6 +143,9 @@ export function secretStorageTone(info: SecretStorageInfo): SecretStorageTone {
// 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";
+ // A file we cannot read is one we cannot write either — a warning about
+ // the next save, not just about the past.
+ if (info.encryptionUnknown !== undefined) return "warn";
return "neutral";
}
@@ -149,7 +165,13 @@ export function secretStorageCaveat(
// property of a file only its owner can open.
if (info.looseMode !== undefined) {
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.`;
+ // The consequence differs by encryption, and overstating it is its own
+ // kind of wrong: a reader of an *encrypted* file gets ciphertext, not
+ // secrets. Saying otherwise trains people to discount the warning on the
+ // occasion it is literally true.
+ return info.plaintext === false
+ ? `The secrets file is mode ${mode}, not 0600, and could not be tightened — others can copy it, and the passphrase is then the only thing protecting its contents.`
+ : `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
@@ -157,6 +179,11 @@ export function secretStorageCaveat(
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.`;
}
+ // An unreadable envelope outranks the encryption caveats below, which all
+ // presuppose we know which mode the file is in.
+ if (info.encryptionUnknown !== undefined) {
+ return `The secrets file could not be read (${info.encryptionUnknown}). Saving a secret will fail rather than overwrite 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."
diff --git a/core/react/useInitialConfig.ts b/core/react/useInitialConfig.ts
index 80f8d9b3..475e39f3 100644
--- a/core/react/useInitialConfig.ts
+++ b/core/react/useInitialConfig.ts
@@ -97,7 +97,16 @@ function usableSecretStorage(value: unknown): SecretStorageInfo | undefined {
if (typeof v.durable !== "boolean") return undefined;
if (v.kind === "file") {
if (typeof v.path !== "string" || v.path === "") return undefined;
- if (typeof v.plaintext !== "boolean") return undefined;
+ // Exactly one of the two must be present: either we know the encryption
+ // state, or we know we could not read it. A descriptor carrying neither
+ // (or both) was not built by a backend we understand, and the footer
+ // would have to invent the missing half.
+ const knowsEncryption = typeof v.plaintext === "boolean";
+ const knowsItCannotRead = typeof v.encryptionUnknown === "string";
+ if (knowsEncryption === knowsItCannotRead) return undefined;
+ if (v.encryptionUnknown !== undefined && !knowsItCannotRead) {
+ return undefined;
+ }
// Optional, so absent is fine — but a *present* value must be a real
// boolean. `pendingEncryption: "false"` is a truthy string, and the
// footer would read it as "already re-encrypting" and print advice that
@@ -125,7 +134,8 @@ function usableSecretStorage(value: unknown): SecretStorageInfo | undefined {
v.plaintext !== undefined ||
v.pendingEncryption !== undefined ||
v.looseMode !== undefined ||
- v.permissionsUnknown !== undefined
+ v.permissionsUnknown !== undefined ||
+ v.encryptionUnknown !== undefined
) {
return undefined;
}