Fix MCP proxy "Refetch Server Info" and let it reuse stored credentials - #3046
Conversation
…y of `url` and `proxyId`. Update related tests and types to ensure correct API request structure and validation logic.
…er' as default for gateway-pushed providers. Update related tests and documentation to reflect changes in API request structure and validation logic.
|
Warning Review limit reached
Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesLLM upstream authentication defaults
MCP endpoint refetch selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExternalServersOverview
participant FetchServerInfo
participant StoredProxyConfig
participant MCPBackend
ExternalServersOverview->>FetchServerInfo: Send url and proxyId
FetchServerInfo->>StoredProxyConfig: Load stored URL and credential
FetchServerInfo->>MCPBackend: Call supplied URL with stored credential
MCPBackend-->>FetchServerInfo: Return server information
FetchServerInfo-->>ExternalServersOverview: Return refetch result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…to 'none' for gateway-pushed providers. Update related tests and components to ensure correct behavior and preserve explicit auth types.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
platform-api/internal/service/llm_test.go (1)
107-124: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover credential clearing for credential-less auth.
Add populated-header/value cases for blank, explicit
none, and explicitotherauth; assert the mapped auth clears both fields. This is the security behavior now centralized indefaultUpstreamAuthType.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/service/llm_test.go` around lines 107 - 124, Add table-driven or equivalent populated-header/value test cases around mapLLMUpstreamYAMLToModel for blank, explicit none, and explicit other auth types; assert each mapped auth clears both credential fields while preserving the expected auth type. Reuse the existing defaultUpstreamAuthType behavior and test setup without changing unrelated mapping cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/internal/service/mcp.go`:
- Around line 573-578: Update the MCP endpoint validation around the proxyId and
URL checks to compare the caller-provided URL host with the stored proxy host
before resolving or applying stored credentials. Reject proxyId + URL when hosts
differ, while allowing path and query changes on the same host and preserving
the existing auth restriction.
---
Nitpick comments:
In `@platform-api/internal/service/llm_test.go`:
- Around line 107-124: Add table-driven or equivalent populated-header/value
test cases around mapLLMUpstreamYAMLToModel for blank, explicit none, and
explicit other auth types; assert each mapped auth clears both credential fields
while preserving the expected auth type. Reuse the existing
defaultUpstreamAuthType behavior and test setup without changing unrelated
mapping cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a61a0ef-2655-4aa5-9625-9458a41f811f
📒 Files selected for processing (9)
platform-api/internal/service/artifact_import_llm_provider.goplatform-api/internal/service/llm.goplatform-api/internal/service/llm_test.goplatform-api/internal/service/mcp.goplatform-api/internal/service/mcp_test.goplatform-api/resources/openapi.yamlportals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.jsportals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsxportals/ai-workspace/src/utils/types.ts
Thushani-Jayasekera
changed the title
|
@CodeRabbit review |
✅ Action performedReview finished.
|
2 participants
Fix MCP proxy "Refetch Server Info" and let it reuse stored credentials
Problem
Clicking Refetch Server Info on an MCP proxy's Backend Connection tab always failed:
{ "status": "error", "code": "VALIDATION_FAILED", "message": "Provide either url or proxyId, not both." }The UI sent
{ url, proxyId }on every unedited refetch, butMCPServerInfoFetchRequestdeclared the two mutually exclusive (
oneOf) andFetchServerInforejected the pairoutright. The refetch button was broken for every proxy.
Fixing the immediate error exposed a second, worse gap.
auth.valueis write-only — itcomes back to the browser masked as
******— so the only way to refetch with a storedcredential was
proxyId, which also forces the stored URL. That left no way to answer"does my edited endpoint work?" without first re-typing a credential the user may never
have had a copy of. Editing just the URL dead-ended in:
What changed
urlandproxyIdare no longer mutually exclusive. Which fields are present now selectsthe target and the credential independently, giving three valid shapes:
{ url, auth? }{ proxyId }{ url, proxyId }authstill may never accompanyproxyId: whenever a proxy is referenced its stored authis authoritative, and an override is rejected rather than silently ignored. A request with
neither
urlnorproxyIdis still rejected.The supplied URL is used verbatim — no path normalization, no
/mcpappended — thesame invariant the
proxyId-only flow already guarantees and thatTestFetchServerInfoRefetchUsesStoredURLVerbatimguards.Backend —
platform-apiFetchServerInfo(internal/service/mcp.go): dropped the "not both" rejection. WhenproxyIdis given, the stored URL and credential are resolved as before, then asupplied
urloverrides the URL only. Theauth+proxyIdand neither-fieldrejections are unchanged.
MCPServerInfoFetchRequest(resources/openapi.yaml):oneOf→anyOf(at least oneof
url/proxyId) plusnot: required: [proxyId, auth], which expresses "auth neverwith proxyId" without also forbidding the new pair. Field descriptions updated.
Frontend —
portals/ai-workspaceExternalServersOverview.handleRefetch: rewritten to pick the shape by which credentialthe fetch runs with rather than by "is anything edited". Ordered so a typed credential
always wins, a cleared/absent auth header fetches unauthenticated, and an edited endpoint
with an untouched credential takes the new
{ url, proxyId }path.header while the value stays masked has no valid shape (
proxyIdwould resolve the oldheader name too), so it asks for the value or a save first.
MCPServerInfoFetchRequest(src/utils/types.ts): modelled as a union soauth+proxyIdfails to compile rather than failing at runtime.Testing
go test ./internal/service/— passes.TestFetchServerInfoSuppliedURLWithStoredCredential(new): asserts the supplied URL iscontacted verbatim and the proxy's stored credential header still rides along.
TestFetchServerInfoRejectsAmbiguousTargets: the "both url and proxyId" case isremoved (now valid); the
auth-with-proxyIdand neither-field cases stay.tsc --noEmit— no new errors.005-mcp-backend-connection-refetch.cy.jsupdated: TC-98/TC-100 asserted the oldurl-present shape and now asserturlis omitted; TC-102 (new) covers the{ url, proxyId }path — edited endpoint, credential left masked, noauthon the wire.Not executed locally (needs a running stack).
Reviewer notes
Scope of the relaxation.
fetchMCPProxyServerInfois granted byap:mcp_proxy:reador
ap:mcp_proxy:manage, whileupdateMCPProxyneedsap:mcp_proxy:updateormanage. So aread-only caller can now have the server attach a proxy's storedcredential to a request aimed at a URL they supply. This was a deliberate call for
simplicity over a same-origin restriction on the supplied URL. Note the capability is not
entirely new — a caller holding
updatecould already reach the same outcome in two steps,since
preserveMCPUpstreamAuthValuecarries the stored secret across a URL-only save(asserted by TC-101) — but it is new at the read tier. If that tier matters, the smallest
follow-up is to require
ap:mcp_proxy:updatefor theurl+proxyIdshape specifically.Also in this branch: LLM provider upstream auth defaults to
none. Commite24cb2bbdhad changed the gateway-import path to default an absent upstream auth block to
other(
defaultUpstreamAuthToOther); that is reverted here, somapLLMUpstreamYAMLToModeldefaults to
noneas before. The AI Workspace provider-creation form is aligned with it:picking a credential-bearing auth type but leaving the API key empty now records
nonerather than
other(ServiceProviderNew.tsx). An explicitly chosenotherornoneisstill preserved either way. Independent of the MCP work above and reviewable separately.