Skip to content

MCP OAuth Stage 3: /oauth/mcp/authorize endpoint (PKCE-S256)#99

Open
heskew wants to merge 1 commit into
mainfrom
mcp/authorize-endpoint
Open

MCP OAuth Stage 3: /oauth/mcp/authorize endpoint (PKCE-S256)#99
heskew wants to merge 1 commit into
mainfrom
mcp/authorize-endpoint

Conversation

@heskew
Copy link
Copy Markdown
Member

@heskew heskew commented May 21, 2026

Drafted by Claude (Opus 4.7, 1M context) on Nathan's behalf, following the harper-engineering-guidelines development lifecycle. Code, tests, the pre-push Gemini cross-review, and this description are all agent-authored; pushed under Nathan's account because the agent runs locally on his machine. Human review expected before merge.

Closes #93. Sub-issue of #86.

Summary

Stage 3 of MCP OAuth. Adds the user-facing authorize endpoint plus the surrounding plumbing — auth-code table with native TTL, bridge through the existing upstream IdP flow, callback branch that mints the code and redirects to the MCP client. No JWTs yet (that's Stage 4 / #94).

Where to look

  • src/lib/mcp/authorize.tshandleAuthorize with two-phase validation per OAuth 2.1 §3.1.2.5: client_id + redirect_uri must validate before any redirect; everything else routes errors to the verified redirect_uri. The interesting helper is redirectUriMatches — implements RFC 8252 §7.3 loopback port flexibility for 127.0.0.1 / [::1] / localhost. Native MCP clients bind dynamic ports at runtime and can't pre-register them.
  • src/lib/handlers.tshandleCallback restructured. CSRF state is now verified BEFORE the upstream error param is processed, so MCP-initiated errors (user denied at GitHub, etc.) route back to the MCP client's redirect_uri with OAuth-spec error codes instead of bouncing to the Harper app's default path. The legacy "no state, has error" UX is preserved for human-OAuth flows. On the success path, the onLogin-mapped username (hookData?.user ?? user.username) flows into the auth-code record, so Stage 4's JWT inherits the mapped identity.
  • src/lib/mcp/callback.ts — the MCP branch from handleCallback. Mints the code with crypto.randomBytes(32).toString('base64url'), persists the binding fields (client_id, user, resource, PKCE challenge, redirect_uri, scope), redirects to the client URI. No Harper session is created on this branch (independent MCP/human lifecycle per Add MCP OAuth flow support #86 resolved decision). Asserts no upstream IdP token leaks into the redirect URL.
  • src/lib/mcp/authCodeStore.ts — CRUD against the new mcp_auth_codes table. Explicit field access in encode/decode (CLAUDE.md tracked-object spread gotcha).
  • schema/oauth.graphql — new mcp_auth_codes table with @table(database: "oauth", expiration: 300) for native TTL.
  • src/types.tsMCPAuthorizeState (carried through CSRF metadata) and MCPAuthCodeRecord. New mcp.providers config field.

Design decisions worth flagging

  • Single-provider constraint for v1. mcp.providers (or, when unset, the full registry) must resolve to exactly one effective provider; 0 or >1 returns server_error. Multi-provider chooser UI is v1.1. Documented in the selectMCPProvider helper.
  • resource param exact match. Validated against resolveResource(request, mcpConfig) from Stage 2 (same Host-header caveat applies — pin mcp.resource in production).
  • Auth code storage. Single Harper table with expiration: 300 — 5-minute TTL is a safety net; Stage 4's /token will read-then-delete on exchange for one-time use.

What was reviewed

Gemini CLI 0.42.0 cross-reviewed the pre-push diff. Four real findings, all addressed before push:

  1. RFC 8252 §7.3 loopback port flexibility — original code did exact-string match on redirect_uri, breaking native clients. Now uses port-flexible matching on loopback hosts. Tests added covering all loopback variants.
  2. onLogin username mapping lost in MCP branch — auth code was binding the raw OAuth username, ignoring the mapped identity from the hook. Now passes hookData?.user ?? user.username through.
  3. handleCallback error ordering — upstream IdP error was handled before CSRF verification, so MCP errors bounced to the Harper default path instead of the MCP client. CSRF verify is now first; legacy no-state UX preserved.
  4. MCP-unaware error redirects — the cross-provider mismatch and catch blocks ignored tokenData.mcp and used originalUrl || postLoginRedirect (undefined for MCP flows). Both branches now route to the MCP client redirect_uri when mcpState is present.

One Gemini point (resource URI stability via Host header) was informational — already covered by Stage 2's documentation.

Test plan

  • 603 unit tests pass locally (npm test); +51 from this PR
  • npm run lint clean
  • npm run format:check clean
  • No regression in the human-OAuth callback path — existing handleCallback tests cover the legacy behavior including the "no state, has error" preservation
  • End-to-end verification deferred to Stage 7 (MCP OAuth Stage 7: end-to-end integration fixture #97) — mcp-remote driving the full discovery → DCR → authorize → token → bearer call is the conformance proof

Out of scope

🤖 Generated with Claude Code

Closes #93. Stage 3 of MCP OAuth support (parent #86). Bridges incoming
MCP authorization requests into the existing upstream-IdP flow, mints a
single-use authorization code on the upstream callback, and redirects
the user-agent back to the MCP client's redirect_uri.

- New `mcp_auth_codes` Harper table (`@table(expiration: 300)`) — codes
  auto-expire after 5 min via native TTL; Stage 4's /token will
  read-then-delete for one-time use
- New MCPAuthCodeStore with the explicit-field-access encode/decode
  pattern (CLAUDE.md tracked-object spread gotcha)
- New handleAuthorize: two-phase validation per OAuth 2.1 §3.1.2.5 —
  client_id + redirect_uri exact-match before any redirect, everything
  else returns errors to the verified redirect_uri
- RFC 8252 §7.3 loopback port flexibility — native MCP clients (Claude
  Desktop, mcp-remote) bind a dynamic port at runtime; we accept any
  port on registered loopback URIs (127.0.0.1 / [::1] / localhost)
- PKCE S256 only (OAuth 2.1 §7.5.2 forbids plain); reject `plain` with
  invalid_request to the client URI
- RFC 8707 `resource` parameter required, validated against canonical
  resource URI from Stage 2
- v1 single-provider constraint: mcp.providers (or the full registry)
  must resolve to exactly one upstream provider; 0 or >1 returns
  server_error. Multi-provider chooser is v1.1
- Bridge via CSRF state: the existing CSRFTokenData carries an `mcp`
  payload that handlers.ts handleCallback detects and detours into
  handleMCPCallback (callback.ts) — mints auth code, redirects to
  MCP client. Upstream IdP token never reaches the MCP client (spec
  MUST NOT, MCP authorization spec 2025-06-18)
- handleCallback restructured: verify CSRF state FIRST so MCP-initiated
  upstream errors route back to the MCP client redirect_uri instead of
  the Harper app's default path. Legacy human flow behavior preserved
  for the no-state edge case
- onLogin-mapped username (hookData.user) flows through to the issued
  auth code — Stage 4's JWT inherits the mapped identity, not the raw
  OAuth username
- No Harper session is created on the MCP branch (independent
  lifecycle per #86 resolved decision)

Gemini cross-review caught 4 of the above before push (loopback ports,
mapped username, CSRF-before-IdP-error ordering, MCP-aware error
redirects); all addressed in this PR before opening.

603 unit tests pass locally (+51 from this PR), including new MCP-branch
integration tests in handlers.test.js.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@heskew heskew requested a review from kriszyp May 21, 2026 20:20
@github-actions
Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

The PR successfully implements the MCP Authorization Server flow (Stage 2 of issue #86), including the core /oauth/mcp/authorize endpoint with proper Phase 1/Phase 2 validation separation and RFC 8252 loopback port flexibility. The implementation respects Harper's Resource API v2 and maintains strict security boundaries between MCP and human-session lifecycles.

Surfaces verified:

  • Authorization Flow: Traced /oauth/mcp/authorize from OAuthResource dispatch to handleAuthorize. Confirmed that Phase 1 (client/redirect-URI) fails with 400 JSON while Phase 2 (protocol/PKCE/resource) correctly redirects back to the client with error parameters.
  • Callback Handling: Verified the branching in handleCallback to handleMCPCallback. Confirmed that the onLogin hook is preserved and that the issued auth code is bound to the correctly mapped Harper identity.
  • Security Invariants: Validated RFC 8707 resource binding, PKCE S256 requirement, and the 2048-character path limit. Confirmed that cross-provider CSRF protection is correctly routed back to the MCP client.
  • Storage Safety: Verified that MCPAuthCodeStore and MCPClientStore use explicit field copying to avoid the GenericTrackedObject spread gotcha.
  • Testing: The new test suites are thorough, especially in simulating Harper's Proxy-based tracked objects and verifying RFC-specific edge cases like loopback port selection.

@claude
Copy link
Copy Markdown

claude Bot commented May 21, 2026

Reviewed; no blockers found.

Copy link
Copy Markdown
Member Author

@heskew heskew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers.


🤖 Posted by Antigravity on Nathan's behalf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCP OAuth Stage 3: /oauth/mcp/authorize endpoint

1 participant