feat: add saved MCP connection diagnostics and approval controls - #523
Conversation
![mogplex[bot]](https://avatars.githubusercontent.com/in/3107927?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Solid, well-tested feature. The security-sensitive parts hold up: the new POST /api/mcp-servers/[id]/test authenticates first, ignores any caller-supplied body and reads only the owner's row (.eq user_id + .eq id), validates the URL with assertSafeOutboundHttpUrlWithDns before any Vault secret read, re-validates every request with redirect:"error", returns only fixed diagnostic codes (no upstream bodies or header values), and sets Cache-Control: no-store. Tool policy handling preserves unknown CLI fields because updateToolPolicy spreads the raw extra rather than Zod's projection, and toolApproval's new "auto" default is behaviorally identical to the previous undefined in chat.ts. No critical issues. Three things worth resolving before merge, all around the shared deadline being reused for teardown and the new server-side policy validation on unchanged rows, plus two minor suggestions.
2 findings were added inline.
Warnings
- PATCH now validates the stored extra even when the request does not change it, locking out legacy rows (lib/mcp-servers/normalization.ts)
InnormalizeMcpServerUpdateInput,extrafalls back toexisting.extrawheninput.extra === undefined, andvalidateExtraKeys(extra, nextTransport)then runsreadToolPolicyon it for http servers. Any existing row whose policy fields fall outsidetoolPolicySchema— a CLI-writtenapproval_modevalue not in the enum,enabled_toolsas a string, etc. — will now reject unrelated updates such as{ "enabled": false }or a rename, with a generic 400INVALID_TOOL_POLICY("Correct the tool permissions before saving."). The web UI offers a path out via Extra JSON, but CLI/API clients that PATCH a single field get a confusing failure on data they never touched, and the message does not say which field is wrong.
Suggested direction: only validate when the caller actually supplies extra (if (input.extra !== undefined) validateExtraKeys(...)), keeping the fail-closed check for writes, or include the Zod issue path in the error message so the offending key is identifiable. Note the schema is a subset of CLI fields, so it will reject values the CLI may legitimately write.
Suggestions
- Allowed/blocked tool lines are not trimmed, so a stray space silently blocks a tool (components/settings/mcp-servers/permissions-editor.tsx)
lines(value) => value ? value.split("\n") : []stores entries verbatim, while the "Add tool rule" input usesnewTool.trim(). Entering " search" in the allowlist producesenabled_tools: [" search"], which never matches intoolApproval— the tool stays blocked with no feedback. Blank lines also persist as""entries in the saved CLI config (they are hidden in the UI byfilter(Boolean)). Suggest normalizing on save inbuildPayload(trim + drop empties + dedupe) rather than on each keystroke, so typing is unaffected. - No rate limit on the new outbound-connection endpoint (app/api/mcp-servers/[id]/test/route.ts)
Each POST holds a request for up to 6s and opens an outbound connection to the saved URL. It is authenticated and SSRF-guarded, so the exposure is limited, but there is no throttle on repeated calls. If the codebase has an existing rate-limit helper used by other mutating routes, applying it here would be cheap insurance; if not, this is fine to defer. Minor:components/settings/mcp-servers/server-card.tsxwraps<ConnectionTest>in<div className="mt-3">, which renders an empty spacing div for stdio servers since the component returns null.
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Reviewed the saved-MCP diagnostics + approval controls end to end (new POST /api/mcp-servers/[id]/test route, lib/mcp-servers/diagnostics.ts, the extracted policy module, mcp-tools signal handling, and the new client components).
Security and authorization look sound: the endpoint authenticates first, validates the id as a UUID, ignores the request body entirely (covered by tests/unit/mcp-server-test-route.test.ts), reads only the owner-scoped saved row (.eq("user_id", userId).eq("id", id), exercised against a real table in tests/db/mcp-servers-chat.test.ts), re-validates every outbound URL with DNS guards plus redirect: "error", returns only enumerated diagnostic codes (never upstream error text), and sends Cache-Control: no-store.
Concurrency handling is careful and correct as far as I can trace it: the 6s discovery deadline and the independent 2s teardown signal are separated, deadline.abort() in the finally cannot mask an already-computed success because the teardown promise is .catch-swallowed, and the DELETE branch in getRemoteMcpTools deliberately ignores the aborted startup signal so teardown still works after cancellation. SAVED_MCP_STARTUP_TIMEOUT_MS matches the previous inline CONNECTION_TOOL_STARTUP_TIMEOUT_MS - 2000, so the chat path is behavior-preserving, and toolApproval's new ?? "auto" default is equivalent to the previous undefined at both call sites.
One real gap: a stdio→http transport change can carry a malformed tool policy past validation. Plus a few low-severity notes. Nothing blocking.
4 findings were added inline.
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
Solid, well-tested PR. The new test endpoint is authenticated, owner-scoped, saved-target-only (no caller-supplied URL/secret), DNS/redirect-guarded on every request including the teardown DELETE, deadline-bounded, and returns no-store with redacted codes. Policy handling correctly keeps unknown CLI fields, never turns a malformed policy permissive, and preserves approve semantics; toolApproval behavior for chat is unchanged by the extraction into lib/mcp-servers/policy.ts (verified against the base ref). Cleanup uses an independent bounded controller so cancellation cannot skip teardown and stalled teardown cannot mask a successful discovery — the diagnostics tests pin both. No security or correctness blockers found. Three non-blocking notes below, the first worth addressing before merge: infrastructure (Supabase) read failures are reported to the user as connection ("Check the server URL, Streamable HTTP support, and saved headers") and the underlying error is never logged, so our own outage looks like the user's server misbehaving and produces no diagnosable signal.
Warnings
- Database read failures are misreported as user-side connection failures and logged without any detail (lib/mcp-servers/diagnostics.ts)
IntestSavedMcpServer, the saved-record read is inside the main try block:if (error) throw error;. A Postgrest/Supabase error carries nostatusCode401/403 and is not a ZodError/UnsafeOutboundUrlError/MissingMcpSecretError, sofailureCodefalls through to"connection". The user then gets HTTP 200 with "The connection test failed. Check the server URL, Streamable HTTP support, and saved headers." for an outage on our side, andconsole.warn("[mcp-servers] Connection test failed", { userId, serverId, code })records only the derived code — the error itself is dropped, so there is nothing to alert or debug on.lib/mcp-servers/diagnostics.test.tsasserts this exact mapping, so it looks deliberate; keeping details out of the response is right, but conflating our infra failure with the user's server is not.
Suggestion: handle the storage read separately — either rethrow so the route returns 500 (the client already renders a generic "The test could not complete. Try again." for non-ok responses), or add a distinct code such as unavailable with a "Something went wrong on our side" message. Either way log the error server-side for this branch (error.code/error.message from Postgrest contains no MCP secrets, unlike upstream transport errors, which is why the chat path suppresses them).
Suggestions
- Teardown detection depends on the SDK passing
methodvia init, and the test fixture cannot catch a regression (lib/connections/mcp-tools.ts)
The guarded fetch wrapper decides whether to use the bounded cleanup signal withinit?.method === "DELETE". If@ai-sdk/mcpever passes aRequestobject asinputwith the method on it and noinit.method,cleanupSignalbecomes undefined, the wrapper falls back toinit?.signal(already aborted byclient.close()),signal.throwIfAborted()fires, and session teardown silently stops happening. The diagnostics fixture normalizes withnew Request(input, init)before checkingrequest.method === "DELETE", so it would still pass. It works with the pinned version today (the cancellation test asserting one DELETE proves it), but considerconst method = input instanceof Request ? input.method : (init?.method ?? "GET");so the production check matches what the fixture observes. - Unrate-limited endpoint that initiates outbound connections (app/api/mcp-servers/[id]/test/route.ts)
Each POST holds a request for up to ~8s and opens an outbound connection to a user-controlled (public, DNS-validated) host. The UI disables duplicate clicks, but that is a client-side guard only. You have explicitly deferred this per AGENTS.md, which is reasonable given the endpoint is authenticated, owner-scoped, saved-target-only and deadline-bounded — noting it only so the tradeoff is recorded on the PR rather than just in the description.
Mogplex PR ReviewStatus: No material issues found Summary This is a well-constructed PR. The diagnostics path is carefully ordered (ownership filter → transport check → policy parse → URL/DNS validation → secret resolution → discovery), the teardown budget is genuinely bounded rather than best-effort, and the tests exercise the real MCP SDK instead of mocking it. I found no security or correctness defects that should block the merge. Verdict: ✅ APPROVE. What I verified
Suggestions (all optional, none blocking)
I did not re-raise rate limiting on Affected files:
|
1 participant
Enabled saved MCP servers could fail to load without a visible explanation, and approval rules required JSON edits. Add an on-demand Test connection action and normal Tool permissions controls in Connections → MCP Servers.
The connection test reads only the authenticated user's saved definition, validates public URLs before reading secrets, discovers tools without executing them, and closes the session. Results show tool counts, workspace availability, Control-only approval requirements, blocked tools, and actionable redacted failures. Discovery has a six-second deadline. Teardown gets a separate two-second deadline, remains possible after cancellation, and cannot hide successful discovery. Results are labeled as the last test in the current visit and reset after edits/reloads; they are not continuous health monitoring.
Permission controls edit the existing CLI-compatible fields: default approval, allowlists, blocklists, and per-tool automatic/prompt/block overrides. Preserve unknown CLI fields and existing restrictions; malformed HTTP policies fail validation rather than becoming permissive.
approveretains its existing pre-approved meaning. Prompted tools remain unavailable to workspace chat and require Control.Validation:
Merge only after code reviewer findings are addressed and CI passes. Companion docs update will merge after the app deploys.
Review fixes: keep discovery results when cleanup stalls; use an independent bounded DELETE signal after cancellation; allow unrelated updates to unchanged legacy policy records; trim/deduplicate edited lists on blur without disrupting typing; remove the empty stdio diagnostics spacer.
The advisory rate-limit suggestion is deferred: AGENTS.md prohibits new artificial user limits without approval. The endpoint remains authenticated, owner-scoped, saved-target-only, DNS/redirect guarded, deadline-bounded, and duplicate clicks are disabled in the UI.
Mutation review: the aggregate score includes whole pre-existing files (not only changed lines), notably normalization paths exercised in the separate Node unit tier. Added focused regressions for meaningful survivors in the new policy and diagnostic code, and verified each fails against its mutation.
Companion docs: Mogplex/docs#138
Final review fixes also validate policies when changing transport to HTTP, cancel browser tests when the saved record changes/unmounts, and disable approval edits while a per-tool disabled rule applies. Regression cases failed against the previous implementation before passing with these fixes. Cancellation intentionally shares the bounded-timeout diagnostic classification; the disconnected caller receives no response.
Database lookup failures now return a distinct safe settings-unavailable result rather than blaming the remote server; logs retain the database error code without copying its message. Runtime and browser regressions cover the distinction. The SDK Request-object note is forward compatibility advice: the installed SDK supplies DELETE through RequestInit.method, as exercised by the actual-SDK cancellation and slow-cleanup tests.