fix(mcp): stabilize authenticated tool discovery#5754
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Runtime auth: Settings copy: Disconnected OAuth servers show OAuth authorization required; a refresh that ends disconnected with no concrete error shows Authorization required instead of generic Failed / Not Connected (persisted errors still win). OAuth start API: Servers that don’t support dynamic client registration return 422 with guidance to configure a token instead of a generic 500. Logging: New Networking: Lifecycle: Updating a server to OAuth (or flipping auth type) resets connectionStatus to disconnected and clears workspace cache when transport or auth type changes. Reviewed by Cursor Bugbot for commit ea7a148. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR stabilizes authenticated MCP discovery and refresh behavior. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (6): Last reviewed commit: "fix(mcp): reset connectionStatus when a ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- preserve typed static-header authentication failures\n- order discovery publication by start time\n- atomically publish tools and failure cache state
Match configuration generations within the JavaScript millisecond window so PostgreSQL microseconds do not suppress valid publication.
Addressed in 89dce25: both status-publication predicates now match the JavaScript millisecond window, accepting PostgreSQL sub-millisecond precision while still rejecting the next generation. A boundary regression covers the behavior. |
- fall back to best-effort cache writes when mutation ordering is unavailable\n- share bounded redacted MCP error diagnostics across client and service
Retry cache mutation ownership before publishing discovery state, avoid unordered publication that older writers can supersede, and keep explicit invalidation best effort.
Use timestamp-based mutation tokens for both cache ownership and database CAS ordering, and invalidate Redis mutation owners before deleting entries during a full clear.
Recompute consecutive failures after status CAS conflicts, reload winning cached tools for superseded discoveries, and return live tools without unordered publication when cache ordering is unavailable.
Expose discovery publication metadata to refresh callers, report live tools during cache degradation, and compare lastToolsRefresh mutation tokens instead of wall clocks when preserving a newer success.
Probe mutation ownership after database CAS misses, keep valid live tools for metadata-only edits, and invalidate ownership for transport and auth-type changes.
- Preserve OAuth-pending state when a status reread is stale - Keep concrete refresh errors sanitized and visible
- Make cache invalidation retries ownership-safe - Preserve proven newer successes for superseded refreshes
|
@cursor review |
… minimal fix set A four-angle LOC review (correctness, overengineering, dead-code, intent) found the fencing-token / discoveryRevision / conditional-publication apparatus and its 5 bounded-retry loops (~700 lines) to be correct but disproportionate to a low-contention, self-healing per-workspace-per-server discovery path — and it introduced two regressions: failing closed on a transient Redis blip (blocking DB status writes) and clearCache writing the DB for every server in the workspace. Revert service.ts, the storage adapters, the refresh route, and types to the production-tested base (which keeps the simple lastConnected<=discoveryStartedAt ordering guard and plain cache set/delete), and keep only the genuinely-good, decoupled fixes: authType-aware UnauthorizedError classification, OAuth-pending UX labels, DCR->422, credential-safe error diagnostics, and HTTP/2 negotiation.
bb5e320 to
99e235a
Compare
An authType flip to oauth (via a plain authType change or an implicit oauthClientId) cleared the cache but left connectionStatus='connected', so a server that had not completed its OAuth flow falsely read as connected and the OAuth-authorization-required prompt never showed. Reset to disconnected on any switch to oauth, matching the create path. Adds the missing test assertion.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ea7a148. Configure here.
|
Superseded by #5757. After a four-angle LOC review found the ordered-publication consistency layer disproportionate (correct but ~700 lines of fencing/CAS/retry machinery for a low-contention, self-healing path, plus two regressions), we carved the genuinely-good fixes — auth-type classification, OAuth-pending UX, DCR→422, safe error diagnostics, and HTTP/2 — into a clean, minimal PR (#5757) off staging. The original commits here remain in git history. Closing this in favor of the simpler replacement. |
2 participants
Summary
Authenticated MCP servers that advertise OAuth can now be added and refreshed with an explicitly configured static
Authorization: Bearer ...credential without being diverted into OAuth. The previous fix made the pre-save connection test try the configured header, but runtime discovery still mapped every SDKUnauthorizedErrorto “authorization required.” This PR makes that classification depend on the server's configured auth type, so static-header servers stay on the normal credential path and only OAuth-configured servers enter OAuth.OAuth-pending connections now remain actionable in Workspace Settings: the persistent server status says
OAuth authorization required, and a refresh that cleanly returns to that state saysAuthorization requiredinstead of the genericFailed/Not Connected. Concrete connection errors still take precedence, so timeouts and other failures are not mislabeled as an OAuth prompt.Discovery publication is ordered end to end. Redis and memory cache adapters allocate a monotonic millisecond mutation token; conditional cache publication and the database
lastToolsRefreshcompare-and-swap use that same token, so retries, failures, successes, and invalidations choose one winner across both layers. A credential-safe discovery revision covers every connection-affecting field, preserving valid tools across metadata-only edits while failing closed after URL, credential, auth-type, or transport changes.Each retry acquires fresh mutation ownership. Successful, failed, and OAuth-pending status publication share a bounded retry for metadata-only edit races: every attempt rechecks cache ownership and the discovery revision before publishing against the latest row token. If ordering or an atomic cache transition is unavailable, freshly fetched tools may be returned to the current caller but unordered cache/database state is not persisted. Successful discovery returns its exact publication token, so workflow schemas sync only while the post-discovery reread still belongs to that winning
publishedrefresh; later success, failure, or invalidation barriers take precedence, andwinner-cache,superseded, and degradedunavailableresults cannot propagate as winners. Cache invalidation retries reacquire fresh ownership, publish a database barrier only after an atomic delete wins, and never fall back to an unordered delete that could erase a newer discovery.Refresh responses now prove that a newer successful discovery actually won before preserving connected state after a failure: both
lastToolsRefreshandlastConnectedmust advance beyond the request's original snapshot. Invalidation advances the publication token but notlastConnected, so it can no longer hide a real discovery failure behind a stale connected row. Failure counters also recompute through an optimistic JSONB compare-and-swap when success races them.Security-sensitive paths remain bounded: MCP targets still pass domain and SSRF validation, public targets are DNS-pinned when policy permits, and HTTP/2 negotiation does not relax that pinning. Logs contain header names and structural diagnostics but never values, session IDs, or raw upstream bodies, while UI/API errors come from an allowlist. OAuth servers without dynamic client registration return an actionable 422 that directs users to configure a token. The settings copy change consumes only the existing auth type and sanitized error state; it does not alter or expose credentials, headers, tokens, or upstream responses.
Related: #5665 and #5595.
Type of Change
Testing
bun run --cwd apps/sim test -- lib/mcp app/api/mcp 'app/workspace/[workspaceId]/settings/components/mcp'— 454 tests passed across all 29 MCP suites, including static Bearer classification, concurrent publication, repeated metadata-only races, ownership-safe invalidation retries, degraded-cache behavior, publication-token-gated workflow sync, secret-safe diagnostics, SSRF pinning, invalidation-only refresh races, and OAuth-specific settings status behavior.bun run check:api-validation:strict— passed with all 964 routes Zod-backed and no boundary-policy drift.git diff --check— passed.tools/listwith 47 tools using both normal and SSRF-pinned transports; the SDK transport carried the session ID without logging it.bun run --cwd apps/sim type-checkreaches only unrelated shared-worktree dependency errors in fork audit events,recordAuditBatch, and@google-cloud/storage; no MCP TypeScript errors are reported.Reviewer focus: auth-type-aware handling of typed 401s, OAuth-pending UI status with concrete-error precedence, unified cache/database mutation ownership, bounded same-revision status retries, fresh-token invalidation retries, Redis conditional-mutation scripts, winner-only workflow synchronization, fail-closed degraded-cache behavior, and the two-timestamp proof for a racing successful refresh.
Checklist
Screenshots/Videos
N/A — status-copy-only UI change with no layout change.