MCP OAuth Stage 1: Dynamic Client Registration (RFC 7591)

[heskew]

Summary

Stage 1 of MCP OAuth support for #86. Adds the persistent client registry plus the POST /oauth/mcp/register endpoint. No /authorize, no /token, no withMCPAuth yet — those are Stages 3–5.

The endpoint is dispatched through OAuthResource.post when providerName === 'mcp'. Stage 4 will extend the same dispatcher for /token.

Why

MCP clients (Claude Desktop, Cursor, mcp-remote) register at runtime with no pre-baked client_id and cache the issued ID across launches. A persistent server-side store is the foundation for everything downstream — without it the spec-compliant DCR flow can't even start, and Claude Desktop silently breaks across deploys.

Where to look

• src/lib/mcp/dcr.ts — RFC 7591 validation logic. Note the deliberate deviation from the RFC's client_secret_basic default (we default to none because MCP clients are overwhelmingly public; documented at the call site).
• src/lib/mcp/clientStore.ts — table-backed CRUD. Array fields (redirect_uris, grant_types, etc.) JSON-encode on write/decode on read, matching the existing csrf_tokens.data pattern. null from the DB is normalized to undefined on read.
• schema/oauth.graphql — new harper_oauth_mcp_clients table. No expiration: registrations must outlive restarts so cached client_ids in MCP clients keep working.
• src/lib/config.ts + src/index.ts — new expandEnvVarsDeep walks string leaves on the mcp config block so secrets like mcp.dynamicClientRegistration.initialAccessToken support ${ENV_VAR} like every other secret field in the plugin.

What was reviewed

Gemini CLI 0.42.0 cross-reviewed the diff against RFC 7591 and surfaced four real issues, all addressed in this PR before pushing:

1. mcp config block was skipping env-var expansion → fixed via expandEnvVarsDeep
2. Scalar metadata fields (client_name, logo_uri, software_id, software_version, etc.) weren't validated as strings → could persist arbitrary objects → fixed
3. decodeRecord returned null for array-valued fields when the DB stored null → fixed (normalized to undefined)
4. Redundant capitalized Authorization header check (Node's HTTP parser lowercases) → dropped, test updated to reflect production contract

Test plan

• All 516 unit tests pass locally (npm test); +7 from this PR (expandEnvVarsDeep, scalar validation)
• npm run lint clean
• npm run format:check clean
• Manual verification of the endpoint against a real MCP client (deferred to Stage 7 — full end-to-end integration fixture; meaningful only once /authorize + /token land)
• Confirm replicated Harper deployments share the registry across nodes (deferred to Stage 7 integration)

Out of scope

• /.well-known/oauth-protected-resource, /.well-known/oauth-authorization-server, /.well-known/jwks.json — Stage 2
• /oauth/mcp/authorize — Stage 3
• /oauth/mcp/token + JWT signing — Stage 4
• withMCPAuth wrapper — Stage 5
• Refresh-token revocation, audit hooks — Stages 4/6
• End-to-end fixture against an MCP client — Stage 7

🤖 Generated with Claude Code

[github-actions]

Reviewed; no blockers found.

[claude]

Reviewed; no blockers found.

[heskew]

Addressed both bot reviews in acc5147. All three flagged blockers were real:

1. Constant-time initialAccessToken compare (Claude, src/lib/mcp/dcr.ts:43) → timingSafeEqual from node:crypto, length-checked first.

2. Spread on Harper tracked object dropped scalar fields (Gemini, src/lib/mcp/clientStore.ts:55) → the exact gotcha called out in CLAUDE.md. { ...raw } against a GenericTrackedObject Proxy produces {}, so client_id and every other scalar would have vanished on read in production. encodeRecord and decodeRecord rewritten to use explicit field access. The test mock now wraps stored rows in a tracked-object Proxy so this can't regress, with an explicit "preserves scalar fields" assertion as a tripwire.

3. handleMCPPost dispatcher untested (Claude, src/lib/mcp/index.ts:28) → new test/lib/mcp/index.test.js covers the master enable gate (4 cases), action routing (3 cases), and the layered deny when MCP is enabled but DCR is explicitly off. Each deny path asserts the protected action did not run.

525 unit tests now passing locally (+9 from these fixes). CI re-running.

---

🤖 Posted by Claude on Nathan's behalf

[heskew]

Addressed Gemini's second-round finding in f1185e8, partially.

The docs gap is real for one specific reason: mcp.dynamicClientRegistration.initialAccessToken ships open-by-default per RFC 7591, and an admin reading the README post-merge has no way to learn it exists or how to lock it down. That's a security-visibility issue, not a docs nicety.

So this PR now adds:

• README "Database Schema": one paragraph naming harper_oauth_mcp_clients with a pointer to Add MCP OAuth flow support #86
• README "Security Considerations": a bullet explicitly flagging open-by-default DCR + the two gates (initialAccessToken, allowedRedirectUriHosts)
• docs/configuration.md MCP OAuth section: the mcp config block, sub-fields table, env-expansion note, and an in-line warning that without initialAccessToken registration is open

What's intentionally NOT in this PR: full MCP API walkthrough, /authorize//token docs, withMCPAuth usage, quickstart, OAuth dance diagram. Those are deferred to Stage 8 of #86 — documenting one third of an unusable flow (PR #89 ships only /register; /authorize and /token land in Stages 3–4) teaches integrators something they can't act on, which is worse than no docs. The marker "(work in progress)" in the section heading signals this.

---

🤖 Posted by Claude on Nathan's behalf