From a87e15908f8da03a565dae4e91b5168b16b24780 Mon Sep 17 00:00:00 2001 From: Thushani Jayasekera Date: Thu, 30 Jul 2026 22:22:28 +0530 Subject: [PATCH 1/3] Refactor MCP backend connection handling to enforce mutual exclusivity of `url` and `proxyId`. Update related tests and types to ensure correct API request structure and validation logic. --- .../005-mcp-backend-connection-refetch.cy.js | 10 ++++--- .../ExternalServersOverview.tsx | 6 +++-- portals/ai-workspace/src/utils/types.ts | 27 +++++++++++++------ 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js b/portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js index 9dbddb43c0..b12d7d9ed3 100644 --- a/portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js +++ b/portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js @@ -23,10 +23,11 @@ * shapes, chosen by whether the endpoint URL / auth header / auth value differ * from what is currently persisted on the server: * - * - Unedited: POST /mcp-proxies/fetch-server-info { url, proxyId } — the + * - Unedited: POST /mcp-proxies/fetch-server-info { proxyId } — the * backend resolves the stored URL and auth (via the saved secret handle) * itself, so the browser never needs to re-send a credential it can't - * read back (auth.value is writeOnly). + * read back (auth.value is writeOnly). url and proxyId are mutually + * exclusive by API contract, so this omits url. * - Edited: POST /mcp-proxies/fetch-server-info { url, auth: { type, * header, value } } — validates the live, not-yet-saved values directly. * proxyId and an auth override are mutually exclusive by API contract, so @@ -199,7 +200,7 @@ describe('AI Workspace — MCP proxy Backend Connection tab (Refetch Server Info cy.wait('@refetch').then((pi) => { const body = pi.request.body; expect(body.proxyId, 'refetch request carries proxyId').to.equal(createdServerId); - expect(body.url, 'refetch request still carries the current url').to.equal(ORIGINAL_URL); + expect(body.url, 'url is omitted — mutually exclusive with proxyId').to.be.undefined; expect(body.auth, 'no auth override sent alongside proxyId').to.be.undefined; }); @@ -297,7 +298,8 @@ describe('AI Workspace — MCP proxy Backend Connection tab (Refetch Server Info cy.wait('@refetchAfterSave').then((pi) => { const body = pi.request.body; expect(body.proxyId, 'refetch after save uses proxyId again').to.equal(createdServerId); - expect(body.url, 'refetch after save uses the newly saved url').to.equal(newUrl); + expect(body.url, 'url omitted — the backend resolves the newly saved url').to.be + .undefined; expect(body.auth, 'no auth override needed post-save').to.be.undefined; }); }); diff --git a/portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx b/portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx index de7599718d..d0b9d4fe90 100644 --- a/portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx +++ b/portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx @@ -694,8 +694,10 @@ export default function ExternalServersOverview(): JSX.Element { if (endpointUnchanged && headerNameUnchanged && credentialUnchanged) { // Nothing edited yet — let the backend resolve the stored URL and auth (via the // saved secret handle) from the persisted proxy config instead of us re-entering - // a credential we can never read back. - request = { url: trimmedUrl, proxyId: server.id }; + // a credential we can never read back. url and proxyId are mutually exclusive + // (the backend rejects both), and the unchanged url is exactly what it would + // resolve anyway, so send proxyId alone. + request = { proxyId: server.id }; } else if (trimmedHeaderName && !isCredentialMasked && authHeaderValue.trim()) { // Something was edited and we have a live credential value to validate with. // proxyId + auth override together aren't allowed, so this omits proxyId. diff --git a/portals/ai-workspace/src/utils/types.ts b/portals/ai-workspace/src/utils/types.ts index d85327a33e..9fd9a368ee 100644 --- a/portals/ai-workspace/src/utils/types.ts +++ b/portals/ai-workspace/src/utils/types.ts @@ -837,19 +837,30 @@ export type MCPServerListResponse = ApiListResponse; /** * MCP Server Info Fetch Request + * + * `url` and `proxyId` are mutually exclusive (the API's oneOf), and `auth` is only + * allowed alongside `url` — with `proxyId` the stored auth is authoritative and an + * override is rejected. Modelled as a union so the invalid combinations don't compile. */ -export interface MCPServerInfoFetchRequest { - url: string; - proxyId?: string; +interface MCPServerInfoFetchRequestBase { transportType?: string; headers?: Record; - auth?: { - type: string; - header: string; - value: string; - }; } +export type MCPServerInfoFetchRequest = MCPServerInfoFetchRequestBase & + ( + | { + url: string; + proxyId?: never; + auth?: { + type: string; + header: string; + value: string; + }; + } + | { proxyId: string; url?: never; auth?: never } + ); + /** * MCP Server Info Fetch Response */ From e24cb2bbd1863dc18e6b552dc825ca86999bd0f4 Mon Sep 17 00:00:00 2001 From: Thushani Jayasekera Date: Thu, 30 Jul 2026 22:45:17 +0530 Subject: [PATCH 2/3] Refactor upstream authentication handling in LLM provider to use 'other' as default for gateway-pushed providers. Update related tests and documentation to reflect changes in API request structure and validation logic. --- .../service/artifact_import_llm_provider.go | 2 +- platform-api/internal/service/llm.go | 26 +++++-- platform-api/internal/service/llm_test.go | 34 ++++++--- platform-api/internal/service/mcp.go | 28 +++++--- platform-api/internal/service/mcp_test.go | 69 +++++++++++++++++-- platform-api/resources/openapi.yaml | 30 ++++---- .../005-mcp-backend-connection-refetch.cy.js | 51 +++++++++++--- .../ExternalServersOverview.tsx | 43 +++++++----- portals/ai-workspace/src/utils/types.ts | 14 ++-- 9 files changed, 222 insertions(+), 75 deletions(-) diff --git a/platform-api/internal/service/artifact_import_llm_provider.go b/platform-api/internal/service/artifact_import_llm_provider.go index bb1c83b498..6927802f0d 100644 --- a/platform-api/internal/service/artifact_import_llm_provider.go +++ b/platform-api/internal/service/artifact_import_llm_provider.go @@ -175,6 +175,6 @@ func mapLLMProviderSpecToConfig(spec dto.LLMProviderDeploymentSpec) model.LLMPro // single endpoint to the main endpoint. Returns nil when no upstream is present. func mapLLMUpstreamYAMLToModel(in dto.LLMUpstreamYAML) *model.UpstreamConfig { endpoint := &model.UpstreamEndpoint{URL: in.URL, Ref: in.Ref} - endpoint.Auth = defaultUpstreamAuthToNone(mapUpstreamAuthAPIToModel(in.Auth)) + endpoint.Auth = defaultUpstreamAuthToOther(mapUpstreamAuthAPIToModel(in.Auth)) return &model.UpstreamConfig{Main: endpoint} } diff --git a/platform-api/internal/service/llm.go b/platform-api/internal/service/llm.go index eece1699a3..d3b3fef365 100644 --- a/platform-api/internal/service/llm.go +++ b/platform-api/internal/service/llm.go @@ -3456,14 +3456,32 @@ func resolveTemplateOpenAPISpec(ctx context.Context, tpl *model.LLMProviderTempl return "" } -// defaultUpstreamAuthToNone applies the platform's default upstream auth type and -// strips credentials from credential-less types. +// defaultUpstreamAuthToNone applies the platform's default upstream auth type for +// caller-authored requests (create/update), where an omitted auth block means the +// upstream genuinely takes no credentials. func defaultUpstreamAuthToNone(auth *model.UpstreamAuth) *model.UpstreamAuth { + return defaultUpstreamAuthType(auth, string(api.None)) +} + +// defaultUpstreamAuthToOther applies the default upstream auth type for LLM providers +// imported from a gateway. An absent auth block there doesn't imply the upstream is +// unauthenticated — the gateway may authenticate it via user-attached policies the +// control plane doesn't model — so "other" is used rather than "none". This applies to +// the provider import path only; every other path defaults to "none" (or leaves auth +// absent) as before. The two are equivalent to the gateway at runtime — it attaches no +// auth policy for either — so this affects what the control plane reports, not routing. +func defaultUpstreamAuthToOther(auth *model.UpstreamAuth) *model.UpstreamAuth { + return defaultUpstreamAuthType(auth, string(api.Other)) +} + +// defaultUpstreamAuthType fills in an absent/blank auth type with defaultType and +// strips credentials from credential-less types. +func defaultUpstreamAuthType(auth *model.UpstreamAuth, defaultType string) *model.UpstreamAuth { if auth == nil { - return &model.UpstreamAuth{Type: string(api.None)} + return &model.UpstreamAuth{Type: defaultType} } if strings.TrimSpace(auth.Type) == "" { - auth.Type = string(api.None) + auth.Type = defaultType } if isCredentialLessUpstreamAuthType(auth.Type) { auth.Header = "" diff --git a/platform-api/internal/service/llm_test.go b/platform-api/internal/service/llm_test.go index e28bc25b0a..7840b38857 100644 --- a/platform-api/internal/service/llm_test.go +++ b/platform-api/internal/service/llm_test.go @@ -92,24 +92,36 @@ func TestNormalizeUpstreamAuthType(t *testing.T) { } } -// TestMapLLMUpstreamYAMLToModel_DefaultsToNone verifies the DP->CP import default: -// a gateway-pushed provider whose upstream.auth block is absent (or empty-typed) -// is stored with auth type "none", while an explicit "other" type is preserved. -func TestMapLLMUpstreamYAMLToModel_DefaultsToNone(t *testing.T) { - // No auth block => "none". +// TestMapLLMUpstreamYAMLToModel_DefaultsToOther verifies the DP->CP import default: +// a gateway-pushed provider whose upstream.auth block is absent (or empty-typed) is +// stored with auth type "other" — the gateway may authenticate the upstream via +// user-attached policies, so absence must not be recorded as "no auth". An explicit +// type is preserved. +func TestMapLLMUpstreamYAMLToModel_DefaultsToOther(t *testing.T) { + // No auth block => "other". got := mapLLMUpstreamYAMLToModel(dto.LLMUpstreamYAML{URL: "https://api.openai.com/v1"}) - if got == nil || got.Main == nil || got.Main.Auth == nil || got.Main.Auth.Type != "none" { - t.Fatalf("expected auth type 'none' for absent auth, got %+v", got) + if got == nil || got.Main == nil || got.Main.Auth == nil || got.Main.Auth.Type != "other" { + t.Fatalf("expected auth type 'other' for absent auth, got %+v", got) } - // Explicit "other" is preserved. - otherType := api.Other + // Empty-typed auth block => "other". + emptyType := api.UpstreamAuthType("") got = mapLLMUpstreamYAMLToModel(dto.LLMUpstreamYAML{ URL: "https://api.openai.com/v1", - Auth: &api.UpstreamAuth{Type: &otherType}, + Auth: &api.UpstreamAuth{Type: &emptyType}, }) if got == nil || got.Main == nil || got.Main.Auth == nil || got.Main.Auth.Type != "other" { - t.Fatalf("expected auth type 'other' preserved, got %+v", got) + t.Fatalf("expected auth type 'other' for empty-typed auth, got %+v", got) + } + + // An explicit "none" is preserved — it is not overwritten by the default. + noneType := api.None + got = mapLLMUpstreamYAMLToModel(dto.LLMUpstreamYAML{ + URL: "https://api.openai.com/v1", + Auth: &api.UpstreamAuth{Type: &noneType}, + }) + if got == nil || got.Main == nil || got.Main.Auth == nil || got.Main.Auth.Type != "none" { + t.Fatalf("expected auth type 'none' preserved, got %+v", got) } } diff --git a/platform-api/internal/service/mcp.go b/platform-api/internal/service/mcp.go index c4d708cbb5..dd498fb6a2 100644 --- a/platform-api/internal/service/mcp.go +++ b/platform-api/internal/service/mcp.go @@ -549,8 +549,16 @@ func (s *MCPProxyService) Delete(orgUUID, handle, deletedBy string) error { } // FetchServerInfo fetches server information from an MCP backend. -// When proxyId is provided, the URL and auth are fetched from the stored proxy configuration. -// When proxyId is not provided, url is required and auth is optional. +// +// The target and credentials are selected by which fields the caller supplies: +// - url alone, or url + auth: contact that URL with the caller's own credentials +// (the creation flow, where nothing is stored yet). +// - proxyId alone: contact the proxy's stored URL with its stored credentials. +// - proxyId + url: contact the caller's URL with the proxy's stored credentials, for +// validating an unsaved endpoint edit without re-entering a write-only secret. +// +// auth may never accompany proxyId: in any stored-credential flow the stored auth is +// authoritative, so an override is rejected rather than silently ignored. func (s *MCPProxyService) FetchServerInfo(orgUUID string, req *api.MCPServerInfoFetchRequest) (*api.MCPServerInfoFetchResponse, error) { if req == nil { return nil, apperror.ValidationFailed.New("A request body is required.") @@ -562,22 +570,15 @@ func (s *MCPProxyService) FetchServerInfo(orgUUID string, req *api.MCPServerInfo hasProxyID := req.ProxyId != nil && *req.ProxyId != "" hasURL := req.Url != nil && *req.Url != "" - // Exactly one of url / proxyId selects the target, matching the oneOf constraint on - // MCPServerInfoFetchRequest. Accepting both would leave which one wins implicit. - if hasProxyID && hasURL { - return nil, apperror.ValidationFailed.New("Provide either url or proxyId, not both.") - } if !hasProxyID && !hasURL { return nil, apperror.ValidationFailed.New("Either url or proxyId is required.") } - // Auth is only meaningful for the direct-url flow; in refetch mode the stored auth is - // authoritative, so a caller-supplied override is rejected rather than silently ignored. if hasProxyID && req.Auth != nil { return nil, apperror.ValidationFailed.New("The auth field is not allowed when proxyId is provided.") } if hasProxyID { - // ProxyId provided - fetch stored configuration (refetch flow) + // ProxyId provided - resolve stored configuration (refetch flow) // Auth override is NOT allowed in refetch - use exactly what's stored proxy, err := s.repo.GetByHandle(*req.ProxyId, orgUUID) if err != nil { @@ -595,6 +596,13 @@ func (s *MCPProxyService) FetchServerInfo(orgUUID string, req *api.MCPServerInfo url = proxy.Configuration.Upstream.Main.URL } + // A caller-supplied URL overrides the stored one, and is used verbatim — this is + // what lets the UI validate an unsaved endpoint edit without re-entering a + // write-only credential. + if hasURL { + url = *req.Url + } + // Use stored auth from proxy configuration. The stored value is a // {{ secret "handle" }} placeholder, not the plaintext credential — resolve it // through the secret store before using it as the actual upstream header value. diff --git a/platform-api/internal/service/mcp_test.go b/platform-api/internal/service/mcp_test.go index bbdf6448bc..5a1d4e722f 100644 --- a/platform-api/internal/service/mcp_test.go +++ b/platform-api/internal/service/mcp_test.go @@ -162,11 +162,10 @@ func TestFetchServerInfoRefetchUsesStoredURLVerbatim(t *testing.T) { } // TestFetchServerInfoRejectsAmbiguousTargets asserts the request-shape rules enforced by -// FetchServerInfo, matching the oneOf constraint on MCPServerInfoFetchRequest: exactly one -// of url/proxyId selects the target, and auth may not accompany proxyId (where the stored -// auth is authoritative) — no branch may silently ignore a supplied field. +// FetchServerInfo: at least one of url/proxyId must select the target, and auth may not +// accompany proxyId (where the stored auth is authoritative) — no branch may silently +// ignore a supplied field. func TestFetchServerInfoRejectsAmbiguousTargets(t *testing.T) { - url := "https://mcp.example.com/mcp" proxyID := "mcp-proxy-1" header, value := "X-API-Key", "secret" @@ -174,7 +173,6 @@ func TestFetchServerInfoRejectsAmbiguousTargets(t *testing.T) { name string req *api.MCPServerInfoFetchRequest }{ - {"both url and proxyId", &api.MCPServerInfoFetchRequest{Url: &url, ProxyId: &proxyID}}, {"neither url nor proxyId", &api.MCPServerInfoFetchRequest{}}, {"auth supplied with proxyId", &api.MCPServerInfoFetchRequest{ ProxyId: &proxyID, @@ -192,3 +190,64 @@ func TestFetchServerInfoRejectsAmbiguousTargets(t *testing.T) { }) } } + +// TestFetchServerInfoSuppliedURLWithStoredCredential covers the url+proxyId shape, which +// lets the UI validate an unsaved endpoint edit using the proxy's stored (write-only) +// credential. The supplied URL must override the stored one verbatim — no path +// manipulation, same as the proxyId-only flow — while the stored auth header still rides +// along, since that is the whole point of referencing the proxy. +func TestFetchServerInfoSuppliedURLWithStoredCredential(t *testing.T) { + var mu sync.Mutex + var paths []string + var authHeaders []string + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + paths = append(paths, r.URL.Path) + // Only the MCP calls themselves carry credentials; CheckURLReachability's + // preceding HEAD probe is a deliberately unauthenticated diagnostic. + if r.Method != http.MethodHead { + authHeaders = append(authHeaders, r.Header.Get("X-API-Key")) + } + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + w.Header().Set("mcp-session-id", "test-session") + _, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":1,"result":{"protocolVersion":"2025-06-18","serverInfo":{"name":"test","version":"1.0"}}}`)) + })) + defer srv.Close() + + proxy := &model.MCPProxy{ + Handle: "mcp-proxy-1", + Configuration: model.MCPProxyConfiguration{ + Upstream: model.UpstreamConfig{ + Main: &model.UpstreamEndpoint{ + URL: srv.URL + "/mcp", + // A literal (non-placeholder) stored value, so this exercises the + // request shape without needing the secret store. + Auth: &model.UpstreamAuth{Header: "X-API-Key", Value: "stored-secret"}, + }, + }, + }, + } + repo := &mockMCPProxyRepository{getByHandleResult: proxy} + service := NewMCPProxyService(repo, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + + proxyID := "mcp-proxy-1" + editedURL := srv.URL + "/api/v2/mcp" // an unsaved endpoint edit + _, err := service.FetchServerInfo("org-1", &api.MCPServerInfoFetchRequest{ + ProxyId: &proxyID, + Url: &editedURL, + }) + require.NoError(t, err) + + mu.Lock() + defer mu.Unlock() + require.NotEmpty(t, paths, "backend should have received at least one request") + for _, p := range paths { + assert.Equal(t, "/api/v2/mcp", p, "the supplied URL must override the stored one, verbatim") + } + require.NotEmpty(t, authHeaders, "backend should have received at least one MCP call") + for _, h := range authHeaders { + assert.Equal(t, "stored-secret", h, "the proxy's stored credential must still be sent") + } +} diff --git a/platform-api/resources/openapi.yaml b/platform-api/resources/openapi.yaml index 677f199f1b..46f76544e1 100644 --- a/platform-api/resources/openapi.yaml +++ b/platform-api/resources/openapi.yaml @@ -8396,37 +8396,37 @@ components: MCPServerInfoFetchRequest: type: object - oneOf: + anyOf: - required: [ "url" ] - required: [ "proxyId" ] - not: - required: [ "auth" ] + not: + required: [ "proxyId", "auth" ] description: | - Target MCP server to introspect. Provide exactly one of `url` (a direct backend URL, - optionally with `auth`) or `proxyId` (refetch using a stored proxy configuration) — never - both. `auth` must not be sent together with `proxyId`; the stored auth is always used in - the refetch flow. + Target MCP server to introspect, and the credentials to introspect it with. At least + one of `url`/`proxyId` must be provided properties: url: type: string format: uri description: | - Endpoint URL of the MCP server to fetch information from. Mutually exclusive - with `proxyId`; exactly one of the two must be provided. + Endpoint URL of the MCP server to fetch information from. Required unless + `proxyId` is given. When sent together with `proxyId` it overrides that proxy's + stored upstream URL, while the proxy's stored credentials are still used — this + validates an unsaved endpoint edit without re-sending a write-only secret. example: https://mcp.server.com/mcp proxyId: type: string description: | - MCP proxy handle (identifier) for refresh operations. When provided, the server - fetches URL and auth from the stored proxy configuration. Mutually exclusive - with `url`; exactly one of the two must be provided. + MCP proxy handle (identifier) for refresh operations. The stored credentials of + this proxy are used for the fetch, and its stored upstream URL too unless `url` + overrides it. Required unless `url` is given. example: my-mcp-proxy auth: $ref: '#/components/schemas/UpstreamAuth' description: | - Authentication configuration for the fetch request. Allowed only alongside - `url` (initial creation flow); sending it with `proxyId` is rejected, as the - stored auth is used in the refetch flow. + Authentication configuration for the fetch request. Allowed only when `proxyId` + is absent (initial creation flow); sending it with `proxyId` is rejected, as the + stored auth is used whenever a proxy is referenced. MCPServerInfoFetchResponse: type: object diff --git a/portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js b/portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js index b12d7d9ed3..ecbfd94277 100644 --- a/portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js +++ b/portals/ai-workspace/cypress/e2e/002-mcp-proxies/005-mcp-backend-connection-refetch.cy.js @@ -19,19 +19,19 @@ /** * "Refetch Server Info" behaviour on the MCP Proxy — Backend Connection tab. * - * ExternalServersOverview's handleRefetch has two mutually-exclusive request - * shapes, chosen by whether the endpoint URL / auth header / auth value differ - * from what is currently persisted on the server: + * ExternalServersOverview's handleRefetch picks one of three request shapes, + * according to which credential the fetch has to run with: * * - Unedited: POST /mcp-proxies/fetch-server-info { proxyId } — the * backend resolves the stored URL and auth (via the saved secret handle) * itself, so the browser never needs to re-send a credential it can't - * read back (auth.value is writeOnly). url and proxyId are mutually - * exclusive by API contract, so this omits url. - * - Edited: POST /mcp-proxies/fetch-server-info { url, auth: { type, - * header, value } } — validates the live, not-yet-saved values directly. - * proxyId and an auth override are mutually exclusive by API contract, so - * this omits proxyId. + * read back (auth.value is writeOnly). + * - Endpoint edited, credential untouched: { url, proxyId } — the backend + * fetches the unsaved URL using the stored secret, so an endpoint change + * can be verified without re-entering a credential the browser can't read. + * - Credential typed: { url, auth: { type, header, value } } — validates + * the live, not-yet-saved values directly. auth and proxyId are mutually + * exclusive by API contract, so this omits proxyId. * * Covers: * TC-98 Refetch with nothing edited → request uses proxyId, omits auth @@ -342,4 +342,37 @@ describe('AI Workspace — MCP proxy Backend Connection tab (Refetch Server Info }); }); }); + + // --------------------------------------------------------------------------- + // TC-102 + // --------------------------------------------------------------------------- + it('TC-102: refetching after a URL-only edit sends url + proxyId, reusing the stored credential', () => { + // An unsaved endpoint edit. The credential field is never focused, so it stays + // masked and the browser has no value to send; proxyId is what lets the backend + // supply it. + const editedPathUrl = 'https://sample.mcp.example.com/v2/mcp'; + + cy.get('[data-testid="backend-connection-endpoint-url"]') + .clear() + .type(editedPathUrl); + cy.get('[data-testid="backend-connection-auth-value"]') + .should('have.value', '******'); + + stubFetchServerInfo('refetchEditedUrl'); + + cy.get('[data-testid="backend-connection-refetch"]', { timeout: 15000 }) + .should('not.be.disabled') + .click(); + + cy.wait('@refetchEditedUrl').then((pi) => { + const body = pi.request.body; + expect(body.url, 'the unsaved, edited url is what gets fetched').to.equal(editedPathUrl); + expect(body.proxyId, 'proxyId supplies the stored credential').to.equal(createdServerId); + // The masked sentinel must never be sent as if it were a credential, and no + // plaintext is available client-side to send either. + expect(body.auth, 'auth is never combined with proxyId').to.be.undefined; + }); + + cy.contains('Connection verified', { timeout: 15000 }).should('be.visible'); + }); }); diff --git a/portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx b/portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx index d0b9d4fe90..4b3be11b7c 100644 --- a/portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx +++ b/portals/ai-workspace/src/pages/appShell/appShellPages/externalServers/ExternalServersOverview.tsx @@ -682,25 +682,22 @@ export default function ExternalServersOverview(): JSX.Element { } const trimmedHeaderName = authHeaderName.trim(); + const storedHeaderName = (server.upstream?.main?.auth?.header ?? '').trim(); const endpointUnchanged = trimmedUrl === (server.upstream?.main?.url ?? '').trim(); - const headerNameUnchanged = - trimmedHeaderName === (server.upstream?.main?.auth?.header ?? '').trim(); - const credentialUnchanged = isCredentialMasked || !hasCredentialChanged; + const headerNameUnchanged = trimmedHeaderName === storedHeaderName; setIsRefetching(true); try { + // Which credential the fetch runs with is what picks the request shape. A live + // value the user just typed goes on the wire directly; otherwise the credential + // is write-only (masked as ******) and only the backend can resolve it, which it + // does from proxyId. auth never accompanies proxyId — whenever a proxy is + // referenced, its stored auth is authoritative and an override is rejected. let request: MCPServerInfoFetchRequest; - if (endpointUnchanged && headerNameUnchanged && credentialUnchanged) { - // Nothing edited yet — let the backend resolve the stored URL and auth (via the - // saved secret handle) from the persisted proxy config instead of us re-entering - // a credential we can never read back. url and proxyId are mutually exclusive - // (the backend rejects both), and the unchanged url is exactly what it would - // resolve anyway, so send proxyId alone. - request = { proxyId: server.id }; - } else if (trimmedHeaderName && !isCredentialMasked && authHeaderValue.trim()) { - // Something was edited and we have a live credential value to validate with. - // proxyId + auth override together aren't allowed, so this omits proxyId. + if (trimmedHeaderName && !isCredentialMasked && authHeaderValue.trim()) { + // A new or rotated credential was typed — validate the endpoint against it + // directly, before anything is saved. request = { url: trimmedUrl, auth: { @@ -709,14 +706,28 @@ export default function ExternalServersOverview(): JSX.Element { value: authHeaderValue.trim(), }, }; - } else if (trimmedHeaderName && isCredentialMasked) { + } else if (!trimmedHeaderName) { + // No auth header configured, or the user is clearing it — validate the way the + // saved proxy would actually behave, unauthenticated. + request = { url: trimmedUrl }; + } else if (!headerNameUnchanged) { + // The header was renamed but the value is still masked. proxyId would resolve + // the stored header name too, so there is no shape that pairs the new name with + // the stored value — the user has to supply one or the other. showSnackbar( - 'Enter the authentication header value to refetch with the updated endpoint or header.', + 'Enter the authentication header value, or save your changes first, to refetch with the renamed header.', 'error' ); return; + } else if (endpointUnchanged) { + // Nothing to override — the backend resolves both the stored URL and the stored + // credential from the persisted config. + request = { proxyId: server.id }; } else { - request = { url: trimmedUrl }; + // The endpoint was edited but the credential was not. Send both: the backend + // fetches the unsaved URL using the stored secret, so the user can verify an + // endpoint change without re-entering a credential they can never read back. + request = { url: trimmedUrl, proxyId: server.id }; } const response = await mcpServerValidationApis.fetchMCPProxyServerInfo( diff --git a/portals/ai-workspace/src/utils/types.ts b/portals/ai-workspace/src/utils/types.ts index 9fd9a368ee..d359d5c606 100644 --- a/portals/ai-workspace/src/utils/types.ts +++ b/portals/ai-workspace/src/utils/types.ts @@ -838,9 +838,15 @@ export type MCPServerListResponse = ApiListResponse; /** * MCP Server Info Fetch Request * - * `url` and `proxyId` are mutually exclusive (the API's oneOf), and `auth` is only - * allowed alongside `url` — with `proxyId` the stored auth is authoritative and an - * override is rejected. Modelled as a union so the invalid combinations don't compile. + * At least one of `url`/`proxyId` is required, and `auth` may never accompany `proxyId` + * — whenever a proxy is referenced its stored auth is authoritative and an override is + * rejected. Modelled as a union so that invalid combination doesn't compile. The three + * valid shapes are: + * + * { url, auth? } — contact this URL with these (or no) credentials + * { proxyId } — contact the stored URL with the stored credentials + * { url, proxyId } — contact this URL with the stored credentials, so an unsaved + * endpoint edit can be validated without re-sending the secret */ interface MCPServerInfoFetchRequestBase { transportType?: string; @@ -858,7 +864,7 @@ export type MCPServerInfoFetchRequest = MCPServerInfoFetchRequestBase & value: string; }; } - | { proxyId: string; url?: never; auth?: never } + | { proxyId: string; url?: string; auth?: never } ); /** From 16f62135ee2d2d26a04ab95644855a7354b8432e Mon Sep 17 00:00:00 2001 From: Thushani Jayasekera Date: Thu, 30 Jul 2026 22:51:15 +0530 Subject: [PATCH 3/3] Refactor upstream authentication handling in LLM provider to default to 'none' for gateway-pushed providers. Update related tests and components to ensure correct behavior and preserve explicit auth types. --- .../service/artifact_import_llm_provider.go | 2 +- platform-api/internal/service/llm.go | 26 +++----------- platform-api/internal/service/llm_test.go | 34 ++++++------------- .../serviceProvider/ServiceProviderNew.tsx | 5 ++- 4 files changed, 18 insertions(+), 49 deletions(-) diff --git a/platform-api/internal/service/artifact_import_llm_provider.go b/platform-api/internal/service/artifact_import_llm_provider.go index 6927802f0d..bb1c83b498 100644 --- a/platform-api/internal/service/artifact_import_llm_provider.go +++ b/platform-api/internal/service/artifact_import_llm_provider.go @@ -175,6 +175,6 @@ func mapLLMProviderSpecToConfig(spec dto.LLMProviderDeploymentSpec) model.LLMPro // single endpoint to the main endpoint. Returns nil when no upstream is present. func mapLLMUpstreamYAMLToModel(in dto.LLMUpstreamYAML) *model.UpstreamConfig { endpoint := &model.UpstreamEndpoint{URL: in.URL, Ref: in.Ref} - endpoint.Auth = defaultUpstreamAuthToOther(mapUpstreamAuthAPIToModel(in.Auth)) + endpoint.Auth = defaultUpstreamAuthToNone(mapUpstreamAuthAPIToModel(in.Auth)) return &model.UpstreamConfig{Main: endpoint} } diff --git a/platform-api/internal/service/llm.go b/platform-api/internal/service/llm.go index d3b3fef365..eece1699a3 100644 --- a/platform-api/internal/service/llm.go +++ b/platform-api/internal/service/llm.go @@ -3456,32 +3456,14 @@ func resolveTemplateOpenAPISpec(ctx context.Context, tpl *model.LLMProviderTempl return "" } -// defaultUpstreamAuthToNone applies the platform's default upstream auth type for -// caller-authored requests (create/update), where an omitted auth block means the -// upstream genuinely takes no credentials. -func defaultUpstreamAuthToNone(auth *model.UpstreamAuth) *model.UpstreamAuth { - return defaultUpstreamAuthType(auth, string(api.None)) -} - -// defaultUpstreamAuthToOther applies the default upstream auth type for LLM providers -// imported from a gateway. An absent auth block there doesn't imply the upstream is -// unauthenticated — the gateway may authenticate it via user-attached policies the -// control plane doesn't model — so "other" is used rather than "none". This applies to -// the provider import path only; every other path defaults to "none" (or leaves auth -// absent) as before. The two are equivalent to the gateway at runtime — it attaches no -// auth policy for either — so this affects what the control plane reports, not routing. -func defaultUpstreamAuthToOther(auth *model.UpstreamAuth) *model.UpstreamAuth { - return defaultUpstreamAuthType(auth, string(api.Other)) -} - -// defaultUpstreamAuthType fills in an absent/blank auth type with defaultType and +// defaultUpstreamAuthToNone applies the platform's default upstream auth type and // strips credentials from credential-less types. -func defaultUpstreamAuthType(auth *model.UpstreamAuth, defaultType string) *model.UpstreamAuth { +func defaultUpstreamAuthToNone(auth *model.UpstreamAuth) *model.UpstreamAuth { if auth == nil { - return &model.UpstreamAuth{Type: defaultType} + return &model.UpstreamAuth{Type: string(api.None)} } if strings.TrimSpace(auth.Type) == "" { - auth.Type = defaultType + auth.Type = string(api.None) } if isCredentialLessUpstreamAuthType(auth.Type) { auth.Header = "" diff --git a/platform-api/internal/service/llm_test.go b/platform-api/internal/service/llm_test.go index 7840b38857..e28bc25b0a 100644 --- a/platform-api/internal/service/llm_test.go +++ b/platform-api/internal/service/llm_test.go @@ -92,36 +92,24 @@ func TestNormalizeUpstreamAuthType(t *testing.T) { } } -// TestMapLLMUpstreamYAMLToModel_DefaultsToOther verifies the DP->CP import default: -// a gateway-pushed provider whose upstream.auth block is absent (or empty-typed) is -// stored with auth type "other" — the gateway may authenticate the upstream via -// user-attached policies, so absence must not be recorded as "no auth". An explicit -// type is preserved. -func TestMapLLMUpstreamYAMLToModel_DefaultsToOther(t *testing.T) { - // No auth block => "other". +// TestMapLLMUpstreamYAMLToModel_DefaultsToNone verifies the DP->CP import default: +// a gateway-pushed provider whose upstream.auth block is absent (or empty-typed) +// is stored with auth type "none", while an explicit "other" type is preserved. +func TestMapLLMUpstreamYAMLToModel_DefaultsToNone(t *testing.T) { + // No auth block => "none". got := mapLLMUpstreamYAMLToModel(dto.LLMUpstreamYAML{URL: "https://api.openai.com/v1"}) - if got == nil || got.Main == nil || got.Main.Auth == nil || got.Main.Auth.Type != "other" { - t.Fatalf("expected auth type 'other' for absent auth, got %+v", got) + if got == nil || got.Main == nil || got.Main.Auth == nil || got.Main.Auth.Type != "none" { + t.Fatalf("expected auth type 'none' for absent auth, got %+v", got) } - // Empty-typed auth block => "other". - emptyType := api.UpstreamAuthType("") + // Explicit "other" is preserved. + otherType := api.Other got = mapLLMUpstreamYAMLToModel(dto.LLMUpstreamYAML{ URL: "https://api.openai.com/v1", - Auth: &api.UpstreamAuth{Type: &emptyType}, + Auth: &api.UpstreamAuth{Type: &otherType}, }) if got == nil || got.Main == nil || got.Main.Auth == nil || got.Main.Auth.Type != "other" { - t.Fatalf("expected auth type 'other' for empty-typed auth, got %+v", got) - } - - // An explicit "none" is preserved — it is not overwritten by the default. - noneType := api.None - got = mapLLMUpstreamYAMLToModel(dto.LLMUpstreamYAML{ - URL: "https://api.openai.com/v1", - Auth: &api.UpstreamAuth{Type: &noneType}, - }) - if got == nil || got.Main == nil || got.Main.Auth == nil || got.Main.Auth.Type != "none" { - t.Fatalf("expected auth type 'none' preserved, got %+v", got) + t.Fatalf("expected auth type 'other' preserved, got %+v", got) } } diff --git a/portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsx b/portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsx index 28ef2fa2f0..a0c29920ba 100644 --- a/portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsx +++ b/portals/ai-workspace/src/pages/appShell/appShellPages/serviceProvider/ServiceProviderNew.tsx @@ -345,8 +345,7 @@ export default function ServiceProviderNew() { const providerId = toProviderId(formState.name); // The API key is optional. A credential-bearing auth type with no key - // entered stores nothing to inject, so record it as 'other' — upstream - // authentication is then left to user-attached policies. Sending + // entered stores nothing to inject, so record it as 'none'. Sending // 'api-key' with an empty value creates a provider the gateway rejects // at deployment time. const hasCredential = Boolean(formState.upstreamAuthValue.trim()); @@ -358,7 +357,7 @@ export default function ServiceProviderNew() { ? { type: isNoCredentialsAuthType ? formState.upstreamAuthType - : 'other', + : 'none', header: '', value: '', }