fix(mcp): use DO ID hex in OAuth callback path to side-step email URL encoding - #88
Merged
Conversation
…il-encoding mismatch) After hitting "Redirect URI not allowed by application configuration" from cf-portal even after matching Seal's callback shape, dug into the actual wire traffic and found the bug: registered redirect_uri: https://dodo.../agents/coding-agent/ruskin.constant@gmail.com/callback authorize redirect_uri: https://dodo.../agents/coding-agent/ruskin.constant%40gmail.com/callback The OAuth client URL-encodes the redirect_uri when it goes into the authorize URL's query string (`%40` for `@`). cf-portal's authorize endpoint appears to do a strict string comparison against the as-registered redirect_uri and rejects the encoded form. Switch the instance segment from email to UserControl DO ID hex (`env.USER_CONTROL.idFromName(email).toString()`) which is a-z0-9 only — no encoding ambiguity. The /agents/* route handler doesn't parse the URL path for routing anyway (uses CF Access cookie to identify the user), so this changes the URL shape without any functional impact downstream. Tests: 817/817 pass. Typecheck clean. beep-boop-🤖
This was referenced May 26, 2026
jonnyparris
added a commit
that referenced
this pull request
May 26, 2026
Six holes were identified in the audit of PRs #82-#91. This commit fixes all of them. Hole 1 (medium) — /test endpoint broken for refresh_token configs POST /mcp-configs/:id/test was building the HttpMcpClient via resolveMcpConfigHeaders, which doesn't know about refresh_token configs (no headerKeys). The client connected with no Authorization header → 401. Every Test click on a refresh_token integration returned 'Connection failed'. Fix: the /test handler now branches on auth_type. refresh_token rows pull the bearer via getMcpAccessToken (which auto-refreshes if stale) and inject it as 'Authorization: Bearer <token>'. The SELECT was also missing auth_type — added it so the mapper sees the real value. Hole 2 (low) — updateMcpConfigEncrypted could corrupt refresh_token rows mcpConfigUpdateSchema only allowed auth_type ∈ {oauth, static_headers}. A client sending {auth_type:'static_headers'} would silently downgrade a refresh_token row. {headers:{...}} would call deleteMcpConfigSecrets, wiping the entire token chain (access_token, refresh_token, expires_at). Fix: schema now accepts refresh_token. updateMcpConfigEncrypted explicitly rejects mutations of headers/url/auth_type/type on refresh_token rows with a clear error. Only enabled and name can be patched — enough for UI toggle and rename to work. Hole 3 (minor) — pending DCR row leaked on upsert failure completeOauthDcrFlow deleted the pending row AFTER upsertRefreshTokenMcp. If the upsert threw (e.g. envelope dropped, sqlite error), the row sat around until its 10-min TTL. The captured 'code' is single-use at the OAuth provider anyway, so the row is dead either way. Fix: wrap delete in try/finally so it runs unconditionally. Hole 4 (cosmetic) — stale comment in dodo-settings.js Comment claimed the callback was at /agents/oauth/callback. Actual path is /agents/coding-agent/<userId-hex>/callback (PR #88). Fix: comment updated. Hole 5 (minor) — test blind spot on rotation chaining The original suite mocked a rotated refresh token in the response but never made a second refresh to assert that the new value was what got sent. The code was correct — this just adds the assertion. Fix: new test 'rotates the stored refresh token — second refresh uses the rotated value' verifies the sent refresh_token over two refreshes is [v0, v1] (not [v0, v0]). Hole 6 (deferred → fixed) — no UI affordance for refresh_token configs refresh_token configs rendered through the generic renderIntegCard, with no visual indicator and no way to force-refresh. Fix: new renderRefreshTokenCard with an explicit 'OAuth · auto-refresh' badge and a 'Refresh token' action. Backed by a new POST /api/mcp-configs/:id/refresh-token endpoint that proxies to /mcp-configs/:id/access-token?force=1. Three new tests added in test/refresh-token-mcp-unit.test.ts: - /mcp-configs/:id/test injects the refreshed bearer for refresh_token - PUT /mcp-configs/:id refuses to mutate headers/url/auth_type on a refresh_token config (and that enabled toggles still work) - rotation chain — second refresh uses the rotated value Tests: 832/832 pass (829 + 3 new). Typecheck clean. beep-boop-🤖
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.
Bug
Even after matching Seal's callback path shape, cf-portal kept returning
Redirect URI not allowed by application configuration. Worker tail showed why:The OAuth client URL-encodes the redirect_uri when embedding it in the authorize URL's query string (
%40for@). cf-portal's authorize endpoint does a strict string comparison against the as-registered URI and rejects the encoded form, even though both decode to the same URI.Seal works because their
userIdis shapeduser-{base64-ish}— no@, no encoding mismatch.Fix
Use
env.USER_CONTROL.idFromName(email).toString()(hex string) as the instance segment. Same per-user uniqueness, all a-z0-9, no encoding ambiguity.The
/agents/*route handler doesn't parse the URL path for routing — it identifies the user via CF Access cookie and forwards to the SDK's MCP callback handler. So changing the path segment is purely cosmetic from the worker's POV.Verification
npm run typecheckcleannpx vitest run817/817 passbeep-boop-🤖