Skip to content

feat: register external MCP servers as custom apps - #107

Merged
barockok merged 22 commits into
amarthaid:mainfrom
feryadialoi:feature/connectors
Sep 19, 2026
Merged

barockok merged 22 commits into
amarthaid:mainfrom
feryadialoi:feature/connectors

Conversation

@feryadialoi

@feryadialoi feryadialoi commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

What

Register an external MCP server (HTTP streamable, OAuth 2.1) as a per-user custom app, modeled as an App beside the native apps. Its tools surface through workbench's own search_tools / execute_tools.

Why

Workbench connects to SaaS integrations but not arbitrary MCP servers. A custom app exposes a third-party MCP server's tools to agents, through this app only.

Changes

  • custom_apps table + store; OAuth 2.1 discovery (401 WWW-Authenticate → protected-resource → authorization_servers) and RFC 7591 dynamic client registration from a base URL; PKCE token exchange + refresh (honours client_secret_basic/_post, defaults to client_secret_basic).
  • MCP streamable-HTTP client (SDK) for live tools/list + tools/call, namespaced appName__tool; SSE fallback for legacy servers, tool-call timeout, per-user session cache.
  • SSRF hardening: every discovered endpoint + redirect hop validated; IPv4-mapped IPv6 blocked; loopback gated to dev.
  • Per-user tool index wired into search_tools / get_tool_schema / execute_tools / list_integrations; args pass through unvalidated (JSON Schema, no zod).
  • Portal: custom apps in the Apps catalog (custom: true), "New custom app" CTA, connect through the /connect/:integration handshake, AppDetail lists remote tools + Delete app.
  • Vite dev proxy tightened (/c/c/, /authorize exact-match) so SPA routes aren't forwarded to the server; encryption.ts key computed lazily (no load-time config read).

How to test

  • npm run test (server + portal suites pass).
  • Portal → Apps → New custom app → name + URL → Connect (OAuth redirect) → tools appear via search_tools.

Not in this PR

  • Agent-side connect() for custom apps (portal-only).
  • Manual client_id/client_secret fallback when DCR is unsupported.
  • Image content blocks dropped to a marker; server→client elicitation/sampling not declared.

Screenshot

Description Image
Before Connect image
After Connect image
MCP Inspector search_tools image
MCP Inspector execute_tools image
Apps catalog with custom app image
Tools from Custom App image
Add Custom App image
Delete Custom App image
Conflict Error when name exists image

@barockok

barockok commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Review

Must fix

  1. Refresh wipes refresh tokenconnectors/oauth.ts ensureConnectorToken stores refreshed.refreshToken; many ASes omit it on refresh → NULL → reconnect after next expiry. Keep old: refreshed.refreshToken ?? data.refreshToken (as plugins/context.ts does).
  2. SSRF via remote metadata — only the typed base URL is checked. authorization/token/registration_endpoint from .well-known are fetched unchecked, and fetch follows redirects. A hostile server can point token_endpoint at 169.254.169.254/internal hosts; hit on register, connect, every refresh. [::ffff:a9fe:a9fe] also passes isPrivateHost. Validate every discovered endpoint + each redirect hop (redirect: "manual").
  3. Loopback allowed in prodconnectors/ssrf.ts allows localhost/127.x unconditionally ("for dev" but no dev gate). Any user can reach the server's own loopback (CDP port, admin/metrics). Gate behind explicit dev flag.

Should fix

  1. Discovery on hot pathconnectors/index.ts ensureIndex: cold cache → serial tools/list on every connector, no single-flight, no timeout; one hung server stalls search_tools. executeMany (8 concurrent) → 8 parallel refreshes with same refresh token → rotation rejects / family revoked. listConnectors outside try/catch breaks executeSingle never-throws.
  2. Discovery not per MCP auth spec — only origin-root /.well-known/oauth-authorization-server. Follow 401 WWW-Authenticate → protected-resource authorization_servers → AS metadata (incl. path-qualified well-knowns). Servers with a separate IdP currently 400.
  3. Default auth method — registration response without token_endpoint_auth_method defaults to client_secret_post; RFC 7591 default is client_secret_basic.
  4. Remote errors logged as successmcp/meta-tools.ts executeConnectorSingle: isError: true → audit success: true, metrics success, REST 200.

Client identity / parity

  • DCR client_name: "workbench:<name>" → consent screen shows that. Use "Workbench" + client_uri/logo_uri.
  • new Client({ name: "a-workbench", version: "0.1.0" }) — use real name + package version.
  • New client + initialize per call (withClient) → loses Mcp-Session-Id, breaks stateful servers. Cache per (user, server).
  • No-auth servers rejected; no manual client id/secret fallback when DCR unsupported (per-id callback URL blocks pre-registration — use one fixed callback, resolve via state).
  • Requests all scopes_supported; use scope from WWW-Authenticate / PRM.
  • Disconnected servers' tools vanish silently — surface "not connected" in list_integrations.
  • tools/list annotations/title dropped (readOnlyHint/destructiveHint lost).

Product model: Custom App, not "Connectors"

Model an external MCP server as an App, same as native apps — no separate Connectors concept.

  • Drop pages/Connectors.tsx, its sidebar item and /connectors route.
  • Apps.tsx: add New Custom App CTA → <Modal size="md"> (name + URL) → create.
  • Connect through the existing /connect/:integration/connected/:integration handshake pages; drop /api/auth/connector/:id and the /connectors?status= redirect.
  • Detail at /apps/:name (AppDetail) listing the remote tools like any other app.
  • Server: return custom apps from the existing apps/integrations endpoints (custom: true); rename connector:<id> keys to e.g. custom:<id>.
  • Rename finding doc + CLAUDE.md index entry accordingly.

Transport / SSE

Workbench /mcp is JSON-only (no SSE, no GET stream); execute_tools buffers results for native and custom apps alike — remote progress/partial output never reaches the agent. Downstream (StreamableHTTPClientTransport):

  • Streamable HTTP with text/event-stream responses — works (SDK collects to final result).
  • Legacy HTTP+SSE-only servers (2024-11-05) — break: no fallback. On 4xx from Streamable connect, retry with SSEClientTransport (SDK-recommended pattern).
  • Tools > 60s — break: SDK default request timeout, resetTimeoutOnProgress unset. Pass callTool(..., { timeout, resetTimeoutOnProgress: true, maxTotalTimeout }).
  • Server→client requests mid-call (elicitation/sampling) — break: no client capabilities declared. Keep unsupported, document it.

@feryadialoi feryadialoi changed the title feat: register external MCP servers as connectors feat: register external MCP servers as custom apps Sep 18, 2026
@barockok

Copy link
Copy Markdown
Collaborator

Re-review @ 3c6f866

Most of the previous comment is addressed — refresh-token keep + single-flight, SSRF on discovered endpoints + no redirects, IPv4-mapped IPv6, discovery single-flight/timeout, spec discovery (WWW-Authenticateauthorization_servers), client_secret_basic default, isError → failure, client_name: "Workbench", session cache, SSE fallback, tool timeouts, annotations kept, Custom App UI (Apps CTA → md modal → /connect handshake → AppDetail tools). 👍

New bugs

  1. Refresh failure crashes the processcustom-apps/oauth.ts void refresh.finally(() => refreshLocks.delete(key)): .finally() returns a promise that rejects with the refresh error and nothing handles it. No unhandledRejection handler in the server → Node exits. Fix: refresh.finally(...).catch(() => {}).
  2. Migration not awaiteddb.ts calls migrateConnectorsToCustomApps(db, …) without await: races startup queries; a failure is another unhandled rejection. It also DELETEs connector:% rows from audit_log — audit history shouldn't be destroyed.
  3. DNS-based SSRF still open — the block is literal-only, so 169.254.169.254.nip.io or any internal DNS name passes. Now the main remaining hole; any signed-in user can register an app. Resolve the host and check the resolved IPs (pin the resolved address against rebinding).

Remaining / minor

  • Loopback gate is NODE_ENV === "production", but config defaults to development → a prod run without NODE_ENV allows loopback. Invert: allow only when explicitly development.
  • MCP transports use the SDK's own fetch (follows redirects, no SSRF check) — pass fetch: safeFetch to StreamableHTTPClientTransport / SSEClientTransport.
  • Session cache: a server-side expired session (404) leaves a dead cached client until the token changes; no eviction on delete/disconnect.
  • SSE fallback triggers on any 4xx incl. 401/403 → misleading error on auth failure.
  • CLIENT_INFO.version hardcoded "0.29.0" (repo is 0.30.0) — read from package.json.
  • GET /api/integrations (Apps page) awaits live discovery; discovery is serial across apps (N × 10s) and the timeout doesn't cover token refresh. Run apps in parallel, include the refresh in the timeout.
  • AS metadata URL is built by appending /.well-known/oauth-authorization-server — wrong for path-qualified issuers (RFC 8414 §3.1 inserts it after the host); no OIDC openid-configuration fallback.
  • list_integrations returns name: c.name, while tools/connections use custom:<id> — inconsistent for agents.
  • App name becomes the tool prefix (My custom app__tool, with spaces) — slugify.
  • Carried over: no-auth servers and manual client id/secret (non-DCR) unsupported.

@feryadialoi

Copy link
Copy Markdown
Contributor Author

@barockok ready for another pass — re-review items addressed:

New bugs

  • Refresh .finally rejection now swallowed (no unhandled-rejection crash).
  • Migration dropped entirely (feature never shipped, nothing to migrate).
  • DNS SSRF: hosts are resolved and rejected on any private IP; plain-HTTP fetches pinned to the resolved address (rebinding).

Remaining / minor

  • Loopback only when NODE_ENV === "development" explicitly.
  • MCP transports use safeFetch (redirects refused).
  • Session cache evicted on 404/410 and on delete/disconnect.
  • SSE fallback narrowed to 404/405 (401/403 surface as errors).
  • CLIENT_INFO.version read from package.json.
  • Discovery parallel per app; timeout covers token refresh + tools/list.
  • RFC 8414 path-qualified issuer + OIDC openid-configuration fallback.
  • list_integrations uses custom:<id>; app names slugified for tool prefixes.

Carried over (unchanged): no-auth servers and non-DCR manual client — OAuth + DCR only, per the original spec.

Also fixed: deleting a custom app no longer leaks its raw custom:<uuid> into the Home "Most used app" stat (shows a muted "Deleted app").

@barockok

Copy link
Copy Markdown
Collaborator

Approving. Tested end-to-end locally at 511ecc3: register a custom app → Apps card → Connect → /connected/:integration handshake → detail page lists tools → search_tools / execute_tools / list_integrations over /mcp all work.

Non-blocking notes (follow-up)

  • Vite proxy regex breaks dev OAuth"^/authorize(?:/resume)?$" is matched against the URL including the query string, so /authorize?response_type=… falls through to the SPA → lands on Home. Affects every dev OAuth flow (incl. agent MCP OAuth). Fix: "^/authorize(?:/resume)?(?:\\?|$)". Dev-only.
  • DNS pin breaks plain-http:// appssafeFetch connects to the resolved IP and sets Host, but Node fetch (undici) ignores a Host override; upstream sees Host: <ip> → name-routed HTTP servers 404. Pin via an undici Agent with connect.lookup returning the vetted IP instead (keeps hostname/SNI, covers HTTPS rebinding too).
  • clientInfo.version = 0.0.0 in DockerreadVersion resolves __dirname/../../../../package.json/package.json in the runtime image (code at /app/server/custom-apps; root package.json not copied). Inject version at build time.
  • IPv6 resolved address never pinned (hostname = "2001:db8::1" without brackets is silently ignored) — moot with the Agent fix.
  • list_integrations custom items expose only custom:<uuid> — add displayName: c.name.
  • withTimeout never clears its setTimeout — clear in finally.

@barockok barockok closed this Sep 19, 2026
@barockok barockok reopened this Sep 19, 2026
@barockok
barockok merged commit 00a52d2 into amarthaid:main Sep 19, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants