diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index 39476682..2a10e59b 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -67,6 +67,11 @@ export async function startHonoServer( sandboxUrl: sandboxController.getUrl() ?? undefined, logger: config.logger, initialConfig: webServerConfigToInitialPayload(config, secretStorage), + // The startup value above is what the banner printed; the route + // re-resolves per request, because the first write under a newly-set + // passphrase encrypts a pre-existing plaintext file and a captured + // descriptor would keep reporting the old state until a restart. + secretStorageResolver: getSecretStorageInfo, }); const app = new Hono(); diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index 1be0674b..ab4a9fe4 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -86,6 +86,11 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { sandboxUrl: sandboxController.getUrl() ?? undefined, logger: config.logger, initialConfig: webServerConfigToInitialPayload(config, secretStorage), + // The startup value above is what the banner printed; the route + // re-resolves per request, because the first write under a newly-set + // passphrase encrypts a pre-existing plaintext file and a captured + // descriptor would keep reporting the old state until a restart. + secretStorageResolver: getSecretStorageInfo, }); // Expose the resolved token to `transformIndexHtml`. Left empty when 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 1d60a92d..c21afe43 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 @@ -103,6 +103,26 @@ describe("secretStorageCaveat", () => { expect(caveat).not.toContain("MCP_INSPECTOR_SECRET_KEY"); }); + it("reports a mode it could not tighten, ahead of the encryption caveat", () => { + // A file other users can read is a live exposure; "unencrypted" is a + // property of a file only its owner can open. When both hold, the + // permission problem is the one to say out loud. + const loose: SecretStorageInfo = { ...plaintextFile, looseMode: 0o644 }; + expect(secretStorageTone(loose)).toBe("warn"); + expect(secretStorageCaveat(loose)).toContain("0644"); + expect(secretStorageCaveat(loose)).not.toContain( + "MCP_INSPECTOR_SECRET_KEY", + ); + }); + + it("warns about a loose mode even on an encrypted file", () => { + // The passphrase is then the only thing between a reader and the values, + // which is worth saying rather than rendering the quiet neutral state. + const loose: SecretStorageInfo = { ...encryptedFile, looseMode: 0o640 }; + expect(secretStorageTone(loose)).toBe("warn"); + expect(secretStorageCaveat(loose)).toContain("0640"); + }); + 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/client/node-persistence.test.ts b/clients/web/src/test/core/client/node-persistence.test.ts index c0055925..7fadb821 100644 --- a/clients/web/src/test/core/client/node-persistence.test.ts +++ b/clients/web/src/test/core/client/node-persistence.test.ts @@ -5,6 +5,7 @@ import * as os from "node:os"; import * as path from "node:path"; import { InMemorySecretStore, + SessionSecretStore, KeychainUnavailableError, SECRET_FIELD_IDP_CLIENT_SECRET, type SecretStore, @@ -62,6 +63,23 @@ describe("client node-persistence", () => { ).toBe("plain"); }); + it("keeps the plaintext on disk when the store is session-scoped", async () => { + // The container fallback. Migrating here would delete a secret that + // survives restarts and keep only a copy that dies with the process — + // and it happens on an ordinary read, so merely loading the app would + // do it. The session still works: the value is loaded into the store. + const filePath = await makeTmpFile( + JSON.stringify(configWithPlaintextSecret), + ); + const secretStore = new SessionSecretStore(); + + const loaded = await readClientConfigStore(filePath, secretStore); + + expect(loaded.enterpriseManagedAuth?.idp.clientSecret).toBe("plain"); + // The disk copy is deliberately left alone. + expect(readFileSync(filePath, "utf-8")).toContain("plain"); + }); + it("does not overwrite an existing keychain secret during migration", async () => { const filePath = await makeTmpFile( JSON.stringify(configWithPlaintextSecret), diff --git a/clients/web/src/test/core/react/useInitialConfig.test.tsx b/clients/web/src/test/core/react/useInitialConfig.test.tsx index ac70b6a1..33523bbc 100644 --- a/clients/web/src/test/core/react/useInitialConfig.test.tsx +++ b/clients/web/src/test/core/react/useInitialConfig.test.tsx @@ -275,6 +275,26 @@ describe("useInitialConfig", () => { expect(result.current.secretStorage).toEqual(info); }); + it("passes through the optional file fields when they are well-typed", async () => { + const info = { + kind: "file", + reason: "fallback", + durable: true, + plaintext: true, + path: "/home/node/.mcp-inspector/secrets.json", + pendingEncryption: true, + looseMode: 0o644, + }; + 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(() => @@ -310,6 +330,42 @@ describe("useInitialConfig", () => { "a descriptor with a non-boolean durable", { kind: "memory", reason: "fallback", durable: "no" }, ], + [ + // The truthy-string trap: the footer would read this as "already + // re-encrypting" and print advice that is the opposite of the truth. + "a stringly-typed pendingEncryption", + { + kind: "file", + reason: "fallback", + durable: true, + plaintext: true, + path: "/x", + pendingEncryption: "false", + }, + ], + [ + "a non-numeric looseMode", + { + kind: "file", + reason: "fallback", + durable: true, + plaintext: true, + path: "/x", + looseMode: "644", + }, + ], + [ + // 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' + // answers is worse than rendering nothing. + "file fields on a memory descriptor", + { + kind: "memory", + reason: "fallback", + durable: false, + plaintext: true, + }, + ], ])("rejects %s", async (_label, value) => { const fetchFn = vi .fn() 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 26e433a3..2d9d1bd6 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 @@ -15,6 +15,7 @@ import * as path from "node:path"; import { acquireLock, FileSecretStore, + SecretFileLockTimeoutError, SecretFileKeyMismatchError, tightenSecretFilePermissions, } from "@inspector/core/auth/node/file-secret-store.js"; @@ -676,16 +677,102 @@ describe("acquireLock", () => { await release(); }); - it("proceeds unlocked rather than failing when the lock cannot be created", async () => { - // A path whose parent does not exist: mkdir fails with ENOENT, not - // EEXIST. Refusing to write here would deny a real user a real secret to - // prevent a race with a second process that may not exist. - const release = await acquireLock( - path.join(tmpDir, "no-such-dir", "secrets.json"), - ); + it("creates the parent directory so a first write is locked too", async () => { + // The first run has no secrets directory yet — `writeStoreFile` creates + // it later. Without creating it here, mkdir failed ENOENT and the lock + // was skipped, so two processes' *first* writes could both proceed + // unlocked and overwrite each other. First writes are exactly when two + // processes are most likely to start together. + const target = path.join(tmpDir, "no-such-dir", "secrets.json"); + const release = await acquireLock(target); + // A real lock was taken: a second acquisition must not be handed out. + let secondTaken = false; + void acquireLock(target).then((r) => { + secondTaken = true; + return r(); + }); + await new Promise((r) => setTimeout(r, 60)); + expect(secondTaken).toBe(false); await release(); }); + it("releases only a lock it still owns", async () => { + // A holder whose work outran the stale threshold must not delete the + // next owner's lock on its way out — that would drop a third writer into + // a section two processes believed they held. + const target = path.join(tmpDir, "owned.json"); + const lockDir = `${target}.lock`; + const release = await acquireLock(target); + // Simulate a steal: someone judged us stale, removed our lock, and took + // their own. + await fs.rm(lockDir, { recursive: true, force: true }); + await fs.mkdir(lockDir); + await fs.writeFile(path.join(lockDir, "owner"), "someone-else", "utf-8"); + await release(); + // Their lock is untouched. + expect(await fs.readFile(path.join(lockDir, "owner"), "utf-8")).toBe( + "someone-else", + ); + await fs.rm(lockDir, { recursive: true, force: true }); + }); + + it("fails the mutation rather than writing past a live holder", async () => { + // Falling through to an unlocked write under contention would be the + // worst possible moment for it: another writer is provably active. `set` + // is allowed to hard-fail, so this surfaces as the documented 503. + // + // The holder heartbeats faster than it goes stale, so the waiter can + // never mistake it for dead and must time out instead. + const target = path.join(tmpDir, "contended.json"); + const timings = { + staleMs: 400, + timeoutMs: 250, + heartbeatMs: 50, + pollMs: 10, + }; + const release = await acquireLock(target, timings); + try { + await expect(acquireLock(target, timings)).rejects.toBeInstanceOf( + SecretFileLockTimeoutError, + ); + } finally { + await release(); + } + }); + + it("does not steal from a holder that is merely slow", async () => { + // The heartbeat is what separates "dead" from "slow". Without it a + // holder on a slow filesystem loses its lock mid-section and the lost + // update comes straight back — so the waiter here must time out rather + // than acquire, even though it waits well past the stale threshold. + const target = path.join(tmpDir, "slow.json"); + const release = await acquireLock(target, { + staleMs: 120, + timeoutMs: 60_000, + heartbeatMs: 20, + pollMs: 10, + }); + try { + let stolen = false; + void acquireLock(target, { + staleMs: 120, + timeoutMs: 400, + pollMs: 10, + }) + .then((r) => { + stolen = true; + return r(); + }) + .catch(() => { + // Timed out, which is the expected outcome. + }); + await new Promise((r) => setTimeout(r, 500)); + expect(stolen).toBe(false); + } finally { + await release(); + } + }); + it("serializes concurrent writers through two independent store instances", async () => { // Two `FileSecretStore` objects have separate in-process queues, so this // is the cross-process race in miniature: without the file lock the second 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 ffc0f24f..0a5be897 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 @@ -333,6 +333,22 @@ describe("warnAboutSecretStorage", () => { expect(warn).not.toHaveBeenCalled(); }); + it("announces a fallback with no cause to name", () => { + // `detail` is optional — an explicitly configured store has no keychain + // error behind it, and the banner must not print an empty error line. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + warnAboutSecretStorage({ + kind: "file", + reason: "fallback", + durable: true, + path: "/x/secrets.json", + plaintext: false, + }); + const printed = warn.mock.calls.map((c) => String(c[0])).join("\n"); + expect(printed).toContain("fell back from the OS keychain"); + expect(printed).not.toContain("Keychain error:"); + }); + it("announces a fallback, names the cause, and points at the override", () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); warnAboutSecretStorage({ @@ -503,6 +519,59 @@ describe("defaultSecretFilePath", () => { }); }); +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 + // states the mode as fact, and on this box that statement was untrue + // while the browser had no way to know. + 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", + ); + await fs.chmod(filePath, 0o644); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + vi.resetModules(); + vi.doMock("node:fs/promises", async () => { + const actual = + await vi.importActual( + "node:fs/promises", + ); + return { + ...actual, + default: actual, + chmod: vi.fn(() => + Promise.reject(new Error("EPERM: operation not permitted")), + ), + }; + }); + 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.looseMode).toBe(0o644); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("0644")); + } finally { + vi.doUnmock("node:fs/promises"); + vi.doUnmock("@inspector/core/auth/node/secret-store.js"); + vi.resetModules(); + } + }); +}); + describe("the descriptor reports the file, not the write policy", () => { it("says plaintext while a pre-existing plaintext file is only *going* to be encrypted", async () => { // The transitional state: the passphrase is set, so the next write will diff --git a/clients/web/src/test/integration/mcp/remote/server-config.test.ts b/clients/web/src/test/integration/mcp/remote/server-config.test.ts index a25d81f8..122ebf07 100644 --- a/clients/web/src/test/integration/mcp/remote/server-config.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-config.test.ts @@ -81,3 +81,60 @@ describe("createRemoteApp GET /api/config", () => { ).toBe(false); }); }); + +describe("GET /api/config re-resolves secretStorage per request (#1950)", () => { + it("serves the resolver's current answer, not the startup snapshot", async () => { + // The descriptor states where a secret lands, and it changes while the + // process runs — the first write under a newly-set passphrase encrypts a + // pre-existing plaintext file. A value captured at boot would keep + // telling every page load "still unencrypted" until a restart. + let plaintext = true; + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + initialConfig: { + defaultEnvironment: {}, + secretStorage: { + kind: "file", + reason: "fallback", + durable: true, + plaintext: true, + path: "/tmp/secrets.json", + }, + }, + secretStorageResolver: async () => ({ + kind: "file", + reason: "fallback", + durable: true, + plaintext, + path: "/tmp/secrets.json", + }), + }); + + const first = (await ( + await app.request(new Request("http://test/api/config")) + ).json()) as { secretStorage?: { plaintext?: boolean } }; + expect(first.secretStorage?.plaintext).toBe(true); + + plaintext = false; // the upgrading write happens + const second = (await ( + await app.request(new Request("http://test/api/config")) + ).json()) as { secretStorage?: { plaintext?: boolean } }; + expect(second.secretStorage?.plaintext).toBe(false); + }); + + it("falls back to the startup payload when no resolver is supplied", async () => { + // Embedders and the test suite pass no resolver; they must keep the + // previous behaviour rather than losing the field. + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + initialConfig: { + defaultEnvironment: {}, + secretStorage: { kind: "keyring", reason: "default", durable: true }, + }, + }); + const body = (await ( + await app.request(new Request("http://test/api/config")) + ).json()) as { secretStorage?: { kind?: string } }; + expect(body.secretStorage?.kind).toBe("keyring"); + }); +}); diff --git a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts index 0a286253..a5f6f013 100644 --- a/clients/web/src/test/integration/mcp/remote/servers-route.test.ts +++ b/clients/web/src/test/integration/mcp/remote/servers-route.test.ts @@ -21,6 +21,7 @@ import { DEFAULT_SEED_CONFIG } from "@inspector/core/mcp/serverList.js"; import { InMemorySecretStore, KeychainUnavailableError, + SessionSecretStore, SECRET_FIELD_OAUTH_CLIENT_SECRET, envSecretField, type SecretStore, @@ -2622,3 +2623,69 @@ describe("/api/servers read-only sessions (#1481/#1483)", () => { } }); }); + +describe("plaintext migration against a session-scoped store (#1950)", () => { + // The container fallback selects an in-memory store in production. The + // migration must not take that as licence to delete the durable copy: it + // runs on an ordinary GET, so merely opening the app would move the user's + // secrets into RAM and lose them at exit. + let tempDir: string; + let configPath: string; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), "inspector-session-store-")); + configPath = join(tempDir, "mcp.json"); + writeFileSync( + configPath, + JSON.stringify({ + mcpServers: { + srv: { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { clientId: "cid", clientSecret: "must-survive" }, + }, + }, + }), + ); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + it("leaves the disk plaintext alone and still serves the secret", async () => { + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: new SessionSecretStore(), + }); + const before = readFileSync(configPath, "utf-8"); + + const res = await app.request(new Request("http://test/api/servers")); + expect(res.status).toBe(200); + const body = (await res.json()) as MCPConfig; + // The session behaves normally — the value is loaded and served. + expect( + (body.mcpServers.srv as { oauth?: { clientSecret?: string } }).oauth + ?.clientSecret, + ).toBe("must-survive"); + // But the durable copy is untouched. + expect(readFileSync(configPath, "utf-8")).toBe(before); + }); + + it("still strips it against a durable store, so the guard is the difference", async () => { + // The control: same config, same request, a store that outlives the + // process — and the migration does what it always did. + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: new InMemorySecretStore(), + }); + + const res = await app.request(new Request("http://test/api/servers")); + expect(res.status).toBe(200); + expect(readFileSync(configPath, "utf-8")).not.toContain("must-survive"); + }); +}); diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index 5ab95096..29589d36 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -60,6 +60,7 @@ import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; +import * as path from "node:path"; import { readStoreFile, writeStoreFile } from "../../storage/store-io.js"; import { SecretStoreUnavailableError, @@ -201,13 +202,50 @@ function decodeParts( * lock exists to close. */ const LOCK_STALE_MS = 10_000; -/** Total time to wait for a lock before giving up and reporting it. */ -const LOCK_TIMEOUT_MS = 5_000; +/** + * How long to wait for a live lock. Deliberately longer than + * {@link LOCK_STALE_MS}, so a waiter always outlives the point at which the + * holder becomes stealable — with the shorter value it used to have, a + * waiter gave up before it could ever steal an abandoned lock, which is the + * one case the stealing exists for. + */ +const LOCK_TIMEOUT_MS = 15_000; const LOCK_POLL_MS = 25; +/** + * How often a holder touches its lock to prove it is alive. + * + * Comfortably under {@link LOCK_STALE_MS} so a live holder is never judged + * stale, which is what makes the two mechanisms coherent: staleness then + * means *dead*, not merely *slow*. Without the heartbeat, a holder on a + * slow filesystem — or one whose scrypt derivation ran long — has its lock + * stolen while it is still inside the critical section, reinstating exactly + * the lost update the lock exists to prevent. + */ +const LOCK_HEARTBEAT_MS = 3_000; const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); +/** + * Thrown when a lock is held by someone else for longer than + * {@link LOCK_TIMEOUT_MS}. + * + * A timeout must **fail the mutation**, never fall through and write + * anyway: proceeding unlocked past a live holder is precisely the + * lost-update the lock exists to prevent, and doing it under contention — + * the one moment another writer is provably active — would be the worst + * possible time. `set` is allowed to hard-fail by the store contract, so + * this surfaces as the documented 503 and the user retries. + */ +export class SecretFileLockTimeoutError extends SecretStoreUnavailableError { + constructor(filePath: string) { + super( + `Timed out waiting for another process to finish writing the secrets file at ${filePath}. Another Inspector may be busy; try again.`, + ); + this.name = "SecretFileLockTimeoutError"; + } +} + /** * Take an exclusive, cross-process lock on `filePath`. * @@ -215,56 +253,112 @@ const sleep = (ms: number): Promise => * fails with `EEXIST` on every POSIX filesystem and on Windows, and it * needs no `O_EXCL` open-flag juggling. Returns the release function. * - * **Stale locks are stolen, and that is not optional.** A container killed - * with SIGKILL leaves the directory behind, and a lock that can only be - * released by a cooperative exit would make one `docker kill` brick secret - * writes permanently — turning a rare lost-write race into a permanent - * outage, which is a strictly worse failure. Staleness is judged by the - * directory's mtime. + * **Ownership is stamped, not assumed.** The holder writes a random token + * inside the lock directory and release only removes a lock still carrying + * that token. Without it, a holder whose work outran the stale threshold + * would delete the *next* owner's lock on its way out, letting a third + * writer into a critical section two processes believed they held — a + * rarer and much more confusing corruption than the one being prevented. * - * Never throws for lock-infrastructure reasons alone. If the lock cannot - * be created at all (a read-only mount, a filesystem without mkdir - * semantics we can rely on), the caller proceeds unlocked: the in-process - * queue still protects the common single-process case, and refusing to - * write would deny a real user a real secret in order to prevent a race - * with a second process that may not exist. + * **Stale locks are stolen, and that is not optional.** A container killed + * with SIGKILL leaves the directory behind, and a lock that only a + * cooperative exit can release would make one `docker kill` brick secret + * writes permanently — turning a rare lost write into a permanent outage, + * which is strictly worse. Staleness is judged by the directory's mtime, + * which a live holder refreshes on a heartbeat — so "stale" means the + * holder is *dead*, never merely slow. That distinction is what keeps the + * stealing from reinstating the very race the lock prevents. + * + * Throws {@link SecretFileLockTimeoutError} when another holder keeps it + * past the timeout. The *only* path that proceeds unlocked is a filesystem + * that cannot create the lock at all (a read-only mount, an exotic + * filesystem) — there, refusing would deny every write on a box where the + * single-process case is still perfectly safe, and a genuinely unwritable + * location fails at the write itself with a better message than a lock + * error would give. */ +/** + * Timing policy, overridable so the tests can exercise contention, + * staleness, and timeout without spending the real intervals. Production + * callers pass nothing and get the constants above. + */ +export interface LockTimings { + staleMs?: number; + timeoutMs?: number; + heartbeatMs?: number; + pollMs?: number; +} + export async function acquireLock( filePath: string, + timings: LockTimings = {}, ): Promise<() => Promise> { + const staleMs = timings.staleMs ?? LOCK_STALE_MS; + const timeoutMs = timings.timeoutMs ?? LOCK_TIMEOUT_MS; + const heartbeatMs = timings.heartbeatMs ?? LOCK_HEARTBEAT_MS; + const pollMs = timings.pollMs ?? LOCK_POLL_MS; const lockDir = `${filePath}.lock`; + const ownerFile = `${lockDir}/owner`; + const token = crypto.randomBytes(16).toString("hex"); const noop = async (): Promise => {}; - const deadline = Date.now() + LOCK_TIMEOUT_MS; + const deadline = Date.now() + timeoutMs; + + // The parent must exist before the lock can. On a first run it does not — + // `writeStoreFile` creates it later — so without this the very first write + // of every process failed with ENOENT and proceeded unlocked, meaning two + // first writes could both bypass the lock, read an empty map, and overwrite + // each other. The first write is exactly when two processes are most likely + // to start together. + try { + await fs.mkdir(path.dirname(filePath), { recursive: true }); + } catch { + // Unwritable parent. The mkdir below will fail too, and the write after + // it will fail with a message about the actual problem. + } for (;;) { try { await fs.mkdir(lockDir, { recursive: false }); + await fs.writeFile(ownerFile, token, "utf-8"); + // Prove liveness for as long as we hold it. `unref` so a held lock + // never keeps the process alive on its own — the interval is a + // side-channel, not work anyone is waiting for. + const heartbeat = setInterval(() => { + const now = new Date(); + void fs.utimes(lockDir, now, now).catch(() => { + // Lock already released or stolen; the release below copes. + }); + }, heartbeatMs); + heartbeat.unref?.(); return async () => { + clearInterval(heartbeat); try { - await fs.rmdir(lockDir); + // Release only what we still own. A lock judged stale and stolen + // now belongs to someone else, and removing it would drop them + // into a race with whoever comes next. + const held = await fs.readFile(ownerFile, "utf-8"); + if (held !== token) return; + await fs.rm(lockDir, { recursive: true, force: true }); } catch { - // Already gone — another process judged us stale and stole it. - // Nothing to undo, and nothing the caller can act on. + // Already gone, or unreadable. Either way there is nothing to + // release and nothing the caller can act on. } }; } catch (err) { const code = (err as NodeJS.ErrnoException).code; - if (code !== "EEXIST") { - // Not "someone else holds it" — the lock itself is unavailable. - // Proceed unlocked rather than failing the write; see above. - return noop; - } + if (code !== "EEXIST") return noop; // cannot lock here at all — see above try { const held = (await fs.stat(lockDir)).mtimeMs; - if (Date.now() - held > LOCK_STALE_MS) { - await fs.rmdir(lockDir); + if (Date.now() - held > staleMs) { + await fs.rm(lockDir, { recursive: true, force: true }); continue; } } catch { // The holder released it between our mkdir and our stat. Retry. } - if (Date.now() >= deadline) return noop; - await sleep(LOCK_POLL_MS); + if (Date.now() >= deadline) + throw new SecretFileLockTimeoutError(filePath); + await sleep(pollMs); } } } diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index f447b0e3..637b7148 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -52,9 +52,10 @@ import { tightenSecretFilePermissions, } from "./file-secret-store.js"; import { - InMemorySecretStore, KeyringSecretStore, probeKeyringAvailable, + secretStoreIsDurable, + SessionSecretStore, type SecretStore, } from "./secret-store.js"; @@ -248,8 +249,12 @@ async function buildStore( }; } if (kind === "memory") { + // `SessionSecretStore`, not the bare `InMemorySecretStore` the tests + // use: it reports `isDurable() === false`, which is what stops the + // mcp.json / client.json migrations from stripping a plaintext secret + // off disk in exchange for a copy that dies with this process. return { - store: new InMemorySecretStore(), + store: new SessionSecretStore(), info: { kind, reason, durable: false, detail }, }; } @@ -293,11 +298,16 @@ async function describeFileStore( ): Promise { const onDisk = await store.readOnDiskEncryption(); const plaintext = onDisk === null ? !store.encrypted : onDisk === "none"; + // 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. + const perms = await tightenSecretFilePermissions(filePath); return { kind, reason, path: filePath, plaintext, + ...(perms.state === "loose" ? { looseMode: perms.mode } : {}), // Only meaningful while the two disagree; omitted otherwise so the // payload does not carry a flag that says nothing. ...(plaintext && store.encrypted ? { pendingEncryption: true } : {}), @@ -414,6 +424,9 @@ class DeferredSecretStore implements SecretStore { private async target(): Promise { return (await resolveSecretStore()).store; } + async isDurable(): Promise { + return secretStoreIsDurable(await this.target()); + } async get(serverId: string, field: string): Promise { return (await this.target()).get(serverId, field); } diff --git a/core/auth/node/secret-store.ts b/core/auth/node/secret-store.ts index 7944a23b..5bce41e4 100644 --- a/core/auth/node/secret-store.ts +++ b/core/auth/node/secret-store.ts @@ -301,6 +301,24 @@ const PROBE_ACCOUNT = "__inspector:probe"; * be lost. */ export interface SecretStore { + /** + * Do values written here outlive the process? + * + * Optional, and **absent means durable** — every store that predates + * #1950 is, and a custom one is far more likely to be backed by + * something than by RAM. + * + * It exists because the *migrations* need it. Both of them (mcp.json in + * `server.ts`, client.json in `client/node-persistence.ts`) lift a + * plaintext secret off disk, write it to this store, and then strip it + * from the file — a safe trade only while "written to the store" is at + * least as durable as "left on disk". `InMemorySecretStore` used to be + * a test double only, so that held by construction; making it a + * production fallback broke it, and a plain `GET /api/servers` on an + * unmounted container would have moved a user's existing secrets into + * RAM and lost them at exit. + */ + isDurable?(): Promise; get(serverId: string, field: string): Promise; set(serverId: string, field: string, value: string): Promise; /** No-op if no entry exists. */ @@ -439,6 +457,21 @@ export class KeyringSecretStore implements SecretStore { * server factory. Mirrors the keyring contract exactly so swapping it * in/out doesn't change behavior beyond persistence. */ +/** + * Is `store` durable? Absent `isDurable` means yes — see the interface. + * + * A free function rather than a required method so the existing test + * doubles (and any third-party implementation) keep compiling, while the + * one store that is *not* durable has to say so explicitly. Defaulting the + * other way would make every double silently non-durable and quietly + * disable the migrations they exist to exercise. + */ +export async function secretStoreIsDurable( + store: SecretStore, +): Promise { + return store.isDurable ? store.isDurable() : true; +} + export class InMemorySecretStore implements SecretStore { private readonly map = new Map(); @@ -461,3 +494,26 @@ export class InMemorySecretStore implements SecretStore { } } } + +/** + * The in-memory store as a **production** choice — the fallback for a + * container with no keychain and nothing durable mounted (#1950). + * + * Behaviorally identical to {@link InMemorySecretStore}; it exists to + * answer `isDurable()` with `false`, and that one bit is load-bearing. + * Both migrations (mcp.json, client.json) lift a plaintext secret off + * disk, write it to the store, and then delete it from the file — safe + * only while the store outlives the process. Against a session-scoped + * store that trade destroys the secret, and it runs on an ordinary read, + * so merely opening the app would do it. + * + * Kept as a separate class rather than a flag on the base so the test + * suite's many `new InMemorySecretStore()` doubles keep standing in for a + * *working keychain*, which is what they are there to be. Only the code + * that deliberately chooses RAM in production says so. + */ +export class SessionSecretStore extends InMemorySecretStore { + async isDurable(): Promise { + return false; + } +} diff --git a/core/auth/secret-storage-info.ts b/core/auth/secret-storage-info.ts index 38c9ca79..d1b42ab6 100644 --- a/core/auth/secret-storage-info.ts +++ b/core/auth/secret-storage-info.ts @@ -62,6 +62,16 @@ export interface SecretStorageInfo { * what will actually clear the condition. */ pendingEncryption?: boolean; + /** + * Octal mode of the secrets file when it is **not** `0600` and could not + * be tightened — a file owned by another user, or on a read-only mount. + * + * Carried into the descriptor rather than left in a startup log line + * because the caveat below states the mode as fact, at the point where a + * user types a secret. On such a box that statement was simply untrue, + * and the browser had no way to know. + */ + looseMode?: number; /** * True when secrets outlive the process. False only for `memory`, and * it is the *promise* being made rather than an implementation detail: @@ -103,6 +113,9 @@ export type SecretStorageTone = "neutral" | "warn"; export function secretStorageTone(info: SecretStorageInfo): SecretStorageTone { if (info.kind === "memory") return "warn"; if (info.plaintext) return "warn"; + // 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"; return "neutral"; } @@ -117,6 +130,13 @@ export function secretStorageCaveat( if (info.kind === "memory") { return "Secrets are not written anywhere and are lost when the Inspector exits."; } + // The permission problem outranks the encryption one when both hold: a + // file other users can read is a live exposure, while "unencrypted" is a + // 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.`; + } 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/client/node-persistence.ts b/core/client/node-persistence.ts index 0d769d38..f1e83fcb 100644 --- a/core/client/node-persistence.ts +++ b/core/client/node-persistence.ts @@ -3,6 +3,7 @@ */ import { + secretStoreIsDurable, SecretStoreUnavailableError, type SecretStore, } from "../auth/node/secret-store.js"; @@ -55,6 +56,14 @@ async function migrateClientPlaintextSecret( value, ); } + // Only strip the plaintext once it is somewhere that outlives us. + // Against a session-scoped store (the container fallback added in + // #1950) this migration would trade a secret that survives restarts + // for one that dies with the process — and it runs on an ordinary + // read, so merely loading the app would destroy it. The value is + // still loaded into the store above, so this session behaves + // normally; only the delete is withheld. + if (!(await secretStoreIsDurable(secretStore))) return config; await writeStoreFile(filePath, serializeStore(stripped)); return stripped; } catch (err) { diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 4341954f..90241927 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -65,6 +65,7 @@ import { RemoteSession } from "./remote-session.js"; import { createRemoteAuthProvider } from "./tokenAuthProvider.js"; import { API_SERVER_ENV_VARS } from "../constants.js"; import { + secretStoreIsDurable, SecretStoreUnavailableError, type SecretStore, } from "../../../auth/node/secret-store.js"; @@ -260,6 +261,15 @@ export interface RemoteServerOptions { * runners. */ secretStore?: SecretStore; + /** + * Re-resolve the secret-storage descriptor for each `GET /api/config`. + * + * A function rather than a value because the answer changes while the + * process runs (see the route). Optional: a backend that doesn't supply + * one falls back to whatever `initialConfig` carried, which is what the + * tests and any embedder that doesn't care get. + */ + secretStorageResolver?: () => Promise; } export interface CreateRemoteAppResult { @@ -662,10 +672,22 @@ export function createRemoteApp( app.use("*", createAuthMiddleware(authToken)); } - app.get("/api/config", (c) => { + app.get("/api/config", async (c) => { + // `secretStorage` is re-resolved per request, not taken from the + // startup payload. It describes bytes on disk that this very process + // changes — the first `set` under a newly-set passphrase encrypts a + // pre-existing plaintext file — so a value captured at boot would keep + // telling every page load "still unencrypted" until a restart. That is + // stale in the safe direction, but stale about the single fact this + // field exists to state, which makes re-reading a small file per config + // fetch (once per page load) the obviously right trade. + const secretStorage = options.secretStorageResolver + ? await options.secretStorageResolver() + : options.initialConfig?.secretStorage; const payload = { ...options.initialConfig, writable, + ...(secretStorage ? { secretStorage } : {}), ...(options.sandboxUrl ? { sandboxUrl: options.sandboxUrl } : {}), }; return c.json(payload); @@ -1918,6 +1940,12 @@ export function createRemoteApp( ): Promise<{ migrated: MCPConfig; changed: boolean }> => { let changed = false; const next: MCPConfig = { mcpServers: {} }; + const durable = await secretStoreIsDurable(secretStore); + if (!durable && fileLogger) { + fileLogger.warn( + "Secrets are kept in memory for this session, so plaintext values in mcp.json are left on disk rather than migrated away. They would otherwise be lost when the Inspector exits.", + ); + } try { for (const [id, stored] of Object.entries(config.mcpServers)) { const { stripped, secrets } = extractSecretsFromStored(stored); @@ -1935,6 +1963,17 @@ export function createRemoteApp( // plaintext from disk. This handles the case where a user has // edited mcp.json by hand after the original migration. } + // Only strip the plaintext once it is somewhere that outlives us. + // Against a session-scoped store (the container fallback added in + // #1950) this would trade a secret that survives restarts for one + // that dies with the process — and it runs on an ordinary GET, so + // merely opening the app would destroy it. The values are still + // loaded into the store above, so this session behaves normally; + // only the disk delete is withheld. + if (!durable) { + next.mcpServers[id] = stored; + continue; + } next.mcpServers[id] = stripped; changed = true; } diff --git a/core/react/useInitialConfig.ts b/core/react/useInitialConfig.ts index 88c26c5c..91904305 100644 --- a/core/react/useInitialConfig.ts +++ b/core/react/useInitialConfig.ts @@ -98,6 +98,29 @@ function usableSecretStorage(value: unknown): SecretStorageInfo | undefined { if (v.kind === "file") { if (typeof v.path !== "string" || v.path === "") return undefined; if (typeof v.plaintext !== "boolean") 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 + // is the opposite of the truth. + if ( + v.pendingEncryption !== undefined && + typeof v.pendingEncryption !== "boolean" + ) { + return undefined; + } + if (v.looseMode !== undefined && typeof v.looseMode !== "number") { + 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 + // alternative is rendering a mixture of two stores' answers. + v.path !== undefined || + v.plaintext !== undefined || + v.pendingEncryption !== undefined || + v.looseMode !== undefined + ) { + return undefined; } return value as SecretStorageInfo; }