fix: prevent addMcpServer from reporting restored OAuth as ready#1869
Open
mattzcarey wants to merge 5 commits into
Open
fix: prevent addMcpServer from reporting restored OAuth as ready#1869mattzcarey wants to merge 5 commits into
mattzcarey wants to merge 5 commits into
Conversation
…ctions A connection restored after a Durable Object wake keeps its persisted AUTHENTICATING state, but the OAuth provider's in-memory authUrl is only assigned during a live redirectToAuthorization() and is never rehydrated from storage. addMcpServer's existing-connection early return required that in-memory URL to report authenticating, so a restored connection awaiting consent fell through to READY — disagreeing with getMcpServers() for the same server in the same tick, and wedging callers that drive the OAuth prompt off the return value. addMcpServer now falls back to the persisted auth_url for authenticating connections. If no auth URL exists in memory or storage, it re-runs the connect flow to mint a fresh one instead of reporting READY. The HTTP path also reuses an existing server's id (as the RPC path already did) so re-adding a known server never orphans its storage row. Fixes #1855
…e-ready-on-restored-auth
…links Second-round hardening on top of the persisted-auth_url fallback: - The persisted auth_url is only served while its OAuth state is still redeemable, validated through the provider's own checkState. The state nonce and code verifier expire (10 minutes for the built-in provider), so a URL restored after a long hibernation can be a dead link; serving it would wedge the flow at the callback with 'State expired'. Expired or unverifiable-invalid URLs now fall through to the reconnect path, which mints and re-persists a fresh sign-in link. The check fails open: URLs without a state parameter and providers without checkState are served as before. - Pins the HTTP-path id-reuse behavior with a dedicated test: re-adding a known (name, url) whose in-memory connection is gone reuses the stored row's id instead of minting a fresh one and orphaning the row. Fixes #1855
🦋 Changeset detectedLatest commit: 9f1d0ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
agents
@cloudflare/ai-chat
@cloudflare/codemode
create-think
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
…e-ready-on-restored-auth
Restored AUTHENTICATING connections lose the provider's in-memory authUrl, so addMcpServer previously fell through to READY even while storage still reported OAuth in progress. Prefer live then persisted HTTP(S) authorization URLs, and reconnect the existing connection when neither is servable, mapping authentication, discovery, and failure outcomes without re-registering it.
mattzcarey
changed the title
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
1 participant
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.
Fixes #1855
Problem
After a Durable Object wake, an HTTP MCP connection restored during OAuth is still
AUTHENTICATING, but its provider no longer has the live in-memoryauthUrl.addMcpServer()required that live URL to reportauthenticating, so it returnedreadywhilegetMcpServers()reportedauthenticatingfor the same server.Fix
For an existing
AUTHENTICATINGconnection,addMcpServer()now:http:orhttps:URL;AUTHENTICATINGwith a URL returnsauthenticatingand persists/broadcasts that URL;CONNECTEDruns discovery and returnsreadyonly after discovery succeeds;FAILEDthrows the normal HTTP connection error;AUTHENTICATINGresult without a URL throws an explicit OAuth configuration error.Other established/non-failed connection states retain the existing
readybehavior, and existing failed connections retain their existing error behavior.Scope
checkState()or claim that a stored URL is fresh or callback-redeemable.agents.TDD evidence
The restored-state regression failed against the
origin/mainimplementation with{ state: "ready", authUrl: undefined }instead of the persistedauthenticatingresult. Subsequent vertical slices were also observed red before their minimal outcome mapping was added: no-URL reconnect returnedready; connected, failed, and incomplete reconnect outcomes were unmapped; and ajavascript:persisted URL was returned instead of reconnecting.The focused OAuth file is now green: 29/29 tests. Coverage includes live-URL precedence, restored persisted HTTP(S) URLs, URL persistence/broadcast with the same id, valid stored-token discovery, connection failure, incomplete OAuth output, and malformed/relative/
javascript:/non-HTTP persisted URLs.Verification
pnpm install --frozen-lockfile— passedpnpm --filter agents exec vitest --project workers --run src/tests/mcp/oauth2-mcp-client.test.ts— 29/29 passedpnpm --filter agents test:workers— 75 files, 1,515 tests passedpnpm --filter agents build— passedpnpm run typecheck— 115/115 projects passedpnpm run check— passedpnpm run build— 24/24 projects passedCI=true pnpm exec nx affected -t test— passed for 13 projects and 8 dependent tasks on rerun; Nx identified the first-run@cloudflare/think:testtime-bound sweep failure as flaky, its exact test passed in isolation, and the full required command then passedThe branch includes current
origin/mainand was pushed without force-pushing.