diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index 410a25beb..63d611f34 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -481,6 +481,40 @@ const TOKEN_TIMEOUT_MS = 10_000; */ export type OAuthClient = { clientId: string; clientSecret: string }; +/** + * Whether a value read back out of the vault is a client this deployment can actually present. + * + * Guarding the parse is only half of the question. `JSON.parse` answers for SYNTAX, and the + * `as OAuthClient` cast behind it answers for nothing at all — so a row holding + * `{"client_id":"","client_secret":""}`, which is what a hand-repair or a half-written row leaves, + * parses cleanly and yields a client whose `clientId` is `undefined`. + * + * SHAPE AND SYNTAX ARE ONE CONCERN, which is why all three readers ask this beside their parse + * rather than only around it: either way the deployment holds a client it cannot use, and the + * operator's signal has to survive both. `unusableClient` is that signal, and it is deliberately + * distinct from `noClient`'s holding none. + * + * The shape half earns the check by ending WORSE than the syntax half beside it. An `undefined` + * client id is sent to the vendor, the vendor answers `invalid_client`, and + * {@link refuseAndReplaceEvictedClient} reads that as the vendor having disowned our registration — + * so a corrupt LOCAL row replaces the deployment-wide client every existing consent was granted + * against, and reports it as the vendor's doing rather than naming the credential that broke. + * + * The id has to be there; the secret only has to be a string. A public client registered + * dynamically proves itself with PKCE and is stored with an empty secret ON PURPOSE — + * `registerDynamicClient` checks the id exactly this way and defaults the secret to `""` — so + * demanding a non-empty secret here would refuse every self-registering entry in the catalogue. + */ +function isUsableClient(value: unknown): value is OAuthClient { + if (typeof value !== "object" || value === null) return false; + const { clientId, clientSecret } = value as Partial; + return ( + typeof clientId === "string" && + clientId !== "" && + typeof clientSecret === "string" + ); +} + /** * The client and when the vault row holding it was written. * @@ -867,12 +901,49 @@ export function createPluginStore(options: PluginStoreOptions) { if (!server?.credentialId) { throw new PluginRefusedError(noClient, null); } - return { - client: JSON.parse( - await secretFor(server.credentialId, unusableClient), - ) as OAuthClient, - registeredAt: server.registeredAt, - }; + /* + * Decrypted outside the guard below, so that `secretFor`'s own refusal for a revoked or + * missing row is not caught here and relabelled. Only the parse is guarded. + */ + const decrypted = await secretFor(server.credentialId, unusableClient); + let parsed: unknown; + try { + parsed = JSON.parse(decrypted); + } catch { + /* + * Unreadable is the same as none, exactly as it is for {@link heldOAuthClient} and + * {@link storedOAuthClient}: there is nothing here to present to a vendor. Those two answer + * null because their callers are deciding whether a consent flow can start; this one is + * already mid-call, and its every caller would have to turn a null into this same refusal + * on the next line — so it raises it. + * + * THE PARSER'S OWN WORDS ARE NEVER CARRIED. `JSON.parse` reports failure by quoting the + * input it choked on, and the input here is the DECRYPTED OAuth client. Rethrowing it puts + * a fragment of the client secret — under Bun's parser, the whole of it when the stored + * value is a bare token — into the `mcp.call_failed` payload and into + * `mcp_servers.last_error`, two durable stores that the Plugins page draws for anybody who + * can read it. + * + * A reader should conclude that the signal is not lost, only the bytes: `unusableClient` + * says the deployment holds a client it cannot use, which is distinct from `noClient`'s + * holding none, and it names connecting again as what replaces it. An operator can tell + * the credential is broken; nobody learns what was in it. + */ + throw new PluginRefusedError(unusableClient, null); + } + /* + * The same refusal for a value that parsed and is not a client — see {@link isUsableClient}, + * which is where the criterion and the reason live, because all three readers share both. + * + * Raised rather than answered null, for the reason above: this caller is mid-call. It is the + * third throw in a row here — `noClient`, then the parse, then this — and that is the shape + * of the contract rather than a repetition to collapse. Each names a different state of the + * deployment's credential, and only the sentence is shared between the last two. + */ + if (!isUsableClient(parsed)) { + throw new PluginRefusedError(unusableClient, null); + } + return { client: parsed, registeredAt: server.registeredAt }; } /* @@ -1272,9 +1343,13 @@ export function createPluginStore(options: PluginStoreOptions) { if (!held || held.revokedAt) return null; try { - return JSON.parse( + const parsed: unknown = JSON.parse( await decryptSecret(encryptionKey, held.encryptedValue), - ) as OAuthClient; + ); + // A value that parsed and is not a client is unreadable in the same way and for the same + // caller — see {@link isUsableClient}. Null, because that is what this reader's caller acts + // on: it goes and registers one, which is the answer to holding none. + return isUsableClient(parsed) ? parsed : null; } catch { // Unreadable is the same as none: there is nothing to send anybody to consent with. return null; @@ -1407,13 +1482,16 @@ export function createPluginStore(options: PluginStoreOptions) { if (!row?.credentialId) return null; try { - return JSON.parse( + const parsed: unknown = JSON.parse( await decryptCredentialForUse( encryptionKey, credentials, row.credentialId, ), - ) as OAuthClient; + ); + // A client that parsed and is not one is as unusable as the revoked or missing row the catch + // below answers for, and is the same none to every caller — see {@link isUsableClient}. + return isUsableClient(parsed) ? parsed : null; } catch { // A revoked, missing or unreadable client is the same as none for every caller: there is // nothing to send anybody to consent with, and the answer is to obtain one again. diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index da407716b..b6f442e94 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -1458,6 +1458,153 @@ describe("refresh token rotation", () => { await decryptSecret(ROTATION_KEY, live[0]?.encryptedValue ?? ""), ).toBe("rt-3"); }); + + /** + * A stored OAuth client whose decrypted bytes are not a client at all. + * + * CRITERION: neither the decrypted plaintext nor a parser's account of it may reach + * `audit_events` or `mcp_servers.last_error`. REASON: that plaintext IS the deployment's OAuth + * client secret, and `JSON.parse` reports failure by quoting the input it choked on — so an + * unguarded parse writes a fragment of the secret into two durable stores, both of which the + * Plugins page draws for an administrator. + * + * A corrupted row is not hypothetical: a partially written value, a row encrypted under a key + * this deployment no longer holds, or a hand-edited vault all produce bytes that decrypt and are + * not JSON. + * + * The refusal is asserted alongside the absence, because an unreadable client that produced + * nothing at all would be its own bug: the operator would see a connector failing with no reason + * given, and the credential is the reason. + */ + describe("a stored OAuth client that does not read back as one", () => { + /* + * A bare secret where a client object belongs — the shape a wrongly encrypted row really has, + * and the worst case for the leak. It decrypts, so the vault is happy; it is not JSON, so the + * parse fails; and it is a single identifier token, which is what the parser quotes back + * WHOLE. Distinctive, so an assertion can look for the plaintext itself rather than a shape. + */ + const UNREADABLE_PLAINTEXT = `secret_notJsonClient${suite}`; + /** What the person and the trail are told instead, which is the operator's signal. */ + const UNUSABLE = "Notion has no usable OAuth client for this deployment."; + + /** The client the suite registered, restored after each test repoints the server. */ + let registeredClientId: string | null = null; + + /** Point the server at a vault row that decrypts to something that is not a client. */ + async function pointAtUnreadableClient() { + const [server] = await database + .select({ credentialId: mcpServers.credentialId }) + .from(mcpServers) + .where(eq(mcpServers.id, rotationServerId)); + registeredClientId = server?.credentialId ?? null; + + const [credential] = await database + .insert(credentials) + .values({ + kind: "mcp_oauth_client", + provider: rotationServerId, + // Fresh per call, because `credentials_active_key_idx` holds one live row per + // (kind, provider, key_id) and the row this leaves behind is never revoked. + keyId: `oauth-client-unreadable-${randomUUID().slice(0, 8)}`, + metadata: {}, + encryptedValue: await encryptSecret( + ROTATION_KEY, + UNREADABLE_PLAINTEXT, + ), + }) + .returning({ id: credentials.id }); + if (!credential) throw new Error("unreadable client was not stored"); + vaultRows.push(credential.id); + + await database + .update(mcpServers) + .set({ credentialId: credential.id }) + .where(eq(mcpServers.id, rotationServerId)); + } + + /** Put the readable client back, so the tests after this one still have one. */ + async function restoreClient() { + await database + .update(mcpServers) + .set({ credentialId: registeredClientId }) + .where(eq(mcpServers.id, rotationServerId)); + } + + test("the trail of a refused call carries neither the plaintext nor the parser", async () => { + await connect(); + await pointAtUnreadableClient(); + try { + /* + * The throw is held rather than asserted on first, because what this test is about is the + * ROW. Asserting the thrown type up front would fail on the unguarded code before any + * durable store had been read, and report the wrong thing. + */ + const refusal = await rotationStore + .callTool({ + ref: rotationRef, + args: {}, + botId: rotationBotId, + actorId: rotationUserId, + }) + .then( + () => null, + (error: unknown) => error, + ); + + const failures = (await auditRowsFor(rotationRef)).filter( + (row) => row.eventType === "mcp.call_failed", + ); + const written = JSON.stringify(failures); + expect(written).not.toContain(UNREADABLE_PLAINTEXT); + /* + * The parser's vocabulary as well as the plaintext. A parser quotes only a window of its + * input — how wide is the runtime's business, not ours — so a message could carry a + * fragment the assertion above would miss, and any of these words reaching the trail means + * a parse wrote it. + */ + expect(written).not.toContain("JSON Parse error"); + expect(written).not.toContain("SyntaxError"); + expect(written).not.toContain("Unexpected"); + // And the operator is still told which thing is broken, in the trail and to the caller. + expect(written).toContain(UNUSABLE); + expect(refusal).toBeInstanceOf(PluginRefusedError); + } finally { + await restoreClient(); + } + }); + + test("a refresh leaves the same absence in the server's last error", async () => { + await connect(); + const [before] = await database + .select({ lastError: mcpServers.lastError }) + .from(mcpServers) + .where(eq(mcpServers.id, rotationServerId)); + await pointAtUnreadableClient(); + try { + // Refuses before the vendor is asked, so nothing here needs a reachable Notion. + expect( + await rotationStore.refreshTools(rotationServerId, rotationUserId), + ).toEqual({ tools: 0 }); + + const [after] = await database + .select({ lastError: mcpServers.lastError }) + .from(mcpServers) + .where(eq(mcpServers.id, rotationServerId)); + const written = after?.lastError ?? ""; + expect(written).not.toContain(UNREADABLE_PLAINTEXT); + expect(written).not.toContain("JSON Parse error"); + expect(written).not.toContain("SyntaxError"); + expect(written).not.toContain("Unexpected"); + expect(written).toContain(UNUSABLE); + } finally { + await restoreClient(); + await database + .update(mcpServers) + .set({ lastError: before?.lastError ?? null }) + .where(eq(mcpServers.id, rotationServerId)); + } + }); + }); }); /** @@ -2409,6 +2556,166 @@ describe("a dynamic client the vendor has evicted", () => { } }); + /** + * A stored client that parses cleanly and is not a client. + * + * The sibling of the unparseable row, and its worse half. Guarding the parse answers for SYNTAX + * only, and the `as OAuthClient` cast behind it answers for nothing — so a row holding + * snake_case keys, which is what a hand-repair or a half-written row leaves, yields a client + * whose `clientId` is `undefined` and is handed on as usable. + * + * WHAT MAKES IT WORSE THAN A SYNTAX ERROR is where it ends. The unparseable row is refused before + * the transaction; this one is not refused at all, so the `undefined` id goes to the vendor, the + * vendor answers `invalid_client`, and {@link refuseAndReplaceEvictedClient} reads that as the + * vendor having disowned this deployment's registration. A corrupt LOCAL row then buys a + * DEPLOYMENT-WIDE remedy: the client every existing consent was granted against is replaced, and + * the operator is told the vendor forgot us rather than which credential actually broke. + */ + describe("a stored OAuth client whose shape is not a client's", () => { + /** Snake_case where the type is camelCase, with a secret distinctive enough to search for. */ + const MISSHAPEN = JSON.stringify({ + client_id: "dyn-snake", + client_secret: `shh_notAClient_${suite}`, + }); + /** What the operator must be told instead: the credential named, and nothing else claimed. */ + const UNUSABLE = + "Notion has no usable OAuth client for this deployment. Connect Notion again in Settings: the deployment registers itself with the vendor on the next connect."; + + /** + * Plant arbitrary stored bytes as this server's client, the way {@link putClient} plants a real + * one — aged an hour, so the re-registration window is not what refuses the call. A row younger + * than the window would pass these tests for the wrong reason. + */ + async function putStoredBytes(plaintext: string) { + await database + .update(credentials) + .set({ revokedAt: new Date(), updatedAt: new Date() }) + .where( + and( + eq(credentials.kind, "mcp_oauth_client"), + eq(credentials.provider, dynamicServerId), + eq(credentials.keyId, `oauth-client-${dynamicServerId}`), + sql`${credentials.revokedAt} IS NULL`, + ), + ); + const [row] = await database + .insert(credentials) + .values({ + kind: "mcp_oauth_client", + provider: dynamicServerId, + keyId: `oauth-client-${dynamicServerId}`, + metadata: {}, + encryptedValue: await encryptSecret(DYNAMIC_KEY, plaintext), + createdAt: new Date(Date.now() - 60 * 60 * 1000), + }) + .returning({ id: credentials.id }); + if (!row) throw new Error("misshapen client was not stored"); + vaultRows.push(row.id); + await database + .update(mcpServers) + .set({ credentialId: row.id }) + .where(eq(mcpServers.id, dynamicServerId)); + return row.id; + } + + /** + * This tool's failure rows with their ids, so one call's can be told from the suite's. + * + * Every test in this describe calls the SAME tool, and the ones above this deliberately produce + * the eviction sentence — so an assertion that no failure row anywhere mentions it would be + * about its siblings rather than about this call. The ids are what separate them; there is no + * ordering finer than the millisecond these rows are written in. + */ + async function failureRows() { + return database + .select({ id: auditEvents.id, payload: auditEvents.payload }) + .from(auditEvents) + .where( + and( + eq(auditEvents.eventType, "mcp.call_failed"), + eq(auditEvents.targetType, "mcp_tool"), + eq(auditEvents.targetId, dynamicRef), + ), + ); + } + + /** Which credential the server row names, which is the thing a re-registration replaces. */ + async function pointedAt() { + const [row] = await database + .select({ credentialId: mcpServers.credentialId }) + .from(mcpServers) + .where(eq(mcpServers.id, dynamicServerId)); + return row?.credentialId ?? null; + } + + test("a call is refused, and the deployment's client is not replaced", async () => { + const planted = await putStoredBytes(MISSHAPEN); + await connect(); + const registeredBefore = await registeredRows(); + // The vendor would honour a fresh client, so registering is available here and would look + // like a recovery. The point is that it is never reached: there is nothing in this row for a + // vendor to refuse, so there is nothing to read as an eviction. + accepted = new Set([FRESH.clientId]); + issue = () => FRESH; + + await expect(call()).rejects.toThrow(UNUSABLE); + + // Refused before the exchange, so the vendor is never offered an `undefined` client id and + // never gets to answer `invalid_client` about it. + expect(offered).toEqual([]); + // And so the destructive remedy never runs. These are the property: a corrupt local row costs + // this one call, not every consent in the deployment. + expect(registrations).toEqual([]); + expect(await pointedAt()).toBe(planted); + expect((await registeredRows()).length).toBe(registeredBefore.length); + }); + + test("the refusal names the credential rather than carrying it", async () => { + await putStoredBytes(MISSHAPEN); + await connect(); + accepted = new Set([FRESH.clientId]); + issue = () => FRESH; + + const before = new Set((await failureRows()).map((row) => row.id)); + const refusal = await call().then( + () => null, + (error: unknown) => error, + ); + const written = JSON.stringify( + (await failureRows()).filter((row) => !before.has(row.id)), + ); + // The same absence the unparseable row is held to: a misshapen value is still the decrypted + // client, and half of this one IS a client secret. + expect(written).not.toContain(`shh_notAClient_${suite}`); + // Never the eviction sentence either. It would claim a re-registration that did not happen + // and point the operator at the vendor instead of at the row. + expect(written).not.toContain("no longer recognises"); + expect(written).toContain(UNUSABLE); + expect(refusal).toBeInstanceOf(PluginRefusedError); + }); + + /** + * The readers answer none, which is their existing contract for a value they cannot read. + * + * `ensureOAuthClient` consults the stored client first and then again under the lock, so both + * reads are on this path. Unguarded, the first hands back the misshapen object and a consent + * URL is built with an `undefined` client id — the person reaches a vendor screen for a client + * that does not exist. None is the answer that instead gets them a client that works. + */ + test("the consent flow reads it as none and obtains one that works", async () => { + await putStoredBytes(MISSHAPEN); + issue = () => FRESH; + + expect(await dynamicStore.oauthClientFor(dynamicServerId)).toBeNull(); + expect( + await dynamicStore.ensureOAuthClient( + dynamicServerId, + "admin@openbot.test", + ), + ).toEqual(FRESH); + }); + }); + /** * The vault row and the pointer that names it commit together, or neither does. *