mcp: do not re-prompt OAuth after cancelled Authorize#885
mcp: do not re-prompt OAuth after cancelled Authorize#885ravyg wants to merge 4 commits intomodelcontextprotocol:mainfrom
Conversation
When OAuthHandler.Authorize returned a context-cancelled error, the cancellation notification the call layer sent in response to the same ctx cancellation was routed back through the streamable client transport, re-triggering the 401 -> Authorize path and prompting the user a second time for a request they had already abandoned. Fix in two layers: 1. mcp/transport.go: bound the cancellation Notify with a fresh context.Background()-derived timeout instead of reusing the cancelled caller context. This guarantees the caller never blocks on a doomed notify against a degraded connection. 2. mcp/streamable.go: when Authorize returns a context-cancelled error, mark the streamable client connection as failed via c.fail(). The existing failure check in Write() then short-circuits the cancellation notification without re-invoking the OAuth handler, matching the bug reporter's expectation that cancelling Authorize abandons the connection. Includes a regression test with a blocking OAuth handler that asserts Authorize is invoked exactly once. The test was verified to fail on unfixed code (got 2 calls) and pass with the fix. Fixes modelcontextprotocol#882
| // attempt to deliver but never blocks the caller indefinitely. | ||
| notifyCtx, cancelNotify := context.WithTimeout(context.Background(), notifyCancellationTimeout) | ||
| defer cancelNotify() | ||
| err := conn.Notify(notifyCtx, notificationCancelled, &CancelledParams{ |
There was a problem hiding this comment.
Won't this still trigger a second auth flow?
There was a problem hiding this comment.
On its own, yes, it would, but the companion change in mcp/streamable.go (calling c.fail() when Authorize returns a cancelled error) is what actually prevents the second auth flow.
There was a problem hiding this comment.
Doesn't xcontext.Detach accomplish the same thing?
mcp/streamable.go
Outdated
| // sends in response to ctx cancellation) short-circuit instead of | ||
| // re-invoking the OAuth handler. Otherwise the user gets prompted | ||
| // to authorize a request they have already abandoned. See #882. | ||
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { |
There was a problem hiding this comment.
This PR doesn't fix the bug I'm seeing and I think it is because this logic is not quite right.
You shouldn't check the error returned from Authorize() because it could be anything. The error to inspect is the one returned from the context passed into Authorize()
| if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { | |
| ctxErr := ctx.Err() | |
| if errors.Is(ctxErr, context.Canceled) || errors.Is(ctxErr, context.DeadlineExceeded) { |
I tested this patch on top of your PR changes and it fixes the bug for me :-)
The cancellation check in streamableClientConn.Write inspected the
error returned by OAuthHandler.Authorize to decide whether the caller
abandoned the request. This failed for real-world handlers that return
a custom error rather than wrapping context.Canceled (e.g.
fmt.Errorf("oauth flow interrupted")).
Check ctx.Err() directly — it is the authoritative source for whether
the caller's context was cancelled, regardless of what Authorize
returns.
Update the regression test mock to return a non-wrapping error so it
exercises the real-world path. Verified: test fails (got 2 Authorize
calls) with the old errors.Is(err, ...) check, passes with ctx.Err().
Thanks to @smlx for identifying the bug and suggesting the fix.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ravyg
left a comment
ravyg
left a comment
There was a problem hiding this comment.
Good catch @smlx — you're right, the handler is user-implemented and may not wrap context.Canceled. Pushed the fix in 8d130aa checking ctx.Err() directly per your suggestion, and updated the test mock to return a non-wrapping error so it exercises the real-world path. Thanks for testing against your setup!
Going to spend some more time today (PST) diving deeper into this area as well.
|
Good catch @smlx — you're right, the handler is user-implemented and may not wrap Going to spend some more time today (PST) diving deeper into this area as well. |
3 participants
When
OAuthHandler.Authorizereturns a context-cancelled error, the cancellation notification that the call layer sends in response to the same ctx cancellation is routed back through the streamable client transport, re-triggering the401→Authorizepath and prompting the user a second time for a request they have already abandoned.Fix in two layers:
mcp/transport.go— bound the cancellationNotifywith a freshcontext.Background()-derived timeout instead of reusing the cancelled caller context. This guarantees the caller never blocks on a doomed notify against a degraded connection.mcp/streamable.go— whenAuthorizereturns a context-cancelled error, mark the streamable client connection as failed viac.fail(). The existing failure check inWrite()then short-circuits the cancellation notification without re-invoking the OAuth handler, matching the bug reporter's expectation that cancellingAuthorizeabandons the connection.Includes a regression test (
TestStreamableClientOAuth_CancelledAuthorize_NoReprompt) with a blockingOAuthHandlerthat assertsAuthorizeis invoked exactly once. The test was verified to fail on unfixed code (got 2calls) and pass with the fix.Fixes #882