feat(mcp): standalone delegated consent — type:user without a platform (#332) #344
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| package runtime | ||
|
|
||
| import ( | ||
| "context" | ||
| "net/http" | ||
| "strings" | ||
| "sync" | ||
| "time" | ||
|
|
||
|
|
@@ -26,10 +28,18 @@ import ( | |
| // stateBinding is what an issued OAuth `state` is bound to. The callback | ||
| // must match all of it (modulo an empty session) to be honored. | ||
| type stateBinding struct { | ||
| subject string | ||
| server string | ||
| session string // the initiating session; callback must match (cross-session guard) | ||
| // verifier is the PKCE code_verifier minted when the authorize URL was | ||
| // built; the callback needs it to exchange the code (#332). Empty for | ||
| // managed flows (Forge never sees the code) or where PKCE isn't used. | ||
| verifier string | ||
| // authorizeURL is the IdP authorize URL this state was minted for. The | ||
| // GET /mcp/oauth/start endpoint redirects the browser here after setting | ||
| // the session cookie (#332). Empty when Forge doesn't front the redirect. | ||
| authorizeURL string | ||
| deadline time.Time | ||
| } | ||
|
|
||
| // stateBinder issues single-use, expiring OAuth `state` values bound to the | ||
|
|
@@ -56,7 +66,8 @@ func newStateBinder(ttl time.Duration) *stateBinder { | |
| } | ||
|
|
||
| // Issue mints a cryptographically-random state bound to {subject, server, | ||
| // session} and returns it for embedding in the authorize URL. Used where the | ||
| // caller doesn't front the redirect (no verifier/authorizeURL captured). | ||
| func (s *stateBinder) Issue(subject, server, session string) (string, error) { | ||
| state, err := oauth.GenerateState() | ||
| if err != nil { | ||
|
|
@@ -69,6 +80,38 @@ func (s *stateBinder) Issue(subject, server, session string) (string, error) { | |
| return state, nil | ||
| } | ||
|
|
||
| // Bind stores a fully-formed binding under a caller-provided state. The | ||
| // standalone front-half (#332) generates the state itself (it must embed it in | ||
| // the authorize URL before binding), then records the session, PKCE verifier, | ||
| // and authorize URL here so GET /mcp/oauth/start can set the session cookie and | ||
| // redirect, and GET /mcp/oauth/callback can complete the exchange. | ||
| func (s *stateBinder) Bind(state, subject, server, session, verifier, authorizeURL string) { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| s.sweepLocked() | ||
| s.m[state] = stateBinding{ | ||
| subject: subject, server: server, session: session, | ||
| verifier: verifier, authorizeURL: authorizeURL, | ||
| deadline: s.now().Add(s.ttl), | ||
| } | ||
| } | ||
|
|
||
| // Peek returns the binding for state WITHOUT consuming it (unlike Consume), | ||
| // for the GET /mcp/oauth/start redirect: the state is spent later, at the | ||
| // callback. Returns ok=false for unknown or expired states. | ||
| func (s *stateBinder) Peek(state string) (stateBinding, bool) { | ||
| if state == "" { | ||
| return stateBinding{}, false | ||
| } | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| b, ok := s.m[state] | ||
| if !ok || s.now().After(b.deadline) { | ||
| return stateBinding{}, false | ||
| } | ||
| return b, true | ||
| } | ||
|
|
||
| // Consume looks up and REMOVES the binding for state. It returns ok=false | ||
| // for an unknown state (forged), an already-used state (replay — the entry | ||
| // is gone after the first Consume), or an expired one. Single-use + expiry | ||
|
|
@@ -100,12 +143,13 @@ func (s *stateBinder) sweepLocked() { | |
| } | ||
| } | ||
|
|
||
| // CallbackCompleter exchanges an OAuth authorization code (with the PKCE | ||
| // verifier bound to the state) for a token and stores it for {subject, server}, | ||
| // so the resumed call finds a grant. The standalone resolver provides one; when | ||
| // nil the loopback callback is NOT registered (managed mode hosts its own | ||
| // callback and never hands Forge a code). ctx is the request context so the | ||
| // token exchange inherits a finite deadline. | ||
| type CallbackCompleter func(ctx context.Context, subject, server, code, verifier string) error | ||
|
|
||
| // sessionFromRequest extracts the caller's session id for the cross-session | ||
| // check. Prefers an explicit forge session header; falls back to a session | ||
|
|
@@ -123,20 +167,71 @@ func sessionFromRequest(r *http.Request) string { | |
| return "" | ||
| } | ||
|
|
||
| // registerMCPCallbackEndpoint wires the standalone consent endpoints. They are | ||
| // registered ONLY when a CallbackCompleter is set (standalone interactive | ||
| // mode); managed deployments never register them. | ||
| // | ||
| // - GET /mcp/oauth/start: the link Forge delivers. It sets the forge_session | ||
| // cookie (the producer the callback's mandatory session match requires) and | ||
| // redirects the browser to the IdP authorize URL. | ||
| // - GET /mcp/oauth/callback: the IdP redirect target. Validates state + | ||
| // session, exchanges the code, resumes the parked call. | ||
| func (r *Runner) registerMCPCallbackEndpoint(srv *server.Server, auditLogger *coreruntime.AuditLogger) { | ||
| if r.authGateEngine == nil || r.callbackCompleter == nil { | ||
| return | ||
| } | ||
| if r.stateBinder == nil { | ||
| r.stateBinder = newStateBinder(defaultStateTTL) | ||
| } | ||
| srv.RegisterHTTPHandler("GET /mcp/oauth/start", makeMCPStartHandler(r.stateBinder)) | ||
| srv.RegisterHTTPHandler("GET /mcp/oauth/callback", | ||
| makeMCPCallbackHandler(r.stateBinder, r.authGateEngine, r.callbackCompleter, sessionFromRequest, auditLogger)) | ||
| } | ||
|
|
||
| // forgeSessionCookie is the cookie name the callback's cross-session guard | ||
| // reads (sessionFromRequest). The start endpoint is its only producer. | ||
| const forgeSessionCookie = "forge_session" | ||
|
|
||
| // makeMCPStartHandler serves GET /mcp/oauth/start?state=<state>. It is the | ||
| // session producer for the standalone consent flow (#332): the callback | ||
| // mandates a matching forge_session, and this is where the browser gets it. | ||
| // The step is necessary because the user's browser only ever touches Forge at | ||
| // the callback (after the IdP), so the cookie must be planted on an earlier | ||
| // same-origin visit — this redirect. | ||
| // | ||
| // The session value lives ONLY in the server-side binding and the cookie, never | ||
| // in a URL query, so it is never leaked to the IdP via Referer (unlike the | ||
| // state, which round-trips through the IdP by design). SameSite=Lax lets the | ||
| // cookie survive the top-level cross-site redirect back from the IdP. | ||
| // | ||
| // Scope of the guarantee (see docs/mcp/configuration.md — Trust model): the | ||
| // browser here is ANONYMOUS, and this plants the cookie for whoever holds the | ||
| // link. So the session binding proves browser CONTINUITY across the round-trip | ||
| // (a stolen state alone, replayed from a different browser, can't complete) — | ||
| // it does NOT prove the completing user is the parked subject. The link is a | ||
| // bearer capability; its real containment is authenticated-channel delivery + | ||
| // single-use short-TTL state. | ||
| func makeMCPStartHandler(binder *stateBinder) http.HandlerFunc { | ||
| return func(w http.ResponseWriter, req *http.Request) { | ||
| state := req.URL.Query().Get("state") | ||
| b, ok := binder.Peek(state) // Peek, not Consume — the state is spent at the callback. | ||
| if !ok || b.authorizeURL == "" || b.session == "" { | ||
| http.Error(w, "invalid or expired consent link", http.StatusBadRequest) | ||
| return | ||
| } | ||
| http.SetCookie(w, &http.Cookie{ | ||
| Name: forgeSessionCookie, | ||
| Value: b.session, | ||
|
Contributor
Author
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 Consequence: if the consent link leaks, an attacker can hit 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 |
||
| Path: "/mcp/oauth/", | ||
| HttpOnly: true, | ||
| Secure: req.TLS != nil || strings.EqualFold(req.Header.Get("X-Forwarded-Proto"), "https"), | ||
| SameSite: http.SameSiteLaxMode, | ||
| Expires: b.deadline, | ||
| }) | ||
| http.Redirect(w, req, b.authorizeURL, http.StatusFound) | ||
| } | ||
| } | ||
|
|
||
| // makeMCPCallbackHandler validates the state, enforces the session match, | ||
| // exchanges the code for a token, and resumes the parked call. Extracted so | ||
| // tests can drive every rejection path without a real IdP. | ||
|
|
@@ -162,26 +257,34 @@ func makeMCPCallbackHandler( | |
| http.Error(w, "invalid, expired, or already-used state", http.StatusBadRequest) | ||
| return | ||
| } | ||
| // Session-continuity guard — MANDATORY. This endpoint is unauthenticated | ||
| // (a browser redirect carries no bearer token) and is registered on | ||
| // the network-exposed main server (cfg.Host, possibly 0.0.0.0), so | ||
| // the state binding is its ENTIRE security. Single-use + expiry alone | ||
| // would let anyone who obtains a state within its TTL complete the | ||
| // flow from a DIFFERENT browser, so we additionally require the callback | ||
| // to land in the SAME browser that started it (the forge_session cookie | ||
| // planted at /start). An empty bound session is a config/Issue-side bug | ||
| // (a network-exposed flow must bind a session) and is rejected | ||
| // fail-closed rather than silently downgrading to single-use+expiry. | ||
| // | ||
| // What this does NOT guarantee (see docs/mcp/configuration.md — Trust | ||
| // model): in standalone mode the browser is anonymous, so this proves | ||
| // browser continuity across the IdP round-trip, NOT that the completing | ||
| // user is b.subject. Containment against a leaked link is | ||
| // authenticated-channel delivery + the short single-use TTL, not this | ||
| // cookie. A future loopback-bound variant would bind the listener to | ||
| // localhost; the tamper-proof alternative verifies IdP userinfo == subject. | ||
| if b.session == "" || sessionOf(req) != b.session { | ||
| http.Error(w, "state/session mismatch", http.StatusBadRequest) | ||
| return | ||
| } | ||
| // Exchange the code for a token and store it for {subject, server}. | ||
| // Only AFTER this succeeds do we resume — resolving the gate with no | ||
| // grant would just re-park the call (delegation follows | ||
| // authorization: never resume before the grant exists). The PKCE | ||
| // verifier bound to the state proves this is the same flow we started. | ||
| if err := complete(req.Context(), b.subject, b.server, code, b.verifier); err != nil { | ||
| http.Error(w, "authorization exchange failed", http.StatusBadGateway) | ||
| return | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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, butauth.DefaultSkipPaths()(forge-core/auth/middleware.go) exempts only the callback:Matching is an exact lookup (
skip[r.Method+" "+r.URL.Path]), and the runner usesDefaultSkipPaths()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) hitsGET /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.)/startis safe to exempt for the same reason the callback is: its authenticity rests on the Peek'd session-bound state (it rejects any binding lackingauthorizeURL+session), not a bearer token — and it is not an open redirect (b.authorizeURLis server-built from config, reachable only via a Forge-minted state).Why CI didn't catch it:
TestStandaloneConsent_LinkAndStartcallsmakeMCPStartHandler(...)directly, so it never exercises the middleware — green handler, broken wiring. Same shape as the #331 callback bug and the #323mcp loginbypass.Fix: in
DefaultSkipPaths()addand add a middleware-level test asserting
/startis reachable unauthenticated and thatPOST /mcp/consentstill requires auth (the both-directions test #331 added for the callback).