feat(mcp): auth-required gate for delegated MCP consent (#330)

[initializ-mk]

Carved out of the #317 epic (tracked as #330). The delegated token (#327) and connection (#329) resolution already landed; this is the remaining Forge-side half — the runtime auth-required gate that turns "no grant yet" from a hard ErrNoToken failure into a pause that resumes once the user consents. Built in self-contained increments (same pattern as #329).

Inc 1 — authgate park/resume engine ✅

forge-core/security/authgate — the generalized DEFER-style pause-and-resume primitive. Keyed by {subject, server} (not taskID): one consent fans out to resume every parked call of that user's for that server, across tasks. First Await delivers the prompt; joiners attach silently. Binary resolution (granted/timeout/canceled). Never mints or sees a token — it only unblocks the executor to re-resolve.

Inc 2 — gate wired into MCPTool.Execute ✅

On ErrNoToken from the per-user resolver, Execute calls AuthGate.Await (a narrow seam) instead of failing; on a granted resume it re-resolves and proceeds. nil gate ⇒ pre-#330 behavior. Only ErrNoToken is gated.

Inc 3 — runtime gate + consent-resume endpoint ✅

mcpAuthGate bridges the engine to runtime concerns: subject extraction, task-status flip to auth-required (restored on resume), consent delivery, audit (mcp_auth_required/_resolved/_timeout). POST /mcp/consent is the resume signal (managed platform / operator) — carries no token, mirrors /tasks/{id}/decisions status codes; granted:false fails fast. ConsentDeliverer seam (+ SetConsentDeliverer).

Inc 4 — standalone loopback consent ✅

stateBinder issues single-use, expiring OAuth state bound to {subject, server, session}. GET /mcp/oauth/callback validates state, enforces the session match (rejects cross-session / replayed / expired), exchanges the code via the injected CallbackCompleter, and only then resumes the gate (never resume before the grant exists). Registered only when a completer is set (standalone); managed hosts its own.

Inc 5 — swappable session token store ✅

SubjectTokenStore interface (§18.8 #2) formalizes the per-subject delegated-token cache; memSubjectTokenStore is the in-process default (skew + opportunistic sweep + evict-on-stale). A managed broker substitutes a shared/durable impl via PlatformSourceConfig.SubjectStore without the resolver or agent changing.

Acceptance (the #317 boxes) — all covered

• type: user call lacking a grant pauses (not fails) and resumes after consent — inc 1–3.
• state binding rejects cross-session / replayed / expired callbacks (standalone) — inc 4.
• Managed: Forge parks/resumes only; callback + consent + token custody are platform-side — inc 3 (POST /mcp/consent, no token).
• Session token store is a swappable interface a broker-backed impl substitutes — inc 5.
• Never mints speculatively — park until authorized, then resolve — inc 1 + inc 4 (resume only after the token exchange succeeds).

Remaining (follow-up, not blocking)

Standalone Issue-side wiring — building the authorize URL + delivering the prompt via ConsentDeliverer (reuses oauth_flow.go). The security core (state binding + callback + gate) is in place; managed mode is fully functional now.

All increments green under -race + golangci-lint. Closes #330.

Epic #317 · tokens #327 · connections #329 · AARM R10 #319 · authority design-tool-registry.md §18.4/§18.5.

[initializ-mk]

Review: auth-required gate for delegated MCP consent (#330)

The last Forge-side piece of the #317 arc, and the security fundamentals are strong. I scrutinized the two highest-risk surfaces — the OAuth state/callback and the park/resume engine — and the core is right:

• gateKey is collision-proof: subject + "\x00" + server. NUL can't appear in an email or server name, so no delimiter-injection collision that would let one user's consent resolve another's gate. That's the exact composite-key hazard, defended correctly.
• State binding is textbook: single-use (delete before the expiry check, so a replay finds nothing), TTL-bounded, session-bound, with opportunistic sweep. Crypto-random via oauth.GenerateState().
• Resume-ordering is correct: the callback exchanges the code and stores the token BEFORE engine.Resolve — never resume before the grant exists; a failed exchange returns BadGateway with no resume.
• The gate never sees a token (it only unblocks re-resolution), /mcp/consent carries no token and granted:false fails fast, and only ErrNoToken is gated (narrow seam; nil gate → pre-#330 behavior).
• Fan-out is clean: closed-channel broadcast, resolved sync.Once, waiter-count teardown so an abandoned prompt doesn't idle to full timeout.
• /mcp/consent is correctly authenticated — it's not in DefaultSkipPaths, so the managed platform authenticates when it POSTs the resume signal. Right call.

CI fully green (-race). Two related findings, both on the standalone callback (inc 4):

1. Medium (latent-functional + security): the standalone callback must be auth-EXEMPT, but it's auth-wrapped

GET /mcp/oauth/callback is registered via RegisterHTTPHandler on the auth-wrapped mux, and it is not in auth.DefaultSkipPaths() (which lists only /.well-known/* and /healthz). But an OAuth callback is a browser redirect from the IdP — it carries no bearer token, so the auth middleware will 401 it before makeMCPCallbackHandler ever runs. The standalone consent flow can't complete.

It's masked today: the Issue-side wiring is follow-up, and the callback tests call the handler function directly, bypassing the middleware — so nothing exercises the real request path. But it's a hard break the moment standalone lands. The callback's authenticity comes from the state binding, not bearer auth, so it must be skip-listed (add GET /mcp/oauth/callback to the exempt set). Inline.

2. Medium (security, ties to #1): the empty-session skip is a CSRF gap on an unauthenticated, network-exposed callback

Once #1 is fixed (callback unauthenticated, as it must be), the state binding is the only security — and if b.session != "" skips the cross-session guard when the session is empty, leaving only single-use + expiry. The callback is registered on the main server (bound to cfg.Host, potentially 0.0.0.0 in a container), not loopback. So an empty-session state on a network-reachable, unauthenticated callback can be completed by anyone who obtains the state within the 10-min window. The "CLI loopback degradation" rationale only holds if the endpoint is actually loopback-bound. Recommend: require a non-empty session (reject empty) unless the callback is provably loopback-bound — so an unauthenticated, network-exposed callback is never guardable by single-use+expiry alone. Best designed in now, before the Issue-side issues real state. Inline.

3. Minor (test gap): the callback tests bypass the auth middleware

Because the tests call makeMCPCallbackHandler's returned func directly, they can't catch finding 1 (the 401). A test that drives GET /mcp/oauth/callback through the registered server with the auth middleware would surface it — worth adding alongside the skip-list fix so the exemption is pinned.

Verdict

Merge-ready for the managed path — managed consent is fully functional and correctly authenticated, the gate/state/keying fundamentals are sound, and CI is green. Findings 1–2 are about the standalone callback, which the PR itself scopes as partially follow-up — but I'd fix them (or explicitly defer with a tracking note) before the standalone Issue-side lands, because #1 makes standalone non-functional and #2 makes its only security control skippable. Both are small: a skip-list entry and a non-empty-session requirement. Excellent close to the #317 epic otherwise — the NUL-keyed gate and resume-after-exchange ordering are exactly right.

[initializ-mk]

Medium (latent-functional): this callback must be auth-EXEMPT, but it's auth-wrapped.

It registers on the auth-wrapped mux and is not in auth.DefaultSkipPaths() (only /.well-known/* + /healthz are). An OAuth callback is a browser redirect from the IdP — no bearer token — so the auth middleware 401s it before this handler runs, and the standalone flow can't complete. Masked today only because the Issue-side is follow-up and the tests call the handler directly (bypassing the middleware).

The callback's authenticity is the state binding, not bearer auth, so it must be skip-listed: add GET /mcp/oauth/callback to the exempt set. Note the contrast with POST /mcp/consent, which correctly stays auth-wrapped (the managed platform authenticates) — these two endpoints have opposite auth requirements, and only the callback needs exempting.

[initializ-mk]

Medium (security, pairs with the auth-exempt finding): don't skip the cross-session guard on a network-exposed callback.

Since the callback is (correctly) unauthenticated, the state binding is its entire security. This if b.session != "" skips the cross-session check whenever the session is empty — leaving only single-use + expiry. The callback is registered on the main server (cfg.Host, possibly 0.0.0.0), not loopback, so an empty-session state is completable by anyone who obtains it within the 10-min TTL. The "CLI loopback" rationale only holds if the endpoint is actually loopback-bound.

Recommend requiring a non-empty session (reject empty here) unless the callback is provably loopback-bound — so an unauthenticated, network-reachable callback is never protected by single-use+expiry alone. Worth designing in before the Issue-side starts issuing real state.

[initializ-mk]

Thanks — sharp catch on the two coupled callback findings. Both fixed in the latest commit; they're exactly right, and better to design in now before the Issue-side issues real state.

Finding 1 — callback must be auth-exempt (functional break). Added GET/OPTIONS /mcp/oauth/callback to auth.DefaultSkipPaths(). As you noted, its authenticity is the state binding, not a bearer token, so the tokenless IdP redirect must bypass the middleware. Safe unconditionally: the handler is registered only in standalone mode (a CallbackCompleter is set); in managed mode the path is unregistered and a request just 404s after the skip. POST /mcp/consent stays auth-wrapped — opposite requirement, kept.

Finding 2 — mandatory session bind (CSRF gap). Removed the if b.session != "" skip. The guard is now fail-closed: reject when the bound session is empty or the request session doesn't match. On a network-exposed, unauthenticated callback the binding is the entire security, so single-use+expiry-alone is never reachable. An empty bound session is treated as an Issue-side/config bug and rejected rather than silently downgraded. (A future loopback-bound variant that wanted the relaxation would bind the listener to localhost.)

Finding 3 — test gap. Pinned the exemption at the middleware level in middleware_test.go: GET /mcp/oauth/callback is public, and POST /mcp/consent still 401s — so a future refactor can't accidentally flip either. Added empty-bound-session and missing-request-session rejection tests to the callback suite (neither exchanges the code).

All green under -race + lint locally; CI running.

[initializ-mk]

Re-review of 7e0363f — both findings resolved fail-closed, with tests that pin the security property. CI green.

Finding 1 (auth-exempt callback) — fixed. GET/OPTIONS /mcp/oauth/callback added to DefaultSkipPaths() (tokenless browser redirect; authenticity is the state binding, not a bearer token). The unconditional-exemption reasoning is sound — the handler registers only in standalone mode, so managed mode 404s the now-public path, leaking nothing — and POST /mcp/consent deliberately stays auth-wrapped. middleware_test now pins BOTH directions: GET /mcp/oauth/callback public AND POST /mcp/consent requires auth. That also closes finding 3 (the tests previously bypassed the middleware; now they drive it).

Finding 2 (mandatory session) — fixed exactly as recommended. The reject is now if b.session == "" || sessionOf(req) != b.session (line 176) — an empty bound session rejects, and any mismatch rejects. So the network-exposed, unauthenticated callback can never downgrade to single-use+expiry alone; the cross-session guard is unconditional. The comment names an empty bound session as an Issue-side bug, pushing the session-binding constraint onto the follow-up before that code exists — the right sequencing.

The tests exceed the ask. TestMCPCallback_EmptySessionRejected / _CrossSessionRejected / _MissingRequestSessionRejected each assert not just the 400 but that the code is NOT exchanged — a rejected callback mints no token and resumes no gate, the fail-closed property pinned directly. The replay test asserts "code exchanged exactly 1 time," pinning single-use end-to-end.

Verdict

Merge-ready. The managed path was already sound (NUL-keyed gate, single-use/session-bound state, resume-only-after-exchange, /mcp/consent authenticated, gate never sees a token); the standalone callback is now correctly auth-exempt with the state binding as its complete, fail-closed security. Closes PR #331 and the Forge-side of #317.

Worth noting: the auth-wrapped-callback bug was invisible to the original test suite because it exercised the handler directly rather than through the middleware chain — the same "green because the bypassed path isn't tested" shape as the #323 mcp login bypass. The fix's middleware_test cases now drive the real registered path both ways, so the exemption (and the consent endpoint's non-exemption) can't silently regress.