Stop a decrypted OAuth client from being quoted back by its own parser

[mxmzb]

What

currentClient() parsed a decrypted OAuth client with an unguarded JSON.parse. When the stored value is not valid JSON, the parser reports failure by quoting the input it choked on — and that input is the decrypted client secret. The throw escapes into callTool's catch, which writes mcp.call_failed and sets mcp_servers.last_error.

So a client secret could be written into two durable stores, both of which the Plugins page draws for anybody who can read it.

Reproduced before fixing

With a vault row that decrypts cleanly to a non-JSON value — the shape a wrongly-encrypted row really has, a bare secret where a client object belongs:

audit_events.payload on mcp.call_failed:

"failure":"JSON Parse error: Unexpected identifier \"secret_notJsonClient…\""

mcp_servers.last_error:

JSON Parse error: Unexpected identifier "secret_notJsonClient…"

Two details worth recording, because they change the severity:

• Bun runs JavaScriptCore, not V8. JSC quotes the whole offending token. Real client secrets are identifier-shaped (secret_…, sk_…), so the entire secret is disclosed, not a fixed-width window. V8 would leak a smaller slice. Both leak.
• Truncated-but-valid-prefix JSON yields only Unterminated string and leaks nothing. The exposure comes from wrongly-encrypted, hand-edited or wrong-key rows rather than partial writes.

The fix

Guard only the parse, and never carry the parser's words.

The decrypt is hoisted out of the guard so secretFor's own refusal for a revoked or missing row is not caught and relabelled. The catch throws PluginRefusedError(unusableClient) — the message secretFor is already handed for an unusable client row.

This follows the two sibling readers in the same file, heldOAuthClient and storedOAuthClient, which both swallow this parse under "unreadable is the same as none". They return null because their callers are deciding whether a consent flow can start. currentClient() cannot: its contract is a StoredClient or a throw, and its single caller is mid-call and would convert a null into this same refusal one line later. So it raises the fact instead of returning it.

The same defect on the vendor-reply path was already fixed at exchangeRefreshTokenOverHttp, whose test suite describes this exact mechanism. That is the established local precedent this follows.

The operator signal survives. unusableClient is deliberately distinct from noClient — "holds a client it cannot use" versus "holds none" — and names connecting again as the remedy. An operator can still tell the credential is broken; nobody learns what was in it.

Tests

Two tests asserting the plaintext appears in neither the audit payload nor last_error. Both verified red against the unfixed code, quoting the secret verbatim in the failure. The guard was then mutated to catch (e) { throw e } and both failed identically, so they are load-bearing rather than passing by construction.

Sweep

store.ts has exactly three parses of decrypted material; the other two were already guarded, and this was the only one that was not. Nothing else in the file puts a decrypted value into a template, a log line or an error text. Checked outside the file too: credentials.ts's parseEnvelope, plugins/oauth.ts and agents/callback-token.ts all guard and none echo ciphertext. No further instances.

Scope

Found during an unrelated review of the Composio transport branch, verified to predate it, and deliberately split out so a security fix is not tangled with a large feature branch.

🤖 Generated with Claude Code

https://claude.ai/code/session_01P2mm9gARmD13tnasEztb4x

[guidovizoso]

Approving — the fix is correct and well-scoped.

What I checked rather than took on trust:

• The decrypt really is hoisted out of the try, so secretFor's own revoked/not-found refusal isn't caught and relabelled. JSON.parse is the only statement inside the guard that can throw.
• currentClient() has exactly one call site (store.ts:933), before the transaction and outside the refuseAndReplaceEvictedClient catch — so the new PluginRefusedError can't be mistaken for a vendor invalid_client and can't trigger a re-registration. (The description says "both its callers"; there's only one. Harmless.)
• The sweep holds: three parses of decrypted material in this file (877, 1301, 1436), and the other two already swallow. The credentials.ts error strings carry no ciphertext or plaintext, so the rethrow path is clean.
• The tests aren't vacuous: listNeedsCredential is true for MCP so refreshTools refuses inside connectionTokenFor before any network call; recordConnection doesn't call refreshTools, so connect() can't wipe the suite's tool row; the fresh keyId per call is required by the unique partial index in drizzle/0015_credentials_one_live_key.sql; and auditRowsFor is scoped to the suite-suffixed ref with no earlier mcp.call_failed row that could satisfy the toContain(UNUSABLE) assertion for free.
• tsc --noEmit and biome check clean on this head. I didn't run the integration tests — locally they target the live dev database.

One nice-to-have inline, on the shape of the parsed client. Pre-existing and not a blocker.

[mxmzb]

Done in e9b3872. You were right, and the destructive path is real — I reproduced it before guarding it rather than taking the reasoning on trust. Against ebf3bf8 unmodified, with a snake_case stored client aged past the re-registration backoff so that was not what stopped it:

offered to vendor:  [null]                          <- client.clientId undefined
operator was told:  "Notion no longer recognises this deployment's OAuth client ...
                     The deployment has registered itself again"
client now held:    {"clientId":"dyn-2","clientSecret":""}    <- replaced
trail:              mcp.oauth_client_registered, actor "deployment"

Corrupt local row, deployment-wide remedy, and the credential that actually broke never named. No guard, no narrowing, no backoff caught it.

One correction to what I was going to do. I had this as "both fields present and non-empty". Non-empty on the secret would have been a regression: registerDynamicClient (oauth.ts:508-514) validates the id non-empty and deliberately defaults the secret to "", because a public DCR client proves itself with PKCE — both exchangeRefreshTokenOverHttp and redeemAuthorizationCode omit the field when it is falsy, and every DCR fixture in the repo including EVICTED/FRESH is clientSecret: "". So the criterion is id a non-empty string, secret a string that may be empty. Your example row is still caught, on the id.

The check is one shared predicate, isUsableClient, carrying the criterion and the reason, called from all three readers — so the reasoning is not triplicated where a later edit would update one copy and miss two. All three as OAuthClient casts are gone, replaced by a type predicate rather than papered over. currentClient raises (contract is a client or a throw, and its caller is mid-call); heldOAuthClient and storedOAuthClient return null, following the "unreadable is the same as none" comments each already carries.

It also closes a quieter one at the null sites: unguarded, oauthClientFor handed the misshapen object straight back, so a consent URL was built with client_id=undefined and the person reached a vendor screen for a client that does not exist.

Three tests, all red first — the refusal naming the credential instead of carrying it, the client not being replaced, and the consent flow reading it as none. 60 pass on a fresh database, format:check/lint/typecheck clean.

Two things you flagged, both correct:

• "both its callers" — there is one. Fixed in the description.
• Worth noting the OAuthClient I reasoned from is store.ts:482, its own local declaration; config.ts:80 is a separate, identically-shaped type for auth providers. Same camelCase, so the conclusion holds, but the duplication is real.

One pre-existing failure you should know about, unrelated to this change: credential material is refused and never copied into the audit trail passes alone and fails in a full local server/tests run. I confirmed it fails identically on origin/main with nothing of mine applied — it reads all audit rows for the stable ref google-drive/search_files with no run scoping, so on a reused database it matches an earlier run's row. CI uses a fresh database, so CI does not see it.