feat(mcp): standalone delegated consent — type:user without a platform (#332)#344
Conversation
#332) Standalone (no-platform) delegated identity: a `type: user` MCP server with `grant: authorization_code` and explicit endpoints now runs the per-user OAuth in Forge itself instead of requiring a platform token endpoint. forge-core: - ServerConfig.PublicURL — the agent's external base URL for redirect_uri. - Manager/ServerDeps.SubjectStore — the per-subject token cache the standalone resolver reads and the consent callback writes. - NewServer accepts standalone type:user (no platform block) when grant is authorization_code and authorize_url/token_url/client_id are explicit; the buildAuthFn user-case branches managed (platform) vs standalone (reads SubjectStore, ErrNoToken on miss → trips the auth-required gate). - validate mirrors the standalone rules (endpoints + grant). - Exported BuildAuthorizeURL and NewInMemorySubjectTokenStore for the runtime. forge-cli: - stateBinder carries the PKCE verifier + authorize URL; adds Bind + Peek. - New GET /mcp/oauth/start — the session producer the callback's mandatory cross-session guard requires: it plants the forge_session cookie (SameSite=Lax, never in a URL so it can't leak to the IdP via Referer) and 302s to the IdP. - CallbackCompleter now takes (ctx, subject, server, code, verifier) so the exchange uses the state-bound verifier and a request-scoped deadline. - Standalone front-half: buildStandaloneConsentLink (PKCE+state+session+Issue), the code→token completer (ExchangeCodeCtx → SubjectStore.Put), and the A2A-artifact ConsentDeliverer that publishes the login link on the parked task. All auto-wired by enableStandaloneConsent when a standalone type:user server is configured; managed deployments are untouched. - SetAuthorizeURLProvider seam so #343's Slack delivery can supply the link in both standalone (Forge builds) and managed (platform supplies) modes. Tests: standalone resolver (ErrNoToken/hit/isolation) + NewServer validation; validate accept/reject; stateBinder Bind/Peek; /start cookie+redirect; completer exchange→store; deliverer artifact; managed-mode no-op. Docs: standalone type:user section in docs/mcp/configuration.md.
initializ-mk
left a comment
There was a problem hiding this comment.
Review — standalone delegated consent (#332)
Request changes. One blocking bug that breaks the feature in exactly the deployment it targets, plus one design question worth an explicit decision. The OAuth mechanics themselves are correct and well-tested — PKCE bound to state and replayed at exchange, one callbackRedirectURI() feeding both the authorize URL and the token exchange, exchange routed through the egress allowlist, in-memory-only per-subject tokens keyed off the authenticated identity, and a single Platform.TokenEndpoint != "" switch used identically across the resolver, NewServer, validate, and standaloneDelegatedServers so config-validation, construction, and runtime wiring can't disagree.
🔴 [High / Blocking] GET /mcp/oauth/start is behind auth — the flow can't start in production
Inline on the /start registration. The new browser-facing endpoint is registered behind the auth middleware but was never added to auth.DefaultSkipPaths() (only the callback is exempt), so the user's anonymous browser is 401'd before the handler runs. Breaks the flow at its first hop in every auth-enforcing deployment — i.e. every real multi-user agent. CI is green because TestStandaloneConsent_LinkAndStart calls makeMCPStartHandler directly, bypassing the middleware — the same shape as the #331 callback-exemption bug and the #323 mcp login bypass.
🟡 [Medium / Design] What does the session binding actually buy in standalone mode?
Inline on the cookie set in makeMCPStartHandler. The consent browser is anonymous, and /start plants forge_session for anyone holding the link — so the cookie proves browser continuity, not that the completer is the parked subject. A leaked link lets an attacker complete consent with their own IdP account and have that token filed under the victim's subject (confused deputy). Bounded by single-use + short-TTL state and authenticated-channel delivery, so it's delivery-gated rather than open — but the naming reads stronger than the guarantee. Worth a decision + doc note.
🟢 Verified solid
PKCE / redirect_uri consistency / egress-routed exchange / in-memory per-subject isolation / the four-site standalone-vs-managed switch / the NewServer reject matrix (missing endpoints, client_credentials, missing SubjectStore, required:true). Thorough tests on everything that runs through a real code path.
| if r.stateBinder == nil { | ||
| r.stateBinder = newStateBinder(defaultStateTTL) | ||
| } | ||
| srv.RegisterHTTPHandler("GET /mcp/oauth/start", makeMCPStartHandler(r.stateBinder)) |
There was a problem hiding this comment.
[High / Blocking] This endpoint is registered behind the auth middleware but isn't exempted — the standalone consent flow can't start in any auth-enforcing deployment.
Both /mcp/oauth/start (here) and /mcp/oauth/callback (next line) are wrapped by the auth middleware, but auth.DefaultSkipPaths() (forge-core/auth/middleware.go) exempts only the callback:
"GET /mcp/oauth/callback": true,
"OPTIONS /mcp/oauth/callback": true,
// GET /mcp/oauth/start ← missingMatching is an exact lookup (skip[r.Method+" "+r.URL.Path]), and the runner uses DefaultSkipPaths() unmodified at all three sites (runner.go:3029/3071/3078). So when the user clicks the delivered "Connect" link, their anonymous browser (no bearer token) hits GET /mcp/oauth/start → writeAuthError(...) → 401 before the handler runs. The flow dies at its first hop in every deployment that enforces auth — which is the entire point of delegated per-user consent. (Works only under --no-auth.)
/start is safe to exempt for the same reason the callback is: its authenticity rests on the Peek'd session-bound state (it rejects any binding lacking authorizeURL+session), not a bearer token — and it is not an open redirect (b.authorizeURL is server-built from config, reachable only via a Forge-minted state).
Why CI didn't catch it: TestStandaloneConsent_LinkAndStart calls makeMCPStartHandler(...) directly, so it never exercises the middleware — green handler, broken wiring. Same shape as the #331 callback bug and the #323 mcp login bypass.
Fix: in DefaultSkipPaths() add
"GET /mcp/oauth/start": true,
"OPTIONS /mcp/oauth/start": true, // symmetry; GET is the load-bearing oneand add a middleware-level test asserting /start is reachable unauthenticated and that POST /mcp/consent still requires auth (the both-directions test #331 added for the callback).
| } | ||
| http.SetCookie(w, &http.Cookie{ | ||
| Name: forgeSessionCookie, | ||
| Value: b.session, |
There was a problem hiding this comment.
[Medium / Design question] In standalone mode this cookie proves browser continuity, not that the completer is the parked subject.
The consent browser is anonymous until it reaches /start, and /start plants forge_session = b.session for whoever presents the link. So the callback's session match only guarantees the same browser did /start and /callback across the IdP round-trip — it does not tie the flow to the authenticated identity that triggered the park.
Consequence: if the consent link leaks, an attacker can hit /start, authenticate at the IdP as themselves, and the callback files their token under the victim's b.subject (fixed server-side from the parked call). The victim's resumed call then runs against the attacker's account — a confused-deputy / token-fixation vector (attacker data flows into the victim's agent, or victim data is written to the attacker's account).
This is bounded: the state is single-use + short-TTL and the link is delivered on the authenticated A2A artifact (Slack DM in #343), so it's delivery-gated rather than open. Raising it because "session-bound cross-session guard" reads stronger than the standalone guarantee — the flow structurally can't bind the browser to the parked subject. Worth an explicit decision + a doc note (like #339 documented its spoofing limitation). The tamper-proof-but-heavy alternative is verifying the IdP userinfo identity against b.subject at exchange time.
…del (#344 review) [High/Blocking] GET /mcp/oauth/start was registered behind the auth middleware but never added to auth.DefaultSkipPaths(), so the user's anonymous browser was 401'd at the flow's first hop in every auth-enforcing deployment (CI missed it because the test drove makeMCPStartHandler directly). Exempt GET + OPTIONS /mcp/oauth/start alongside the callback — its authenticity is the Peek'd session-bound state, not a bearer token, and it is not an open redirect (authorizeURL is server-built, reachable only via a Forge-minted state). Added a middleware-level test asserting /start is public (the both-directions pin already covers POST /mcp/consent still requiring auth). [Medium/Design] Documented the standalone trust model: the anonymous consent browser means the session cookie proves browser CONTINUITY across the IdP round-trip, not that the completing user is the parked subject — a leaked link is a confused-deputy / token-fixation vector, contained by authenticated-channel delivery + single-use short-TTL state. Reframed the overclaiming "cross-session guard" comments and added a Trust model note to docs/mcp/configuration.md; noted the heavier IdP-userinfo-verification alternative.
|
Both addressed in 🔴 [High/Blocking] 🟡 [Medium/Design] Session binding overclaims — documented + reframed. Took the documented-limitation decision (matching the #339 precedent you cited), since the flow structurally can't bind the anonymous browser to the parked subject:
Local: |
initializ-mk
left a comment
There was a problem hiding this comment.
Re-review — fix commit 8adf5ce ✅
Both findings resolved; nothing new introduced (skip-paths + tests + comments/docs only).
🔴 [High/Blocking] /mcp/oauth/start behind auth — fixed, and pinned at the right layer
DefaultSkipPaths() now exempts GET and OPTIONS /mcp/oauth/start; all three runner sites call DefaultSkipPaths() so they pick it up automatically. The important part is where the new test sits — it drives the real middleware, not the handler in isolation:
name: "GET /mcp/oauth/start is public (anonymous browser link click)"
opts: MiddlewareOptions{Chain: chain, SkipPaths: DefaultSkipPaths()} → 200
That's exactly the wired-path assertion whose absence let the bug ship green — and the opposite-direction case (POST /mcp/consent must NOT be exempt) is retained, so the both-directions invariant is now pinned for /start too. The regression that was invisible before is now the thing the test catches.
🟡 [Medium/Design] Session trust model — addressed correctly for a design finding
Keep the mechanism, document it precisely, defer the heavier hardening — and both the overclaim and the missing decision are fixed:
- The callback comment is renamed "Cross-session guard" → "Session-continuity guard" and now states outright it proves browser continuity across the round-trip, not that the completer is
b.subject. - A new "Trust model / limitation (standalone)" doc block names the link a bearer capability, spells out the confused-deputy / token-fixation vector, states the containment (treat the link as a secret + single-use short-TTL + authenticated-channel delivery), and records the deferred tamper-proof alternative (verify IdP
userinfo == subject), noting managed mode sidesteps it entirely.
Test + Lint green at time of posting (Integration still running; change is test-only + a map entry, prior commit was green across all 10 checks). Approve — ready to merge once Integration lands.
When a type: user MCP call parks on the auth-required gate, Forge now presents
a "Connect <server>" login link to the requesting user over its existing Slack
connection, in both standalone and managed modes. Delivery is decoupled from
token custody — the same Slack code serves both.
forge-core:
- channels.ConsentDeliverer capability (DeliverConsent + SetConsentCanceler),
ConsentPrompt{Subject,Server,AuthorizeURL,Deadline,Origin}, ChannelOrigin,
ConsentCanceler — mirrors ApprovalDeliverer.
- PlatformConfig.AuthorizeEndpoint + mcp.FetchAuthorizeURL: the managed
consent-URL contract — POST {server, subject} (Bearer agent identity +
tenancy headers) → {authorize_url}. The platform builds the URL with its own
client_id/redirect_uri/state and hosts the callback, so the code + refresh
token never reach Forge.
forge-plugins/slack:
- consent.go: DeliverConsent DMs the subject (users.lookupByEmail →
conversations.open → chat.postMessage) or replies in the Slack origin thread;
Block Kit Connect URL button + optional Cancel; Cancel routes through the
existing block-action dispatch to the wired canceler. Reuses the socket-mode
connection + bot token (adds im:write scope).
forge-cli:
- Runner.AuthorizeURL resolves the link via a provider — standalone builds it
(#332), managed fetches it from the platform, an embedder may inject its own
(SetAuthorizeURLProvider takes precedence). enableManagedConsentProvider
auto-wires the managed HTTP provider; the authorize_endpoint host is merged
into the egress allowlist.
- cmd/run.go wires SetConsentDeliverer + SetConsentCanceler to the first active
consent-capable adapter (mirrors the DEFER block); postMCPConsent helper for
the Cancel path. No adapter → falls back to the A2A artifact / audit event.
Tests: Slack payload shape, DM-by-email + origin-thread delivery, cancel
routing; FetchAuthorizeURL contract; managed provider fetch + embedder
precedence. Docs: managed consent delivery + authorize_endpoint in
docs/mcp/configuration.md.
Stacked on #344 (uses the SetAuthorizeURLProvider seam).
When a type: user MCP call parks on the auth-required gate, Forge now presents
a "Connect <server>" login link to the requesting user over its existing Slack
connection, in both standalone and managed modes. Delivery is decoupled from
token custody — the same Slack code serves both.
forge-core:
- channels.ConsentDeliverer capability (DeliverConsent + SetConsentCanceler),
ConsentPrompt{Subject,Server,AuthorizeURL,Deadline,Origin}, ChannelOrigin,
ConsentCanceler — mirrors ApprovalDeliverer.
- PlatformConfig.AuthorizeEndpoint + mcp.FetchAuthorizeURL: the managed
consent-URL contract — POST {server, subject} (Bearer agent identity +
tenancy headers) → {authorize_url}. The platform builds the URL with its own
client_id/redirect_uri/state and hosts the callback, so the code + refresh
token never reach Forge.
forge-plugins/slack:
- consent.go: DeliverConsent DMs the subject (users.lookupByEmail →
conversations.open → chat.postMessage) or replies in the Slack origin thread;
Block Kit Connect URL button + optional Cancel; Cancel routes through the
existing block-action dispatch to the wired canceler. Reuses the socket-mode
connection + bot token (adds im:write scope).
forge-cli:
- Runner.AuthorizeURL resolves the link via a provider — standalone builds it
(#332), managed fetches it from the platform, an embedder may inject its own
(SetAuthorizeURLProvider takes precedence). enableManagedConsentProvider
auto-wires the managed HTTP provider; the authorize_endpoint host is merged
into the egress allowlist.
- cmd/run.go wires SetConsentDeliverer + SetConsentCanceler to the first active
consent-capable adapter (mirrors the DEFER block); postMCPConsent helper for
the Cancel path. No adapter → falls back to the A2A artifact / audit event.
Tests: Slack payload shape, DM-by-email + origin-thread delivery, cancel
routing; FetchAuthorizeURL contract; managed provider fetch + embedder
precedence. Docs: managed consent delivery + authorize_endpoint in
docs/mcp/configuration.md.
Stacked on #344 (uses the SetAuthorizeURLProvider seam).
Implements #332 — standalone (no-platform) delegated MCP consent. Closes the standalone half of the #317 arc; the Forge-driven Slack delivery layer stays in #343.
What it does
A
type: userMCP server withgrant: authorization_codeand explicit endpoints now runs the per-user OAuth in Forge itself — no platform token endpoint required. End to end:Before this,
type: userwas construction-rejected without a platform block (server.go:192); standalone delegated identity didn't exist.Design decisions (confirmed with maintainer)
type: user+ noplatformblock +grant: authorization_code. Requires explicitauthorize_url/token_url/client_id(no runtime discovery);client_credentialsis rejected.server.public_urlfield, falling back to the existingAGENT_URLenv var →redirect_uri = <public_url>/mcp/oauth/callback.Notable: the session producer (
GET /mcp/oauth/start)The #330 callback mandates a matching
forge_session(fail-closed) but nothing produced it — the browser only ever touches Forge at the callback./mcp/oauth/startis that producer: it plants the cookie (SameSite=Lax so it survives the IdP round-trip) and redirects to the IdP. The session lives only in the server-side binding + cookie — never a URL query — so it can't leak to the IdP viaReferer(unlike the state, which round-trips by design). This is the one piece beyond the issue's "reuse these seams" list; it's what makes the existing session-bound callback usable on a network-exposed agent.Layering
ServerConfig.PublicURL;SubjectStoreon Manager/ServerDeps; standalone branch inNewServer+buildAuthFn; exportedBuildAuthorizeURL+NewInMemorySubjectTokenStore.stateBinder(verifier + authorizeURL,Bind/Peek);/mcp/oauth/start;CallbackCompleter(ctx,…,verifier); front-half builder + completer + A2A-artifact deliverer, auto-wired byenableStandaloneConsent;SetAuthorizeURLProviderseam for Forge-driven Slack delivery of MCP consent prompts (channel-adapter ConsentDeliverer) #343.Tests
ErrNoTokenon miss, token on hit, per-subject isolation;NewServeraccept/reject matrix.validateaccept/reject (endpoints, grant, managed-skips).stateBinderBind/Peek;/startcookie + redirect + bad-state 400.All
forge-core/{mcp,validate,types}andforge-cli/{runtime,cmd}suites green; lint 0 issues.Follow-ups (not in this PR)
SetAuthorizeURLProvider).