Conversation
4390417 to
700cf15
Compare
marekdano
left a comment
There was a problem hiding this comment.
1. 🔴 High — OAuth popup blocked on first-time setup
File: src/pages/ServerCatalog.tsx:638
handleOAuthSubmit does await registerServer(...) (an HTTP POST) before calling serversApi.triggerOAuthAuthorization(gatewayId). By the time window.open() runs inside that call, the browser no longer treats it as a direct response to the click, so popup blockers will block it (authWindow is null) — this will trigger on essentially every first-time OAuth registration.
Failure scenario: User clicks "Configure and authorize" on a new OAuth catalog server → popup blocked → "Failed to open OAuth authorization window" error. Retrying then hits the pendingOAuthGatewayId fast path, which calls triggerOAuthAuthorization synchronously and succeeds — so it looks like a transient glitch but is actually deterministic.
2. 🟠 Medium-High — Removed auth-type filter exposes unsupported servers
File: src/pages/ServerCatalog.tsx:454
The catalog's auth-type allow-list filter (previously SUPPORTED_AUTH_TYPE_SET) was removed. supportedServers = data?.servers ?? [] no longer filters by auth type, so servers with types outside Open/API Key/OAuth (e.g. Basic, mTLS) now render and are "Add"-able.
Failure scenario: Clicking "Add" on such a server falls through handleAdd's if/else chain straight to void registerServer(server) with no credentials — likely a generic server-side failure, or a silent no-auth registration. No test covers this path.
3. 🟡 Medium — Disconnect/Test not blocked during in-flight authorization
File: src/components/server-catalog/CatalogResults.tsx:202
The new Authorize action shares the isAdding pending flag with Test and Disconnect, but Test's/Disconnect's own disabled conditions were never updated to also check isAdding.
Failure scenario: User clicks Authorize (popup opens, isAdding true) → reopens the dropdown and clicks Disconnect (only checks isTesting || isDisconnecting) → gateway is deleted mid-authorization → when the popup resolves, toggleEnabled/fetchToolsAfterOAuth run against a deleted gateway id, surfacing a confusing error instead of being prevented up front.
|
NIT: Add |
Could you also share a screen recording or screenshots of the OAuth setup flow? Useful to see: the dialog form with validation errors, the card status badges, and the authorize action triggering the popup. |
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
9525bcb to
f1456a7
Compare
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
marekdano
left a comment
There was a problem hiding this comment.
Manual Testing Notes (catalog OAuth add flow)
Environment
- UI (BFF):
localhost:3000 - API (mcpgateway):
localhost:8000 - Branch under test:
feature/catalog-oauth-add-flow
Steps
-
Opened the MCP server catalog, picked a GitHub-backed entry, and used the new OAuth add flow (
CatalogOAuthDialog) to register it. -
Triggered the OAuth popup and completed GitHub's consent screen.
-
GitHub redirected the popup to: http://localhost:8080/oauth/callback?code=...&iss=https%3A%2F%2Fgithub.com%2Flogin%2Foauth&state=popup...
-
Browser showed
502 Bad Gateway (nginx)— flow did not complete.
Result: ❌ Fail
Root cause
redirect_uri resolved to http://localhost:8080, which is neither the UI (:3000) nor the API (:8000) in this environment.
APP_DOMAIN=http://localhost:8080in the siblingmcp-context-forge/.env— a stale value left over from a docker-compose (nginx+gateway) stack, unrelated to the native:8000/:3000setup used here.- mcpgateway's
GET /oauth/authorize/{id}defaults the outboundredirect_uriit sends to GitHub fromAPP_DOMAINwhenever the server's ownoauth_config.redirect_uriisn't set. CatalogOAuthDialog.tsx(src/components/server-catalog/CatalogOAuthDialog.tsx:159-168) fetches/oauth/callback-urland displays it for the user to copy into their IdP app settings, but never includesredirect_uriin theoauth_credentialspayload it submits. So a catalog-registered OAuth server always falls back to mcpgateway'sAPP_DOMAINdefault instead of this deployment's own callback URL.
Fixing APP_DOMAIN to http://localhost:3000 (the BFF's public origin, which proxies the final hop to the gateway) resolves this catalog-registered OAuth server always falls back to mcpgateway's APP_DOMAIN default instead of this deployment's own callback URL.
Fixing APP_DOMAIN to http://localhost:3000 (the BFF's public origin, which proxies the final hop to the gateway) resolves this specific environment. Not a bug in this PR's code by itself, but it exposed a gap below.
Related gap found (worth a PR comment)
The regular "Add MCP Server" form (src/hooks/useMCPServerForm.ts) does not have this problem:
- Submits it:
redirect_uri: oauthRedirectUri || undefinedinoauth_config(:580). - Guards submission with
oauthRedirectUriUnresolved(:401) while that fetch is loading/failed, specifically to avoid silently sendingredirect_uri: undefinedand falling back toAPP_DOMAIN.
CatalogOAuthDialog has none of this — no submitted redirect_uri, no unresolved-fetch guard. It's silently dependent on the gateway operator's APP_DOMAIN being correct for split deployments, which is exactly the scenario the /oauth/callback proxy (server/src/routes/proxy/oauth-callback.ts) was built to handle.
Suggested fix: have CatalogOAuthDialog submit redirect_uri: callbackUrl in oauth_credentials, matching useMCPServerForm.ts's behavior, and block submit while the callback URL fetch is unresolved.
Screen.Recording.2026-09-15.at.11.53.30.mov
Summary
Backend dependency
Requires backend PRs IBM/mcp-context-forge#6588 and IBM/mcp-context-forge#6620. Handwritten frontend contract types are temporary until OpenAPI generation includes the merged API.
Validation
NOTE
Please do not merge this till backend dependency is resolved. For the purpose of development handwritten contracts is added but it needs to be cleaned up once backend dependency is resolved