diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index 26ebbe66a..73bde734a 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -16,6 +16,7 @@ "@mantine/notifications": "^8.3.17", "@modelcontextprotocol/ext-apps": "^1.7.1", "@modelcontextprotocol/sdk": "^1.29.0", + "@napi-rs/keyring": "^1.3.0", "ajv": "^8.17.1", "atomically": "^2.1.1", "chokidar": "^4.0.3", @@ -1442,6 +1443,240 @@ } } }, + "node_modules/@napi-rs/keyring": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring/-/keyring-1.3.0.tgz", + "integrity": "sha512-WrOw/bcXm0f9qHkumlT1QlArXSTWqaY9sunsDpOk+yCCorCKMxvWT/a3xko4EYHVdeZoh00yI2TydXn6eyICDA==", + "license": "MIT", + "engines": { + "node": ">= 10" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/Brooooooklyn" + }, + "optionalDependencies": { + "@napi-rs/keyring-darwin-arm64": "1.3.0", + "@napi-rs/keyring-darwin-x64": "1.3.0", + "@napi-rs/keyring-freebsd-x64": "1.3.0", + "@napi-rs/keyring-linux-arm-gnueabihf": "1.3.0", + "@napi-rs/keyring-linux-arm64-gnu": "1.3.0", + "@napi-rs/keyring-linux-arm64-musl": "1.3.0", + "@napi-rs/keyring-linux-riscv64-gnu": "1.3.0", + "@napi-rs/keyring-linux-x64-gnu": "1.3.0", + "@napi-rs/keyring-linux-x64-musl": "1.3.0", + "@napi-rs/keyring-win32-arm64-msvc": "1.3.0", + "@napi-rs/keyring-win32-ia32-msvc": "1.3.0", + "@napi-rs/keyring-win32-x64-msvc": "1.3.0" + } + }, + "node_modules/@napi-rs/keyring-darwin-arm64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-darwin-arm64/-/keyring-darwin-arm64-1.3.0.tgz", + "integrity": "sha512-pl76hJvdYUBn6I24bXiOBMA9nbDapo3I5B+f3OorjDU4dUMSypXeKbOVehJe8fhgTiH24flMyTS3aAIy43xegQ==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-darwin-x64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-darwin-x64/-/keyring-darwin-x64-1.3.0.tgz", + "integrity": "sha512-YcJtEV5LA3cvA4z3BurgxH5IhTsW1JfIvcAAcqcecwk06Si9F9NqkxbZVIfDwQ8oRHgaBmT3zZJnLAotCrVahw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-freebsd-x64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-freebsd-x64/-/keyring-freebsd-x64-1.3.0.tgz", + "integrity": "sha512-vlLf31TGhfRAaxLDBhg8b89ss0HHD/lyNmL5F3UjSaz5CUXElsJmKYq9fqA/B+cZKUEUcLHHGhF0I/CqcFdaVw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "freebsd" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm-gnueabihf": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm-gnueabihf/-/keyring-linux-arm-gnueabihf-1.3.0.tgz", + "integrity": "sha512-KiWdMMu/Inz/bHHIAGrnF7r54FZDYXuHO6UFF/rhIrshUsxbMG1Rl9lEymNtqqsVo927G0VYcb02FzWQ3iBQRQ==", + "cpu": [ + "arm" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm64-gnu/-/keyring-linux-arm64-gnu-1.3.0.tgz", + "integrity": "sha512-eyKGpY40lm9Jvs1aD294XRH4y7+TlJM0YVAryZeXA6TX0mb4gMkxVXwSQv7MCwgah7raeUd0dKUb4BPAYIgcMg==", + "cpu": [ + "arm64" + ], + "libc": [ + "glibc" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm64-musl": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm64-musl/-/keyring-linux-arm64-musl-1.3.0.tgz", + "integrity": "sha512-iIK6JWHXAJqDrEyLY3TmswwloVyt2vj+04TZnew+uSJ9gnDO8EwRbp3/iw3LpWaXiDO7VomGO6y8I0Id8uBZSw==", + "cpu": [ + "arm64" + ], + "libc": [ + "musl" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-riscv64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-riscv64-gnu/-/keyring-linux-riscv64-gnu-1.3.0.tgz", + "integrity": "sha512-/PGqrwn6EwgtK6vccASSXJRfOSP4vN1F4ASsIQ+7MdrK6hNvAJ1FZPrIuD5gGGdxezo3F++To2Wq7DbuGIeuNQ==", + "cpu": [ + "riscv64" + ], + "libc": [ + "glibc" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-x64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-x64-gnu/-/keyring-linux-x64-gnu-1.3.0.tgz", + "integrity": "sha512-2PDK1WKWTu9lBGq9VvNEkSlQD3O7YwVpmnyN2M3cy4v7NJ/8gDMd9GXv3G+FVXN13uhp4gnnPBS+ScefmEeD2A==", + "cpu": [ + "x64" + ], + "libc": [ + "glibc" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-x64-musl": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-x64-musl/-/keyring-linux-x64-musl-1.3.0.tgz", + "integrity": "sha512-oJ2HkX8YUo46QBkn0pG+HuIKQNqr523q6vBobCn+P95s4C4K6/kLBqHY/1bg5J4ap31DzsznhnFKcfBNBsjCnw==", + "cpu": [ + "x64" + ], + "libc": [ + "musl" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-arm64-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-arm64-msvc/-/keyring-win32-arm64-msvc-1.3.0.tgz", + "integrity": "sha512-tOd3c/uAaeoE4ycVlmAdSvygz0Zt3zdca6Y7gokBeIbaRDWpjDIUOpU3MvML59XAaqyuKGsVVu0F/DZb1lHPmw==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-ia32-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-ia32-msvc/-/keyring-win32-ia32-msvc-1.3.0.tgz", + "integrity": "sha512-sPSqeAFZMGqP1R++M2JTza7GQJJ/TpCo6JU6Vcd4jnebvOaEDs9b7eipakU1PJdSvhpC2yXMCNRk9gXfrhuwHQ==", + "cpu": [ + "ia32" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-x64-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-x64-msvc/-/keyring-win32-x64-msvc-1.3.0.tgz", + "integrity": "sha512-4DnCWXwDc0HRKwyRlG5y0VhKZW2tNRQfKKfyj6IX/KWfDNyq9hn4n+GL1auyDcOO/v8PwnhmYo2+rOOqCkvvOg==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, "node_modules/@napi-rs/wasm-runtime": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/@napi-rs/wasm-runtime/-/wasm-runtime-1.1.1.tgz", diff --git a/clients/web/package.json b/clients/web/package.json index 7fbdc3aa7..98d75583a 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -31,6 +31,7 @@ "@mantine/notifications": "^8.3.17", "@modelcontextprotocol/ext-apps": "^1.7.1", "@modelcontextprotocol/sdk": "^1.29.0", + "@napi-rs/keyring": "^1.3.0", "ajv": "^8.17.1", "atomically": "^2.1.1", "chokidar": "^4.0.3", diff --git a/clients/web/server/vite-base-config.ts b/clients/web/server/vite-base-config.ts index 1d8a7d082..5596fe4b0 100644 --- a/clients/web/server/vite-base-config.ts +++ b/clients/web/server/vite-base-config.ts @@ -30,6 +30,12 @@ export function getViteBaseConfig() { "chokidar", "cross-spawn", "which", + // `@napi-rs/keyring` is loaded only inside + // `core/auth/node/secret-store.ts` from the Hono `/api/servers` + // handlers. It's a native-binding package (no browser code path) so + // excluding it keeps Vite's dep scanner from chasing into the + // platform-specific binaries during dev startup. + "@napi-rs/keyring", ], }, }; diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index a4bfcdaad..6220d7d5e 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -1,12 +1,23 @@ import { describe, it, expect } from "vitest"; import { DEFAULT_SEED_CONFIG, + expectedSecretFields, + extractSecretsFromStored, mcpConfigToServerEntries, + mergeSecretsIntoStored, normalizeServerType, serverEntriesToMcpConfig, serializeMcpConfig, } from "@inspector/core/mcp/serverList.js"; -import type { MCPConfig, ServerEntry } from "@inspector/core/mcp/types.js"; +import { + SECRET_FIELD_OAUTH_CLIENT_SECRET, + envSecretField, +} from "@inspector/core/auth/secret-fields.js"; +import type { + MCPConfig, + ServerEntry, + StoredMCPServer, +} from "@inspector/core/mcp/types.js"; describe("normalizeServerType", () => { it("defaults missing type to stdio", () => { @@ -378,3 +389,173 @@ describe("DEFAULT_SEED_CONFIG", () => { } }); }); + +describe("extractSecretsFromStored", () => { + it("lifts oauth.clientSecret into the secrets record and drops it from the stripped shape", () => { + const stored: StoredMCPServer = { + type: "streamable-http", + url: "https://x.test", + oauth: { clientId: "cid", clientSecret: "shh", scopes: "read" }, + }; + const { stripped, secrets } = extractSecretsFromStored(stored); + expect(secrets).toEqual({ [SECRET_FIELD_OAUTH_CLIENT_SECRET]: "shh" }); + expect(stripped).toEqual({ + type: "streamable-http", + url: "https://x.test", + oauth: { clientId: "cid", scopes: "read" }, + }); + }); + + it("removes the oauth block entirely when clientSecret was its only property", () => { + const stored: StoredMCPServer = { + type: "streamable-http", + url: "https://x.test", + oauth: { clientSecret: "shh" }, + }; + const { stripped, secrets } = extractSecretsFromStored(stored); + expect(secrets).toEqual({ [SECRET_FIELD_OAUTH_CLIENT_SECRET]: "shh" }); + expect(stripped).not.toHaveProperty("oauth"); + }); + + it("clears stdio env values into the keychain map but preserves the keys on disk", () => { + const stored: StoredMCPServer = { + type: "stdio", + command: "node", + env: { API_KEY: "secret-1", DB_PASS: "secret-2", DEBUG: "" }, + }; + const { stripped, secrets } = extractSecretsFromStored(stored); + expect(secrets).toEqual({ + [envSecretField("API_KEY")]: "secret-1", + [envSecretField("DB_PASS")]: "secret-2", + }); + // Empty values aren't written to the secrets record but the key stays. + if (stripped.type === "stdio") { + expect(stripped.env).toEqual({ API_KEY: "", DB_PASS: "", DEBUG: "" }); + } else { + throw new Error("expected stdio"); + } + }); + + it("treats type-undefined entries as stdio for env handling", () => { + const stored = { + command: "node", + env: { API_KEY: "v" }, + } as unknown as StoredMCPServer; + const { stripped, secrets } = extractSecretsFromStored(stored); + expect(secrets).toEqual({ [envSecretField("API_KEY")]: "v" }); + expect((stripped as { env?: Record }).env).toEqual({ + API_KEY: "", + }); + }); + + it("is a no-op for entries with no secrets", () => { + const stored: StoredMCPServer = { + type: "stdio", + command: "node", + }; + const { stripped, secrets } = extractSecretsFromStored(stored); + expect(secrets).toEqual({}); + expect(stripped).toEqual(stored); + }); + + it("does not mutate the input", () => { + const stored: StoredMCPServer = { + type: "stdio", + command: "node", + env: { K: "v" }, + oauth: { clientSecret: "shh" }, + }; + const snapshot = JSON.parse(JSON.stringify(stored)); + extractSecretsFromStored(stored); + expect(stored).toEqual(snapshot); + }); +}); + +describe("mergeSecretsIntoStored", () => { + it("inverses extractSecretsFromStored for OAuth", () => { + const stored: StoredMCPServer = { + type: "streamable-http", + url: "https://x.test", + oauth: { clientId: "cid" }, + }; + const merged = mergeSecretsIntoStored(stored, { + [SECRET_FIELD_OAUTH_CLIENT_SECRET]: "shh", + }); + expect(merged.oauth).toEqual({ clientId: "cid", clientSecret: "shh" }); + }); + + it("inverses extractSecretsFromStored for stdio env values", () => { + const stored: StoredMCPServer = { + type: "stdio", + command: "node", + env: { K: "" }, + }; + const merged = mergeSecretsIntoStored(stored, { + [envSecretField("K")]: "real", + }); + if (merged.type === "stdio") { + expect(merged.env).toEqual({ K: "real" }); + } else { + throw new Error("expected stdio"); + } + }); + + it("round-trips extract then merge identically", () => { + const stored: StoredMCPServer = { + type: "stdio", + command: "node", + env: { API_KEY: "secret-1", DEBUG: "" }, + oauth: { clientId: "cid", clientSecret: "shh" }, + }; + const { stripped, secrets } = extractSecretsFromStored(stored); + expect(mergeSecretsIntoStored(stripped, secrets)).toEqual(stored); + }); + + it("leaves env keys not present in the secrets map alone", () => { + const stored: StoredMCPServer = { + type: "stdio", + command: "node", + env: { ANSWERED: "", UNANSWERED: "" }, + }; + const merged = mergeSecretsIntoStored(stored, { + [envSecretField("ANSWERED")]: "v", + }); + if (merged.type === "stdio") { + expect(merged.env).toEqual({ ANSWERED: "v", UNANSWERED: "" }); + } else { + throw new Error("expected stdio"); + } + }); +}); + +describe("expectedSecretFields", () => { + it("always lists the OAuth slot first", () => { + const fields = expectedSecretFields({ + type: "streamable-http", + url: "https://x.test", + }); + expect(fields[0]).toBe(SECRET_FIELD_OAUTH_CLIENT_SECRET); + }); + + it("includes one entry per stdio env key", () => { + const fields = expectedSecretFields({ + type: "stdio", + command: "node", + env: { A: "", B: "" }, + }); + expect(fields).toEqual([ + SECRET_FIELD_OAUTH_CLIENT_SECRET, + envSecretField("A"), + envSecretField("B"), + ]); + }); + + it("returns only the OAuth slot when env is absent", () => { + expect( + expectedSecretFields({ + type: "stdio", + command: "node", + }), + ).toEqual([SECRET_FIELD_OAUTH_CLIENT_SECRET]); + }); +}); diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index 7ff3f2fe8..9e2adc881 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -13,6 +13,7 @@ import { join } from "node:path"; import { useServers } from "@inspector/core/react/useServers"; import { createRemoteApp } from "@inspector/core/mcp/remote/node/server"; import { DEFAULT_SEED_CONFIG } from "@inspector/core/mcp/serverList"; +import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store"; import type { MCPConfig } from "@inspector/core/mcp/types"; interface Harness { @@ -29,6 +30,10 @@ function setupHarness(): Harness { dangerouslyOmitAuth: true, mcpConfigPath: configPath, initialConfig: { defaultEnvironment: {} }, + // Inject an in-memory secret store so the test never touches the + // developer's real OS keychain (and so the suite passes on Linux CI + // runners without libsecret). + secretStore: new InMemorySecretStore(), }); // Wrap so the typing matches Web fetch — Hono's app.fetch expects a Request // (not a URL string), so build one from string/URL inputs before dispatch. diff --git a/clients/web/src/test/integration/auth/node/secret-store.test.ts b/clients/web/src/test/integration/auth/node/secret-store.test.ts new file mode 100644 index 000000000..345bd1a7f --- /dev/null +++ b/clients/web/src/test/integration/auth/node/secret-store.test.ts @@ -0,0 +1,325 @@ +/** + * Contract tests for the secret-store abstraction. + * + * `InMemorySecretStore` is exercised directly. `KeyringSecretStore` is + * exercised via a `vi.mock` of `@napi-rs/keyring` — the native bindings + * aren't reliably present in CI (Linux runners ship without libsecret), + * so the suite stubs the native side and asserts the tolerance contract + * (`get` returns null on failure, destructive ops no-op, `set` is the + * one operation that hard-fails with `KeychainUnavailableError`). + */ +import { describe, it, expect, beforeEach, vi } from "vitest"; + +// The mock must be hoisted above the `await import` of secret-store +// inside the `KeyringSecretStore` describe block. Use `vi.hoisted` so +// references are captured before the import is evaluated. +const keyringMocks = vi.hoisted(() => { + const password = new Map(); + const reset = () => password.clear(); + // Behavior hooks each test can flip to simulate keychain unavailability + // on specific operations. Defaults: real-ish in-memory behavior. + const failures = { + getThrows: false, + setThrows: false, + deleteThrows: false, + findThrows: false, + deleteThrowsNoEntry: false, + }; + const credentials = (): Array<{ account: string; password: string }> => { + const out: Array<{ account: string; password: string }> = []; + for (const [k, v] of password.entries()) { + if (v !== null) out.push({ account: k, password: v }); + } + return out; + }; + class AsyncEntry { + private readonly key: string; + constructor(_service: string, username: string) { + this.key = username; + } + async getPassword(): Promise { + if (failures.getThrows) throw new Error("keychain get unavailable"); + const v = password.get(this.key); + return v === undefined || v === null ? undefined : v; + } + async setPassword(value: string): Promise { + if (failures.setThrows) throw new Error("keychain set unavailable"); + password.set(this.key, value); + } + async deleteCredential(): Promise { + if (failures.deleteThrowsNoEntry) throw new Error("No entry found"); + if (failures.deleteThrows) throw new Error("keychain delete unavailable"); + return password.delete(this.key); + } + } + const findCredentialsAsync = async (): Promise< + Array<{ account: string; password: string }> + > => { + if (failures.findThrows) throw new Error("keychain find unavailable"); + return credentials(); + }; + return { AsyncEntry, findCredentialsAsync, failures, password, reset }; +}); + +vi.mock("@napi-rs/keyring", () => ({ + AsyncEntry: keyringMocks.AsyncEntry, + findCredentialsAsync: keyringMocks.findCredentialsAsync, +})); + +import { + InMemorySecretStore, + KeyringSecretStore, + KeychainUnavailableError, + SECRET_FIELD_OAUTH_CLIENT_SECRET, + envSecretField, + parseAccount, + type SecretStore, +} from "@inspector/core/auth/node/secret-store.js"; + +describe("InMemorySecretStore", () => { + let store: SecretStore; + + beforeEach(() => { + store = new InMemorySecretStore(); + }); + + it("returns null for a missing entry", async () => { + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + }); + + it("round-trips a value set then get", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "shh", + ); + }); + + it("treats different server ids as separate namespaces", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "alpha-secret"); + await store.set("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET, "beta-secret"); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "alpha-secret", + ); + expect(await store.get("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "beta-secret", + ); + }); + + it("treats different fields under the same server id as separate entries", async () => { + await store.set("alpha", envSecretField("API_KEY"), "k1"); + await store.set("alpha", envSecretField("DB_PASS"), "k2"); + expect(await store.get("alpha", envSecretField("API_KEY"))).toBe("k1"); + expect(await store.get("alpha", envSecretField("DB_PASS"))).toBe("k2"); + }); + + it("overwrites an existing entry on set", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v1"); + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v2"); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "v2", + ); + }); + + it("delete is a no-op for a missing entry", async () => { + await store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET); + // No throw, no state change. + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + }); + + it("delete removes only the targeted (id, field)", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); + await store.set("alpha", envSecretField("KEY"), "b"); + await store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + expect(await store.get("alpha", envSecretField("KEY"))).toBe("b"); + }); + + it("deleteAllForServer removes every field under that id", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); + await store.set("alpha", envSecretField("KEY1"), "b"); + await store.set("alpha", envSecretField("KEY2"), "c"); + await store.set("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET, "untouched"); + + await store.deleteAllForServer("alpha"); + + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + expect(await store.get("alpha", envSecretField("KEY1"))).toBe(null); + expect(await store.get("alpha", envSecretField("KEY2"))).toBe(null); + expect(await store.get("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "untouched", + ); + }); + + it("deleteAllForServer does not delete entries on a different id that happens to share a prefix", async () => { + // The account scheme is `${serverId}:${field}` — a literal prefix match + // would incorrectly sweep "alpha-prime" entries when deleting "alpha". + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); + await store.set("alpha-prime", SECRET_FIELD_OAUTH_CLIENT_SECRET, "p"); + + await store.deleteAllForServer("alpha"); + + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + expect( + await store.get("alpha-prime", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("p"); + }); + + it("round-trips an empty-string value (set + get returns '')", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, ""); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(""); + }); +}); + +describe("parseAccount", () => { + it("splits `${serverId}:${field}` on the first colon", () => { + expect(parseAccount("srv:oauth-client-secret")).toEqual({ + serverId: "srv", + field: "oauth-client-secret", + }); + }); + + it("allows the field to contain colons (env:KEY uses one)", () => { + expect(parseAccount("srv:env:API_KEY")).toEqual({ + serverId: "srv", + field: "env:API_KEY", + }); + }); + + it("returns null when no separator is present", () => { + expect(parseAccount("noseparator")).toBe(null); + }); + + it("returns null for a leading or trailing colon (empty side)", () => { + expect(parseAccount(":field")).toBe(null); + expect(parseAccount("srv:")).toBe(null); + }); +}); + +describe("KeyringSecretStore (mocked native bindings)", () => { + let store: KeyringSecretStore; + + beforeEach(() => { + keyringMocks.reset(); + keyringMocks.failures.getThrows = false; + keyringMocks.failures.setThrows = false; + keyringMocks.failures.deleteThrows = false; + keyringMocks.failures.findThrows = false; + keyringMocks.failures.deleteThrowsNoEntry = false; + store = new KeyringSecretStore(); + }); + + it("round-trips a set then get", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "shh", + ); + }); + + it("get returns null when getPassword throws (keychain unavailable)", async () => { + // get is tolerant: there's no value to surface so degrading to "null" + // matches the absence semantic the caller already handles. + keyringMocks.failures.getThrows = true; + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + }); + + it("get returns null when the underlying entry is absent (no value set)", async () => { + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + }); + + it("set throws KeychainUnavailableError when setPassword throws", async () => { + // set is the one operation that hard-fails — losing data silently + // is worse than surfacing a clear error the user can act on. + keyringMocks.failures.setThrows = true; + await expect( + store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"), + ).rejects.toBeInstanceOf(KeychainUnavailableError); + }); + + it("delete silently treats a 'no entry' error as success", async () => { + keyringMocks.failures.deleteThrowsNoEntry = true; + await expect( + store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).resolves.toBeUndefined(); + }); + + it("delete silently no-ops when the keychain is unavailable", async () => { + keyringMocks.failures.deleteThrows = true; + await expect( + store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).resolves.toBeUndefined(); + }); + + it("delete actually removes the value when the keychain is available", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"); + await store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + }); + + it("deleteAllForServer no-ops when findCredentialsAsync throws", async () => { + // We don't even know what was written, so there's nothing to sweep. + // Critically, this must not throw — the route's defensive sweep on + // POST and DELETE depends on it. + keyringMocks.failures.findThrows = true; + await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); + }); + + it("deleteAllForServer removes every entry under the given id", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); + await store.set("alpha", envSecretField("K"), "b"); + await store.set("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET, "untouched"); + + await store.deleteAllForServer("alpha"); + + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + expect(await store.get("alpha", envSecretField("K"))).toBe(null); + expect(await store.get("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "untouched", + ); + }); + + it("deleteAllForServer ignores entries on a different id that share a prefix", async () => { + // The `parseAccount` check guards against a literal startsWith match + // wrongly sweeping `alpha-prime:...` when deleting `alpha`. + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); + await store.set("alpha-prime", SECRET_FIELD_OAUTH_CLIENT_SECRET, "p"); + + await store.deleteAllForServer("alpha"); + + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + expect( + await store.get("alpha-prime", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("p"); + }); + + it("KeychainUnavailableError carries the underlying error message", async () => { + keyringMocks.failures.setThrows = true; + try { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"); + throw new Error("expected throw"); + } catch (err) { + expect(err).toBeInstanceOf(KeychainUnavailableError); + expect((err as Error).message).toMatch(/keychain set unavailable/); + expect((err as Error).message).toMatch(/libsecret/); + } + }); +}); diff --git a/clients/web/src/test/integration/mcp/remote/servers-events.test.ts b/clients/web/src/test/integration/mcp/remote/servers-events.test.ts index ed27b452d..28e5d16d9 100644 --- a/clients/web/src/test/integration/mcp/remote/servers-events.test.ts +++ b/clients/web/src/test/integration/mcp/remote/servers-events.test.ts @@ -27,6 +27,7 @@ import { join } from "node:path"; import { serve } from "@hono/node-server"; import type { ServerType } from "@hono/node-server"; import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; +import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store.js"; interface Harness { baseUrl: string; @@ -43,6 +44,10 @@ async function setup(): Promise { dangerouslyOmitAuth: true, mcpConfigPath: configPath, initialConfig: { defaultEnvironment: {} }, + // Inject an in-memory secret store so the suite passes on Linux CI + // runners without libsecret (and so a developer running locally + // doesn't pollute their real OS keychain with test entries). + secretStore: new InMemorySecretStore(), }); const { baseUrl, server } = await new Promise<{ baseUrl: string; 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 dd5397ca3..734b2e799 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 @@ -18,6 +18,13 @@ import { serve } from "@hono/node-server"; import type { ServerType } from "@hono/node-server"; import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js"; import { DEFAULT_SEED_CONFIG } from "@inspector/core/mcp/serverList.js"; +import { + InMemorySecretStore, + KeychainUnavailableError, + SECRET_FIELD_OAUTH_CLIENT_SECRET, + envSecretField, + type SecretStore, +} from "@inspector/core/auth/node/secret-store.js"; import type { MCPConfig } from "@inspector/core/mcp/types.js"; interface Harness { @@ -25,9 +32,13 @@ interface Harness { server: ServerType; configPath: string; tempDir: string; + secretStore: InMemorySecretStore; } -async function startServer(configPath: string): Promise<{ +async function startServer( + configPath: string, + secretStore: InMemorySecretStore, +): Promise<{ baseUrl: string; server: ServerType; }> { @@ -35,6 +46,7 @@ async function startServer(configPath: string): Promise<{ dangerouslyOmitAuth: true, mcpConfigPath: configPath, initialConfig: { defaultEnvironment: {} }, + secretStore, }); return new Promise((resolve, reject) => { const server = serve( @@ -54,8 +66,9 @@ async function startServer(configPath: string): Promise<{ async function setup(): Promise { const tempDir = mkdtempSync(join(tmpdir(), "inspector-servers-route-")); const configPath = join(tempDir, "mcp.json"); - const { baseUrl, server } = await startServer(configPath); - return { baseUrl, server, configPath, tempDir }; + const secretStore = new InMemorySecretStore(); + const { baseUrl, server } = await startServer(configPath, secretStore); + return { baseUrl, server, configPath, tempDir, secretStore }; } async function teardown(h: Harness): Promise { @@ -965,4 +978,572 @@ describe("/api/servers routes", () => { expect(res.status).toBe(400); }); }); + + describe("keychain secrets (#1356)", () => { + it("POST writes oauthClientSecret to the keychain, not the disk file", async () => { + const res = await fetch(`${h.baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "oauth-srv", + config: { type: "streamable-http", url: "https://x.test/mcp" }, + settings: { + headers: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + oauthClientId: "cid", + oauthClientSecret: "very-secret", + oauthScopes: "read", + }, + }), + }); + expect(res.status).toBe(200); + + // Disk: keeps clientId/scopes, no clientSecret. + const stored = readConfig(h.configPath).mcpServers["oauth-srv"] as { + oauth?: Record; + }; + expect(stored.oauth).toEqual({ clientId: "cid", scopes: "read" }); + // The raw file text must not contain the secret value either — guards + // against the secret accidentally landing in a different field. + const raw = readFileSync(h.configPath, "utf-8"); + expect(raw).not.toContain("very-secret"); + + // Keychain: holds the secret under the expected (id, field) tuple. + expect( + await h.secretStore.get("oauth-srv", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("very-secret"); + }); + + it("POST writes stdio env values to the keychain and leaves empty placeholders on disk", async () => { + const res = await fetch(`${h.baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "stdio-srv", + config: { + type: "stdio", + command: "node", + env: { API_KEY: "abc-123", DEBUG: "" }, + }, + }), + }); + expect(res.status).toBe(200); + + const stored = readConfig(h.configPath).mcpServers["stdio-srv"] as { + env?: Record; + }; + // Keys preserved, values stripped to "". + expect(stored.env).toEqual({ API_KEY: "", DEBUG: "" }); + expect(readFileSync(h.configPath, "utf-8")).not.toContain("abc-123"); + + // Keychain has the non-empty value; empty values aren't written. + expect( + await h.secretStore.get("stdio-srv", envSecretField("API_KEY")), + ).toBe("abc-123"); + expect( + await h.secretStore.get("stdio-srv", envSecretField("DEBUG")), + ).toBe(null); + }); + + it("GET rehydrates secrets from the keychain so the wire shape is unchanged", async () => { + // Set up disk + keychain by hand to simulate what POST would have done. + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + "hydrate-srv": { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { clientId: "cid" }, + }, + }, + }), + ); + await h.secretStore.set( + "hydrate-srv", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "very-secret", + ); + + const res = await fetch(`${h.baseUrl}/api/servers`); + expect(res.status).toBe(200); + const body = (await res.json()) as MCPConfig; + const entry = body.mcpServers["hydrate-srv"] as { + oauth?: Record; + }; + expect(entry.oauth).toEqual({ + clientId: "cid", + clientSecret: "very-secret", + }); + }); + + it("PUT in place reconciles obsolete env keys (removed key drops from keychain)", async () => { + // Pre-seed: server with two env keys, both backed by keychain values. + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + srv: { + type: "stdio", + command: "node", + env: { A: "", B: "" }, + }, + }, + }), + ); + await h.secretStore.set("srv", envSecretField("A"), "value-A"); + await h.secretStore.set("srv", envSecretField("B"), "value-B"); + + // PUT keeps only A. + const res = await fetch(`${h.baseUrl}/api/servers/srv`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + config: { + type: "stdio", + command: "node", + env: { A: "value-A-updated" }, + }, + }), + }); + expect(res.status).toBe(200); + + // Disk: only A's key remains, value stripped to "". + const stored = readConfig(h.configPath).mcpServers.srv as { + env?: Record; + }; + expect(stored.env).toEqual({ A: "" }); + + // Keychain: A updated, B swept. + expect(await h.secretStore.get("srv", envSecretField("A"))).toBe( + "value-A-updated", + ); + expect(await h.secretStore.get("srv", envSecretField("B"))).toBe(null); + }); + + it("PUT clearing oauthClientSecret deletes the keychain entry", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + srv: { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { clientId: "cid" }, + }, + }, + }), + ); + await h.secretStore.set( + "srv", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "old-secret", + ); + + // PUT with settings.oauthClientSecret unset (user cleared the field). + const res = await fetch(`${h.baseUrl}/api/servers/srv`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + settings: { + headers: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + oauthClientId: "cid", + }, + }), + }); + expect(res.status).toBe(200); + expect( + await h.secretStore.get("srv", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe(null); + }); + + it("PUT rename moves keychain entries from the old id to the new id", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + "old-name": { + type: "stdio", + command: "node", + env: { K: "" }, + }, + }, + }), + ); + await h.secretStore.set("old-name", envSecretField("K"), "v"); + + const res = await fetch(`${h.baseUrl}/api/servers/old-name`, { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "new-name", + config: { type: "stdio", command: "node", env: { K: "v" } }, + }), + }); + expect(res.status).toBe(200); + + expect(await h.secretStore.get("old-name", envSecretField("K"))).toBe( + null, + ); + expect(await h.secretStore.get("new-name", envSecretField("K"))).toBe( + "v", + ); + }); + + it("DELETE sweeps every keychain entry for the deleted server", async () => { + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + srv: { + type: "stdio", + command: "node", + env: { K1: "", K2: "" }, + oauth: { clientId: "cid" }, + }, + untouched: { type: "stdio", command: "node" }, + }, + }), + ); + await h.secretStore.set("srv", envSecretField("K1"), "v1"); + await h.secretStore.set("srv", envSecretField("K2"), "v2"); + await h.secretStore.set( + "srv", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "secret", + ); + await h.secretStore.set( + "untouched", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "untouched-secret", + ); + + const res = await fetch(`${h.baseUrl}/api/servers/srv`, { + method: "DELETE", + }); + expect(res.status).toBe(200); + + expect(await h.secretStore.get("srv", envSecretField("K1"))).toBe(null); + expect(await h.secretStore.get("srv", envSecretField("K2"))).toBe(null); + expect( + await h.secretStore.get("srv", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe(null); + // Different server's keychain entries are not touched. + expect( + await h.secretStore.get("untouched", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("untouched-secret"); + }); + + it("migrates plaintext secrets on first GET (idempotent)", async () => { + // mcp.json from a pre-#1356 build, hand-edited, or from another tool. + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + srv: { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { clientId: "cid", clientSecret: "leaked-from-disk" }, + }, + }, + }), + ); + + // First GET migrates and returns the rehydrated shape. + const res1 = await fetch(`${h.baseUrl}/api/servers`); + expect(res1.status).toBe(200); + const body1 = (await res1.json()) as MCPConfig; + const wireOauth1 = ( + body1.mcpServers.srv as { oauth?: Record } + ).oauth; + expect(wireOauth1).toEqual({ + clientId: "cid", + clientSecret: "leaked-from-disk", + }); + + // Disk: secret has been lifted out. + const onDisk = readConfig(h.configPath).mcpServers.srv as { + oauth?: Record; + }; + expect(onDisk.oauth).toEqual({ clientId: "cid" }); + expect(readFileSync(h.configPath, "utf-8")).not.toContain( + "leaked-from-disk", + ); + + // Keychain: value present under the canonical field. + expect( + await h.secretStore.get("srv", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("leaked-from-disk"); + + // Subsequent GET is a no-op (file unchanged, response identical). + const beforeSecondGet = readFileSync(h.configPath, "utf-8"); + const res2 = await fetch(`${h.baseUrl}/api/servers`); + expect(res2.status).toBe(200); + expect(readFileSync(h.configPath, "utf-8")).toBe(beforeSecondGet); + }); + + it("migration prefers an existing keychain value over a re-introduced disk plaintext", async () => { + // Simulate the user hand-editing the file to put plaintext back in + // after migration. The keychain already holds the canonical value; + // we must not let the disk plaintext overwrite it. + await h.secretStore.set( + "srv", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "kept-in-keychain", + ); + writeFileSync( + h.configPath, + JSON.stringify({ + mcpServers: { + srv: { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { clientId: "cid", clientSecret: "disk-version" }, + }, + }, + }), + ); + + const res = await fetch(`${h.baseUrl}/api/servers`); + expect(res.status).toBe(200); + const body = (await res.json()) as MCPConfig; + const wireOauth = ( + body.mcpServers.srv as { oauth?: Record } + ).oauth; + // Wire reflects the keychain value, not the disk plaintext. + expect(wireOauth?.clientSecret).toBe("kept-in-keychain"); + // Keychain untouched. + expect( + await h.secretStore.get("srv", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("kept-in-keychain"); + // Disk: plaintext has been stripped out regardless. + expect(readFileSync(h.configPath, "utf-8")).not.toContain("disk-version"); + }); + }); + + describe("keychain unavailable (Linux without libsecret)", () => { + // A SecretStore impl that simulates keychain unavailability: set is + // the hard-fail path; get / delete / deleteAllForServer silently no-op + // to match the production `KeyringSecretStore` tolerance contract. + class UnavailableSecretStore implements SecretStore { + async get(): Promise { + return null; + } + async set(): Promise { + throw new KeychainUnavailableError(new Error("libsecret missing")); + } + async delete(): Promise { + // no-op + } + async deleteAllForServer(): Promise { + // no-op + } + } + + async function startUnavailableHarness(): Promise<{ + baseUrl: string; + server: import("@hono/node-server").ServerType; + configPath: string; + tempDir: string; + }> { + const tempDir = mkdtempSync( + join(tmpdir(), "inspector-keychain-unavailable-"), + ); + const configPath = join(tempDir, "mcp.json"); + const { app } = createRemoteApp({ + dangerouslyOmitAuth: true, + mcpConfigPath: configPath, + initialConfig: { defaultEnvironment: {} }, + secretStore: new UnavailableSecretStore(), + }); + const { baseUrl, server } = await new Promise<{ + baseUrl: string; + server: import("@hono/node-server").ServerType; + }>((resolve, reject) => { + const s = serve( + { fetch: app.fetch, port: 0, hostname: "127.0.0.1" }, + (info) => { + const port = + info && typeof info === "object" && "port" in info + ? (info as { port: number }).port + : 0; + resolve({ baseUrl: `http://127.0.0.1:${port}`, server: s }); + }, + ); + s.on("error", reject); + }); + return { baseUrl, server, configPath, tempDir }; + } + + it("GET succeeds when there are no plaintext secrets to migrate", async () => { + const u = await startUnavailableHarness(); + try { + writeFileSync( + u.configPath, + JSON.stringify({ + mcpServers: { plain: { type: "stdio", command: "node" } }, + }), + ); + const res = await fetch(`${u.baseUrl}/api/servers`); + expect(res.status).toBe(200); + const body = (await res.json()) as MCPConfig; + expect(body.mcpServers.plain).toBeDefined(); + } finally { + await new Promise((r) => u.server.close(() => r())); + rmSync(u.tempDir, { recursive: true }); + } + }); + + it("GET preserves disk plaintext when migration can't write to the keychain", async () => { + const u = await startUnavailableHarness(); + try { + // Pre-#1356 plaintext on disk. With the keychain unavailable, + // we must NOT strip it out — the user's secret would be lost. + writeFileSync( + u.configPath, + JSON.stringify({ + mcpServers: { + srv: { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { + clientId: "cid", + clientSecret: "still-here", + }, + }, + }, + }), + ); + const beforeFile = readFileSync(u.configPath, "utf-8"); + + const res = await fetch(`${u.baseUrl}/api/servers`); + expect(res.status).toBe(200); + + // Disk file unchanged — plaintext stays put. + expect(readFileSync(u.configPath, "utf-8")).toBe(beforeFile); + } finally { + await new Promise((r) => u.server.close(() => r())); + rmSync(u.tempDir, { recursive: true }); + } + }); + + it("POST without secrets succeeds (defensive sweep is a no-op)", async () => { + const u = await startUnavailableHarness(); + try { + const res = await fetch(`${u.baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "no-secrets", + config: { type: "stdio", command: "node" }, + }), + }); + expect(res.status).toBe(200); + } finally { + await new Promise((r) => u.server.close(() => r())); + rmSync(u.tempDir, { recursive: true }); + } + }); + + it("POST with a secret returns 503 (the moment that matters)", async () => { + const u = await startUnavailableHarness(); + try { + const res = await fetch(`${u.baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "needs-keychain", + config: { type: "streamable-http", url: "https://x.test/mcp" }, + settings: { + headers: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + oauthClientId: "cid", + oauthClientSecret: "shh", + }, + }), + }); + expect(res.status).toBe(503); + const body = (await res.json()) as { error?: string }; + expect(body.error).toMatch(/keychain/i); + expect(body.error).toMatch(/libsecret/); + } finally { + await new Promise((r) => u.server.close(() => r())); + rmSync(u.tempDir, { recursive: true }); + } + }); + + it("POST 503 leaves no disk entry — retry is not trapped at 409", async () => { + // Regression for the write-ordering review comment. If keychain + // set ran AFTER the disk write, a failed `set` would leave the + // entry on disk and the user's retry would hit 409. With the + // reversed order, a failed `set` leaves disk untouched and the + // retry can proceed. + const u = await startUnavailableHarness(); + try { + const first = await fetch(`${u.baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "retry-test", + config: { type: "streamable-http", url: "https://x.test/mcp" }, + settings: { + headers: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + oauthClientId: "cid", + oauthClientSecret: "shh", + }, + }), + }); + expect(first.status).toBe(503); + + // Disk: no entry persisted. A GET should not see "retry-test". + const cfg = existsSync(u.configPath) + ? (JSON.parse(readFileSync(u.configPath, "utf-8")) as MCPConfig) + : { mcpServers: {} }; + expect(cfg.mcpServers).not.toHaveProperty("retry-test"); + + // Retry with no secret should now succeed (no 409). + const second = await fetch(`${u.baseUrl}/api/servers`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + id: "retry-test", + config: { type: "stdio", command: "node" }, + }), + }); + expect(second.status).toBe(200); + } finally { + await new Promise((r) => u.server.close(() => r())); + rmSync(u.tempDir, { recursive: true }); + } + }); + + it("DELETE succeeds (sweep silently no-ops)", async () => { + const u = await startUnavailableHarness(); + try { + writeFileSync( + u.configPath, + JSON.stringify({ + mcpServers: { srv: { type: "stdio", command: "node" } }, + }), + ); + const res = await fetch(`${u.baseUrl}/api/servers/srv`, { + method: "DELETE", + }); + expect(res.status).toBe(200); + } finally { + await new Promise((r) => u.server.close(() => r())); + rmSync(u.tempDir, { recursive: true }); + } + }); + }); }); diff --git a/clients/web/tsconfig.app.json b/clients/web/tsconfig.app.json index 8e54add85..84a48d2a6 100644 --- a/clients/web/tsconfig.app.json +++ b/clients/web/tsconfig.app.json @@ -45,7 +45,8 @@ ], "@hono/node-server": ["./node_modules/@hono/node-server"], "atomically": ["./node_modules/atomically"], - "chokidar": ["./node_modules/chokidar"] + "chokidar": ["./node_modules/chokidar"], + "@napi-rs/keyring": ["./node_modules/@napi-rs/keyring"] } }, "include": ["src", "../../core/**/*.ts"], diff --git a/clients/web/tsconfig.node.json b/clients/web/tsconfig.node.json index 23d92428e..68fbf6621 100644 --- a/clients/web/tsconfig.node.json +++ b/clients/web/tsconfig.node.json @@ -42,7 +42,8 @@ ], "@hono/node-server": ["./node_modules/@hono/node-server"], "atomically": ["./node_modules/atomically"], - "chokidar": ["./node_modules/chokidar"] + "chokidar": ["./node_modules/chokidar"], + "@napi-rs/keyring": ["./node_modules/@napi-rs/keyring"] } }, "include": ["vite.config.ts", "server/**/*.ts"] diff --git a/clients/web/tsconfig.test.json b/clients/web/tsconfig.test.json index 3648e3d9d..03da74b01 100644 --- a/clients/web/tsconfig.test.json +++ b/clients/web/tsconfig.test.json @@ -21,7 +21,8 @@ "atomically": ["./node_modules/atomically"], "chokidar": ["./node_modules/chokidar"], "express": ["./node_modules/@types/express"], - "yaml": ["./node_modules/yaml"] + "yaml": ["./node_modules/yaml"], + "@napi-rs/keyring": ["./node_modules/@napi-rs/keyring"] } }, "include": [ diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index a9f3534b3..e702b2669 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -53,6 +53,7 @@ const nodeModulesAliases = [ { find: /^@hono\/node-server$/, replacement: path.resolve(dirname, 'node_modules/@hono/node-server') }, { find: /^atomically$/, replacement: path.resolve(dirname, 'node_modules/atomically') }, { find: /^chokidar$/, replacement: path.resolve(dirname, 'node_modules/chokidar') }, + { find: /^@napi-rs\/keyring$/, replacement: path.resolve(dirname, 'node_modules/@napi-rs/keyring') }, { find: /^express$/, replacement: path.resolve(dirname, 'node_modules/express') }, { find: /^yaml$/, replacement: path.resolve(dirname, 'node_modules/yaml') }, // Pin the SDK auth subpath so test and source resolve to the exact same @@ -126,6 +127,19 @@ export default defineConfig({ ) { return; } + // `@napi-rs/keyring` is the native-binding keychain backend + // consumed by `core/auth/node/secret-store.ts` from the Hono + // `/api/servers` handlers. Same reasoning as the entries above: + // unreachable from the browser bundle, so the unresolved-import + // warning is noise. + if ( + warning.code === 'UNRESOLVED_IMPORT' && + typeof warning.message === 'string' && + warning.message.includes("'@napi-rs/keyring'") && + warning.message.includes('secret-store.ts') + ) { + return; + } defaultHandler(warning); }, }, diff --git a/core/auth/node/secret-store.ts b/core/auth/node/secret-store.ts new file mode 100644 index 000000000..238543e74 --- /dev/null +++ b/core/auth/node/secret-store.ts @@ -0,0 +1,179 @@ +/** + * Per-server secret storage backed by the OS keychain. + * + * Service name `mcp-inspector`; account `${serverId}:${field}`. Fields used + * by the current code: `oauth-client-secret` and `env:${KEY}` (one per + * stdio env variable). Keeping the account namespaced by `serverId` lets + * us drop every entry for a server in one sweep when DELETE + * /api/servers/:id runs, and lets `findCredentials(SERVICE)` enumerate + * everything we own for migration / debugging. + * + * Node-only — `@napi-rs/keyring` uses native bindings (Keychain Services + * on macOS, Credential Manager on Windows, libsecret on Linux). The + * browser side never imports this; it gets values rehydrated into the + * `/api/servers` response by the Hono handler. + */ +import { + AsyncEntry, + findCredentialsAsync, +} from "@napi-rs/keyring"; + +const SERVICE_NAME = "mcp-inspector"; + +export { + SECRET_FIELD_OAUTH_CLIENT_SECRET, + envSecretField, +} from "../secret-fields.js"; + +/** Parse a stored account key back into its server id and field. */ +export function parseAccount( + account: string, +): { serverId: string; field: string } | null { + const idx = account.indexOf(":"); + if (idx <= 0 || idx === account.length - 1) return null; + return { + serverId: account.slice(0, idx), + field: account.slice(idx + 1), + }; +} + +const buildAccount = (serverId: string, field: string): string => + `${serverId}:${field}`; + +/** + * Thrown when the OS keychain is unavailable — typically Linux without + * libsecret / gnome-keyring installed. Surfaced as a 503 by the API + * handlers so the UI can show an actionable error rather than a generic + * 500. macOS and Windows always have a working keychain, so this only + * realistically fires on minimal Linux installs. + */ +export class KeychainUnavailableError extends Error { + constructor(cause: unknown) { + super( + `OS keychain is not available. On Linux, install libsecret / gnome-keyring. ` + + `Underlying error: ${cause instanceof Error ? cause.message : String(cause)}`, + ); + this.name = "KeychainUnavailableError"; + } +} + +/** + * Storage interface for the per-server secrets we lift off + * `~/.mcp-inspector/mcp.json`. Implemented by `KeyringSecretStore` (the + * production impl) and `InMemorySecretStore` (used in tests so the suite + * doesn't require libsecret in CI). + */ +export interface SecretStore { + get(serverId: string, field: string): Promise; + set(serverId: string, field: string, value: string): Promise; + /** No-op if no entry exists. */ + delete(serverId: string, field: string): Promise; + /** Remove every secret stored for this server id (called on DELETE /api/servers/:id). */ + deleteAllForServer(serverId: string): Promise; +} + +/** + * Default implementation. Each operation constructs a fresh `AsyncEntry`; + * the native side is cheap and the alternative (caching entries by + * (serverId, field)) just trades native-handle bookkeeping for an + * allocation that's measured in microseconds. `getPassword` returns + * `undefined` for a missing entry — we normalize to `null` so callers + * can use `=== null` rather than truthiness (an empty-string secret is + * a real value and must round-trip). + * + * **Availability behavior.** When the keychain is unavailable (the + * typical case is Linux without libsecret / gnome-keyring), `set` is + * the only operation that throws `KeychainUnavailableError` — that's + * the moment where data would actually be lost. `get` returns `null` + * (as if no entry existed) and the destructive operations silently + * no-op (there's nothing to delete anyway). This keeps non-secret + * flows working on a stock CI runner / minimal Linux box; the user + * only hits a hard error when they actually try to save a secret. + */ +export class KeyringSecretStore implements SecretStore { + async get(serverId: string, field: string): Promise { + const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field)); + try { + const v = await entry.getPassword(); + return v ?? null; + } catch { + // Tolerate keychain unavailability on reads: there's no value to + // surface either way. Hard-failing here would break GET flows + // that don't touch any secret material (most of the test suite, + // and most user sessions on a Linux box without libsecret). + return null; + } + } + + async set(serverId: string, field: string, value: string): Promise { + const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field)); + try { + await entry.setPassword(value); + } catch (err) { + // The only operation that hard-fails — if we can't persist the + // secret, the user needs to know now rather than discover later + // that their value disappeared. Routes translate this to a 503. + throw new KeychainUnavailableError(err); + } + } + + async delete(serverId: string, field: string): Promise { + const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field)); + try { + await entry.deleteCredential(); + } catch { + // Both reasons for a throw collapse to the same desired outcome + // ("the entry isn't there anymore"): `deleteCredential` raises + // NoEntry for a missing credential, and the native binding + // raises a runtime error when the keychain itself is unavailable. + // We treat both as success — there's no value to lose either + // way, and `set` is the operation that hard-fails when the + // keychain is actually down. + } + } + + async deleteAllForServer(serverId: string): Promise { + let creds: Array<{ account: string; password: string }>; + try { + creds = await findCredentialsAsync(SERVICE_NAME); + } catch { + // Same reasoning as `delete`: nothing was written, nothing to sweep. + return; + } + const prefix = `${serverId}:`; + for (const c of creds) { + if (!c.account.startsWith(prefix)) continue; + const parsed = parseAccount(c.account); + if (!parsed || parsed.serverId !== serverId) continue; + await this.delete(serverId, parsed.field); + } + } +} + +/** + * Test double — substituted via the `secretStore` option on the remote + * server factory. Mirrors the keyring contract exactly so swapping it + * in/out doesn't change behavior beyond persistence. + */ +export class InMemorySecretStore implements SecretStore { + private readonly map = new Map(); + + async get(serverId: string, field: string): Promise { + return this.map.get(buildAccount(serverId, field)) ?? null; + } + + async set(serverId: string, field: string, value: string): Promise { + this.map.set(buildAccount(serverId, field), value); + } + + async delete(serverId: string, field: string): Promise { + this.map.delete(buildAccount(serverId, field)); + } + + async deleteAllForServer(serverId: string): Promise { + const prefix = `${serverId}:`; + for (const key of [...this.map.keys()]) { + if (key.startsWith(prefix)) this.map.delete(key); + } + } +} diff --git a/core/auth/secret-fields.ts b/core/auth/secret-fields.ts new file mode 100644 index 000000000..341ef9eb2 --- /dev/null +++ b/core/auth/secret-fields.ts @@ -0,0 +1,14 @@ +/** + * Field identifiers for the per-server values held in the OS keychain. + * + * Pure constants — kept out of `core/auth/node/` so the converter in + * `core/mcp/serverList.ts` (browser-safe) and the Node-only keychain + * backend in `core/auth/node/secret-store.ts` agree on the spelling + * without dragging the native binding import into browser code. + */ + +/** Field name for an entry's OAuth client secret. */ +export const SECRET_FIELD_OAUTH_CLIENT_SECRET = "oauth-client-secret"; + +/** Field name for one stdio env-variable value. */ +export const envSecretField = (envKey: string): string => `env:${envKey}`; diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index c0a31cac0..3ab6f8671 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -33,8 +33,11 @@ import type { } from "../../types.js"; import { DEFAULT_SEED_CONFIG, + expectedSecretFields, + extractSecretsFromStored, INSPECTOR_FIELD_KEYS, inspectorSettingsToStoredFields, + mergeSecretsIntoStored, normalizeServerType, storedFieldsToInspectorSettings, stripInspectorFields, @@ -42,6 +45,11 @@ import { import { RemoteSession } from "./remote-session.js"; import { createTokenAuthProvider } from "./tokenAuthProvider.js"; import { API_SERVER_ENV_VARS } from "../constants.js"; +import { + KeychainUnavailableError, + KeyringSecretStore, + type SecretStore, +} from "../../../auth/node/secret-store.js"; /** * Shape of the initial config returned by GET /api/config (defaults for client). @@ -83,6 +91,15 @@ export interface RemoteServerOptions { /** Initial config for GET /api/config. Caller must pass this (e.g. from webServerConfigToInitialPayload(config)). */ initialConfig: InitialConfigPayload; + + /** + * Backend for per-server secret values that we keep out of mcp.json + * (OAuth client secret, stdio env values). Defaults to a + * `KeyringSecretStore` that talks to the OS keychain via + * `@napi-rs/keyring`. Tests inject `InMemorySecretStore` so the suite + * doesn't need libsecret on Linux CI runners. + */ + secretStore?: SecretStore; } export interface CreateRemoteAppResult { @@ -277,6 +294,8 @@ export function createRemoteApp( const { logger: fileLogger, allowedOrigins } = options; const storageDir = options.storageDir ?? getDefaultStorageDir(); const mcpConfigPath = options.mcpConfigPath ?? getDefaultMcpConfigPath(); + const secretStore: SecretStore = + options.secretStore ?? new KeyringSecretStore(); // --- /api/servers/events: file-watch fanout state ----------------------- // @@ -1120,16 +1139,234 @@ export function createRemoteApp( return { mcpServers: normalizeMcpServers(parsed?.mcpServers) }; }; + // ---- Keychain helpers ------------------------------------------------- + // + // Centralizes the "write the secrets in this entry to the keychain" and + // "fetch all secrets for this entry from the keychain" steps so the + // POST/PUT/DELETE/GET handlers stay readable. Each operation translates a + // `KeychainUnavailableError` from the underlying store into a Hono 503 + // — the only realistic trigger is Linux without libsecret, and the user + // can install it without restarting. + + // Same Promise.all reasoning as `readKeychainEntriesFor`: each set + // is a native round-trip and there's no ordering requirement among + // distinct (id, field) keys. If the keychain is unavailable every + // set will reject — Promise.all surfaces the first rejection, which + // is the right signal for the route handler (translates to 503). + const writeKeychainEntriesFor = async ( + id: string, + secrets: Record, + ): Promise => { + await Promise.all( + Object.entries(secrets).map(([field, value]) => + secretStore.set(id, field, value), + ), + ); + }; + + // Fetch every keychain value an entry could need into a flat record + // keyed by field name. Used by the GET handler to rehydrate the on-disk + // stripped shape into the shape today's browser code expects. Missing + // fields are simply absent from the result (so `mergeSecretsIntoStored` + // leaves the disk value untouched). + // Issue many keychain reads in parallel. On macOS each round-trip to + // Keychain Services is 10-50ms; a server with 20 entries × 5 env vars + // each would otherwise serialize to ~5s of rehydration on every GET. + // `@napi-rs/keyring`'s async APIs release the event loop, so + // Promise.all is a real win, not just stylistic. + const readKeychainEntriesFor = async ( + 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; + }; + + // Cheap structural check used by the GET handler to decide whether + // to enter the write lock for migration. Pure — no keychain or disk + // access. The actual migration (`migratePlaintextSecrets`) writes + // to the keychain, so we must not run it speculatively outside the + // lock; this predicate lets us keep the fast path lock-free. + const hasPlaintextSecrets = (config: MCPConfig): boolean => { + for (const stored of Object.values(config.mcpServers)) { + const { secrets } = extractSecretsFromStored(stored); + if (Object.keys(secrets).length > 0) return true; + } + return false; + }; + + // Migrate plaintext secrets in a freshly-read mcp.json into the + // keychain. Idempotent: when the keychain already has a value for + // `(id, field)`, that value wins and the disk plaintext is dropped + // unread. Returns the rewritten config plus a flag so the GET handler + // knows whether to persist the cleanup. + // + // When the keychain is unavailable (Linux without libsecret), the + // first `secretStore.set` throws `KeychainUnavailableError`. We catch + // it and abandon the migration for this GET — the on-disk file stays + // as-is so the user's secret isn't lost, and the next GET retries + // (e.g. after the user installs libsecret). + const migratePlaintextSecrets = async ( + config: MCPConfig, + ): Promise<{ migrated: MCPConfig; changed: boolean }> => { + let changed = false; + const next: MCPConfig = { mcpServers: {} }; + try { + for (const [id, stored] of Object.entries(config.mcpServers)) { + const { stripped, secrets } = extractSecretsFromStored(stored); + if (Object.keys(secrets).length === 0) { + next.mcpServers[id] = stored; + continue; + } + for (const [field, value] of Object.entries(secrets)) { + const existing = await secretStore.get(id, field); + if (existing === null) { + await secretStore.set(id, field, value); + } + // existing !== null → keychain already had a value for this + // (id, field); keep the keychain authoritative and drop the + // plaintext from disk. This handles the case where a user has + // edited mcp.json by hand after the original migration. + } + next.mcpServers[id] = stripped; + changed = true; + } + } catch (err) { + if (err instanceof KeychainUnavailableError) { + if (fileLogger) { + fileLogger.warn( + { err: err.message }, + "Keychain unavailable; skipping plaintext-secret migration on this read. Existing mcp.json plaintext values are preserved.", + ); + } + // Partial-migration semantics: if `set` threw partway through + // the loop, some servers may already have keychain entries + // while others don't. We deliberately return the original + // `config` (not the partial `next`) so the disk file stays + // intact — the next successful GET runs the migration again, + // and the idempotent "keychain wins on conflict" branch + // (`existing !== null`) silently absorbs the already-set + // entries. No data loss; one wasted set on retry per + // already-migrated entry. + return { migrated: config, changed: false }; + } + throw err; + } + return { migrated: next, changed }; + }; + + // Rehydrate every server's secrets so the GET response matches what + // the browser saw before the keychain split. Runs after migration in + // the GET handler. + const rehydrateConfig = async (config: MCPConfig): Promise => { + const out: MCPConfig = { mcpServers: {} }; + for (const [id, stored] of Object.entries(config.mcpServers)) { + const fields = expectedSecretFields(stored); + const secrets = await readKeychainEntriesFor(id, fields); + out.mcpServers[id] = mergeSecretsIntoStored(stored, secrets); + } + return out; + }; + + // Fields the previous entry held that the new entry doesn't — + // candidates for deletion after the disk write succeeds. Returned + // as an array so the caller can decide where in the write sequence + // to perform the destructive operation (we want it after the disk + // write so a failed disk write doesn't leave the user with their + // old config on disk but missing keychain entries). + const computeObsoleteFields = ( + previousFields: Set, + nextSecrets: Record, + ): string[] => { + const nextFieldSet = new Set(Object.keys(nextSecrets)); + const obsolete: string[] = []; + for (const field of previousFields) { + if (!nextFieldSet.has(field)) obsolete.push(field); + } + return obsolete; + }; + + // Parallel for symmetry with `readKeychainEntriesFor` / + // `writeKeychainEntriesFor`: distinct (id, field) deletes have no + // ordering requirement, and `secretStore.delete` is already a silent + // no-op on unavailability so Promise.all has no failure-mode surprise. + const deleteKeychainFields = async ( + id: string, + fields: string[], + ): Promise => { + await Promise.all(fields.map((field) => secretStore.delete(id, field))); + }; + + const keychainErrorResponse = ( + c: Context, + err: unknown, + ): Response | undefined => { + if (err instanceof KeychainUnavailableError) { + return c.json({ error: err.message }, 503); + } + return undefined; + }; + app.get("/api/servers", async (c) => { try { + // Fast path: peek at the file without taking the write lock. Most + // GETs land here — file exists, no plaintext to migrate. The + // unlocked read is safe because we don't write anything in this + // branch; a concurrent mutation just means the response is a + // snapshot from a moment ago. const raw = await readStoreFile(mcpConfigPath); - if (raw === null) { - await writeMcpAndTrackMtime(serializeStore(DEFAULT_SEED_CONFIG)); - return c.json(DEFAULT_SEED_CONFIG); + if (raw !== null) { + const parsed = parseStore(raw) as { mcpServers?: unknown } | null; + const onDisk: MCPConfig = { + mcpServers: normalizeMcpServers(parsed?.mcpServers), + }; + if (!hasPlaintextSecrets(onDisk)) { + return c.json(await rehydrateConfig(onDisk)); + } } - const parsed = parseStore(raw) as { mcpServers?: unknown } | null; - return c.json({ mcpServers: normalizeMcpServers(parsed?.mcpServers) }); + + // Slow path: either the file is missing (first-launch seed) or it + // contains plaintext we need to migrate. Both branches write the + // file, so the entire read + decide + write sequence must happen + // inside the write lock — otherwise a concurrent POST/PUT/DELETE + // could land between our unlocked read and our locked write, and + // we'd clobber it. Re-read once we hold the lock so the decision + // is based on the same snapshot we're about to mutate. + const settled = await withWriteLock(async () => { + const rawInside = await readStoreFile(mcpConfigPath); + if (rawInside === null) { + // Still absent after taking the lock → no concurrent POST + // beat us to it; seed the file ourselves. The seed has no + // secrets so nothing else to do. + await writeMcpAndTrackMtime(serializeStore(DEFAULT_SEED_CONFIG)); + return DEFAULT_SEED_CONFIG; + } + const parsedInside = parseStore(rawInside) as + | { mcpServers?: unknown } + | null; + const inside: MCPConfig = { + mcpServers: normalizeMcpServers(parsedInside?.mcpServers), + }; + // A concurrent POST may have run between the unlocked peek and + // the lock acquisition — if the file now has no plaintext to + // migrate, leave it alone and return the latest snapshot. + if (!hasPlaintextSecrets(inside)) return inside; + const { migrated, changed } = await migratePlaintextSecrets(inside); + if (changed) { + await writeMcpAndTrackMtime(serializeStore(migrated)); + } + return migrated; + }); + return c.json(await rehydrateConfig(settled)); } catch (error) { + const keychainResp = keychainErrorResponse(c, error); + if (keychainResp) return keychainResp; const msg = error instanceof Error ? error.message : String(error); return c.json({ error: `Failed to read server list: ${msg}` }, 500); } @@ -1174,15 +1411,26 @@ export function createRemoteApp( if (id in current.mcpServers) { return c.json({ error: `Server '${id}' already exists` }, 409); } - current.mcpServers[id] = buildStoredEntry( - id, - body.config, - postSettings, - ); + const built = buildStoredEntry(id, body.config, postSettings); + // Split secret values out of the new entry — the stripped shape + // goes to disk, the values go to the keychain. + const { stripped, secrets } = extractSecretsFromStored(built); + // Order: sweep → keychain → disk. The keychain write is the + // only step that can hard-fail (KeychainUnavailableError on + // `set`); doing it before the disk write means a 503 leaves no + // disk entry behind, so a retry POST isn't trapped at 409. The + // initial sweep handles the case where a previous DELETE failed + // midway and left orphans under the same id the user is now + // reusing; it's a silent no-op when the keychain is unavailable. + await secretStore.deleteAllForServer(id); + await writeKeychainEntriesFor(id, secrets); + current.mcpServers[id] = stripped; await writeMcpAndTrackMtime(serializeStore(current)); return c.json({ ok: true }); }); } catch (error) { + const keychainResp = keychainErrorResponse(c, error); + if (keychainResp) return keychainResp; const msg = error instanceof Error ? error.message : String(error); return c.json({ error: `Failed to add server: ${msg}` }, 500); } @@ -1294,22 +1542,56 @@ export function createRemoteApp( nextSettings = settingsIntent.value; break; } + // Build the new entry, then split off secrets before writing to + // disk. Reconcile uses the previously-stored entry to know which + // keychain fields existed so any field the user removed (env + // key dropped, OAuth secret cleared) gets cleaned up rather + // than orphaned. + const built = buildStoredEntry(newId, nextConfig, nextSettings); + const { stripped, secrets } = extractSecretsFromStored(built); const next: MCPConfig = { mcpServers: {} }; for (const [key, val] of Object.entries(current.mcpServers)) { if (key === originalId) { - next.mcpServers[newId] = buildStoredEntry( - newId, - nextConfig, - nextSettings, - ); + next.mcpServers[newId] = stripped; } else { next.mcpServers[key] = val; } } - await writeMcpAndTrackMtime(serializeStore(next)); + // Ordering: write the new keychain entries first, then the + // disk file, then clean up obsolete keychain entries. + // + // - Keychain set is the only hard-fail step (it raises 503 on + // `KeychainUnavailableError`). Doing it first means a failed + // set leaves both disk and keychain in their pre-PUT state; + // the user retries and nothing is half-applied. + // - The disk write happens after the keychain is fully primed, + // so a successful disk write is also a fully-consistent end + // state. + // - Obsolete deletion comes last because it's destructive: if + // we deleted first and then the disk write failed, the user + // would still see the old config on disk but with missing + // keychain values. With the current order a failed disk + // write leaves orphan keychain entries — recoverable on the + // next reconcile or `deleteAllForServer` sweep. + if (newId !== originalId) { + await writeKeychainEntriesFor(newId, secrets); + await writeMcpAndTrackMtime(serializeStore(next)); + await secretStore.deleteAllForServer(originalId); + } else { + // In-place update: same id, possibly different fields. Set + // the new values first, then write disk, then drop obsolete + // fields (env keys the user removed, OAuth secret cleared). + const previousFields = new Set(expectedSecretFields(existing)); + const obsolete = computeObsoleteFields(previousFields, secrets); + await writeKeychainEntriesFor(newId, secrets); + await writeMcpAndTrackMtime(serializeStore(next)); + await deleteKeychainFields(newId, obsolete); + } return c.json({ ok: true }); }); } catch (error) { + const keychainResp = keychainErrorResponse(c, error); + if (keychainResp) return keychainResp; const msg = error instanceof Error ? error.message : String(error); return c.json({ error: `Failed to update server: ${msg}` }, 500); } @@ -1325,13 +1607,20 @@ export function createRemoteApp( return await withWriteLock(async () => { const current = await readMcpConfig(); if (!(id in current.mcpServers)) { + // Idempotent DELETE — but still sweep the keychain in case a + // prior delete failed after rewriting the file and orphaned + // entries are sitting there. + await secretStore.deleteAllForServer(id); return c.json({ ok: true }); } delete current.mcpServers[id]; await writeMcpAndTrackMtime(serializeStore(current)); + await secretStore.deleteAllForServer(id); return c.json({ ok: true }); }); } catch (error) { + const keychainResp = keychainErrorResponse(c, error); + if (keychainResp) return keychainResp; const msg = error instanceof Error ? error.message : String(error); return c.json({ error: `Failed to delete server: ${msg}` }, 500); } diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index ac131a387..d12f6f7d9 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -11,8 +11,13 @@ import type { MCPServerConfig, ServerEntry, ServerType, + StdioServerConfig, StoredMCPServer, } from "./types.js"; +import { + SECRET_FIELD_OAUTH_CLIENT_SECRET, + envSecretField, +} from "../auth/secret-fields.js"; // The full set of valid `type` discriminator values, used to reject anything // else read off disk so unknown strings can't propagate to narrowing sites. @@ -274,6 +279,147 @@ export function serializeMcpConfig(entries: ServerEntry[]): string { return JSON.stringify(serverEntriesToMcpConfig(entries), null, 2); } +/** + * Result of splitting secret values off a `StoredMCPServer` for keychain + * persistence. `stripped` is what gets written to `mcp.json` on disk; + * `secrets` is the (field → value) map the keychain backend writes. + * + * Empty-string values are not written to keychain (they have the same + * semantic meaning as absence). Callers that want full reconcile + * semantics on update should also call `deleteAllForServer` first. + */ +export interface ExtractedSecrets { + stripped: StoredMCPServer; + secrets: Record; +} + +/** + * Type guard for the stdio branch of `MCPServerConfig`. The `type` field + * is optional on `StdioServerConfig` because stdio is the implicit + * default for entries written without a `type` key (matches Claude + * Desktop). So both `undefined` and the literal "stdio" route here. + */ +const isStdioStored = ( + stored: StoredMCPServer, +): stored is StdioServerConfig & StoredMCPServer => + stored.type === "stdio" || stored.type === undefined; + +/** + * Strip secret values from a single on-disk entry. Returns the + * sanitized disk shape (oauth.clientSecret removed; stdio env values + * cleared to "") plus the map of field → value the keychain should + * hold. + * + * stdio env keys are preserved with empty-string values rather than + * dropped, so the on-disk file still documents the env interface the + * server expects (a user reading mcp.json can see "this server uses + * API_KEY and DB_PASSWORD" even though the values live in the + * keychain). Round-tripping with another tool that reads mcp.json + * gets the same key set but empty values, which is the intended + * trade-off: the secret never reaches another tool, but the key list + * is still visible. + */ +export function extractSecretsFromStored( + stored: StoredMCPServer, +): ExtractedSecrets { + const secrets: Record = {}; + const stripped: StoredMCPServer = { ...stored }; + + if (stored.oauth?.clientSecret) { + secrets[SECRET_FIELD_OAUTH_CLIENT_SECRET] = stored.oauth.clientSecret; + const restOauth: { clientId?: string; scopes?: string } = {}; + if (stored.oauth.clientId !== undefined) + restOauth.clientId = stored.oauth.clientId; + if (stored.oauth.scopes !== undefined) + restOauth.scopes = stored.oauth.scopes; + if (Object.keys(restOauth).length > 0) { + stripped.oauth = restOauth; + } else { + delete (stripped as unknown as Record).oauth; + } + } + + if (isStdioStored(stripped)) { + const env = stripped.env; + if (env) { + const newEnv: Record = {}; + for (const [k, v] of Object.entries(env)) { + if (typeof v === "string" && v.length > 0) { + secrets[envSecretField(k)] = v; + } + newEnv[k] = ""; + } + stripped.env = newEnv; + } + } + + return { stripped, secrets }; +} + +/** + * Inverse of `extractSecretsFromStored`. Merges a pre-fetched secrets + * record back into an on-disk entry. Used at the `/api/servers` GET + * boundary so the browser receives the same effective shape it has + * today. + * + * A missing key in `secrets` leaves the corresponding field alone, + * which matters for stdio env: if the keychain doesn't have a value + * for `env:KEY`, the on-disk empty string passes through unchanged. + */ +export function mergeSecretsIntoStored( + stored: StoredMCPServer, + secrets: Record, +): StoredMCPServer { + const out: StoredMCPServer = { ...stored }; + + const oauthSecret = secrets[SECRET_FIELD_OAUTH_CLIENT_SECRET]; + if (oauthSecret) { + out.oauth = { ...(out.oauth ?? {}), clientSecret: oauthSecret }; + } + + if (isStdioStored(out) && out.env) { + const newEnv: Record = { ...out.env }; + let mutated = false; + for (const k of Object.keys(out.env)) { + const val = secrets[envSecretField(k)]; + if (val !== undefined) { + newEnv[k] = val; + mutated = true; + } + } + if (mutated) { + out.env = newEnv; + } + } + + return out; +} + +/** + * Enumerate the keychain field identifiers an on-disk entry expects + * to find values for. Handlers use this to know which keychain entries + * to fetch when rehydrating a server, and which keys to reconcile on + * update (env keys removed by the user should drop their keychain + * entries). + * + * Order is stable: OAuth first, then env keys in object iteration + * order. Callers that diff old vs new field sets rely on stable + * enumeration to avoid spurious churn. + */ +export function expectedSecretFields(stored: StoredMCPServer): string[] { + const fields: string[] = []; + // Always include OAuth slot — even if the entry has no `oauth` block + // on disk, the keychain may hold a leftover entry from a prior + // configuration that we want callers to be able to reconcile. + fields.push(SECRET_FIELD_OAUTH_CLIENT_SECRET); + if (isStdioStored(stored) && stored.env) { + for (const k of Object.keys(stored.env)) { + fields.push(envSecretField(k)); + } + } + return fields; +} + /** * Default seeds written to `~/.mcp-inspector/mcp.json` on first launch when * the file is absent. Picked to cover the two shapes a developer reaches for diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index 174da4b4f..ca84049c7 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -243,7 +243,12 @@ Each server entry may carry these Inspector-extension fields at the top level: - `connectionTimeout` → `Promise.race` wrapper around `InspectorClient.connect()` in the web client. - `oauth.clientId` / `oauth.clientSecret` / `oauth.scopes` → pre-seeded OAuth client credentials via `InspectorClientOptions.oauth` (the disk-side `oauth` object is lifted into the flat `oauthClientId` / etc. fields on `InspectorServerSettings` for the form). - **First-connect contract**: settings apply on the *first* outbound request after the entry loads from disk — no need to open the settings form. The browser sends `settings` to the backend in the `/api/mcp/connect` body; the backend reads it from `RemoteConnectRequest` and threads it into `createTransportNode`. -- **Secret storage**: `oauth.clientSecret` is persisted in `mcp.json` alongside stdio `env` values, both protected by the file's `0o600` permission. OS-keychain integration is tracked separately in #1356 — when that lands, the secret will be lifted out of the file and replaced with a keychain reference. +- **Secret storage (#1356)**: `oauth.clientSecret` and stdio `env` values are persisted in the OS keychain (macOS Keychain Services / Windows Credential Manager / Linux libsecret via `@napi-rs/keyring`), keyed by `(serverId, field)` under the service name `mcp-inspector`. Field names: `oauth-client-secret`, `env:` (one per stdio env variable). The on-disk `mcp.json` is stripped of these values — `oauth.clientSecret` is omitted entirely, stdio env keys are preserved with empty-string placeholders (`"env": { "API_KEY": "" }`) so the file still documents the env interface the server expects. The wire shape returned by `GET /api/servers` is unchanged from before #1356: the handler rehydrates values from the keychain so browser code sees the same JSON it has always seen. The keychain interactions live in `core/auth/node/secret-store.ts` behind a `SecretStore` interface; `KeyringSecretStore` is the production impl and `InMemorySecretStore` is the test double the integration suite injects via `RemoteServerOptions.secretStore`. + - **Migration**: on every `GET /api/servers`, the handler walks the freshly-read config and, for any entry that still carries plaintext secrets (older Inspector builds, hand-edited files, files imported from another tool), lifts each value into the keychain and rewrites the file with the stripped shape. The migration is idempotent — when the keychain already holds a value for `(serverId, field)`, the keychain wins and the disk plaintext is dropped unread. After the rewrite the disk file no longer contains the secret material. + - **Linux without libsecret**: `KeyringSecretStore` is *tolerant* — only the `set` operation throws `KeychainUnavailableError` (translated to a `503` by the handlers); `get` returns `null` and the destructive operations silently no-op. The result is that no-secret flows (creating a stdio server with no env values, deleting an entry, reading the list, the defensive sweep on POST) all work normally on a minimal Linux box without libsecret. Only the moments where a secret would actually be lost — saving an OAuth client secret, saving a stdio env value, or migrating a plaintext value into the keychain — surface a clear error. macOS and Windows always have a working keychain so this only matters on minimal Linux installs. + - **Migration tolerance**: when migration encounters `KeychainUnavailableError`, the GET handler logs a warning, leaves the on-disk plaintext untouched, and serves the (still-plaintext) response. Subsequent reads retry — installing libsecret later lifts the secrets on the next GET without any user action. + - **Write ordering on POST/PUT**: keychain writes happen before the disk write, and obsolete-field deletions happen after. The intent is that a `set` failure (the only hard-fail path) leaves both stores in their pre-write state — no half-applied entry on disk that would trap a retry POST at `409`, and no premature deletion of an obsolete field whose disk write later fails. + - **Out of scope for this PR**: the OAuth handshake itself still runs in the browser via the MCP SDK, so during the token exchange the secret transits the wire (browser → MCP SDK → OAuth provider's token endpoint). The on-disk win this PR delivers is that the secret is no longer in the shareable / symlinked `mcp.json` and is no longer the source-of-truth on the filesystem. Moving the token exchange to the Node side is tracked separately. - **Hard-cutover legacy behavior (per #1358 decision 4)**: files written by the one pre-#1358 build of v2/main have a nested `settings` block. `normalizeMcpServers` drops the node on read and logs a one-line warn including the server id; the persisted headers / metadata / timeouts / OAuth credentials are intentionally lost on first read. Users re-enter them via the settings form (or hand-edit the file into the flat shape). v2 has not shipped a stable release with the nested shape, so the blast radius is the small set of v2/main dogfooders who edited per-server settings between #1353 merging and this change. - **UI**: `ServerSettingsModal` is opened from the server card's settings affordance. Saving routes through `useServers.updateServerSettings(id, settings)` which issues a settings-only `PUT /api/servers/:id` with `{ id, settings }` — the route preserves the on-disk transport config inside its write lock. Conversely, `useServers.updateServer` (driven by the basic-config modal) issues a config-only PUT with `{ id, config }` and the route preserves the on-disk settings fields. Edits in either modal cannot silently wipe the other half. - **Save cadence**: the form fires `onSettingsChange` on every keystroke. `App.tsx` debounces 300 ms and flushes on modal close so a burst of edits coalesces into a single PUT. If the close-flush PUT fails (network hiccup, server 500), a red `@mantine/notifications` toast surfaces the failure — the modal has already closed so a silent failure would leave the user thinking the last edits saved.