Harden MCP portal authentication changes

[dcartertwo]

Portal authentication mode and deployment-owned tokens can change while an account or facet remains live. This makes auth-mode transitions clear stale OAuth state, refuses fallback/callback paths no longer allowed by current configuration, and invalidates captured connection generations when token authority changes.

OAuth redirects record their configuration revision before handing off to the callback, preventing an existing facet from invalidating the attempt while the user is authorizing.

Stacked on #170. Verified with the MCP and portal suites plus full pnpm lint.

---

[ask-bonk]

The doc-comment above (lines 165-168) is now stale relative to this new logic. It says probing may legitimately move between none and OAuth (bidirectionally), but configured === "oauth" ? connected === "token" : connected !== configured only tolerates that drift when the portal is configured as oauth. When configured as none, a connected oauth state now requires a reconnect (connected !== configured) — the intended new hardening, but it contradicts the comment. Please update the doc to describe the asymmetric rule (an oauth-configured portal may prove public during probing, while an explicitly none-configured portal stays strict).

[ask-bonk]

Review submitted with one inline finding.

I reviewed the PR (hardening MCP portal authentication). The logic is sound: the auth-mode reconnect rule, the OAuth fallback/callback gates, the connection-generation invalidation, and the per-revision in-flight counter all correctly implement "clear stale OAuth state / refuse paths no longer allowed / invalidate on token-authority change." The truth-table change to portalAuthRequiresReconnect flips exactly one case (connected=oauth, configured=none → now requires reconnect), which matches the stated intent, and the concurrency guard around #connectingRevisions and the baseline-revision logic in getConnection avoid clobbering a newer attempt. The new test validly exercises the revision-recording path.

I submitted 1 inline finding: the doc-comment on portalAuthRequiresReconnect in config.ts is now stale — it claims bidirectional none↔OAuth drift is fine, but the new implementation only tolerates that when the portal is configured oauth, and keeps none-configured strict.

github run

[ask-bonk]

I've completed my review. My analysis confirms the logic is sound across all six files:

• account.ts: The allowsOAuthFallback/allowsOAuthCallback hooks default to true (no regression for the generic connector), and authChanged only triggers when a caller passes a target with differing auth — which for user-supplied reconnects it never does (resolveConnectTarget passes existing). The generation/callback invalidation is guarded correctly.
• portal.ts: The per-revision in-flight counter (#connectingRevisions) correctly prevents a losing/older concurrent attempt from clobbering a newer one's baseline. The getConnection migration branch invalidates a pre-revision token account exactly once and only invalidates oauth accounts' state when the revision genuinely changes.
• config.ts: The portalAuthRequiresReconnect truth-table change (connected=oauth, configured=none now requires reconnect) matches the stated hardening.
• Tests and the new vitest.config.ts alias (needed because portal.ts imports cloudflare:workers) are valid.

The only actionable issue is the stale doc-comment on portalAuthRequiresReconnect (config.ts:165-168), which claims bidirectional none↔OAuth drift is tolerated while the new asymmetric rule only tolerates it for oauth-configured portals. However, this exact finding was already submitted inline by the prior ask-bonk review on this same PR. Per review guidelines, I will not repeat a published finding, and I found no additional actionable problems.

No new actionable findings. (1 pre-existing finding — the stale portalAuthRequiresReconnect doc-comment — was already posted inline by an earlier review and is not repeated.)

github run

[ask-bonk]

Now let me examine the authChanged interaction with the portal's allowsOAuthFallback more carefully for a subtle bug.

Consider a portal reconfigured from oauth to none:

• On reconnect, target.auth = "none", existing.auth = "oauth".
• authChanged = true → clears tokens, stores server with auth: "none".
• allowsOAuthFallback(server) for portal returns server.auth !== "none" → false.
• Probe runs. If the server (still requiring auth) 401s, since allowsOAuthFallback is false, it throws "requires authorization but configured for unauthenticated access." Good.
• If probe succeeds (public), records auth: "none". Good.

Now consider none → oauth:

• target.auth = "oauth", existing.auth = "none". authChanged = true, clears state, stores server auth=oauth.
• Probe unauthenticated. If 401, allowsOAuthFallback = "oauth" !== "none" = true → OAuth flows. Good.
• If probe succeeds (public), records auth: "none" (line 378 sets {...server, auth:"none"} since not token). Hmm — so a portal configured oauth but actually public records auth: "none". Then portalAuthRequiresReconnect("none", "oauth") = false. Good, consistent.

The logic is coherent. Now let me double check one concern in getConnection: the #connectingRevisions Map is per-DO-instance in memory. If the DO evicts/restarts between a beginConnect redirect and the eventual getConnection, the map is empty. But that's fine because portalConfigRevision is persisted in storage during beginConnect, so previous === revision and no invalidation. The in-memory map is only an optimization for the concurrent-attempt window, which by nature is within one activation. Correct.

I've completed a thorough review. The implementation is sound. The only actionable finding is the stale doc-comment on portalAuthRequiresReconnect in config.ts (lines 165-168), which was already posted inline by a prior review on this PR. Per the review guidelines, I will not repeat a published finding, and I found no additional actionable issues.

No new actionable findings. (1 pre-existing finding — the stale portalAuthRequiresReconnect doc-comment at config.ts:165-168, which still claims bidirectional none↔OAuth drift is tolerated while the new asymmetric rule only tolerates it for oauth-configured portals — was already posted inline by an earlier review and is not repeated.)

github run

[github-actions]

Preview: pr199-feature-mcp-p-151f663c

https://pr199-feature-mcp-p-151f663c-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 2 additional findings in Devin Review.

[devin-ai-integration]

🟡 An abandoned reconnect attempt can wipe the sign-in state of a newer attempt, so the user's authorization fails

Stored sign-in state is erased (clearCredentials() at packages/mcp-shared/src/account.ts:415) without first checking that this attempt is still the current one, so a superseded attempt can delete the state a newer attempt just created and the user's authorization comes back to nothing.
Impact: When two connect attempts overlap and the portal's authentication mode has just changed, the user finishes signing in and is told the link is invalid, and has to start over.

Missing generation check before clearing OAuth state in the authorization-challenge path

beginConnect captures generation before its probe (packages/mcp-shared/src/account.ts:327). The success path rechecks it (packages/mcp-shared/src/account.ts:376) before touching storage, but the McpAuthRequiredError path does not: after allowsOAuthFallback, it calls clearCredentials() unconditionally when credentialAuthorityChanged is set, deleting tokens, oauthClient, oauthDiscovery, oauthVerifier and pendingAuth.

Trigger: the deployment's portal auth changes (e.g. token -> oauth, so credentialAuthorityChanged is true) and two reconnects overlap — each prepareReconnect mints its own nonce, so both can claim. Attempt A suspends in probe; attempt B advances the generation, gets challenged, saves oauthVerifier/pendingAuth and returns a redirect. A's probe then rejects with the challenge and A runs clearCredentials(), destroying B's verifier and pendingAuth. A's own beginOAuth then aborts on isCurrentConnection, but when the user returns from the authorization server acceptAuthCode finds no pendingAuth and returns false (packages/mcp-shared/src/account.ts:611-612).

Note that beginOAuth's own error path deliberately guards the equivalent deletions with ownsAttempt && isCurrentConnection (packages/mcp-shared/src/account.ts:592-599), which is the check missing here. The adjacent put("server", oauthServer) on the next line has the same staleness exposure.

Suggested change

	const oauthServer: ConnectedServer = { ...server, auth: "oauth" };
	if (credentialAuthorityChanged && !endpointChanged) clearCredentials();
	this.ctx.storage.kv.put("server", oauthServer);
	const oauthServer: ConnectedServer = { ...server, auth: "oauth" };
	if (generation !== this.connectionGeneration()) {
	throw new Error("This connection attempt was replaced by a newer one.");
	}
	if (credentialAuthorityChanged && !endpointChanged) clearCredentials();
	this.ctx.storage.kv.put("server", oauthServer);

---

Was this helpful? React with 👍 or 👎 to provide feedback.