fix(binding-mcp): reject request that races an in-flight connect elicitation - #2443
Open
jfallows wants to merge 5 commits into
Open
fix(binding-mcp): reject request that races an in-flight connect elicitation#2443jfallows wants to merge 5 commits into
jfallows wants to merge 5 commits into
Conversation
…itation McpRequestStream.proceedWithRequest asked binding.guard the same preauthorize question a concurrent McpLifecycleStream connect was already asking, minting a second, independent elicitation for what is really one guard decision. Only one of the two could ever be resolved by the caller's OAuth callback, so the other hung indefinitely. A request landing while the session's own connect is still pending cannot belong to a caller that has legitimately learned the session id yet, since the reply carrying it is withheld until the connect settles -- reject it instead of starting a redundant elicitation. pendingConnect now clears once the south handshake actually finishes (onNetEnd), not merely once the guard decides, and McpLifecycleStream.onReauthorized now also refreshes credentials via guard.credentials(sessionId) on success, matching the pattern used elsewhere on this path.
…'s own reply onNetEnd's final branch called doAppBegin() before clearing pendingConnect. doAppBegin can synchronously cascade through the proxy layer into a request that was queued waiting on this exact connect to settle (e.g. the proxy's own settleRequests(), invoked from the same reply this connect is about to receive) -- and that request would still see pendingConnect as true and be rejected by the check this fixes, even though the connect it was legitimately waiting on had, in that instant, just finished settling.
…on responses McpClientFactory relays a south server's own reverse "elicitation/create" request north to the real caller, then relays the caller's answer back south as a JSON-RPC response. decodeJsonRpcId accepts either a JSON string or number id but only keeps the token text, discarding which one it was, so HttpElicitResponse had no way to know whether to quote the id when re-serializing it. It always wrote it unquoted, which happens to be correct only when the original id was a number (as in the existing elicit.completed tests). For a south server whose reverse-request id is a non-numeric string -- e.g. the "everything" MCP reference server -- this produced invalid JSON (`"id":dfdca290` instead of `"id":"dfdca290"`), which the server rejected with a parse error, silently stalling that connection with no further retry. Since the id token's own shape is the only signal left by the time it reaches HttpElicitResponse, re-derive the type from it directly: quote it unless it parses as a bare JSON-RPC number.
McpListRouteSink.onMessage handled BEGIN/DATA/END/ABORT but had no RESET case, so a cancelled hydration fetch (e.g. a south peer whose transport only serves one in-flight request per session, rejecting a concurrent one) never called settle(). The route's kind never reached pending == 0, McpProxyCache.markAttempted never fired, and cache.populated stayed false forever -- blocking every client's own initialize, not just callers of the affected toolkit, since register() gates on that flag.
…ractive auth A cache-hydration route whose south connect receives an actual preauthorize challenge (elicitation) can never be satisfied automatically -- only a real, interactive caller can answer it. Retrying such a route on every hydration cycle just re-triggers guard.preauthorize() repeatedly, minting a fresh elicitation each time; those compete with a real caller's own in-flight elicitation for the same toolkit. Track per-route "needs interactive auth" state on McpLifecycleServer (not on the per-attempt McpLifecycleClient, which is discarded as soon as its south connect closes, well before the next retry). The signal is scoped to an actual CHALLENGE frame, not any settle-without-session outcome -- a route guarded by a plain, non-interactive guard (e.g. authn_jwt) can also settle without a session for an ordinary transient reason and must stay retriable. McpProxyCacheHydrater.startRoutes() checks this per route before dialing, short-circuiting straight to a failed settle instead of dialing again.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
McpRequestStream.proceedWithRequest(inbinding-mcp'skind: client) askedbinding.guardthe same preauthorize question that the session's ownMcpLifecycleStreamconnect was already asking, whenever a request landed while that connect's guard decision was still outstanding. Since both streams share the samebinding.guard, this minted a second, independent elicitation for what is really one guard decision. An external OAuth callback can only ever resolve one of the two correlation ids, so whichever elicitation didn't win hangs indefinitely — the reported symptom being that the browser completes the login successfully but the original tool call still times out.A request cannot legitimately belong to a caller that has already learned the session id at this point, since the reply carrying it is withheld until the connect itself settles — so this is a protocol violation, not a normal timing overlap to absorb.
McpRequestStream.proceedWithRequestnow rejects such a request outright instead of starting a redundant elicitation. The check runs before the reply-sidedoAppBegin, so a request that's about to be rejected never opens a reply it's only going to abort.Two related fixes landed alongside it, in the same code path:
pendingConnectnow clears once the south handshake actually finishes (onNetEnd), not merely once the guard decision resolves — the window a racing request needs to be rejected in extends through the full handshake, not just the guard round trip. This also has to happen before the reply-sidedoAppBeginis dispatched, not after:doAppBegincan synchronously cascade through a composing proxy layer into a request that was queued waiting on this exact connect to settle, and that request needs to seependingConnectalready false by the time it's redispatched from within that same call.McpLifecycleStream.onReauthorized's success branch now also refreshescredentialsviaguard.credentials(sessionId), matching the pattern already used on the other decision branches on this path (previously onlyguardSessionIdwas set there).A third, unrelated bug surfaced while verifying this fix end-to-end against a real OAuth authorization-code + DCR flow (as opposed to the simpler url-mode elicitation the original report used):
McpClientFactoryrelays a south server's own reverseelicitation/createrequest north to the real caller, then relays the caller's answer back south as a JSON-RPC response. The id decoder accepts either a JSON string or number id but only keeps the token text, discarding which one it was, so the response encoder had no way to know whether to quote the id when echoing it back. It always wrote it unquoted, which happens to be correct only when the original id was a number. For a south server whose reverse-request id is a non-numeric string (e.g. theeverythingMCP reference server), this produced invalid JSON ("id":dfdca290instead of"id":"dfdca290"), which the server rejected with a parse error, silently stalling that connection with no further retry. Fixed by re-deriving the type from the id token's own shape at the point it's re-serialized, since that's the only signal left by then.Fixes #(issue)
Test plan
McpClientIT.shouldCallToolConcurrentWithLifecyclePreauthorize, with pairedbinding-mcp.specscripts, modeling atools/callthat arrives for a session whose connect-level preauthorize elicitation is still outstanding — asserts it is rejected rather than minting a second elicitation.McpClientIT.shouldCallToolElicitCompletedProxiedStringId, with pairedbinding-mcp.specscripts, modeling a reverseelicitation/createwhose id is a non-numeric string — asserts the accept response is re-serialized with the id correctly quoted (the existingshouldCallToolElicitCompletedProxiedtest continues to assert a numeric id is echoed back unquoted).aklivity/zilla-plus'sexamples/mcp.proxy: built this branch asdevelop-SNAPSHOT, built a localzilla-plusDocker image against it, and drove the full OAuth authorization-code + DCR + real-browser login flow through theeverythingMCP reference server — the tool call that previously hung for 150s now completes end-to-end../mvnw -pl specs/binding-mcp.spec,runtime/binding-mcp clean verify checkstyle:check license:check— 303 tests, 0 failures, 0 checkstyle violations, license check clean.Generated by Claude Code