docs: Add PRD-060 MCP OAuth session unification#2145
Conversation
Replaces MCP's direct Dex login redirect with a UI consent flow that reuses the existing browser session. Eliminates double-login, removes Dex as a user-facing surface for MCP auth.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdates the Changes
Sequence Diagram(s)sequenceDiagram
actor UserAgent as Browser
participant MCP
participant SPA as "Meridian SPA"
participant BFF
participant Store as "ConsentCodeStore"
UserAgent->>MCP: GET /mcp/authorize (create OIDCFlowState)
MCP->>UserAgent: 302 Redirect to /auth/mcp-consent?mcp_state=...&client_id=...
UserAgent->>SPA: GET /auth/mcp-consent (load consent UI)
SPA->>MCP: GET /mcp/consent-info?mcp_state=...&client_id=...
MCP-->>SPA: return trusted client metadata (name, redirect_uri, scopes, is_dynamic)
UserAgent->>SPA: user approves
SPA->>BFF: POST /api/auth/mcp-consent (Bearer) {approve, mcp_state, client_id, identity}
BFF->>Store: create one-time consent code (store with TTL)
Store-->>BFF: consent_code
BFF->>UserAgent: 302 Redirect to MCP callback ?consent_code=...&mcp_state=...
UserAgent->>MCP: GET /mcp/callback?consent_code=...&mcp_state=...
MCP->>Store: consume consent_code (atomic)
Store-->>MCP: consent payload (identity, scopes, client_id)
MCP->>MCP: validate mcp_state/client_id, issue scoped session
MCP-->>UserAgent: final redirect (logged-in session)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Claude Code ReviewCommit: SummaryWell-structured PRD that cleanly supersedes PRD-044's Option A (tenant-scoped Dex redirects) with a simpler approach that eliminates Dex as a user-facing surface entirely. The document has been iterated through multiple rounds of feedback — the latest commit addresses return_url open-redirect protection and the invalid_action error contract. The PRD is internally consistent: the end-to-end flow, component changes, security requirements, acceptance criteria, and API contracts all align. The cross-reference to PRD-044 is explicit about what it supersedes and what remains independently needed (BFF SSO fix, HandlerOptionalTenant). The deployment topology constraint (in-memory stores require unified binary) is clearly documented with a forward path for separate deployment. One undescribed change ( Risk Assessment
Findings
Bot Review NotesCodeRabbit thread on line 278 ( All other CodeRabbit threads (explicit action enum, tenant binding type mismatch, invalid_action error) were resolved in prior commits. Questions for the Author
Previously Flagged
|
- Fix Caddy routing conflict: consent page at /auth/mcp-consent (not /oauth/consent which Caddy routes to MCP server) - Fix consent-info endpoint: /mcp/consent-info (under Caddy's MCP matcher) - Clarify in-memory store sharing requires unified binary; document deployment topology constraint for separate MCP container - Add PRD-044 cross-reference: this supersedes PRD-044 MCP auth approach; PRD-044 BFF SSO fix remains needed independently
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/prd/061-mcp-auth-session-unification.md (1)
126-130: Add abuse controls for unauthenticatedconsent-infoendpoint.Because this endpoint is unauthenticated and keyed by query params, the PRD should explicitly require rate limiting and non-enumerable error behavior to reduce state probing/oracle risk.
Also applies to: 280-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/061-mcp-auth-session-unification.md` around lines 126 - 130, Update the GET /mcp/consent-info endpoint spec to mandate abuse controls: add explicit rate-limiting requirements for the unauthenticated, query-param-keyed endpoint (e.g., per-IP and per-client_id buckets) and require "non-enumerable" error responses when mcp_state or client_id are missing/invalid (do not reveal existence or validity of state or client_id in error bodies or status codes). Reference the existing mcp_state validation and client_id equality check in the description and state that dynamically registered flagging (is_dynamic) remains allowed but must not be used to leak verification status; apply the same controls to the other similar clause mentioned for the lines covering the second instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/061-mcp-auth-session-unification.md`:
- Around line 149-150: The PRD currently accepts an optional boolean
`{mcp_state, client_id, denied?}` which risks accidental approval when omitted;
change the consent payload to require an explicit action enum/string (e.g.,
`{mcp_state, client_id, action}` where action ∈ {"approve","deny"}) and update
all places that read/write the field (including the consent generation flow that
writes to ConsentCodeStore and any handlers that produce the one-time consent
code) to validate the presence and allowed values of `action`; also update the
related occurrences referenced around lines 287-290 to use and validate the new
`action` field instead of `denied?`.
- Around line 214-215: The security invariant incorrectly treats MCP
state.tenantSlug, BFF JWT.x-tenant-id, and consent code.tenantID as the same
type; tenantSlug (a human-friendly slug) and tenantID (a UUID) are different
identifiers, so change the wording and validation logic to assert semantic
binding rather than literal equality: require that tenantSlug maps to the
tenantID (or that both resolve to the same canonical tenant record) and that BFF
JWT.x-tenant-id is the canonical tenant ID (or includes both slug and ID),
updating the invariant text and any downstream checks to compare resolved
canonical IDs (e.g., resolve tenantSlug → tenantID then compare canonical IDs)
instead of direct slug==UUID comparisons.
---
Nitpick comments:
In `@docs/prd/061-mcp-auth-session-unification.md`:
- Around line 126-130: Update the GET /mcp/consent-info endpoint spec to mandate
abuse controls: add explicit rate-limiting requirements for the unauthenticated,
query-param-keyed endpoint (e.g., per-IP and per-client_id buckets) and require
"non-enumerable" error responses when mcp_state or client_id are missing/invalid
(do not reveal existence or validity of state or client_id in error bodies or
status codes). Reference the existing mcp_state validation and client_id
equality check in the description and state that dynamically registered flagging
(is_dynamic) remains allowed but must not be used to leak verification status;
apply the same controls to the other similar clause mentioned for the lines
covering the second instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c74a324b-e870-4cef-81ca-4706530360e3
📒 Files selected for processing (3)
.taskmaster.taskmasterdocs/prd/061-mcp-auth-session-unification.md
- Use explicit action enum (approve/deny) instead of optional boolean for consent endpoint to prevent accidental approval on field omission - Fix tenant binding invariant: clarify slug vs UUID distinction and require resolution at each boundary
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/prd/061-mcp-auth-session-unification.md (1)
156-157: Define deterministic behavior whenConsentCodeStorereaches cap.The PRD states TTL/cap/eviction, but not what happens at insertion time when full (reject vs evict oldest vs random). Please lock this down to avoid divergent implementations.
Also applies to: 239-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/prd/061-mcp-auth-session-unification.md` around lines 156 - 157, Define deterministic full-cap behavior for ConsentCodeStore: when ConsentCodeStore (same pattern as MCP's CodeStore) reaches the 10,000-entry cap, reject new insertions (e.g., in the insert/save/putCode method) with a clear error/return value rather than evicting silently; increment a metric (e.g., consent_code_store.insert_rejected) and log the rejection so callers can retry or surface an error. Update the PRD text near the ConsentCodeStore description (and at lines referenced 239-240) to state "insertion is rejected when full; callers must retry" and note the TTL (2-minute), background eviction remains in place to free space. Ensure the implementation and tests assert the rejection behavior and metric/log emission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/prd/061-mcp-auth-session-unification.md`:
- Around line 272-274: Update the login redirect behavior to validate the
return_url query parameter: when generating or consuming /login?return_url=...,
require that return_url is either a same-origin absolute URL or a relative path
(no external hosts), normalize and percent-decode it, reject or replace
invalid/external values with a safe default (e.g., "/"), and log or surface
validation failures; apply this validation in the login redirect handler and the
auth check that reads return_url to prevent open-redirect regressions.
- Around line 291-295: The API spec lists the request shape including the action
field (`action: "approve" | "deny"`) but the 400 error list omits the case for
unrecognized actions; update the error contract to include `"invalid_action"` as
a possible 400 error and document that the backend returns `{ error:
"invalid_action" }` when the `action` field is not one of the allowed values
(i.e., when validation in the handler that processes `mcp_state`, `client_id`,
`action` rejects unrecognized actions).
---
Nitpick comments:
In `@docs/prd/061-mcp-auth-session-unification.md`:
- Around line 156-157: Define deterministic full-cap behavior for
ConsentCodeStore: when ConsentCodeStore (same pattern as MCP's CodeStore)
reaches the 10,000-entry cap, reject new insertions (e.g., in the
insert/save/putCode method) with a clear error/return value rather than evicting
silently; increment a metric (e.g., consent_code_store.insert_rejected) and log
the rejection so callers can retry or surface an error. Update the PRD text near
the ConsentCodeStore description (and at lines referenced 239-240) to state
"insertion is rejected when full; callers must retry" and note the TTL
(2-minute), background eviction remains in place to free space. Ensure the
implementation and tests assert the rejection behavior and metric/log emission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6985b5d4-efcb-4c6a-8993-cd186be555b8
📒 Files selected for processing (1)
docs/prd/061-mcp-auth-session-unification.md
- Add return_url open-redirect protection as security requirement - Add invalid_action to API error contract for unrecognized action values
bjcoombs
dismissed stale reviews from claude[bot], claude[bot], coderabbitai[bot], and coderabbitai[bot]
Addressed in subsequent commits
1 participant
Summary
Context
MCP auth currently bypasses the UI and redirects directly to Dex, forcing users to log in twice.
The new design routes through the SPA consent page, which can detect an existing session
(JWT in sessionStorage) and show a consent screen instead of a login form.
Produced via a Six Thinking Hats huddle with security, backend, and frontend perspectives.
Test plan