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

[ndisidore]

per sol

C1 — HIGH — vitest.config.ts:5-13 omits include, so the suite runs a stale shadow copy that asserts the opposite of this commit's fix
All three flagged this. Two independently produced runtime evidence. I confirmed it directly:
packages/gatekeeper-mcp-portal/vitest.config.ts → no include, no environment
packages/mcp-shared/vitest.config.ts (its source) → include: ["tests/*.test.ts"], environment: "node"

.wrangler/validate/tests/config.test.ts Aug 13 16:15 (pre-branch, collectible)
.wrangler/validate/src/config.ts:131 return (connected === "token") !== (configured === "token");
pnpm test:run reports 5 files / 67 tests where only 3 files exist; a clean checkout gives 3 / 38. The consequence is not merely noise — the run simultaneously asserts both answers for the same input:

• new tests/config.test.ts:150: portalAuthRequiresReconnect("oauth","none") → true
• stale .wrangler/.../config.test.ts:144: same call → false
  So the package's own green suite proves nothing about the function this commit exists to change, and a future revert would be confirmed by a passing test. Compounding it: .wrangler/** is deliberately excluded from the test task's fingerprint (scripts/vitest-task-vite-config.ts), so a cached run can replay green over a snapshot nothing invalidates, and pnpm clean (rm -rf dist src/generated) never removes it.

Fix is one line: include: ["tests/*.test.ts"] (plus environment: "node" for parity). Adding capnwebValidate() is correct and necessary — only the omissions are the problem.

[ndisidore]

this test is very light and doesn't really exercise the riskiest parts of this

Untested branches introduced by the diff: all four guards at portal.ts:328-329; both sides of the legacy previous === undefined branch; the rotation branch at 343-346; the #connectingRevisions refcount; allowsOAuthCallback (both outcomes, in either package); the portal's own allowsOAuthFallback (mcp-shared tests a hand-written fake instead); invalidateConnectionState() (no test invokes it at all); and — noted by agent 2 — portalAuthRequiresReconnect("token","token"), the single most common deployment configuration, asserted nowhere. A regression making it true bricks every token deployment silently.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 5 additional findings in Devin Review.

[devin-ai-integration]

🟡 The portal connector's tests cannot run on a freshly cloned or cleaned checkout

The new test pulls in the connector's worker module (../src/portal.js at packages/gatekeeper-mcp-portal/__tests__/account-revision.test.ts:8), which imports a build-generated file that only exists after the configurator build has run, so running just this package's tests on a clean tree fails to even load.
Impact: Anyone running the package's tests without a full prior build sees an unresolved-import failure instead of test results.

Generated configurator artifact is an undeclared prerequisite of the `test` task

packages/gatekeeper-mcp-portal/src/portal.ts:80 does import MCP_SERVER_CONFIGURATOR_HTML from "./generated/server-configurator-ui.txt". That path is gitignored (.gitignore:7) and is produced by the build:configurator Vite+ task (scripts/gatekeeper-configurator-vite-config.ts). The package's test task comes from withVitestTask(...) (scripts/vitest-task-vite-config.ts:118-133), which declares no dependsOn, and the test:run script is a bare vitest run. Until now no test in this package imported src/portal.ts, so the generated file was never needed at test time; the directory is in fact absent in this checkout. CI happens to be green because .github/workflows/ci.yml runs pnpm build before pnpm test, but the documented iteration path (pnpm --filter <pkg> test:run, vp run -F <pkg> test) breaks after pnpm clean or a fresh clone.

Prompt for agents

The new test suite in packages/gatekeeper-mcp-portal/__tests__/account-revision.test.ts imports ../src/portal.js, which statically imports ./generated/server-configurator-ui.txt. That file is gitignored and only produced by the package's build:configurator Vite+ task, so vitest fails to resolve the import on a clean tree (no src/generated directory). Options: make the package's `test` task depend on `build:configurator` (the package's vite.config.ts re-exports withTests from scripts/gatekeeper-configurator-vite-config.ts, so this needs a variant that adds dependsOn), or stub/alias the generated .txt module in packages/gatekeeper-mcp-portal/vitest.config.ts alongside the existing cloudflare:workers alias, or move the account/revision logic under test into a module that does not pull in the worker entry with its generated assets.

---

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