fix(mcp): close every hole found in the refresh-token/DCR audit - #92
Merged
Conversation
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.
Audit (this conversation) found six holes in the OAuth/refresh-token/DCR work shipped in PRs #82-#91. This PR closes all of them.
Fixes
/testbroken for refresh_token configsrenderRefreshTokenCard+ 'Refresh token' button + endpointNew tests (3)
/mcp-configs/:id/testinjects the refreshed bearer for refresh_token configsVerification
npm run typecheckcleannpx vitest run832/832 pass (829 + 3)6a7364f9-…) should now succeed once tokens are refreshed via the new 'Refresh token' button.beep-boop-🤖