diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx index d8b836da..3eec7f5a 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx @@ -112,4 +112,26 @@ describe("SecretStorageFooter", () => { renderWithMantine(); 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( + , + ); + 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"); + }); }); diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx index 29645107..800e697e 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx @@ -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(" "); diff --git a/clients/web/src/test/core/react/useInitialConfig.test.tsx b/clients/web/src/test/core/react/useInitialConfig.test.tsx index 33523bbc..799cb39f 100644 --- a/clients/web/src/test/core/react/useInitialConfig.test.tsx +++ b/clients/web/src/test/core/react/useInitialConfig.test.tsx @@ -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' 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 bd2c9820..744c300e 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 @@ -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( + "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 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 9670ac44..04059d3b 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 @@ -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( + "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 diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index 9b954a91..06e7a4bb 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -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 | null> { + return this.serialize(() => this.readMap()); + } + private async readMap(): Promise | 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 }; diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index c4fba79d..bcb061a9 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -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 { 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 { 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 { + const filePath = defaultSecretFilePath(); + if (!fsSync.existsSync(filePath)) return; + + const file = new FileSecretStore({ filePath }); + let entries: Record | 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`. * diff --git a/core/auth/secret-storage-info.ts b/core/auth/secret-storage-info.ts index 5435222c..cfb28756 100644 --- a/core/auth/secret-storage-info.ts +++ b/core/auth/secret-storage-info.ts @@ -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." diff --git a/core/react/useInitialConfig.ts b/core/react/useInitialConfig.ts index 91904305..80f8d9b3 100644 --- a/core/react/useInitialConfig.ts +++ b/core/react/useInitialConfig.ts @@ -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; }