From ebf3bf8c052ae50af12cbfcb8bdf4a484cf5adb7 Mon Sep 17 00:00:00 2001 From: Maxim Date: Thu, 10 Sep 2026 04:57:50 +0200 Subject: [PATCH 1/2] Stop a decrypted OAuth client from being quoted back by its own parser `currentClient` parsed the deployment's OAuth client straight out of the vault with nothing around the parse. `JSON.parse` reports failure by quoting the input it choked on, and the input there is the DECRYPTED client -- so a row that is corrupt, wrongly encrypted or half written threw a SyntaxError carrying the client secret, out into `callTool`'s catch and `refreshTools`' catch, which write it to the `mcp.call_failed` payload and to `mcp_servers.last_error`. Both are durable and both are drawn on the Plugins page. Under Bun's parser a stored value that is a bare token comes back whole: JSON Parse error: Unexpected identifier "secret_notJsonClient..." The two sibling readers in the same file already guard this exact parse and answer null, because unreadable is the same as none. This one refuses instead, since its callers are mid-call and would have to turn a null into that refusal on the next line anyway. It raises `unusableClient`, which `secretFor` already raises for a revoked or missing row, so the operator still learns the credential is broken -- distinct from `noClient`'s holding none -- without any of the bytes. The decrypt stays outside the guard, so `secretFor`'s own refusal is not caught and relabelled. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P2mm9gARmD13tnasEztb4x --- server/src/plugins/store.ts | 38 ++++- server/tests/plugin-store.integration.test.ts | 147 ++++++++++++++++++ 2 files changed, 179 insertions(+), 6 deletions(-) diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index 410a25beb..e290b444f 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -867,12 +867,38 @@ 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); + try { + return { + client: JSON.parse(decrypted) as OAuthClient, + registeredAt: server.registeredAt, + }; + } 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); + } } /* diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index da407716b..be55a6037 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)); + } + }); + }); }); /** From e9b38726236a54b64c55aee44494172613501122 Mon Sep 17 00:00:00 2001 From: Maxim Date: Thu, 10 Sep 2026 17:29:19 +0200 Subject: [PATCH 2/2] Refuse a stored OAuth client that is not one in shape, not only in syntax Guarding the parse answered for SYNTAX and left the `as OAuthClient` cast behind it answering for nothing, at all three readers. A vault row holding `{"client_id":"","client_secret":""}` -- snake_case where the type is camelCase, or empty, as a hand-repair or a half-written row leaves it -- parses cleanly and yields a client whose `clientId` is `undefined`. SHAPE AND SYNTAX ARE ONE CONCERN: either way the deployment holds a client it cannot use, and the operator's signal has to survive both. `unusableClient` already says exactly that, and is deliberately distinct from `noClient`'s holding none. The shape half was ending WORSE than the syntax half the guard was written for. The syntax refusal happens before the transaction and outside the eviction catch, so it cannot be mistaken for a vendor's `invalid_client`. A misshapen client was not refused at all: `currentClient` returned it, the exchange inside the transaction sent an `undefined` client id, and the vendor's `invalid_client` WAS caught -- reaching `refuseAndReplaceEvictedClient`, which read it as the vendor having disowned our registration and, past the re-registration backoff, called `registerClient` and `persistOAuthClient({ by: "deployment" })`. So a corrupt LOCAL row replaced the deployment-wide client every existing user's consent was granted against, and told the operator `clientReplaced` -- the vendor forgot us -- rather than naming the credential that actually broke. Reproduced before it was closed: offered to vendor: [null] operator was told: "Notion no longer recognises this deployment's OAuth client, so this cannot be called. The deployment has registered itself again -- connect Notion again in Settings." registrations: [{ registrationUrl: ".../register", redirectUri: ".../callback" }] client now held: { clientId: "dyn-2", clientSecret: "" } The criterion and its reason live in one place, `isUsableClient`, because all three readers share both and a future reader changing one would otherwise miss the others. Each site keeps the answer it already gave: `currentClient` raises, since its contract is a client or a throw and its callers are mid-call; `heldOAuthClient` and `storedOAuthClient` answer null under their existing "unreadable is the same as none" comments, because their caller's response to none is to go and register one. 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 this same way and defaults the secret to `""` -- so demanding a non-empty secret would have refused every self-registering entry in the catalogue, which is most of them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P2mm9gARmD13tnasEztb4x --- server/src/plugins/store.ts | 68 +++++++- server/tests/plugin-store.integration.test.ts | 160 ++++++++++++++++++ 2 files changed, 220 insertions(+), 8 deletions(-) diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index e290b444f..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. * @@ -872,11 +906,9 @@ export function createPluginStore(options: PluginStoreOptions) { * missing row is not caught here and relabelled. Only the parse is guarded. */ const decrypted = await secretFor(server.credentialId, unusableClient); + let parsed: unknown; try { - return { - client: JSON.parse(decrypted) as OAuthClient, - registeredAt: server.registeredAt, - }; + parsed = JSON.parse(decrypted); } catch { /* * Unreadable is the same as none, exactly as it is for {@link heldOAuthClient} and @@ -899,6 +931,19 @@ export function createPluginStore(options: PluginStoreOptions) { */ 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 }; } /* @@ -1298,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; @@ -1433,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 be55a6037..b6f442e94 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -2556,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. *