From 37faf1be8835f17e69646a59d6ce05a4a5d1d1ba Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 17:41:56 -0400 Subject: [PATCH] fix(auth): address Copilot review round 8 (four inline findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lock finding from this round is deliberately not addressed here — see below. - **My round-6 strict-read fix was inert in production.** `DeferredSecretStore` is what `defaultSecretStore()` returns, and it did not forward `getStrict` — so `secretStoreGetStrict` fell through to the tolerant `get` for every real caller, and the strictness existed only in tests that injected a concrete store. Forwarded now, along with `getMany`. - **The same tolerant-read bug remained in both pre-existing migrations.** I added `getStrict` for the new hand-off in round 6 and never applied it to the mcp.json and client.json migrations, which have the identical shape: decide from `get`, then write on `null`. A transient keychain read failure therefore let an older plaintext value overwrite a newer keychain one — and the disk copy was then stripped, so the newer value was gone. Both use the strict read now; the throw is a `SecretStoreUnavailableError`, which both call sites already handle as "abandon the migration, keep the file". - **The permission check made a false statement on Windows.** `stat` returns synthetic bits there, and interpreting them yields a confident falsehood either way — "0666 — not owner-only" about a file the ACL may restrict, or "owner-only" about one it does not. The ACL is the real answer and this does not inspect it, so Windows now reports `unknown`. - **Rehydration cost one full decryption per field.** `get` reads and decrypts the entire file, and both rehydration paths ask field by field, serialized behind the store's own queue. Measured on this machine, scrypt at N=16384 is ~23ms per derivation, so a 20-secret encrypted catalog spent **~450ms of pure key derivation on every `GET /api/servers`** — a visible stall, not a micro-optimization. Added an optional `getMany` seam: `FileSecretStore` decrypts once per server, everything else falls back to the parallel per-field `get` that already suited it. A test counts derivations directly: 1 for a three-field `getMany`, 3 for the three `get`s it replaced. Also broadens a log message that still said "Keychain unavailable" for a store that may not be a keychain. Full `npm run ci` green — 5,762 tests under the coverage gate. Signed-off-by: cliffhall --- .../test/core/client/node-persistence.test.ts | 29 +++++ .../auth/node/file-secret-store.test.ts | 122 ++++++++++++++++++ .../auth/node/secret-store-selection.test.ts | 39 ++++++ .../mcp/remote/server-extra-coverage.test.ts | 4 +- .../mcp/remote/servers-route.test.ts | 31 +++++ core/auth/node/file-secret-store.ts | 43 ++++++ core/auth/node/secret-store-selection.ts | 16 +++ core/auth/node/secret-store.ts | 37 ++++++ core/client/node-persistence.ts | 9 +- core/mcp/node/server-secrets.ts | 18 ++- core/mcp/remote/node/server.ts | 27 ++-- 11 files changed, 351 insertions(+), 24 deletions(-) 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 7fadb821..8d4233f8 100644 --- a/clients/web/src/test/core/client/node-persistence.test.ts +++ b/clients/web/src/test/core/client/node-persistence.test.ts @@ -63,6 +63,35 @@ describe("client node-persistence", () => { ).toBe("plain"); }); + it("aborts rather than overwriting when the keychain read fails", async () => { + // The keychain-wins lookup used the tolerant `get`, which maps an + // unreadable store to `null` — and the branch below writes on `null`. So + // a transient read failure let the older client.json copy overwrite a + // newer stored secret, and the disk copy was then stripped. + const filePath = await makeTmpFile( + JSON.stringify(configWithPlaintextSecret), + ); + let wrote = false; + const flaky: SecretStore = { + get: async () => null, + getStrict: async () => { + throw new KeychainUnavailableError(new Error("temporarily down")); + }, + set: async () => { + wrote = true; + }, + delete: async () => {}, + deleteAllForServer: async () => {}, + }; + + const loaded = await readClientConfigStore(filePath, flaky); + + expect(wrote).toBe(false); + // The plaintext survives for the next attempt. + expect(readFileSync(filePath, "utf-8")).toContain("plain"); + expect(loaded.enterpriseManagedAuth?.idp.clientSecret).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 — 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 39e481c4..8922e4cc 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,50 @@ describe("tightenSecretFilePermissions", () => { expect((await fs.stat(filePath())).mode & 0o777).toBe(0o600); }); + it("declines to interpret Windows' synthetic mode bits", async () => { + // Windows does not model POSIX modes. `stat` still returns bits, and + // reading them as a permission statement is a confident falsehood either + // way — "0666 — not owner-only" about a file the ACL may restrict, or + // "owner-only" about one it does not. The ACL is the real answer and this + // does not inspect it. + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("alpha", "env:A", "1"); + const real = process.platform; + Object.defineProperty(process, "platform", { + value: "win32", + configurable: true, + }); + try { + expect(await readSecretFilePermissions(filePath())).toEqual({ + state: "unknown", + detail: "not checked on Windows", + }); + } finally { + Object.defineProperty(process, "platform", { + value: real, + configurable: true, + }); + } + }); + + it("still reports absent on Windows when there is no file", async () => { + const real = process.platform; + Object.defineProperty(process, "platform", { + value: "win32", + configurable: true, + }); + try { + expect( + await readSecretFilePermissions(path.join(tmpDir, "nope.json")), + ).toEqual({ state: "absent" }); + } finally { + Object.defineProperty(process, "platform", { + value: real, + configurable: true, + }); + } + }); + 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 @@ -700,6 +744,84 @@ async function writeEncryptedFixtureWithPayload( ); } +describe("getMany", () => { + it("returns a server's fields from a single read", async () => { + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + await store.set("srv", SECRET_FIELD_OAUTH_CLIENT_SECRET, "s1"); + await store.set("srv", "env:TOKEN", "s2"); + await store.set("other", "env:TOKEN", "nope"); + + expect( + await store.getMany("srv", [ + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "env:TOKEN", + "env:ABSENT", + ]), + ).toEqual({ [SECRET_FIELD_OAUTH_CLIENT_SECRET]: "s1", "env:TOKEN": "s2" }); + }); + + it("derives the key once for the whole set, not once per field", async () => { + // The reason the seam exists: `get` reads and decrypts the *entire* file, + // so rehydrating field-by-field cost one scrypt derivation per field, + // serialized behind this store's queue. Counting derivations is the + // measurement that matters — scrypt is the expensive part, deliberately + // so. + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + await store.set("srv", "env:A", "1"); + await store.set("srv", "env:B", "2"); + await store.set("srv", "env:C", "3"); + + let derivations = 0; + vi.resetModules(); + vi.doMock("node:crypto", async () => { + const actual = + await vi.importActual("node:crypto"); + return { + ...actual, + default: actual, + scrypt: (...args: Parameters) => { + derivations += 1; + return actual.scrypt(...args); + }, + }; + }); + try { + const mod = + await import("@inspector/core/auth/node/file-secret-store.js"); + const fresh = new mod.FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + + await fresh.getMany("srv", ["env:A", "env:B", "env:C"]); + expect(derivations).toBe(1); + + // And the shape it replaced, for contrast: three fields, three + // derivations. + derivations = 0; + await fresh.get("srv", "env:A"); + await fresh.get("srv", "env:B"); + await fresh.get("srv", "env:C"); + expect(derivations).toBe(3); + } finally { + vi.doUnmock("node:crypto"); + vi.resetModules(); + } + }); + + it("is tolerant like get: an unreadable store yields no fields", async () => { + await fs.writeFile(filePath(), "{ not json", "utf-8"); + const store = new FileSecretStore({ filePath: filePath() }); + expect(await store.getMany("srv", ["env:A"])).toEqual({}); + }); +}); + describe("payload shape validation", () => { // GCM proves who wrote the bytes, not what shape they parse to, and the // plaintext branch proves nothing at all — so neither may be cast. 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 f7a6fe05..4767aab3 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 @@ -545,6 +545,45 @@ describe("defaultSecretFilePath", () => { }); }); +describe("DeferredSecretStore forwards the optional seams", () => { + // This is what `defaultSecretStore()` returns, so a seam it drops is a + // seam that does not exist in production — however well the tests that + // inject a concrete store behave. + it("forwards getStrict rather than degrading to the tolerant get", async () => { + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(false); + const store = mod.defaultSecretStore(); + expect(typeof store.getStrict).toBe("function"); + + const { secretStoreGetStrict } = + await import("@inspector/core/auth/node/secret-store.js"); + // The file store's strict read is its ordinary read; the point here is + // that the call reaches the target at all. + expect(await secretStoreGetStrict(store, "srv", "env:A")).toBe(null); + await store.set("srv", "env:A", "1"); + expect(await secretStoreGetStrict(store, "srv", "env:A")).toBe("1"); + }); + + it("forwards getMany", async () => { + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(false); + const store = mod.defaultSecretStore(); + await store.set("srv", "env:A", "1"); + await store.set("srv", "env:B", "2"); + + const { secretStoreGetMany } = + await import("@inspector/core/auth/node/secret-store.js"); + expect(await secretStoreGetMany(store, "srv", ["env:A", "env:B"])).toEqual({ + "env:A": "1", + "env:B": "2", + }); + }); +}); + describe("absorbFileSecretsIntoKeyring", () => { // The transition: a box without libsecret falls back to a file, the user // saves secrets there, then installs libsecret. Without a hand-off the next diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 427800a6..e6140467 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -991,7 +991,9 @@ describe("server.ts supplemental coverage", () => { const warned = cap.records.filter( (r) => r.level === "warn" && - JSON.stringify(r.args).includes("Keychain unavailable"), + // Wording broadened with the store: a file-backed store fails for + // reasons that have nothing to do with a keychain. + JSON.stringify(r.args).includes("Secret store unavailable"), ); expect(warned.length).toBeGreaterThan(0); } finally { 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 a5f6f013..6062fd7d 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 @@ -2674,6 +2674,37 @@ describe("plaintext migration against a session-scoped store (#1950)", () => { expect(readFileSync(configPath, "utf-8")).toBe(before); }); + it("aborts the migration when the keychain read fails, preserving mcp.json", async () => { + // The keychain-wins lookup used the tolerant `get`, so an unreadable + // keychain read as "nothing there" — and the write that followed would + // put the older plaintext over a newer keychain value before the disk + // copy was stripped. + let wrote = false; + const flaky: SecretStore = { + get: async () => null, + getStrict: async () => { + throw new KeychainUnavailableError(new Error("temporarily down")); + }, + set: async () => { + wrote = true; + }, + delete: async () => {}, + deleteAllForServer: async () => {}, + }; + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: flaky, + }); + const before = readFileSync(configPath, "utf-8"); + + const res = await app.request(new Request("http://test/api/servers")); + expect(res.status).toBe(200); + expect(wrote).toBe(false); + 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. diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index a18f5b26..c4262bff 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -792,6 +792,34 @@ export class FileSecretStore implements SecretStore { return run; } + /** + * One read and one derivation for a whole server's fields. + * + * The reason this exists: `get` reads and decrypts the entire file, and + * the rehydration callers ask field by field — so an encrypted catalog + * paid N scrypt derivations per server, serialized behind this store's own + * queue, every time the server list was loaded. Tolerant like `get`: an + * unreadable store yields no fields rather than throwing. + */ + async getMany( + serverId: string, + fields: string[], + ): Promise> { + let map: Record | null; + try { + map = await this.serialize(() => this.readMap()); + } catch { + return {}; + } + if (!map) return {}; + const out: Record = {}; + for (const field of fields) { + const value = map[buildAccount(serverId, field)]; + if (value !== undefined) out[field] = value; + } + return out; + } + async get(serverId: string, field: string): Promise { try { const map = await this.serialize(() => this.readMap()); @@ -870,6 +898,10 @@ export class FileSecretStore implements SecretStore { /** * Best-effort permission repair for a pre-existing secrets file. * + * A no-op in effect on Windows, where `chmod` cannot express "owner only" + * — {@link readSecretFilePermissions} reports `unknown` there rather than + * interpreting the synthetic mode bits. + * * `writeStoreFile` writes through `atomically`, which creates a temp file * with `mode: 0o600` and renames it over the destination — so every *write* * re-establishes the mode regardless of what the old file had. What it @@ -911,6 +943,17 @@ export async function readSecretFilePermissions( filePath: string, ): Promise { try { + // Windows does not model POSIX modes. `stat` still returns *something* + // — synthetic bits derived from the read-only attribute — and reading + // them as a permission statement produces a confident falsehood either + // way: "0666 — not owner-only" about a file the ACL may well restrict, + // or "owner-only" about one it does not. The real answer lives in the + // ACL, which this does not inspect, so the honest report is that we did + // not check. + if (process.platform === "win32") { + await fs.stat(filePath); // still distinguishes absent from present + return { state: "unknown", detail: "not checked on Windows" }; + } const mode = (await fs.stat(filePath)).mode & 0o777; if (mode === 0o600) return { state: "ok" }; return { state: "loose", mode }; diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 6802a89b..ed05cf3b 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -58,6 +58,7 @@ import { KeyringSecretStore, parseAccount, probeKeyringAvailable, + secretStoreGetMany, secretStoreGetStrict, secretStoreIsDurable, SessionSecretStore, @@ -615,6 +616,21 @@ class DeferredSecretStore implements SecretStore { async isDurable(): Promise { return secretStoreIsDurable(await this.target()); } + /** + * Forwarded, and its absence was a real defect rather than an omission: + * this is the store production actually uses, so a `secretStoreGetStrict` + * call against it fell through to the tolerant `get` and the strictness + * existed only in tests that injected a concrete store. + */ + async getStrict(serverId: string, field: string): Promise { + return secretStoreGetStrict(await this.target(), serverId, field); + } + async getMany( + serverId: string, + fields: string[], + ): Promise> { + return secretStoreGetMany(await this.target(), serverId, fields); + } 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 b59b4e34..3ccf1c73 100644 --- a/core/auth/node/secret-store.ts +++ b/core/auth/node/secret-store.ts @@ -337,6 +337,20 @@ export interface SecretStore { * absent, which is correct for stores whose reads cannot fail. */ getStrict?(serverId: string, field: string): Promise; + /** + * Read several of one server's fields in a single pass. + * + * Optional, and purely a performance seam: {@link secretStoreGetMany} + * falls back to a parallel `get` per field, which is what the keychain + * wants anyway (independent native round-trips). + * + * It exists for `FileSecretStore`, where the shape is inverted — every + * `get` reads and decrypts the *whole* file, so N fields cost N scrypt + * derivations, serialized behind the store's own queue. The rehydration + * callers ask for one server's fields at a time, so a per-server bulk read + * turns that back into one. + */ + getMany?(serverId: string, fields: string[]): Promise>; set(serverId: string, field: string, value: string): Promise; /** No-op if no entry exists. */ delete(serverId: string, field: string): Promise; @@ -514,6 +528,29 @@ export async function secretStoreGetStrict( : store.get(serverId, field); } +/** + * Read several fields of one server, using the store's bulk path when it has + * one. Returns only the fields that are present, matching the callers that + * merge the result into a stored config. + */ +export async function secretStoreGetMany( + store: SecretStore, + serverId: string, + fields: string[], +): Promise> { + if (store.getMany) return store.getMany(serverId, fields); + const entries = await Promise.all( + fields.map( + async (field) => [field, await store.get(serverId, field)] as const, + ), + ); + const out: Record = {}; + for (const [field, value] of entries) { + if (value !== null) out[field] = value; + } + return out; +} + export async function secretStoreIsDurable( store: SecretStore, ): Promise { diff --git a/core/client/node-persistence.ts b/core/client/node-persistence.ts index f1e83fcb..6dbaf0cc 100644 --- a/core/client/node-persistence.ts +++ b/core/client/node-persistence.ts @@ -3,6 +3,7 @@ */ import { + secretStoreGetStrict, secretStoreIsDurable, SecretStoreUnavailableError, type SecretStore, @@ -45,7 +46,13 @@ async function migrateClientPlaintextSecret( if (!value) return config; try { - const existing = await secretStore.get( + // Strict: `get` answers `null` for an unreadable store as well as a + // missing entry, and the branch below *writes* on `null` — so a + // transient failure would overwrite a newer stored secret with the older + // `client.json` copy, inverting keychain-wins. A throw is caught below + // and leaves the plaintext file untouched for the next attempt. + const existing = await secretStoreGetStrict( + secretStore, CLIENT_KEYCHAIN_ID, SECRET_FIELD_IDP_CLIENT_SECRET, ); diff --git a/core/mcp/node/server-secrets.ts b/core/mcp/node/server-secrets.ts index aae0bd14..a35e558c 100644 --- a/core/mcp/node/server-secrets.ts +++ b/core/mcp/node/server-secrets.ts @@ -1,4 +1,7 @@ -import type { SecretStore } from "../../auth/node/secret-store.js"; +import { + secretStoreGetMany, + type SecretStore, +} from "../../auth/node/secret-store.js"; import { expectedSecretFields, mergeSecretsIntoStored } from "../serverList.js"; import type { MCPConfig } from "../types.js"; @@ -15,15 +18,10 @@ export async function rehydrateMcpConfigFromKeychain( await Promise.all( Object.entries(config.mcpServers).map(async ([id, stored]) => { const fields = expectedSecretFields(stored); - const secrets: Record = {}; - await Promise.all( - fields.map(async (field) => { - const value = await secretStore.get(id, field); - if (value !== null) { - secrets[field] = value; - } - }), - ); + // One pass per server, not one per field. For the keychain this is the + // same parallel round-trips as before; for the file store it collapses + // N reads-and-decrypts into one (see `secretStoreGetMany`). + const secrets = await secretStoreGetMany(secretStore, id, fields); out.mcpServers[id] = mergeSecretsIntoStored(stored, secrets); }), ); diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 90241927..cf2f58cf 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -65,6 +65,8 @@ import { RemoteSession } from "./remote-session.js"; import { createRemoteAuthProvider } from "./tokenAuthProvider.js"; import { API_SERVER_ENV_VARS } from "../constants.js"; import { + secretStoreGetMany, + secretStoreGetStrict, secretStoreIsDurable, SecretStoreUnavailableError, type SecretStore, @@ -1899,16 +1901,11 @@ export function createRemoteApp( id: string, fields: string[], ): Promise> => { - const values = await Promise.all( - fields.map( - async (field) => [field, await secretStore.get(id, field)] as const, - ), - ); - const out: Record = {}; - for (const [field, v] of values) { - if (v !== null) out[field] = v; - } - return out; + // One pass per server rather than one per field — see + // `secretStoreGetMany`. Unchanged for the keychain (still independent + // parallel round-trips); for the file store it is the difference between + // one decryption and one per field. + return secretStoreGetMany(secretStore, id, fields); }; // Cheap structural check used by the GET handler to decide whether @@ -1954,7 +1951,13 @@ export function createRemoteApp( continue; } for (const [field, value] of Object.entries(secrets)) { - const existing = await secretStore.get(id, field); + // Strict: `get` maps an unreadable store to `null`, and this + // branch *writes* on `null` — so a transient keychain read + // failure would let the older plaintext value overwrite a newer + // keychain one, and the disk copy would then be stripped. A throw + // is a `SecretStoreUnavailableError`, which the catch below turns + // into "abandon the migration and keep mcp.json as it is". + const existing = await secretStoreGetStrict(secretStore, id, field); if (existing === null) { await secretStore.set(id, field, value); } @@ -1982,7 +1985,7 @@ export function createRemoteApp( if (fileLogger) { fileLogger.warn( { err: err.message }, - "Keychain unavailable; skipping plaintext-secret migration on this read. Existing mcp.json plaintext values are preserved.", + "Secret store unavailable; skipping plaintext-secret migration on this read. Existing mcp.json plaintext values are preserved.", ); } // Partial-migration semantics: if `set` threw partway through