MCP OAuth Stage 3: /oauth/mcp/authorize endpoint (PKCE-S256) by heskew · Pull Request #99 · HarperFast/oauth

heskew · 2026-05-21T20:19:54Z

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.ts — handleAuthorize 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.ts — handleCallback 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.ts — MCPAuthorizeState (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. The plugin now requires mcp.issuer at startup when mcp.enabled (see Rebase + hardening below), so the issuer and audience can't be derived from the client-controlled Host header.
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:

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.
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.
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.
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.

Rebase + hardening (2026-06-09)

Rebased onto current main (was ~10 commits behind — the alpha.2 release + dynamic-provider-cache work); clean, zero conflicts. Then ran a full cross-model review (Codex + Harper-domain pass; the Gemini leg was unavailable this run). No blockers. It surfaced one significant concern and one suggestion, both now fixed in this PR (commit 609b130):

Require mcp.issuer when mcp.enabled (fail fast at startup). With neither mcp.issuer nor mcp.resource pinned, both were derived from the client-controlled Host header — letting a client influence the advertised iss and the aud bound into the auth code (audience confusion once Stage 4 signs tokens). A pinned issuer anchors both (resource defaults to <issuer>/mcp); a pinned resource alone is not sufficient — it leaves iss Host-derived. Codex caught that subtlety on the first cut.
Reject a provider keyed mcp. The /oauth/mcp/* dispatcher reserves that path segment, so a provider named mcp would silently 404 at request time. Now rejected loudly at config load.

Deferred to Stage 4: client-supplied scope must be intersected against permitted scopes before it lands in a JWT (it's only stored, not consumed, in this PR).

Test plan

608 unit tests pass locally (npm test)
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

/oauth/mcp/token exchange (Stage 4, MCP OAuth Stage 4: /oauth/mcp/token endpoint + JWT issuer #94) — codes from this PR are unused until that lands
JWT signing keys + JWKS population (Stage 4)
withMCPAuth wrapper that consumes Stage 4 tokens (Stage 5, MCP OAuth Stage 5: withMCPAuth wrapper #95)
Audit hooks (Stage 6, MCP OAuth Stage 6: audit logging + onMCPTokenIssued hook #96)
Multi-provider chooser UI (v1.1)

🤖 Generated with Claude Code

github-actions · 2026-05-21T20:22:23Z

Reviewed; no blockers found.

Suggestions (non-blocking)

src/lib/mcp/authorize.ts:182 — The CODE_CHALLENGE_PATTERN regex accurately implements the RFC 7636 §4.2 ABNF for code_challenge. While the S256 method specifically results in 43 base64url characters, keeping the broader unreserved set check is a robust approach that aligns with the spec's general syntax requirements.
src/lib/handlers.ts:145 — The handleCallback integration of the MCP branch correctly preserves the independent lifecycle for MCP (no Harper session created) while still allowing the onLogin hook to map the user identity. This maintains the security invariants established in earlier architectural decisions.

claude · 2026-05-21T20:22:59Z

Reviewed; no blockers found.

heskew

No blockers.

🤖 Posted by Antigravity on Nathan's behalf

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 · 2026-06-09T14:52:13Z

Pushed a follow-up commit (609b130): rebased onto current main (was ~10 commits behind) and applied two fail-fast guards surfaced by a cross-model review (Codex + Harper-domain pass):

Require mcp.issuer when mcp.enabled — otherwise iss/aud derive from the client-controlled Host header. A pinned resource alone isn't enough (leaves iss Host-derived).
Reject a provider keyed mcp (reserved by the /oauth/mcp/* dispatcher).

No blockers. Full write-up in the description under Rebase + hardening. 608 unit tests pass.

🤖 Posted by Claude on Nathan's behalf

Cross-model review (Codex + Harper-domain) of the Stage 3 authorize endpoint surfaced two hardening gaps: - MCP issuer/audience could fall back to the client-controlled Host header when unpinned. Fail fast at startup: require `mcp.issuer` when `mcp.enabled` (a pinned issuer anchors both `iss` and the default `<issuer>/mcp` resource; a pinned `resource` alone leaves `iss` Host-derived). Throws on initial load before provider state is mutated; caught + logged on a later options change. - A provider keyed `mcp` is shadowed by the `/oauth/mcp/*` dispatcher and would silently 404 at request time. Reject the collision loudly in initializeProviders. Docs + tests updated. 608 unit tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

heskew

Codex deep-review pass: I found one small Stage 3 contract gap around PKCE validation. Everything else I traced in the authorize/callback bridge looked consistent with the PR scope.

kriszyp

Solid Stage 3 implementation — PKCE-S256 flow looks correct, token exchange and state validation are well-structured.

Reviewed by claude-sonnet-4-6.

The /oauth/mcp/authorize endpoint checked code_challenge presence and the S256 method, but not the value's syntax. A malformed challenge (e.g. `code_challenge=x`) passed both checks and reached the upstream IdP, burning a full user login before Stage 4 could reject the unusable verifier. Add a 43-128 unreserved-charset check (RFC 7636 §4.2) that fails fast with invalid_request before the upstream redirect, plus too-short / too-long / invalid-character tests. Addresses the Stage 3 deep-review finding on #99. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions · 2026-06-18T19:24:08Z

+export async function handleAuthorize(
+	request: Request,
+	target: RequestTarget,
+	mcpConfig: MCPConfig,


💡 Suggestion (non-blocking) — code_challenge regex alignment

The regex accurately mirrors the RFC 7636 ABNF for the unreserved set, providing robust syntax validation before the upstream redirect.

github-actions · 2026-06-18T19:24:09Z

+		url.searchParams.set('error_description', description);
+		if (mcp.clientState) url.searchParams.set('state', mcp.clientState);
+		return { status: 302, headers: { Location: url.toString() } };
+	};


💡 Suggestion (non-blocking) — MCP lifecycle independence

The MCP branch correctly skips session updates, ensuring MCP and human-session lifecycles remain independent as per the project requirements.

…libration) (#125) Picks up the calibration fix that stops the Gemini Suggestions tier from emitting affirmations ("accurately implements…", "correctly handles…") — a Suggestion must now propose a concrete change; validation/"what I verified" routes to the log's Surfaces verified, never the PR. Surfaced by dogfooding the inline reviewer on oauth #99. Includes #65 (inline comments) already on the prior pin. Claude pin unchanged. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

heskew requested a review from kriszyp May 21, 2026 20:20

heskew marked this pull request as ready for review May 21, 2026 20:28

heskew mentioned this pull request May 21, 2026

prompts(gemini): harden no-blocker shape; ban praise phrases HarperFast/ai-review-prompts#43

Closed

3 tasks

heskew commented May 21, 2026

View reviewed changes

heskew force-pushed the mcp/authorize-endpoint branch from 20e4123 to 06527e1 Compare June 8, 2026 23:43

heskew force-pushed the mcp/authorize-endpoint branch from 609b130 to e8e7bbd Compare June 9, 2026 14:54

heskew force-pushed the mcp/authorize-endpoint branch from e8e7bbd to eb707ba Compare June 9, 2026 15:00

heskew commented Jun 10, 2026

View reviewed changes

Comment thread src/lib/mcp/authorize.ts

kriszyp approved these changes Jun 17, 2026

View reviewed changes

github-actions Bot reviewed Jun 18, 2026

View reviewed changes

heskew mentioned this pull request Jun 18, 2026

calibration: non-blocking Suggestions must propose a change, never affirm HarperFast/ai-review-prompts#66

Merged

heskew merged commit 7973b05 into main Jun 18, 2026
10 checks passed

heskew deleted the mcp/authorize-endpoint branch June 18, 2026 19:34

heskew mentioned this pull request Jun 18, 2026

ci: bump Gemini pin to main@48e539a (#66 — no-affirmation calibration) #125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

MCP OAuth Stage 3: /oauth/mcp/authorize endpoint (PKCE-S256)#99
heskew merged 3 commits into
mainfrom
mcp/authorize-endpoint

heskew commented May 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 21, 2026 •

edited

Loading

Uh oh!

claude Bot commented May 21, 2026

Uh oh!

heskew left a comment

Uh oh!

heskew commented Jun 9, 2026

Uh oh!

heskew left a comment

Uh oh!

Uh oh!

kriszyp left a comment

Uh oh!

github-actions Bot Jun 18, 2026

Uh oh!

github-actions Bot Jun 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

heskew commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Where to look

Design decisions worth flagging

What was reviewed

Rebase + hardening (2026-06-09)

Test plan

Out of scope

Uh oh!

github-actions Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Suggestions (non-blocking)

Uh oh!

claude Bot commented May 21, 2026

Uh oh!

heskew left a comment

Choose a reason for hiding this comment

Uh oh!

heskew commented Jun 9, 2026

Uh oh!

heskew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kriszyp left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

heskew commented May 21, 2026 •

edited

Loading

github-actions Bot commented May 21, 2026 •

edited

Loading