diff --git a/gateway/gateway-builder/internal/policyengine/gomod.go b/gateway/gateway-builder/internal/policyengine/gomod.go index ac20534ee8..895eee8305 100644 --- a/gateway/gateway-builder/internal/policyengine/gomod.go +++ b/gateway/gateway-builder/internal/policyengine/gomod.go @@ -34,6 +34,27 @@ import ( "golang.org/x/mod/modfile" ) +// goGetMaxAttempts and goGetRetryBackoff control retries around the 'go get' +// invocation below. Module proxy fetches occasionally fail with transient +// network errors (e.g. an HTTP/2 stream INTERNAL_ERROR from proxy.golang.org) +// that succeed on a bare retry; a timeout or a genuinely missing/invalid +// module will simply fail again on retry at negligible extra cost. +var ( + goGetMaxAttempts = 3 + goGetRetryBackoff = 2 * time.Second +) + +// runGoGet is a seam over exec.CommandContext so tests can stub out the real +// 'go get' invocation. +var runGoGet = func(ctx context.Context, dir, target string) (stderr []byte, err error) { + cmd := exec.CommandContext(ctx, "go", "get", target) + cmd.Dir = dir + var buf bytes.Buffer + cmd.Stderr = &buf + err = cmd.Run() + return buf.Bytes(), err +} + // UpdateGoMod updates go.mod for discovered policies: // - gomodule entries: runs 'go get' to pin the real module at its resolved version // - filePath entries: adds a replace directive pointing to the local path @@ -54,23 +75,42 @@ func UpdateGoMod(srcDir string, policies []*types.DiscoveredPolicy) error { } target := fmt.Sprintf("%s@%s", policy.GoModulePath, policy.GoModuleVersion) - slog.Debug("running go get for remote policy", - "policy", policy.Name, - "target", target) - - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) - cmd := exec.CommandContext(ctx, "go", "get", target) - cmd.Dir = srcDir - var stderr bytes.Buffer - cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { + + var lastErr error + var lastStderr []byte + for attempt := 1; attempt <= goGetMaxAttempts; attempt++ { + slog.Debug("running go get for remote policy", + "policy", policy.Name, + "target", target, + "attempt", attempt) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + stderr, runErr := runGoGet(ctx, srcDir, target) + timedOut := errors.Is(ctx.Err(), context.DeadlineExceeded) cancel() - if errors.Is(ctx.Err(), context.DeadlineExceeded) { - return fmt.Errorf("go get timed out after 5min for %s in %s: %w; stderr: %s", target, srcDir, err, stderr.String()) + + if runErr == nil { + lastErr = nil + break + } + if timedOut { + return fmt.Errorf("go get timed out after 5min for %s in %s: %w; stderr: %s", target, srcDir, runErr, stderr) } - return fmt.Errorf("go get failed for %s in %s: %w; stderr: %s", target, srcDir, err, stderr.String()) + + lastErr, lastStderr = runErr, stderr + if attempt < goGetMaxAttempts { + slog.Warn("go get failed, retrying", + "policy", policy.Name, + "target", target, + "attempt", attempt, + "maxAttempts", goGetMaxAttempts, + "err", runErr) + time.Sleep(goGetRetryBackoff * time.Duration(attempt)) + } + } + if lastErr != nil { + return fmt.Errorf("go get failed for %s in %s after %d attempts: %w; stderr: %s", target, srcDir, goGetMaxAttempts, lastErr, lastStderr) } - cancel() } // Second pass: add replace directives for filePath (local) entries diff --git a/gateway/gateway-builder/internal/policyengine/policyengine_test.go b/gateway/gateway-builder/internal/policyengine/policyengine_test.go index 7e10974c8a..0e2fcff217 100644 --- a/gateway/gateway-builder/internal/policyengine/policyengine_test.go +++ b/gateway/gateway-builder/internal/policyengine/policyengine_test.go @@ -19,10 +19,13 @@ package policyengine import ( + "context" + "errors" "os" "path/filepath" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -551,6 +554,10 @@ func TestUpdateGoMod_RemotePolicy_InvalidModule(t *testing.T) { tmpDir := t.TempDir() testutils.WritePolicyEngineGoMod(t, tmpDir) + // A nonexistent module fails deterministically, not transiently — pin to a + // single attempt so this test isn't slowed down by retry/backoff. + withSingleGoGetAttempt(t) + // Create a remote policy (IsFilePathEntry: false) with invalid module policies := []*types.DiscoveredPolicy{ testutils.NewRemoteDiscoveredPolicy("ratelimit", "v1.0.0", "github.com/nonexistent-org-12345/nonexistent-module", "v1.0.0"), @@ -625,6 +632,10 @@ func TestUpdateGoMod_OnlyRemotePolicies_AllFail(t *testing.T) { tmpDir := t.TempDir() testutils.WritePolicyEngineGoMod(t, tmpDir) + // A nonexistent module fails deterministically, not transiently — pin to a + // single attempt so this test isn't slowed down by retry/backoff. + withSingleGoGetAttempt(t) + // Only remote policies, all with invalid modules policies := []*types.DiscoveredPolicy{ testutils.NewRemoteDiscoveredPolicy("remote-policy", "v1.0.0", "github.com/definitely-not-real-org/fake-module", "v1.0.0"), @@ -634,3 +645,74 @@ func TestUpdateGoMod_OnlyRemotePolicies_AllFail(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "go get failed") } + +// withSingleGoGetAttempt temporarily pins goGetMaxAttempts to 1, restoring the +// original value on test cleanup. Use for tests exercising a deterministic +// (non-transient) 'go get' failure, so they aren't slowed by retry/backoff. +func withSingleGoGetAttempt(t *testing.T) { + t.Helper() + original := goGetMaxAttempts + goGetMaxAttempts = 1 + t.Cleanup(func() { goGetMaxAttempts = original }) +} + +// withStubbedGoGet replaces runGoGet with a stub for the duration of the +// test, restoring the original implementation on cleanup. The stub receives +// the 1-indexed attempt number so tests can vary behavior across retries. It +// also shrinks the retry backoff so retry tests run fast, and returns a +// pointer to the running call count. +func withStubbedGoGet(t *testing.T, stub func(attempt int, ctx context.Context, dir, target string) ([]byte, error)) *int { + t.Helper() + callCount := 0 + originalRunGoGet := runGoGet + originalBackoff := goGetRetryBackoff + runGoGet = func(ctx context.Context, dir, target string) ([]byte, error) { + callCount++ + return stub(callCount, ctx, dir, target) + } + goGetRetryBackoff = time.Millisecond + t.Cleanup(func() { + runGoGet = originalRunGoGet + goGetRetryBackoff = originalBackoff + }) + return &callCount +} + +func TestUpdateGoMod_RemotePolicy_RetriesTransientFailureThenSucceeds(t *testing.T) { + tmpDir := t.TempDir() + testutils.WritePolicyEngineGoMod(t, tmpDir) + + callCount := withStubbedGoGet(t, func(attempt int, ctx context.Context, dir, target string) ([]byte, error) { + if attempt < 3 { + return []byte("stream error: stream ID 1; INTERNAL_ERROR"), errors.New("exit status 1") + } + return nil, nil + }) + + policies := []*types.DiscoveredPolicy{ + testutils.NewRemoteDiscoveredPolicy("semantic-cache", "v1.0.1", "github.com/wso2/gateway-controllers/policies/semantic-cache", "v1.0.1"), + } + + err := UpdateGoMod(tmpDir, policies) + require.NoError(t, err) + assert.Equal(t, 3, *callCount, "expected 2 failed attempts followed by 1 successful attempt") +} + +func TestUpdateGoMod_RemotePolicy_ExhaustsRetriesOnPersistentFailure(t *testing.T) { + tmpDir := t.TempDir() + testutils.WritePolicyEngineGoMod(t, tmpDir) + + callCount := withStubbedGoGet(t, func(attempt int, ctx context.Context, dir, target string) ([]byte, error) { + return []byte("module not found"), errors.New("exit status 1") + }) + + policies := []*types.DiscoveredPolicy{ + testutils.NewRemoteDiscoveredPolicy("ratelimit", "v1.0.0", "github.com/example/policies/ratelimit", "v1.0.0"), + } + + err := UpdateGoMod(tmpDir, policies) + require.Error(t, err) + assert.Contains(t, err.Error(), "go get failed") + assert.Contains(t, err.Error(), "after 3 attempts") + assert.Equal(t, goGetMaxAttempts, *callCount) +} diff --git a/platform-api/internal/handler/api.go b/platform-api/internal/handler/api.go index e2f8c5658e..692b438e2f 100644 --- a/platform-api/internal/handler/api.go +++ b/platform-api/internal/handler/api.go @@ -133,6 +133,9 @@ func (h *APIHandler) CreateAPI(w http.ResponseWriter, r *http.Request) error { return apperror.ValidationFailed.Wrap(err, "Subscription plan not found or not active"). WithLogMessage(fmt.Sprintf("subscription plan not found or not active in org %s", orgId)) } + if errors.Is(err, constants.ErrSecretRefMissing) { + return apperror.ValidationFailed.Wrap(err, "One or more referenced secrets do not exist") + } return apperror.Internal.Wrap(err). WithLogMessage(fmt.Sprintf("failed to create API in org %s", orgId)) } @@ -268,6 +271,9 @@ func (h *APIHandler) UpdateAPI(w http.ResponseWriter, r *http.Request) error { return apperror.ValidationFailed.Wrap(err, "Subscription plan not found or not active"). WithLogMessage(fmt.Sprintf("subscription plan not found or not active for API %s in org %s", apiId, orgId)) } + if errors.Is(err, constants.ErrSecretRefMissing) { + return apperror.ValidationFailed.Wrap(err, "One or more referenced secrets do not exist") + } return apperror.Internal.Wrap(err). WithLogMessage(fmt.Sprintf("failed to update API %s in org %s", apiId, orgId)) } diff --git a/platform-api/internal/handler/llm.go b/platform-api/internal/handler/llm.go index 071c78f051..8d8e2c7a3d 100644 --- a/platform-api/internal/handler/llm.go +++ b/platform-api/internal/handler/llm.go @@ -590,6 +590,8 @@ func (h *LLMHandler) CreateLLMProxy(w http.ResponseWriter, r *http.Request) erro return apperror.ProjectNotFound.Wrap(err) case errors.Is(err, constants.ErrInvalidPolicyVersion): return apperror.ValidationFailed.Wrap(err, "Invalid policy version format") + case errors.Is(err, constants.ErrSecretRefMissing): + return apperror.ValidationFailed.Wrap(err, "One or more referenced secrets do not exist") case errors.Is(err, constants.ErrInvalidInput): return apperror.ValidationFailed.Wrap(err, "Invalid input") default: @@ -712,6 +714,8 @@ func (h *LLMHandler) UpdateLLMProxy(w http.ResponseWriter, r *http.Request) erro return apperror.ValidationFailed.Wrap(err, "The id is immutable and cannot be changed") case errors.Is(err, constants.ErrInvalidPolicyVersion): return apperror.ValidationFailed.Wrap(err, "Invalid policy version format") + case errors.Is(err, constants.ErrSecretRefMissing): + return apperror.ValidationFailed.Wrap(err, "One or more referenced secrets do not exist") case errors.Is(err, constants.ErrInvalidInput): return apperror.ValidationFailed.Wrap(err, "Invalid input") default: diff --git a/platform-api/internal/server/server.go b/platform-api/internal/server/server.go index 3b1b53e2b8..148aaf5262 100644 --- a/platform-api/internal/server/server.go +++ b/platform-api/internal/server/server.go @@ -358,7 +358,9 @@ func StartPlatformAPIServer(cfg *config.Server, slogger *slog.Logger) (*Server, mcpProxyDeploymentHandler := handler.NewMCPProxyDeploymentHandler(mcpDeploymentService, identityService, slogger) // Wire secret placeholder validation into dependent services llmProviderService.SetSecretService(secretService) + llmProxyService.SetSecretService(secretService) mcpProxyService.WithSecretService(secretService) + apiService.SetSecretService(secretService) secretHandler := handler.NewSecretHandler(secretService, identityService, slogger) // Start deployment timeout background job timeoutConfig := service.DeploymentTimeoutConfig{ diff --git a/platform-api/internal/service/api.go b/platform-api/internal/service/api.go index 6a1b43ca1c..637474d3f1 100644 --- a/platform-api/internal/service/api.go +++ b/platform-api/internal/service/api.go @@ -45,6 +45,7 @@ type APIService struct { subscriptionPlanRepo repository.SubscriptionPlanRepository customPolicyRepo repository.CustomPolicyRepository gatewayEventsService *GatewayEventsService + secretService *SecretService apiUtil *utils.APIUtil slogger *slog.Logger auditRepo repository.AuditRepository @@ -75,6 +76,13 @@ func NewAPIService(apiRepo repository.APIRepository, projectRepo repository.Proj } } +// SetSecretService injects the SecretService used to validate {{ secret "..." }} +// placeholders on Create/Update. Called after both services are constructed to +// avoid circular dependency. +func (s *APIService) SetSecretService(ss *SecretService) { + s.secretService = ss +} + // resolveRESTAPIIdentity replaces resp's createdBy/updatedBy UUIDs with the // raw external identity (or constants.DeletedUser), in place. func (s *APIService) resolveRESTAPIIdentity(resp *api.RESTAPI) error { @@ -94,6 +102,20 @@ func (s *APIService) CreateAPI(req *api.CreateRESTAPIRequest, orgUUID, createdBy return nil, err } + // Validate {{ secret "..." }} placeholders anywhere in the request — the + // gateway-controller's template engine resolves placeholders generically + // across the whole artifact (upstream auth and policies alike), so + // validation must cover the same surface. + if s.secretService != nil { + configJSON, err := marshalUpstreamForValidation(req) + if err != nil { + return nil, fmt.Errorf("failed to marshal request for secret validation: %w", err) + } + if err := s.secretService.ValidateSecretRefs(orgUUID, configJSON); err != nil { + return nil, err + } + } + projectHandle := strings.TrimSpace(req.ProjectId) // Check if project exists project, err := s.projectRepo.GetProjectByHandleAndOrgID(projectHandle, orgUUID) @@ -301,6 +323,18 @@ func (s *APIService) UpdateAPI(apiUUID string, req *api.RESTAPI, orgUUID, update return nil, constants.ErrAPINotFound } + // Validate {{ secret "..." }} placeholders anywhere in the request — see + // CreateAPI for why this covers the whole request, not just upstream. + if s.secretService != nil { + configJSON, err := marshalUpstreamForValidation(req) + if err != nil { + return nil, fmt.Errorf("failed to marshal request for secret validation: %w", err) + } + if err := s.secretService.ValidateSecretRefs(orgUUID, configJSON); err != nil { + return nil, err + } + } + // Apply updates using shared helper updatedAPI, err := s.applyAPIUpdates(existingAPIModel, req, orgUUID) if err != nil { @@ -331,6 +365,26 @@ func (s *APIService) UpdateAPI(apiUUID string, req *api.RESTAPI, orgUUID, update return nil, err } + // Best-effort: delete secrets rotated away from on Main/Sandbox auth. Must + // run after the update above persists the new reference, so the in-use + // check no longer sees this API pointing at the old handle. + if s.secretService != nil { + s.secretService.cleanupRotatedSecret( + orgUUID, + mainUpstreamAuthValue(&existingAPIModel.Configuration.Upstream), + mainUpstreamAuthValue(&updatedAPIModel.Configuration.Upstream), + updatedBy, + s.slogger, + ) + s.secretService.cleanupRotatedSecret( + orgUUID, + sandboxUpstreamAuthValue(&existingAPIModel.Configuration.Upstream), + sandboxUpstreamAuthValue(&updatedAPIModel.Configuration.Upstream), + updatedBy, + s.slogger, + ) + } + s.refreshCustomPolicyUsages(apiUUID, orgUUID, updatedAPIModel) _ = s.auditRepo.Record("UPDATE", apiUUID, "rest_api", orgUUID, updatedBy) @@ -735,12 +789,53 @@ func (s *APIService) applyAPIUpdates(existingAPIModel *model.API, req *api.RESTA existingAPI.Policies = req.Policies existingAPI.SubscriptionPlans = req.SubscriptionPlans if !s.isEmptyUpstream(req.Upstream) { - existingAPI.Upstream = req.Upstream + // isEmptyUpstream only looks at Url/Ref, so a request that updates the + // URL but omits auth (routine — auth.value is redacted on GET, so a + // naive read-modify-write round-trip never has it to resend) must not + // wipe the stored credential. Preserve auth from the existing model + // (unredacted) whenever the incoming value is empty. + existingAPI.Upstream = preserveUpstreamAuthOnAPIUpdate(existingAPIModel.Configuration.Upstream, req.Upstream) } return existingAPI, nil } +// preserveUpstreamAuthOnAPIUpdate merges updated's Main/Sandbox auth onto +// existing's stored (unredacted) values whenever updated's auth value is +// empty, then returns updated with those merged auth blocks. Non-auth fields +// (Url, Ref) are taken from updated as-is. +func preserveUpstreamAuthOnAPIUpdate(existing model.UpstreamConfig, updated api.Upstream) api.Upstream { + updated.Main.Auth = preserveAPIUpstreamAuth(existing.Main, updated.Main.Auth) + if updated.Sandbox != nil { + updated.Sandbox.Auth = preserveAPIUpstreamAuth(existing.Sandbox, updated.Sandbox.Auth) + } + return updated +} + +// preserveAPIUpstreamAuth backfills updated's Value from existing's stored +// value when updated is nil or has an empty value. The stored value is only +// reused when updated's Type matches existing's (or leaves Type unspecified); +// an explicit Type change means the old secret is for a different auth +// scheme and must not be silently reattached. +func preserveAPIUpstreamAuth(existing *model.UpstreamEndpoint, updated *api.UpstreamAuth) *api.UpstreamAuth { + if existing == nil || existing.Auth == nil || existing.Auth.Value == "" { + return updated + } + if updated == nil { + authType := api.UpstreamAuthType(existing.Auth.Type) + return &api.UpstreamAuth{ + Type: &authType, + Header: utils.StringPtrIfNotEmpty(existing.Auth.Header), + Value: utils.StringPtrIfNotEmpty(existing.Auth.Value), + } + } + typeChanged := updated.Type != nil && string(*updated.Type) != existing.Auth.Type + if !typeChanged && (updated.Value == nil || *updated.Value == "") { + updated.Value = utils.StringPtrIfNotEmpty(existing.Auth.Value) + } + return updated +} + // validateUpdateAPIRequest checks the validity of the update API request func (s *APIService) validateUpdateAPIRequest(existingAPIModel *model.API, req *api.RESTAPI, orgUUID string) error { if req.DisplayName != "" { diff --git a/platform-api/internal/service/api_secret_integration_test.go b/platform-api/internal/service/api_secret_integration_test.go new file mode 100644 index 0000000000..54f75dbf26 --- /dev/null +++ b/platform-api/internal/service/api_secret_integration_test.go @@ -0,0 +1,274 @@ +/* + * Copyright (c) 2026, WSO2 LLC. (http://www.wso2.org) All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +// Integration tests for REST API secret management, backed by a real SQLite +// DB (not mocks). These exercise the actual artifact_secret_refs reference +// tracking that only the repository layer populates — a scenario the mocked +// unit tests in api_test.go cannot cover, since mockAPIRepository never +// touches that table. This automates the manual curl-based validation used +// to confirm the fix against a live platform-api instance. + +package service + +import ( + "database/sql" + "errors" + "log/slog" + "os" + "path/filepath" + "testing" + + "github.com/wso2/api-platform/platform-api/api" + "github.com/wso2/api-platform/platform-api/internal/constants" + "github.com/wso2/api-platform/platform-api/internal/database" + "github.com/wso2/api-platform/platform-api/internal/dto" + "github.com/wso2/api-platform/platform-api/internal/model" + "github.com/wso2/api-platform/platform-api/internal/repository" + "github.com/wso2/api-platform/platform-api/internal/utils" + "github.com/wso2/api-platform/platform-api/internal/vault" + + _ "github.com/mattn/go-sqlite3" +) + +const apiSecretITOrgUUID = "org-api-secret-it" +const apiSecretITProjectUUID = "proj-api-secret-it" + +// setupAPISecretTestEnv creates a real SQLite-backed APIService and +// SecretService, wired together exactly as server.go wires them in +// production (APIService.SetSecretService), plus a seeded org and project. +func setupAPISecretTestEnv(t *testing.T) (*APIService, *SecretService, func()) { + t.Helper() + + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "api-secret-it.db") + sqlDB, err := sql.Open("sqlite3", dbPath) + if err != nil { + t.Fatalf("open db: %v", err) + } + sqlDB.Exec("PRAGMA foreign_keys = ON") + db := &database.DB{DB: sqlDB} + + schemaPath := filepath.Join("..", "database", "schema.sqlite.sql") + schema, err := os.ReadFile(schemaPath) + if err != nil { + t.Fatalf("read schema: %v", err) + } + if _, err = db.Exec(string(schema)); err != nil { + t.Fatalf("apply schema: %v", err) + } + + if _, err = db.Exec(`INSERT INTO organizations (uuid, handle, display_name, region, idp_organization_ref_uuid, created_at, updated_at) + VALUES (?, 'api-secret-it-org', 'API Secret IT Org', 'default', 'idp-ref', datetime('now'), datetime('now'))`, apiSecretITOrgUUID); err != nil { + t.Fatalf("insert org: %v", err) + } + if _, err = db.Exec(`INSERT INTO projects (uuid, handle, display_name, description, organization_uuid, created_at, updated_at) + VALUES (?, 'api-secret-it-proj', 'API Secret IT Project', '', ?, datetime('now'), datetime('now'))`, apiSecretITProjectUUID, apiSecretITOrgUUID); err != nil { + t.Fatalf("insert project: %v", err) + } + + v, err := vault.NewInHouseVault([]byte("12345678901234567890123456789012")) + if err != nil { + t.Fatalf("create vault: %v", err) + } + + identity := NewIdentityService(repository.NewUserIdentityMappingRepo(db)) + secretRepo := repository.NewSecretRepo(db) + secretSvc := NewSecretService(secretRepo, v, identity) + + apiRepo := repository.NewAPIRepo(db) + projectRepo := repository.NewProjectRepo(db) + auditRepo := repository.NewAuditRepo(db) + + apiSvc := NewAPIService(apiRepo, projectRepo, nil, nil, nil, nil, nil, nil, &utils.APIUtil{}, slog.Default(), auditRepo, identity) + apiSvc.SetSecretService(secretSvc) + + cleanup := func() { sqlDB.Close() } + return apiSvc, secretSvc, cleanup +} + +// createTestSecret creates a secret directly via SecretService and fails the +// test on error. +func createTestSecret(t *testing.T, svc *SecretService, orgUUID, handle, value string) { + t.Helper() + _, err := svc.Create(orgUUID, "alice", &dto.CreateSecretRequest{ + Handle: handle, + DisplayName: handle, + Value: value, + }) + if err != nil { + t.Fatalf("failed to create secret %q: %v", handle, err) + } +} + +// TestAPIServiceSecretLifecycle_Integration exercises the full secret +// lifecycle for a REST API's upstream auth against a real DB: create with a +// placeholder, redaction on read, delete-protection while referenced, +// preservation across an unrelated update, rotation cleanup, and rejection +// of both upstream and policy placeholders that don't resolve. +func TestAPIServiceSecretLifecycle_Integration(t *testing.T) { + apiSvc, secretSvc, cleanup := setupAPISecretTestEnv(t) + defer cleanup() + + createTestSecret(t, secretSvc, apiSecretITOrgUUID, "it-secret-a", "sk-real-backend-token-A") + createTestSecret(t, secretSvc, apiSecretITOrgUUID, "it-secret-b", "sk-real-backend-token-B") + + // --- Create with a placeholder referencing secret A --- + createReq := &api.CreateRESTAPIRequest{ + DisplayName: "IT REST API", + Context: "/it-rest-api", + Version: "v1", + ProjectId: "api-secret-it-proj", + Upstream: api.Upstream{ + Main: api.UpstreamDefinition{ + Url: utils.StringPtrIfNotEmpty("https://backend.internal/api"), + Auth: &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("bearer"), + Header: ptr("Authorization"), + Value: ptr(`{{ secret "it-secret-a" }}`), + }, + }, + }, + } + created, err := apiSvc.CreateAPI(createReq, apiSecretITOrgUUID, "alice") + if err != nil { + t.Fatalf("CreateAPI failed: %v", err) + } + + // --- Redaction: the create response must never carry the real value --- + if created.Upstream.Main.Auth == nil { + t.Fatal("expected auth block to be present in create response") + } + if created.Upstream.Main.Auth.Value != nil { + t.Errorf("expected auth value to be redacted in create response, got %q", *created.Upstream.Main.Auth.Value) + } + if created.Upstream.Main.Auth.Header == nil || *created.Upstream.Main.Auth.Header != "Authorization" { + t.Errorf("expected auth header to survive redaction, got %v", created.Upstream.Main.Auth.Header) + } + + apiUUID := created.Id + if apiUUID == nil || *apiUUID == "" { + t.Fatal("expected created API to have an id") + } + + // --- Delete-protection: secret A must be blocked while referenced --- + if err := secretSvc.Delete(apiSecretITOrgUUID, "it-secret-a", "alice"); err == nil { + t.Fatal("expected secret A deletion to be blocked while referenced by the API, got nil error") + } else { + var inUse *SecretInUseError + if !errors.As(err, &inUse) { + t.Errorf("expected SecretInUseError, got: %v", err) + } + } + + // --- Preservation: update the URL only, omit auth entirely --- + updateReq := &api.RESTAPI{ + DisplayName: "IT REST API", + Context: "/it-rest-api", + Version: "v1", + Upstream: api.Upstream{ + Main: api.UpstreamDefinition{Url: utils.StringPtrIfNotEmpty("https://backend-v2.internal/api")}, + }, + } + updated, err := apiSvc.UpdateAPIByHandle(*apiUUID, updateReq, apiSecretITOrgUUID, "alice") + if err != nil { + t.Fatalf("UpdateAPI (URL-only) failed: %v", err) + } + if updated.Upstream.Main.Url == nil || *updated.Upstream.Main.Url != "https://backend-v2.internal/api" { + t.Errorf("expected URL to be updated, got %v", updated.Upstream.Main.Url) + } + // Secret A must still be protected — proves auth wasn't wiped by the update. + if err := secretSvc.Delete(apiSecretITOrgUUID, "it-secret-a", "alice"); err == nil { + t.Fatal("expected secret A deletion to still be blocked after a URL-only update, got nil error") + } + + // --- Rotation cleanup: switch auth to secret B --- + rotateReq := &api.RESTAPI{ + DisplayName: "IT REST API", + Context: "/it-rest-api", + Version: "v1", + Upstream: api.Upstream{ + Main: api.UpstreamDefinition{ + Url: utils.StringPtrIfNotEmpty("https://backend-v2.internal/api"), + Auth: &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("bearer"), + Header: ptr("Authorization"), + Value: ptr(`{{ secret "it-secret-b" }}`), + }, + }, + }, + } + if _, err := apiSvc.UpdateAPIByHandle(*apiUUID, rotateReq, apiSecretITOrgUUID, "alice"); err != nil { + t.Fatalf("UpdateAPI (rotate) failed: %v", err) + } + + secretA, err := secretSvc.Get(apiSecretITOrgUUID, "it-secret-a") + if err != nil { + t.Fatalf("failed to fetch secret A after rotation: %v", err) + } + if secretA.Status != string(model.SecretStatusDeprecated) { + t.Errorf("expected secret A to be deprecated after rotation, got status=%q", secretA.Status) + } + secretB, err := secretSvc.Get(apiSecretITOrgUUID, "it-secret-b") + if err != nil { + t.Fatalf("failed to fetch secret B after rotation: %v", err) + } + if secretB.Status != string(model.SecretStatusActive) { + t.Errorf("expected secret B to remain active after rotation, got status=%q", secretB.Status) + } + // Secret A is no longer referenced, so it can now be hard-deleted. + if err := secretSvc.Delete(apiSecretITOrgUUID, "it-secret-a", "alice"); err != nil { + t.Errorf("expected secret A to be deletable after rotation freed it, got: %v", err) + } + + // --- Validation: a placeholder in upstream.auth that doesn't resolve is rejected --- + badUpstreamReq := &api.RESTAPI{ + DisplayName: "IT REST API", + Context: "/it-rest-api", + Version: "v1", + Upstream: api.Upstream{ + Main: api.UpstreamDefinition{ + Url: utils.StringPtrIfNotEmpty("https://backend-v2.internal/api"), + Auth: &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("bearer"), + Value: ptr(`{{ secret "nonexistent-handle" }}`), + }, + }, + }, + } + if _, err := apiSvc.UpdateAPIByHandle(*apiUUID, badUpstreamReq, apiSecretITOrgUUID, "alice"); err == nil { + t.Fatal("expected update with a nonexistent upstream secret ref to be rejected, got nil error") + } else if !errors.Is(err, constants.ErrSecretRefMissing) { + t.Errorf("expected ErrSecretRefMissing, got: %v", err) + } + + // --- Validation: the same check covers policy params, not just upstream --- + params := map[string]interface{}{"password": `{{ secret "nonexistent-policy-handle" }}`} + badPolicyReq := &api.RESTAPI{ + DisplayName: "IT REST API", + Context: "/it-rest-api", + Version: "v1", + Upstream: api.Upstream{ + Main: api.UpstreamDefinition{Url: utils.StringPtrIfNotEmpty("https://backend-v2.internal/api")}, + }, + Policies: &[]api.Policy{{Name: "basic-auth", Version: "v1", Params: ¶ms}}, + } + if _, err := apiSvc.UpdateAPIByHandle(*apiUUID, badPolicyReq, apiSecretITOrgUUID, "alice"); err == nil { + t.Fatal("expected update with a nonexistent policy secret ref to be rejected, got nil error") + } else if !errors.Is(err, constants.ErrSecretRefMissing) { + t.Errorf("expected ErrSecretRefMissing, got: %v", err) + } +} diff --git a/platform-api/internal/service/api_test.go b/platform-api/internal/service/api_test.go index 31957d72f2..920204ca76 100644 --- a/platform-api/internal/service/api_test.go +++ b/platform-api/internal/service/api_test.go @@ -18,6 +18,7 @@ package service import ( + "errors" "reflect" "testing" @@ -40,6 +41,9 @@ type mockAPIRepository struct { // Call tracking for verification lastExcludeHandle string + created *model.API + updated *model.API + getByUUIDFunc func(apiUUID, orgUUID string) (*model.API, error) } func (m *mockAPIRepository) CheckAPIExistsByHandleInOrganization(handle, orgUUID string) (bool, error) { @@ -51,6 +55,23 @@ func (m *mockAPIRepository) CheckAPIExistsByNameAndVersionInOrganization(name, v return m.nameVersionExistsResult, m.nameVersionExistsError } +func (m *mockAPIRepository) CreateAPI(api *model.API) error { + m.created = api + return nil +} + +func (m *mockAPIRepository) GetAPIByUUID(apiUUID, orgUUID string) (*model.API, error) { + if m.getByUUIDFunc != nil { + return m.getByUUIDFunc(apiUUID, orgUUID) + } + return nil, nil +} + +func (m *mockAPIRepository) UpdateAPI(api *model.API) error { + m.updated = api + return nil +} + // TestValidateUpdateAPIRequest tests the validateUpdateAPIRequest method func TestValidateUpdateAPIRequest(t *testing.T) { tests := []struct { @@ -576,6 +597,170 @@ func TestApplyAPIUpdatesUpdatesPolicies(t *testing.T) { } } +// TestApplyAPIUpdatesPreservesUpstreamAuthOnEmptyValue proves that updating an +// API's upstream URL without resending auth (routine — auth.value is redacted +// on GET, so a naive read-modify-write round-trip never has it to resend) +// does not wipe the stored credential. +func TestApplyAPIUpdatesPreservesUpstreamAuthOnEmptyValue(t *testing.T) { + service := &APIService{ + apiRepo: &mockAPIRepository{}, + projectRepo: &mockProjectRepository{projectByUUID: &model.Project{ID: "11111111-1111-1111-1111-111111111111", Handle: "test-project"}}, + apiUtil: &utils.APIUtil{}, + identity: newTestIdentityService(), + } + + existing := &model.API{ + Handle: "pets-api", + ProjectID: "11111111-1111-1111-1111-111111111111", + Version: "v1", + Configuration: model.RestAPIConfig{ + Upstream: model.UpstreamConfig{ + Main: &model.UpstreamEndpoint{ + URL: "https://old-backend.internal/api", + Auth: &model.UpstreamAuth{Type: "bearer", Header: "Authorization", Value: "stored-secret-token"}, + }, + }, + }, + } + + // Client updates only the URL — auth is omitted, as it would be after a + // GET (which redacts auth.value) followed by a naive PUT of the same body. + req := &api.RESTAPI{ + Upstream: api.Upstream{Main: api.UpstreamDefinition{Url: utils.StringPtrIfNotEmpty("https://new-backend.internal/api")}}, + } + + updated, err := service.applyAPIUpdates(existing, req, "org-1") + if err != nil { + t.Fatalf("applyAPIUpdates() error = %v", err) + } + + if updated.Upstream.Main.Url == nil || *updated.Upstream.Main.Url != "https://new-backend.internal/api" { + t.Errorf("expected URL to be updated, got %v", updated.Upstream.Main.Url) + } + if updated.Upstream.Main.Auth == nil || updated.Upstream.Main.Auth.Value == nil { + t.Fatal("expected stored auth value to be preserved, got nil auth block/value") + } + if *updated.Upstream.Main.Auth.Value != "stored-secret-token" { + t.Errorf("expected stored auth value to be preserved, got %q", *updated.Upstream.Main.Auth.Value) + } +} + +// TestApplyAPIUpdatesDoesNotReuseSecretAcrossAuthTypeChange proves that +// changing the auth Type without resending a Value does not reattach the +// old secret — the old secret was encrypted/formatted for the previous auth +// scheme (e.g. a bearer token) and must not be silently reused as, say, a +// basic-auth credential. +func TestApplyAPIUpdatesDoesNotReuseSecretAcrossAuthTypeChange(t *testing.T) { + service := &APIService{ + apiRepo: &mockAPIRepository{}, + projectRepo: &mockProjectRepository{projectByUUID: &model.Project{ID: "11111111-1111-1111-1111-111111111111", Handle: "test-project"}}, + apiUtil: &utils.APIUtil{}, + identity: newTestIdentityService(), + } + + existing := &model.API{ + Handle: "pets-api", + ProjectID: "11111111-1111-1111-1111-111111111111", + Version: "v1", + Configuration: model.RestAPIConfig{ + Upstream: model.UpstreamConfig{ + Main: &model.UpstreamEndpoint{ + URL: "https://backend.internal/api", + Auth: &model.UpstreamAuth{Type: "bearer", Header: "Authorization", Value: `{{ secret "bearer-handle" }}`}, + }, + }, + }, + } + + // Client switches auth Type from bearer to basic but does not resend a + // Value (e.g. redacted-field round-trip, or simply an oversight). + basicType := api.UpstreamAuthType("basic") + req := &api.RESTAPI{ + Upstream: api.Upstream{ + Main: api.UpstreamDefinition{ + Url: utils.StringPtrIfNotEmpty("https://backend.internal/api"), + Auth: &api.UpstreamAuth{ + Type: &basicType, + Header: utils.StringPtrIfNotEmpty("Authorization"), + }, + }, + }, + } + + updated, err := service.applyAPIUpdates(existing, req, "org-1") + if err != nil { + t.Fatalf("applyAPIUpdates() error = %v", err) + } + + if updated.Upstream.Main.Auth == nil { + t.Fatal("expected auth block to be present") + } + if updated.Upstream.Main.Auth.Type == nil || *updated.Upstream.Main.Auth.Type != basicType { + t.Errorf("expected auth type to be %q, got %v", basicType, updated.Upstream.Main.Auth.Type) + } + if updated.Upstream.Main.Auth.Value != nil && *updated.Upstream.Main.Auth.Value != "" { + t.Errorf("expected old bearer secret to NOT be reused for the new basic auth type, got value %q", *updated.Upstream.Main.Auth.Value) + } +} + +// TestAPIServiceUpdate_CleansUpRotatedSecret proves that rotating an API's +// upstream auth to a new secret deprecates the secret it replaced. +func TestAPIServiceUpdate_CleansUpRotatedSecret(t *testing.T) { + apiRepo := &mockAPIRepository{ + getByUUIDFunc: func(apiUUID, orgUUID string) (*model.API, error) { + return &model.API{ + ID: apiUUID, + Handle: "pets-api", + OrganizationID: orgUUID, + ProjectID: "11111111-1111-1111-1111-111111111111", + Version: "v1", + Configuration: model.RestAPIConfig{ + Upstream: model.UpstreamConfig{ + Main: &model.UpstreamEndpoint{ + URL: "https://backend.internal/api", + Auth: &model.UpstreamAuth{Type: "bearer", Header: "Authorization", Value: `{{ secret "old-handle" }}`}, + }, + }, + }, + }, nil + }, + } + secretRepo := newMockRepo() + secretRepo.secrets["old-handle"] = &model.Secret{Handle: "old-handle", Status: model.SecretStatusActive} + secretRepo.secrets["new-handle"] = &model.Secret{Handle: "new-handle", Status: model.SecretStatusActive} + secretService := NewSecretService(secretRepo, &mockVault{}, newTestIdentityService()) + + service := &APIService{ + apiRepo: apiRepo, + projectRepo: &mockProjectRepository{projectByUUID: &model.Project{ID: "11111111-1111-1111-1111-111111111111", Handle: "test-project"}}, + apiUtil: &utils.APIUtil{}, + secretService: secretService, + identity: newTestIdentityService(), + auditRepo: &noopAuditRepo{}, + } + + req := &api.RESTAPI{ + Upstream: api.Upstream{ + Main: api.UpstreamDefinition{ + Url: utils.StringPtrIfNotEmpty("https://backend.internal/api"), + Auth: &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("bearer"), + Header: ptr("Authorization"), + Value: ptr(`{{ secret "new-handle" }}`), + }, + }, + }, + } + + _, err := service.UpdateAPI("api-uuid", req, "org-1", "alice") + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if secretRepo.secrets["old-handle"].Status != model.SecretStatusDeprecated { + t.Errorf("expected old secret to be deprecated, got status=%v", secretRepo.secrets["old-handle"].Status) + } +} + // Helper functions // ptr creates a string pointer @@ -599,3 +784,74 @@ func createStatusPtr(s string) *api.CreateRESTAPIRequestLifeCycleStatus { } // Note: contains() and findSubstring() helper functions are defined in gateway_test.go + +func TestAPIServiceCreate_MissingSecretRef_Rejected(t *testing.T) { + apiRepo := &mockAPIRepository{} + secretService := NewSecretService(newMockRepo(), &mockVault{}, newTestIdentityService()) + service := &APIService{ + apiRepo: apiRepo, + secretService: secretService, + identity: newTestIdentityService(), + } + + params := map[string]interface{}{"value": `{{ secret "nonexistent-api-secret" }}`} + req := &api.CreateRESTAPIRequest{ + DisplayName: "Test API", + Context: "/test", + Version: "v1", + ProjectId: "11111111-1111-1111-1111-111111111111", + Upstream: api.Upstream{}, + Policies: &[]api.Policy{{Name: "set-headers", Version: "v1", Params: ¶ms}}, + } + + _, err := service.CreateAPI(req, "org-1", "alice") + if err == nil { + t.Fatal("expected error for non-existent secret placeholder, got nil") + } + if !errors.Is(err, constants.ErrSecretRefMissing) { + t.Errorf("expected ErrSecretRefMissing, got: %v", err) + } + if apiRepo.created != nil { + t.Error("expected API creation to be aborted, but repo.CreateAPI was called") + } +} + +func TestAPIServiceUpdate_MissingSecretRef_Rejected(t *testing.T) { + apiRepo := &mockAPIRepository{ + getByUUIDFunc: func(apiUUID, orgUUID string) (*model.API, error) { + return &model.API{ + ID: apiUUID, + Handle: "pets-api", + OrganizationID: orgUUID, + ProjectID: "11111111-1111-1111-1111-111111111111", + Version: "v1", + Configuration: model.RestAPIConfig{}, + }, nil + }, + } + secretService := NewSecretService(newMockRepo(), &mockVault{}, newTestIdentityService()) + service := &APIService{ + apiRepo: apiRepo, + secretService: secretService, + identity: newTestIdentityService(), + } + + params := map[string]interface{}{"value": `{{ secret "nonexistent-api-secret" }}`} + req := &api.RESTAPI{ + DisplayName: "Test API", + Context: "/test", + Version: "v1", + Policies: &[]api.Policy{{Name: "set-headers", Version: "v1", Params: ¶ms}}, + } + + _, err := service.UpdateAPI("api-uuid", req, "org-1", "alice") + if err == nil { + t.Fatal("expected error for non-existent secret placeholder, got nil") + } + if !errors.Is(err, constants.ErrSecretRefMissing) { + t.Errorf("expected ErrSecretRefMissing, got: %v", err) + } + if apiRepo.updated != nil { + t.Error("expected API update to be aborted, but repo.UpdateAPI was called") + } +} diff --git a/platform-api/internal/service/llm.go b/platform-api/internal/service/llm.go index 691dab3927..40edeb1dea 100644 --- a/platform-api/internal/service/llm.go +++ b/platform-api/internal/service/llm.go @@ -70,6 +70,7 @@ type LLMProxyService struct { deploymentRepo repository.DeploymentRepository gatewayRepo repository.GatewayRepository gatewayEventsService *GatewayEventsService + secretService *SecretService slogger *slog.Logger auditRepo repository.AuditRepository cfg *config.Server @@ -140,6 +141,13 @@ func (s *LLMProviderService) SetSecretService(ss *SecretService) { s.secretService = ss } +// SetSecretService injects the SecretService used to clean up a rotated-away +// credential after Update. Called after both services are constructed to +// avoid circular dependency. +func (s *LLMProxyService) SetSecretService(ss *SecretService) { + s.secretService = ss +} + // toProviderAPI converts m via mapProviderModelToAPI and resolves its // createdBy/updatedBy UUIDs to their raw external identity. func (s *LLMProviderService) toProviderAPI(m *model.LLMProvider, templateHandle string) (*api.LLMProvider, error) { @@ -873,11 +881,14 @@ func (s *LLMProviderService) Create(orgUUID, createdBy string, req *api.LLMProvi } req.Id = &handle - // Validate {{ secret "..." }} placeholders in the upstream config + // Validate {{ secret "..." }} placeholders anywhere in the request — the + // gateway-controller's template engine resolves placeholders generically + // across the whole artifact (policies included), not just upstream.auth, + // so validation must cover the same surface. if s.secretService != nil { - configJSON, err := marshalUpstreamForValidation(req.Upstream) + configJSON, err := marshalUpstreamForValidation(req) if err != nil { - return nil, fmt.Errorf("failed to marshal upstream config for secret validation: %w", err) + return nil, fmt.Errorf("failed to marshal request for secret validation: %w", err) } if err := s.secretService.ValidateSecretRefs(orgUUID, configJSON); err != nil { return nil, err @@ -1076,11 +1087,12 @@ func (s *LLMProviderService) Update(orgUUID, handle, updatedBy string, req *api. return nil, apperror.LLMProviderTemplateNotFound.Wrap(constants.ErrLLMProviderTemplateNotFound) } - // Validate {{ secret "..." }} placeholders in the upstream config + // Validate {{ secret "..." }} placeholders anywhere in the request — see + // Create for why this covers the whole request, not just upstream. if s.secretService != nil { - configJSON, err := marshalUpstreamForValidation(req.Upstream) + configJSON, err := marshalUpstreamForValidation(req) if err != nil { - return nil, fmt.Errorf("failed to marshal upstream config for secret validation: %w", err) + return nil, fmt.Errorf("failed to marshal request for secret validation: %w", err) } if err := s.secretService.ValidateSecretRefs(orgUUID, configJSON); err != nil { return nil, err @@ -1150,6 +1162,19 @@ func (s *LLMProviderService) Update(orgUUID, handle, updatedBy string, req *api. return nil, fmt.Errorf("failed to update provider: %w", err) } + // Best-effort: delete the secret the credential was rotated away from. Must + // run after the update above persists the new reference, so the in-use + // check below no longer sees this provider pointing at the old handle. + if s.secretService != nil { + s.secretService.cleanupRotatedSecret( + orgUUID, + mainUpstreamAuthValue(existing.Configuration.Upstream), + mainUpstreamAuthValue(m.Configuration.Upstream), + updatedBy, + s.slogger, + ) + } + updated, err := s.repo.GetByID(handle, orgUUID) if err != nil { return nil, fmt.Errorf("failed to fetch updated provider: %w", err) @@ -1285,6 +1310,20 @@ func (s *LLMProxyService) Create(orgUUID, createdBy string, req *api.LLMProxy) ( } req.Id = &handle + // Validate {{ secret "..." }} placeholders anywhere in the request — the + // gateway-controller's template engine resolves placeholders generically + // across the whole artifact (policies included), not just provider.auth, + // so validation must cover the same surface. + if s.secretService != nil { + configJSON, err := marshalUpstreamForValidation(req) + if err != nil { + return nil, fmt.Errorf("failed to marshal request for secret validation: %w", err) + } + if err := s.secretService.ValidateSecretRefs(orgUUID, configJSON); err != nil { + return nil, err + } + } + proxyCount, err := s.repo.Count(orgUUID) if err != nil { return nil, fmt.Errorf("failed to count proxies: %w", err) @@ -1534,6 +1573,22 @@ func (s *LLMProxyService) Update(orgUUID, handle, updatedBy string, req *api.LLM return nil, constants.ErrLLMProviderNotFound } + // Validate {{ secret "..." }} placeholders anywhere in the request. Checked + // against the raw request (not the post-preserve merged auth value) — an + // empty auth value here means "no change" and has nothing to validate; the + // value it will be preserved from was already validated when it was + // originally submitted. Covers the whole request (policies included), not + // just provider.auth — see Create for why. + if s.secretService != nil { + configJSON, err := marshalUpstreamForValidation(req) + if err != nil { + return nil, fmt.Errorf("failed to marshal request for secret validation: %w", err) + } + if err := s.secretService.ValidateSecretRefs(orgUUID, configJSON); err != nil { + return nil, err + } + } + contextValue := utils.DefaultStringPtr(req.Context, "/") m := &model.LLMProxy{ OrganizationUUID: orgUUID, @@ -1592,6 +1647,19 @@ func (s *LLMProxyService) Update(orgUUID, handle, updatedBy string, req *api.LLM return nil, fmt.Errorf("failed to update proxy: %w", err) } + // Best-effort: delete the secret the credential was rotated away from. Must + // run after the update above persists the new reference, so the in-use + // check below no longer sees this proxy pointing at the old handle. + if s.secretService != nil { + s.secretService.cleanupRotatedSecret( + orgUUID, + upstreamAuthValue(existing.Configuration.UpstreamAuth), + upstreamAuthValue(m.Configuration.UpstreamAuth), + updatedBy, + s.slogger, + ) + } + updated, err := s.repo.GetByID(handle, orgUUID) if err != nil { return nil, fmt.Errorf("failed to fetch updated proxy: %w", err) diff --git a/platform-api/internal/service/llm_test.go b/platform-api/internal/service/llm_test.go index f2c3f99ce2..c16de920ec 100644 --- a/platform-api/internal/service/llm_test.go +++ b/platform-api/internal/service/llm_test.go @@ -1520,6 +1520,230 @@ func TestLLMProxyServiceUpdatePreservesProviderAuthValue(t *testing.T) { } } +// TestLLMProviderServiceCreate_PolicySecretRef_Rejected proves secret-ref +// validation now covers the whole request, not just upstream.auth — a +// placeholder embedded in a policy param (not upstream) must also be rejected. +func TestLLMProviderServiceCreate_PolicySecretRef_Rejected(t *testing.T) { + now := time.Now() + providerRepo := &mockLLMProviderRepo{} + templateRepo := &mockLLMTemplateRepo{ + getByIDFunc: func(templateID, orgUUID string) (*model.LLMProviderTemplate, error) { + return &model.LLMProviderTemplate{UUID: "tpl-1", ID: templateID, Enabled: true, CreatedAt: now, UpdatedAt: now}, nil + }, + } + orgRepo := &mockOrganizationRepo{org: &model.Organization{ID: "org-1"}} + secretService := NewSecretService(newMockRepo(), &mockVault{}, newTestIdentityService()) + service := NewLLMProviderService(providerRepo, templateRepo, orgRepo, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + service.SetSecretService(secretService) + + request := validProviderRequest("openai") + params := map[string]interface{}{"value": `{{ secret "nonexistent-policy-secret" }}`} + request.GlobalPolicies = &[]api.Policy{{Name: "set-headers", Version: "v1", Params: ¶ms}} + + _, err := service.Create("org-1", "alice", request) + if err == nil { + t.Fatal("expected error for non-existent secret placeholder in a policy param, got nil") + } + if !errors.Is(err, constants.ErrSecretRefMissing) { + t.Errorf("expected ErrSecretRefMissing, got: %v", err) + } + if providerRepo.created != nil { + t.Error("expected provider creation to be aborted, but repo.Create was called") + } +} + +func TestLLMProxyServiceCreate_MissingSecretRef_Rejected(t *testing.T) { + proxyRepo := &mockLLMProxyRepo{} + providerRepo := &mockLLMProviderRepo{ + getByIDFunc: func(providerID, orgUUID string) (*model.LLMProvider, error) { + return &model.LLMProvider{UUID: "provider-uuid", ID: providerID}, nil + }, + } + secretService := NewSecretService(newMockRepo(), &mockVault{}, newTestIdentityService()) + service := NewLLMProxyService(proxyRepo, providerRepo, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + service.SetSecretService(secretService) + + request := validProxyRequest("provider-1", "project-1") + request.Provider.Auth = &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("api-key"), + Header: stringPtr("Authorization"), + Value: stringPtr(`{{ secret "nonexistent-proxy-secret" }}`), + } + + _, err := service.Create("org-1", "alice", request) + if err == nil { + t.Fatal("expected error for non-existent secret placeholder, got nil") + } + if !errors.Is(err, constants.ErrSecretRefMissing) { + t.Errorf("expected ErrSecretRefMissing, got: %v", err) + } + if proxyRepo.created != nil { + t.Error("expected proxy creation to be aborted, but repo.Create was called") + } +} + +func TestLLMProxyServiceUpdate_MissingSecretRef_Rejected(t *testing.T) { + now := time.Now() + proxyRepo := &mockLLMProxyRepo{ + getByIDFunc: func(proxyID, orgUUID string) (*model.LLMProxy, error) { + return &model.LLMProxy{ + UUID: "proxy-uuid", ID: proxyID, Name: "Old Proxy", Version: "v1.0", + ProjectUUID: "project-1", ProviderUUID: "provider-uuid", + CreatedAt: now, UpdatedAt: now, + Configuration: model.LLMProxyConfig{ + Provider: "provider-1", + UpstreamAuth: &model.UpstreamAuth{Type: "api-key", Header: "Authorization", Value: `{{ secret "existing-handle" }}`}, + }, + }, nil + }, + } + providerRepo := &mockLLMProviderRepo{ + getByIDFunc: func(providerID, orgUUID string) (*model.LLMProvider, error) { + return &model.LLMProvider{UUID: "provider-uuid", ID: providerID}, nil + }, + } + secretService := NewSecretService(newMockRepo(), &mockVault{}, newTestIdentityService()) + service := NewLLMProxyService(proxyRepo, providerRepo, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + service.SetSecretService(secretService) + + request := validProxyRequest("provider-1", "project-1") + request.Provider.Auth = &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("api-key"), + Header: stringPtr("Authorization"), + Value: stringPtr(`{{ secret "nonexistent-proxy-secret" }}`), + } + + _, err := service.Update("org-1", "proxy-1", "alice", request) + if err == nil { + t.Fatal("expected error for non-existent secret placeholder, got nil") + } + if !errors.Is(err, constants.ErrSecretRefMissing) { + t.Errorf("expected ErrSecretRefMissing, got: %v", err) + } + if proxyRepo.updated != nil { + t.Error("expected proxy update to be aborted, but repo.Update was called") + } +} + +// TestLLMProviderServiceUpdate_CleansUpRotatedSecret proves rotating a +// provider's upstream credential deprecates the secret it replaced — the +// same cleanupRotatedSecret path LLM Proxy and REST API are also tested +// against, exercised here for the Provider service specifically. +func TestLLMProviderServiceUpdate_CleansUpRotatedSecret(t *testing.T) { + now := time.Now() + providerRepo := &mockLLMProviderRepo{} + providerRepo.getByIDFunc = func(providerID, orgUUID string) (*model.LLMProvider, error) { + if providerRepo.updated == nil { + return &model.LLMProvider{ + UUID: "prov-uuid", + ID: providerID, + Name: "Old Provider", + Version: "v1.0", + TemplateUUID: "tpl-openai", + CreatedAt: now, + UpdatedAt: now, + Configuration: model.LLMProviderConfig{ + Upstream: &model.UpstreamConfig{ + Main: &model.UpstreamEndpoint{ + URL: "https://example.com/openai/v1", + Auth: &model.UpstreamAuth{Type: "api-key", Header: "Authorization", Value: `{{ secret "old-handle" }}`}, + }, + }, + }, + }, nil + } + updated := *providerRepo.updated + updated.UUID = "prov-uuid" + updated.CreatedAt = now + updated.UpdatedAt = now + return &updated, nil + } + templateRepo := &mockLLMTemplateRepo{ + getByIDFunc: func(templateID, orgUUID string) (*model.LLMProviderTemplate, error) { + return &model.LLMProviderTemplate{UUID: "tpl-openai", ID: "openai"}, nil + }, + } + secretRepo := newMockRepo() + secretRepo.secrets["old-handle"] = &model.Secret{Handle: "old-handle", Status: model.SecretStatusActive} + secretRepo.secrets["new-handle"] = &model.Secret{Handle: "new-handle", Status: model.SecretStatusActive} + secretService := NewSecretService(secretRepo, &mockVault{}, newTestIdentityService()) + + service := NewLLMProviderService(providerRepo, templateRepo, nil, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + service.SetSecretService(secretService) + + request := validProviderRequest("openai") + request.Upstream.Main.Auth = &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("api-key"), + Header: stringPtr("Authorization"), + Value: stringPtr(`{{ secret "new-handle" }}`), + } + + if _, err := service.Update("org-1", "provider-1", "alice", request); err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if secretRepo.secrets["old-handle"].Status != model.SecretStatusDeprecated { + t.Fatalf("expected old secret to be deprecated, got status=%v", secretRepo.secrets["old-handle"].Status) + } + if secretRepo.secrets["new-handle"].Status != model.SecretStatusActive { + t.Fatalf("expected new secret to remain active, got status=%v", secretRepo.secrets["new-handle"].Status) + } +} + +func TestLLMProxyServiceUpdate_CleansUpRotatedSecret(t *testing.T) { + now := time.Now() + proxyRepo := &mockLLMProxyRepo{} + proxyRepo.getByIDFunc = func(proxyID, orgUUID string) (*model.LLMProxy, error) { + if proxyRepo.updated == nil { + return &model.LLMProxy{ + UUID: "proxy-uuid", + ID: proxyID, + Name: "Old Proxy", + Version: "v1.0", + ProjectUUID: "project-1", + ProviderUUID: "provider-uuid", + CreatedAt: now, + UpdatedAt: now, + Configuration: model.LLMProxyConfig{ + Provider: "provider-1", + UpstreamAuth: &model.UpstreamAuth{Type: "api-key", Header: "Authorization", Value: `{{ secret "old-handle" }}`}, + }, + }, nil + } + updated := *proxyRepo.updated + updated.UUID = "proxy-uuid" + updated.CreatedAt = now + updated.UpdatedAt = now + return &updated, nil + } + providerRepo := &mockLLMProviderRepo{ + getByIDFunc: func(providerID, orgUUID string) (*model.LLMProvider, error) { + return &model.LLMProvider{UUID: "provider-uuid", ID: providerID}, nil + }, + } + secretRepo := newMockRepo() + secretRepo.secrets["old-handle"] = &model.Secret{Handle: "old-handle", Status: model.SecretStatusActive} + secretRepo.secrets["new-handle"] = &model.Secret{Handle: "new-handle", Status: model.SecretStatusActive} + secretService := NewSecretService(secretRepo, &mockVault{}, newTestIdentityService()) + + service := NewLLMProxyService(proxyRepo, providerRepo, nil, nil, nil, nil, slog.Default(), &noopAuditRepo{}, &config.Server{}, newTestIdentityService()) + service.SetSecretService(secretService) + + request := validProxyRequest("provider-1", "project-1") + request.Provider.Auth = &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("api-key"), + Header: stringPtr("Authorization"), + Value: stringPtr(`{{ secret "new-handle" }}`), + } + + _, err := service.Update("org-1", "proxy-1", "test-user", request) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if secretRepo.secrets["old-handle"].Status != model.SecretStatusDeprecated { + t.Fatalf("expected old secret to be deprecated, got status=%v", secretRepo.secrets["old-handle"].Status) + } +} + func validProviderRequest(template string) *api.LLMProvider { return &api.LLMProvider{ Id: strPointer("provider-1"), diff --git a/platform-api/internal/service/mcp.go b/platform-api/internal/service/mcp.go index fd21df2a2f..224f1aa36f 100644 --- a/platform-api/internal/service/mcp.go +++ b/platform-api/internal/service/mcp.go @@ -175,11 +175,14 @@ func (s *MCPProxyService) Create(orgUUID, createdBy string, req *api.MCPProxy) ( return nil, constants.ErrMCPProxyLimitReached } - // Validate {{ secret "..." }} placeholders in the upstream config + // Validate {{ secret "..." }} placeholders anywhere in the request — the + // gateway-controller's template engine resolves placeholders generically + // across the whole artifact (policies included), not just upstream.auth, + // so validation must cover the same surface. if s.secretService != nil { - configJSON, err := marshalUpstreamForValidation(req.Upstream) + configJSON, err := marshalUpstreamForValidation(req) if err != nil { - return nil, fmt.Errorf("failed to marshal upstream config for secret validation: %w", err) + return nil, fmt.Errorf("failed to marshal request for secret validation: %w", err) } if err := s.secretService.ValidateSecretRefs(orgUUID, configJSON); err != nil { return nil, err @@ -357,11 +360,14 @@ func (s *MCPProxyService) Update(orgUUID, handle, updatedBy string, req *api.MCP return nil, constants.ErrMCPProxyNotFound } - // Validate {{ secret "..." }} placeholders in the upstream config + // Validate {{ secret "..." }} placeholders anywhere in the request — the + // gateway-controller's template engine resolves placeholders generically + // across the whole artifact (policies included), not just upstream.auth, + // so validation must cover the same surface. if s.secretService != nil { - configJSON, err := marshalUpstreamForValidation(req.Upstream) + configJSON, err := marshalUpstreamForValidation(req) if err != nil { - return nil, fmt.Errorf("failed to marshal upstream config for secret validation: %w", err) + return nil, fmt.Errorf("failed to marshal request for secret validation: %w", err) } if err := s.secretService.ValidateSecretRefs(orgUUID, configJSON); err != nil { return nil, err @@ -427,6 +433,19 @@ func (s *MCPProxyService) Update(orgUUID, handle, updatedBy string, req *api.MCP return nil, fmt.Errorf("failed to update MCP proxy: %w", err) } + // Best-effort: delete the secret the credential was rotated away from. Must + // run after the update above persists the new reference, so the in-use + // check below no longer sees this proxy pointing at the old handle. + if s.secretService != nil { + s.secretService.cleanupRotatedSecret( + orgUUID, + mainUpstreamAuthValue(&existingUpstreamConfig), + mainUpstreamAuthValue(&existing.Configuration.Upstream), + updatedBy, + s.slogger, + ) + } + _ = s.auditRepo.Record("UPDATE", existing.UUID, "mcp_proxy", orgUUID, updatedBy) return s.Get(orgUUID, handle) } diff --git a/platform-api/internal/service/mcp_secret_integration_test.go b/platform-api/internal/service/mcp_secret_integration_test.go new file mode 100644 index 0000000000..98fd8bd980 --- /dev/null +++ b/platform-api/internal/service/mcp_secret_integration_test.go @@ -0,0 +1,140 @@ +/* + * Copyright (c) 2026, WSO2 LLC. (http://www.wso2.org) All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +// Integration test for MCP Proxy rotation cleanup, backed by a real SQLite +// DB. No mockMCPProxyRepo exists in this package (unlike LLM Provider/Proxy), +// so this uses the real repository layer instead of hand-rolling one — +// it also exercises the real artifact_secret_refs tracking, which a mock +// cannot. + +package service + +import ( + "database/sql" + "log/slog" + "os" + "path/filepath" + "testing" + + "github.com/wso2/api-platform/platform-api/api" + "github.com/wso2/api-platform/platform-api/config" + "github.com/wso2/api-platform/platform-api/internal/database" + "github.com/wso2/api-platform/platform-api/internal/model" + "github.com/wso2/api-platform/platform-api/internal/repository" + "github.com/wso2/api-platform/platform-api/internal/utils" + "github.com/wso2/api-platform/platform-api/internal/vault" + + _ "github.com/mattn/go-sqlite3" +) + +const mcpSecretITOrgUUID = "org-mcp-secret-it" + +// TestMCPProxyServiceUpdate_CleansUpRotatedSecret_Integration proves rotating +// an MCP proxy's upstream credential deprecates the secret it replaced, +// against a real DB — the same cleanupRotatedSecret path LLM Provider, +// LLM Proxy, and REST API are also tested against. +func TestMCPProxyServiceUpdate_CleansUpRotatedSecret_Integration(t *testing.T) { + tmpDir := t.TempDir() + dbPath := filepath.Join(tmpDir, "mcp-secret-it.db") + sqlDB, err := sql.Open("sqlite3", dbPath) + if err != nil { + t.Fatalf("open db: %v", err) + } + defer sqlDB.Close() + sqlDB.Exec("PRAGMA foreign_keys = ON") + db := &database.DB{DB: sqlDB} + + schemaPath := filepath.Join("..", "database", "schema.sqlite.sql") + schema, err := os.ReadFile(schemaPath) + if err != nil { + t.Fatalf("read schema: %v", err) + } + if _, err = db.Exec(string(schema)); err != nil { + t.Fatalf("apply schema: %v", err) + } + if _, err = db.Exec(`INSERT INTO organizations (uuid, handle, display_name, region, idp_organization_ref_uuid, created_at, updated_at) + VALUES (?, 'mcp-secret-it-org', 'MCP Secret IT Org', 'default', 'idp-ref', datetime('now'), datetime('now'))`, mcpSecretITOrgUUID); err != nil { + t.Fatalf("insert org: %v", err) + } + + v, err := vault.NewInHouseVault([]byte("12345678901234567890123456789012")) + if err != nil { + t.Fatalf("create vault: %v", err) + } + identity := NewIdentityService(repository.NewUserIdentityMappingRepo(db)) + secretRepo := repository.NewSecretRepo(db) + secretSvc := NewSecretService(secretRepo, v, identity) + + createTestSecret(t, secretSvc, mcpSecretITOrgUUID, "mcp-it-secret-a", "sk-mcp-real-token-A") + createTestSecret(t, secretSvc, mcpSecretITOrgUUID, "mcp-it-secret-b", "sk-mcp-real-token-B") + + mcpRepo := repository.NewMCPProxyRepo(db) + mcpSvc := NewMCPProxyService(mcpRepo, nil, nil, nil, nil, slog.Default(), repository.NewAuditRepo(db), &config.Server{}, identity) + mcpSvc.WithSecretService(secretSvc) + + createReq := &api.MCPProxy{ + DisplayName: "IT MCP Proxy", + Version: "v1.0", + Upstream: api.Upstream{ + Main: api.UpstreamDefinition{ + Url: utils.StringPtrIfNotEmpty("https://mcp-backend.internal"), + Auth: &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("bearer"), + Header: ptr("Authorization"), + Value: ptr(`{{ secret "mcp-it-secret-a" }}`), + }, + }, + }, + } + created, err := mcpSvc.Create(mcpSecretITOrgUUID, "alice", createReq) + if err != nil { + t.Fatalf("Create failed: %v", err) + } + + rotateReq := &api.MCPProxy{ + DisplayName: "IT MCP Proxy", + Version: "v1.0", + Upstream: api.Upstream{ + Main: api.UpstreamDefinition{ + Url: utils.StringPtrIfNotEmpty("https://mcp-backend.internal"), + Auth: &api.UpstreamAuth{ + Type: upstreamAuthTypePtr("bearer"), + Header: ptr("Authorization"), + Value: ptr(`{{ secret "mcp-it-secret-b" }}`), + }, + }, + }, + } + if _, err := mcpSvc.Update(mcpSecretITOrgUUID, *created.Id, "alice", rotateReq); err != nil { + t.Fatalf("Update (rotate) failed: %v", err) + } + + secretA, err := secretSvc.Get(mcpSecretITOrgUUID, "mcp-it-secret-a") + if err != nil { + t.Fatalf("failed to fetch secret A after rotation: %v", err) + } + if secretA.Status != string(model.SecretStatusDeprecated) { + t.Errorf("expected secret A to be deprecated after rotation, got status=%q", secretA.Status) + } + secretB, err := secretSvc.Get(mcpSecretITOrgUUID, "mcp-it-secret-b") + if err != nil { + t.Fatalf("failed to fetch secret B after rotation: %v", err) + } + if secretB.Status != string(model.SecretStatusActive) { + t.Errorf("expected secret B to remain active after rotation, got status=%q", secretB.Status) + } +} diff --git a/platform-api/internal/service/secret_service.go b/platform-api/internal/service/secret_service.go index 28877d4579..4469d532eb 100644 --- a/platform-api/internal/service/secret_service.go +++ b/platform-api/internal/service/secret_service.go @@ -23,6 +23,7 @@ import ( "crypto/sha256" "errors" "fmt" + "log/slog" "time" "github.com/wso2/api-platform/platform-api/internal/apperror" @@ -211,6 +212,61 @@ func (s *SecretService) Delete(orgID, handle, updatedBy string) error { return nil } +// extractSecretHandle returns the handle embedded in a {{ secret "handle" }} +// placeholder, or "" if value is empty, plaintext, or otherwise not a placeholder. +func extractSecretHandle(value string) string { + m := constants.SecretPlaceholderRe.FindStringSubmatch(value) + if len(m) < 2 { + return "" + } + return m[1] +} + +// mainUpstreamAuthValue nil-safely reads upstream.main.auth.value from an +// UpstreamConfig, returning "" when any part of the chain is nil. +func mainUpstreamAuthValue(cfg *model.UpstreamConfig) string { + if cfg == nil || cfg.Main == nil || cfg.Main.Auth == nil { + return "" + } + return cfg.Main.Auth.Value +} + +// sandboxUpstreamAuthValue nil-safely reads upstream.sandbox.auth.value from +// an UpstreamConfig, returning "" when any part of the chain is nil. +func sandboxUpstreamAuthValue(cfg *model.UpstreamConfig) string { + if cfg == nil || cfg.Sandbox == nil || cfg.Sandbox.Auth == nil { + return "" + } + return cfg.Sandbox.Auth.Value +} + +// upstreamAuthValue nil-safely reads .Value from an UpstreamAuth. +func upstreamAuthValue(auth *model.UpstreamAuth) string { + if auth == nil { + return "" + } + return auth.Value +} + +// cleanupRotatedSecret best-effort deletes the secret previously referenced by +// oldValue when it has been rotated to a different handle in newValue. Both +// values are expected to be {{ secret "handle" }} placeholders (or empty/ +// plaintext, in which case nothing is deleted). Must be called only after the +// resource's own config has been persisted with newValue, so the old handle +// is no longer referenced by this resource by the time the in-use check runs. +func (s *SecretService) cleanupRotatedSecret(orgUUID, oldValue, newValue, updatedBy string, logger *slog.Logger) { + oldHandle := extractSecretHandle(oldValue) + if oldHandle == "" { + return + } + if oldHandle == extractSecretHandle(newValue) { + return + } + if err := s.Delete(orgUUID, oldHandle, updatedBy); err != nil && logger != nil { + logger.Warn("could not delete rotated-out secret", "handle", oldHandle, "err", err) + } +} + // ValidateSecretRefs checks that every {{ secret "handle" }} placeholder in configText // resolves to an active org-scoped secret. func (s *SecretService) ValidateSecretRefs(orgID, configText string) error { diff --git a/platform-api/internal/service/secret_service_test.go b/platform-api/internal/service/secret_service_test.go index 4989cbdf7a..a62720abe6 100644 --- a/platform-api/internal/service/secret_service_test.go +++ b/platform-api/internal/service/secret_service_test.go @@ -557,3 +557,28 @@ func TestSecretService_Decrypt_DeprecatedSecret_ReturnsError(t *testing.T) { t.Fatal("expected error decrypting deprecated secret") } } + +// TestCleanupRotatedSecret_NilLogger_DoesNotPanicOnDeleteFailure proves +// cleanupRotatedSecret treats logging as best-effort: a nil *slog.Logger must +// not crash the caller when the underlying Delete fails (e.g. the old secret +// is still referenced elsewhere and Delete returns a SecretInUseError). +func TestCleanupRotatedSecret_NilLogger_DoesNotPanicOnDeleteFailure(t *testing.T) { + repo := newMockRepo() + repo.secrets["old-handle"] = &model.Secret{Handle: "old-handle", Status: model.SecretStatusActive} + repo.findRefsFn = func(orgID, handle string) ([]model.SecretReference, error) { + return []model.SecretReference{{Type: "llm_provider", Handle: "still-using-it"}}, nil + } + + svc := NewSecretService(repo, &mockVault{}, newTestIdentityService()) + + defer func() { + if r := recover(); r != nil { + t.Fatalf("cleanupRotatedSecret panicked with nil logger: %v", r) + } + }() + svc.cleanupRotatedSecret("org1", `{{ secret "old-handle" }}`, `{{ secret "new-handle" }}`, "alice", nil) + + if got := repo.secrets["old-handle"].Status; got != model.SecretStatusActive { + t.Errorf("expected still-referenced secret to remain ACTIVE (Delete should have failed), got %q", got) + } +} diff --git a/platform-api/internal/utils/api.go b/platform-api/internal/utils/api.go index 60f0b89452..3c14ce1982 100644 --- a/platform-api/internal/utils/api.go +++ b/platform-api/internal/utils/api.go @@ -461,7 +461,9 @@ func (u *APIUtil) upstreamAuthToAPI(auth *model.UpstreamAuth) *api.UpstreamAuth apiAuth.Type = &value } apiAuth.Header = StringPtrIfNotEmpty(auth.Header) - apiAuth.Value = StringPtrIfNotEmpty(auth.Value) + // Redact value — never expose the auth credential in a read response, + // matching the LLM Provider/Proxy and MCP Proxy mappers. + apiAuth.Value = nil return apiAuth } diff --git a/platform-api/internal/utils/api_test.go b/platform-api/internal/utils/api_test.go index 9c26b7fd7f..ef9cbd5f42 100644 --- a/platform-api/internal/utils/api_test.go +++ b/platform-api/internal/utils/api_test.go @@ -654,3 +654,39 @@ func TestBuildAPIDeploymentYAML(t *testing.T) { } } +// TestUpstreamConfigModelToAPI_RedactsAuthValue proves ModelToRESTAPI never +// exposes the real upstream auth credential in a read response — matching the +// LLM Provider/Proxy and MCP Proxy mappers' redaction behaviour. +func TestUpstreamConfigModelToAPI_RedactsAuthValue(t *testing.T) { + util := &APIUtil{} + + cfg := &model.UpstreamConfig{ + Main: &model.UpstreamEndpoint{ + URL: "https://backend.internal/api", + Auth: &model.UpstreamAuth{Type: "bearer", Header: "Authorization", Value: "super-secret-token"}, + }, + Sandbox: &model.UpstreamEndpoint{ + URL: "https://sandbox.internal/api", + Auth: &model.UpstreamAuth{Type: "bearer", Header: "Authorization", Value: "another-secret-token"}, + }, + } + + result := util.UpstreamConfigModelToAPI(cfg) + + if result.Main.Auth == nil { + t.Fatal("expected main auth block to be present (structure retained)") + } + if result.Main.Auth.Value != nil { + t.Errorf("expected main auth value to be redacted (nil), got %q", *result.Main.Auth.Value) + } + if result.Main.Auth.Header == nil || *result.Main.Auth.Header != "Authorization" { + t.Errorf("expected main auth header to be preserved, got %v", result.Main.Auth.Header) + } + if result.Sandbox == nil || result.Sandbox.Auth == nil { + t.Fatal("expected sandbox auth block to be present (structure retained)") + } + if result.Sandbox.Auth.Value != nil { + t.Errorf("expected sandbox auth value to be redacted (nil), got %q", *result.Sandbox.Auth.Value) + } +} + diff --git a/portals/ai-workspace/cypress/e2e/001-providers/003-llm-proxy-secret-management.cy.js b/portals/ai-workspace/cypress/e2e/001-providers/003-llm-proxy-secret-management.cy.js new file mode 100644 index 0000000000..0ebc799a54 --- /dev/null +++ b/portals/ai-workspace/cypress/e2e/001-providers/003-llm-proxy-secret-management.cy.js @@ -0,0 +1,509 @@ +/* + * Copyright (c) 2026, WSO2 LLC. (https://www.wso2.com). + * + * WSO2 LLC. licenses this file to you under the Apache License, + * Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Secret management behaviour for App LLM Proxy create and update flows. + * + * A proxy stores its own credential to call the underlying LLM provider in + * `provider.auth` (separate from the provider's own `upstream.main.auth`). + * Like providers, this value must never be persisted in plaintext. + * + * Create flow (TC-1 – TC-3): + * TC-1 Create proxy with plaintext API key → POST /secrets called, placeholder + * stored in provider.auth.value, plaintext absent from page + * TC-2 Re-save proxy already holding a placeholder → POST /secrets NOT called + * TC-3 POST /secrets 500 → proxy creation aborted, no proxy created + * + * Update flow via Provider tab (TC-4 – TC-6): + * The Provider tab's "Save API Key" button only stages the new value locally + * (setLocalProxy) — the page-level "Save" button is what actually persists it + * via PUT /llm-proxies/{id} and triggers secret rotation. + * TC-4 Edit API key with a new plaintext value → POST /secrets called, PUT + * body holds placeholder, old secret DELETEd afterward, plaintext absent + * from page + * TC-5 Edit API key by typing an explicit placeholder → POST /secrets NOT + * called, PUT fires with the placeholder as-is + * TC-6 POST /secrets 500 during edit → PUT /llm-proxies NOT called, error shown + */ + +// --------------------------------------------------------------------------- +// Shared helpers +// --------------------------------------------------------------------------- + +function toSlug(value) { + return value + .toLowerCase() + .trim() + .replace(/[^a-z0-9]+/g, '-') + .replace(/^-+|-+$/g, ''); +} + +function loginAndFetchAuthContext(setAuthToken, setOrganizationId) { + cy.login(); + cy.request({ + method: 'POST', + url: '/api/login', + body: { + username: Cypress.env('ADMIN_USER'), + password: Cypress.env('ADMIN_PASSWORD'), + }, + }) + .then((r) => { + setAuthToken(r.body.accessToken); + return cy.request({ + url: '/api/proxy/api/v0.9/organizations', + headers: { Authorization: `Bearer ${r.body.accessToken}` }, + }); + }) + .then((r) => { + setOrganizationId(r.body?.list?.[0]?.id ?? ''); + }); +} + +function createProjectViaUI(projectName) { + cy.intercept('POST', '**/projects').as('createProject'); + cy.contains('Projects', { timeout: 30000 }).should('be.visible').click(); + cy.contains('button, a', /Create Project|Add New Project/, { timeout: 30000 }) + .should('be.visible') + .click(); + cy.get('input[placeholder="My AI Project"]', { timeout: 30000 }) + .should('be.visible') + .type(projectName); + cy.get('textarea[placeholder="Short description of the project."]').type( + 'Cypress LLM proxy secret management project' + ); + cy.contains('button', 'Create').should('not.be.disabled').click(); + cy.wait('@createProject').its('response.statusCode').should('be.oneOf', [200, 201]); +} + +function createProviderViaUI(providerName) { + cy.intercept('POST', /\/llm-providers(\?|$)/).as('createProviderForProxy'); + cy.get('[data-cyid="nav-service-provider"]', { timeout: 30000 }).should('be.visible').click(); + cy.get('[data-cyid="add-new-provider-button"]', { timeout: 30000 }).should('be.visible').click(); + cy.get('[data-cyid="provider-template-openai-card"]', { timeout: 30000 }).should('be.visible').click(); + cy.get('[data-cyid="provider-name-input"] input:visible', { timeout: 30000 }) + .should('be.visible') + .clear() + .type(providerName); + cy.get('[data-cyid="provider-api-key-input"] input:visible').type('sk-provider-backing-key'); + cy.get('[data-cyid="add-provider-button"]').should('not.be.disabled').click(); + return cy.wait('@createProviderForProxy', { timeout: 20000 }).then((pi) => pi.response.body?.id ?? ''); +} + +function navigateToCreateProxy(projectName) { + cy.contains('button', 'Create App LLM Proxy', { timeout: 30000 }).should('be.visible').click(); + cy.contains('label', 'Projects', { timeout: 30000 }).parent().find('[role="combobox"]').click(); + cy.contains('[role="option"]', projectName, { timeout: 15000 }).click(); + cy.contains('button', 'Continue').should('not.be.disabled').click(); + cy.contains('Create App LLM Proxy', { timeout: 30000 }).should('be.visible'); +} + +// --------------------------------------------------------------------------- +// CREATE flow +// --------------------------------------------------------------------------- + +describe('AI Workspace — LLM proxy secret management (create flow)', () => { + const suffix = Date.now().toString().slice(-8); + const orgHandle = Cypress.env('ORG_HANDLE'); + const projectName = `E2E Proxy Secret Project ${suffix}`; + const providerName = `E2E Proxy Secret Provider ${suffix}`; + const proxyName = `E2E Proxy Secret Proxy ${suffix}`; + + let authToken = ''; + let organizationId = ''; + let createdProviderId = ''; + let createdProxyId = ''; + + beforeEach(() => { + createdProxyId = ''; + loginAndFetchAuthContext((v) => { authToken = v; }, (v) => { organizationId = v; }); + + createProjectViaUI(projectName); + createProviderViaUI(providerName).then((id) => { + createdProviderId = id; + }); + navigateToCreateProxy(projectName); + }); + + afterEach(() => { + if (authToken && organizationId && createdProxyId) { + cy.request({ + method: 'DELETE', + url: `/api/proxy/api/v0.9/llm-proxies/${encodeURIComponent(createdProxyId)}?organizationId=${encodeURIComponent(organizationId)}`, + headers: { Authorization: `Bearer ${authToken}` }, + failOnStatusCode: false, + }); + } + if (authToken && organizationId && createdProviderId) { + cy.request({ + method: 'DELETE', + url: `/api/proxy/api/v0.9/llm-providers/${encodeURIComponent(createdProviderId)}?organizationId=${encodeURIComponent(organizationId)}`, + headers: { Authorization: `Bearer ${authToken}` }, + failOnStatusCode: false, + }); + } + if (authToken) { + cy.request({ + url: '/api/proxy/api/v0.9/projects', + headers: { Authorization: `Bearer ${authToken}` }, + failOnStatusCode: false, + }).then((r) => { + const proj = (r.body?.list ?? []).find((p) => p.displayName === projectName); + if (proj?.id) { + cy.request({ + method: 'DELETE', + url: `/api/proxy/api/v0.9/projects/${encodeURIComponent(proj.id)}`, + headers: { Authorization: `Bearer ${authToken}` }, + failOnStatusCode: false, + }); + } + }); + } + }); + + // ------------------------------------------------------------------------- + // TC-1: Create proxy with plaintext key → secret auto-created, placeholder stored + // ------------------------------------------------------------------------- + it('TC-1: creates a secret and stores the placeholder in the proxy provider config', () => { + cy.intercept('POST', '**/secrets').as('createSecret'); + cy.intercept('POST', /\/llm-proxies(\?|$)/).as('createProxy'); + + cy.get('input[placeholder="WSO2 OpenAI Provider Proxy"]', { timeout: 30000 }) + .should('be.visible') + .type(proxyName); + cy.get('textarea[placeholder="Primary OpenAI provider"]').type('Cypress proxy secret create test'); + cy.get('input[placeholder="Enter API key"]').type('sk-tc1-proxy-plaintext-key'); + cy.contains('button', 'Create Proxy', { timeout: 30000 }).should('not.be.disabled').click(); + + cy.wait('@createSecret').then((interception) => { + expect(interception.response.statusCode).to.be.oneOf([200, 201]); + const secretId = interception.response.body?.id; + expect(secretId).to.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/); + expect(JSON.stringify(interception.response.body), 'plaintext not in secret response') + .not.to.include('sk-tc1-proxy-plaintext-key'); + }); + + cy.wait('@createProxy').then((interception) => { + expect(interception.response.statusCode).to.be.oneOf([200, 201]); + const authValue = interception.request.body?.provider?.auth?.value ?? ''; + expect(authValue, 'proxy payload has placeholder').to.include('{{ secret "'); + expect(authValue, 'proxy payload does NOT have plaintext key').not.to.include('sk-tc1-proxy-plaintext-key'); + createdProxyId = interception.response.body?.id ?? ''; + }); + + cy.location('pathname', { timeout: 30000 }).should('match', /\/proxies\/[^/]+$/); + + cy.get('body').invoke('text').then((text) => { + expect(text, 'plaintext key absent from proxy detail page').not.to.include('sk-tc1-proxy-plaintext-key'); + }); + }); + + // ------------------------------------------------------------------------- + // TC-2: Re-save proxy already using a placeholder → POST /secrets NOT called + // ------------------------------------------------------------------------- + it('TC-2: does not create a duplicate secret when the API key is already a placeholder', () => { + const existingHandle = `${toSlug(proxyName)}-provider-api-key`; + cy.request({ + method: 'POST', + url: '/api/proxy/api/v0.9/secrets', + headers: { Authorization: `Bearer ${authToken}` }, + form: true, + body: { + id: existingHandle, + displayName: `${proxyName} Provider API Key`, + value: 'sk-tc2-pre-existing-value', + type: 'GENERIC', + }, + failOnStatusCode: false, + }).then((r) => { + expect(r.status).to.be.oneOf([200, 201, 409]); + }); + + let secretCallCount = 0; + cy.intercept('POST', '**/secrets', (req) => { + secretCallCount += 1; + req.continue(); + }); + cy.intercept('POST', /\/llm-proxies(\?|$)/).as('createProxy'); + + cy.get('input[placeholder="WSO2 OpenAI Provider Proxy"]', { timeout: 30000 }) + .should('be.visible') + .type(proxyName); + cy.get('textarea[placeholder="Primary OpenAI provider"]').type('Cypress proxy secret dedup test'); + cy.get('input[placeholder="Enter API key"]').type( + `{{ secret "${existingHandle}" }}`, + { parseSpecialCharSequences: false } + ); + cy.contains('button', 'Create Proxy', { timeout: 30000 }).should('not.be.disabled').click(); + + cy.wait('@createProxy').then((interception) => { + expect(interception.response.statusCode).to.be.oneOf([200, 201]); + createdProxyId = interception.response.body?.id ?? ''; + expect(secretCallCount).to.equal(0); + }); + }); + + // ------------------------------------------------------------------------- + // TC-3: POST /secrets 500 → proxy creation is aborted (no proxy created) + // ------------------------------------------------------------------------- + it('TC-3: aborts proxy creation when POST /secrets returns 500', () => { + cy.intercept('POST', '**/secrets', { + statusCode: 500, + body: { error: 'simulated vault failure' }, + }).as('failSecret'); + + let proxyCallCount = 0; + cy.intercept('POST', /\/llm-proxies(\?|$)/, (req) => { + proxyCallCount += 1; + req.continue(); + }); + + cy.get('input[placeholder="WSO2 OpenAI Provider Proxy"]', { timeout: 30000 }) + .should('be.visible') + .type(proxyName); + cy.get('textarea[placeholder="Primary OpenAI provider"]').type('Cypress proxy secret abort test'); + cy.get('input[placeholder="Enter API key"]').type('sk-tc3-will-fail'); + cy.contains('button', 'Create Proxy', { timeout: 30000 }).should('not.be.disabled').click(); + + cy.wait('@failSecret'); + + cy.get('[data-testid="aiworkspace-snackbar-notification"]', { timeout: 15000 }) + .should('be.visible'); + + cy.wrap(null).then(() => { + expect(proxyCallCount).to.equal(0); + }); + }); +}); + +// --------------------------------------------------------------------------- +// UPDATE flow (Provider tab: stage via "Save API Key", persist via page Save) +// --------------------------------------------------------------------------- + +describe('AI Workspace — LLM proxy secret management (update flow)', () => { + const suffix = Date.now().toString().slice(-8); + const orgHandle = Cypress.env('ORG_HANDLE'); + const projectName = `E2E Proxy Update Project ${suffix}`; + const providerName = `E2E Proxy Update Provider ${suffix}`; + const proxyName = `E2E Proxy Update Proxy ${suffix}`; + const INITIAL_KEY = `sk-proxy-update-initial-${suffix}`; + + let authToken = ''; + let organizationId = ''; + let createdProviderId = ''; + let proxyId = ''; + let initialSecretHandle = ''; + + // Create a fresh project + provider + proxy for each test, land on the + // proxy's Provider tab. + beforeEach(() => { + loginAndFetchAuthContext((v) => { authToken = v; }, (v) => { organizationId = v; }); + + createProjectViaUI(projectName); + createProviderViaUI(providerName).then((id) => { + createdProviderId = id; + }); + navigateToCreateProxy(projectName); + + cy.intercept('POST', '**/secrets').as('setupSecret'); + cy.intercept('POST', /\/llm-proxies(\?|$)/).as('setupProxy'); + cy.get('input[placeholder="WSO2 OpenAI Provider Proxy"]', { timeout: 30000 }) + .should('be.visible') + .type(proxyName); + cy.get('textarea[placeholder="Primary OpenAI provider"]').type('Cypress proxy update test'); + cy.get('input[placeholder="Enter API key"]').type(INITIAL_KEY); + cy.contains('button', 'Create Proxy', { timeout: 30000 }).should('not.be.disabled').click(); + + cy.wait('@setupSecret', { timeout: 20000 }).then((si) => { + initialSecretHandle = si.response.body?.id ?? ''; + }); + cy.wait('@setupProxy', { timeout: 20000 }).then((pi) => { + proxyId = pi.response.body?.id ?? ''; + }); + + cy.location('pathname', { timeout: 30000 }).should('match', /\/proxies\/[^/]+$/); + cy.contains('[role="tab"]', 'Provider', { timeout: 15000 }).click(); + cy.contains('label', 'API Key', { timeout: 15000 }).should('be.visible'); + }); + + afterEach(() => { + if (authToken && organizationId && proxyId) { + cy.request({ + method: 'DELETE', + url: `/api/proxy/api/v0.9/llm-proxies/${encodeURIComponent(proxyId)}?organizationId=${encodeURIComponent(organizationId)}`, + headers: { Authorization: `Bearer ${authToken}` }, + failOnStatusCode: false, + }); + proxyId = ''; + } + if (authToken && organizationId && createdProviderId) { + cy.request({ + method: 'DELETE', + url: `/api/proxy/api/v0.9/llm-providers/${encodeURIComponent(createdProviderId)}?organizationId=${encodeURIComponent(organizationId)}`, + headers: { Authorization: `Bearer ${authToken}` }, + failOnStatusCode: false, + }); + } + if (authToken) { + cy.request({ + url: '/api/proxy/api/v0.9/projects', + headers: { Authorization: `Bearer ${authToken}` }, + failOnStatusCode: false, + }).then((r) => { + const proj = (r.body?.list ?? []).find((p) => p.displayName === projectName); + if (proj?.id) { + cy.request({ + method: 'DELETE', + url: `/api/proxy/api/v0.9/projects/${encodeURIComponent(proj.id)}`, + headers: { Authorization: `Bearer ${authToken}` }, + failOnStatusCode: false, + }); + } + }); + } + }); + + // ------------------------------------------------------------------------- + // TC-4: Edit API key with new plaintext → secret created, PUT carries + // placeholder, old secret cleaned up after the update succeeds. + // + // Cleanup happens server-side (platform-api's LLMProxyService.Update calls + // SecretService.Delete directly once the new reference is persisted) — not + // as a separate outgoing DELETE the browser makes, so there is no network + // call to intercept here. Verify it via a follow-up GET on the old handle. + // ------------------------------------------------------------------------- + it('TC-4: editing the API key rotates the secret, stores the placeholder, and cleans up the old secret', () => { + const UPDATED_KEY = `sk-proxy-update-new-${suffix}`; + + cy.intercept('POST', '**/secrets').as('createSecret'); + cy.intercept('PUT', /\/llm-proxies\/[^/?]+(\?|$)/).as('updateProxy'); + + // Stage the new key — "Save API Key" only updates local state. + cy.get('input[placeholder="Enter API key"]').type(UPDATED_KEY); + cy.contains('button', 'Save API Key').should('not.be.disabled').click(); + + // Persist — page-level Save actually fires the update + secret rotation. + cy.contains('button', /^Save$/).should('not.be.disabled').click(); + + cy.wait('@createSecret', { timeout: 20000 }).then((si) => { + expect(si.response.statusCode, 'POST /secrets status').to.be.oneOf([200, 201]); + expect(JSON.stringify(si.response.body), 'plaintext not in secret response').not.to.include(UPDATED_KEY); + }); + + cy.wait('@updateProxy', { timeout: 20000 }).then((pi) => { + expect(pi.response.statusCode, 'PUT /llm-proxies status').to.be.oneOf([200, 201]); + const authValue = pi.request.body?.provider?.auth?.value ?? ''; + expect(authValue, 'PUT body has placeholder').to.include('{{ secret "'); + expect(authValue, 'PUT body does NOT have plaintext key').not.to.include(UPDATED_KEY); + }); + + cy.get('body').invoke('text').then((text) => { + expect(text, 'plaintext absent from page').not.to.include(UPDATED_KEY); + }); + + // The old secret backing INITIAL_KEY must be soft-deleted server-side — + // GetByHandle doesn't filter by status, so it still 200s but flips to + // DEPRECATED. This closes the gap left untested on the LLM Provider side. + cy.wrap(null).then(() => { + expect(initialSecretHandle, 'captured the initial secret handle in beforeEach').to.not.equal(''); + cy.request({ + url: `/api/proxy/api/v0.9/secrets/${encodeURIComponent(initialSecretHandle)}?organizationId=${encodeURIComponent(organizationId)}`, + headers: { Authorization: `Bearer ${authToken}` }, + failOnStatusCode: false, + }).then((r) => { + expect(r.status, 'old secret still resolvable').to.eq(200); + expect(r.body?.status, 'old secret marked deprecated after cleanup').to.eq('DEPRECATED'); + }); + }); + }); + + // ------------------------------------------------------------------------- + // TC-5: Edit API key by typing an explicit placeholder → POST /secrets NOT called + // ------------------------------------------------------------------------- + it('TC-5: typing a placeholder value skips secret creation and sends the placeholder directly', () => { + const explicitHandle = `${toSlug(proxyName)}-provider-api-key`; + + cy.request({ + method: 'POST', + url: '/api/proxy/api/v0.9/secrets', + headers: { Authorization: `Bearer ${authToken}` }, + form: true, + body: { + id: explicitHandle, + displayName: `${proxyName} Provider API Key`, + value: 'sk-tc5-explicit-handle-value', + type: 'GENERIC', + }, + failOnStatusCode: false, + }).then((r) => { + expect(r.status).to.be.oneOf([200, 201, 409]); + }); + + let secretCallCount = 0; + cy.intercept('POST', '**/secrets', (req) => { + secretCallCount += 1; + req.continue(); + }); + cy.intercept('PUT', /\/llm-proxies\/[^/?]+(\?|$)/).as('updateProxy'); + + cy.get('input[placeholder="Enter API key"]').type( + `{{ secret "${explicitHandle}" }}`, + { parseSpecialCharSequences: false } + ); + cy.contains('button', 'Save API Key').should('not.be.disabled').click(); + cy.contains('button', /^Save$/).should('not.be.disabled').click(); + + cy.wait('@updateProxy', { timeout: 20000 }).then((pi) => { + expect(pi.response.statusCode, 'PUT /llm-proxies status').to.be.oneOf([200, 201]); + const authValue = pi.request.body?.provider?.auth?.value ?? ''; + expect(authValue, 'PUT body carries the typed placeholder').to.include('{{ secret "'); + cy.wrap(null).then(() => { + expect(secretCallCount, 'POST /secrets not called').to.equal(0); + }); + }); + }); + + // ------------------------------------------------------------------------- + // TC-6: POST /secrets 500 during edit → PUT /llm-proxies NOT called, error shown + // ------------------------------------------------------------------------- + it('TC-6: aborts the API key update when POST /secrets returns 500', () => { + cy.intercept('POST', '**/secrets', { + statusCode: 500, + body: { error: 'simulated vault failure' }, + }).as('failSecret'); + + let proxyCallCount = 0; + cy.intercept('PUT', /\/llm-proxies\/[^/?]+(\?|$)/, (req) => { + proxyCallCount += 1; + req.continue(); + }); + + cy.get('input[placeholder="Enter API key"]').type('sk-tc6-will-fail'); + cy.contains('button', 'Save API Key').should('not.be.disabled').click(); + cy.contains('button', /^Save$/).should('not.be.disabled').click(); + + cy.wait('@failSecret'); + + cy.get('[data-testid="aiworkspace-snackbar-notification"]', { timeout: 15000 }) + .should('be.visible'); + + cy.wrap(null).then(() => { + expect(proxyCallCount, 'PUT /llm-proxies not called').to.equal(0); + }); + }); +}); diff --git a/portals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsx b/portals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsx index 200a5ede81..7ee226a06b 100644 --- a/portals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsx +++ b/portals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsx @@ -26,10 +26,8 @@ import type { import * as llmProviderApis from '../../apis/llmProviderApis'; import { createSecret, - deleteSecret, buildSecretPlaceholder, generateSecretHandle, - extractSecretHandle, } from '../../apis/secretApis'; import { useAppShell } from '../AppShellContext'; import { PLATFORM_API_BASE_URL } from '../../config.env'; @@ -123,8 +121,10 @@ export function LLMProviderProvider({ children, providerId }: LLMProviderProvide try { // If the upstream auth value is a new plain-text credential (not already a // placeholder), create a new secret and substitute the placeholder before - // persisting. After a successful provider update the old secret is deleted - // best-effort so the gateway is not left with a dangling reference. + // persisting. Cleanup of the secret being rotated away from happens + // server-side (platform-api), since auth.value is writeOnly and never + // comes back on a GET — this context's own state can never reliably + // hold the true prior value to delete. let updatesPayload = updates; const authValue = updates.upstream?.main?.auth?.value; const isAlreadyPlaceholder = @@ -160,18 +160,6 @@ export function LLMProviderProvider({ children, providerId }: LLMProviderProvide const updatedProvider = await llmProviderApis.updateLLMProvider(providerId, updatesPayload, organizationId, PLATFORM_API_BASE_URL); setProvider(updatedProvider); - // Best-effort: delete the old secret only after the provider update succeeds. - if (authValue && !isAlreadyPlaceholder) { - const oldHandle = provider?.upstream?.main?.auth?.value - ? extractSecretHandle(provider.upstream.main.auth.value) - : null; - if (oldHandle) { - deleteSecret(oldHandle).catch((err) => { - logger.warn('Could not delete old secret after provider update', { oldHandle, err }); - }); - } - } - return updatedProvider; } catch (err) { logger.error('Failed to update LLM provider:', err); diff --git a/portals/ai-workspace/src/contexts/proxy/ProxyContext.tsx b/portals/ai-workspace/src/contexts/proxy/ProxyContext.tsx index 243973095d..a50341d110 100644 --- a/portals/ai-workspace/src/contexts/proxy/ProxyContext.tsx +++ b/portals/ai-workspace/src/contexts/proxy/ProxyContext.tsx @@ -28,10 +28,8 @@ import * as proxyApis from '../../apis/proxyApis'; import * as llmProxiesApis from '../../apis/llmProxiesApis'; import { createSecret, - deleteSecret, buildSecretPlaceholder, generateSecretHandle, - extractSecretHandle, } from '../../apis/secretApis'; import { useAppShell } from '../AppShellContext'; import { PLATFORM_API_BASE_URL } from '../../config.env'; @@ -129,8 +127,10 @@ export function ProxyProvider({ children, proxyId }: ProxyProviderProps) { try { // If the provider auth value is a new plain-text credential (not already a // placeholder), create a new secret and substitute the placeholder before - // persisting. After a successful proxy update the old secret is deleted - // best-effort so the gateway is not left with a dangling reference. + // persisting. Cleanup of the secret being rotated away from happens + // server-side (platform-api), since auth.value is writeOnly and never + // comes back on a GET — this context's own state can never reliably + // hold the true prior value to delete. let updatesPayload = updates; const providerAuth = typeof updates.provider === 'object' ? updates.provider?.auth : undefined; const authValue = providerAuth?.value; @@ -163,23 +163,12 @@ export function ProxyProvider({ children, proxyId }: ProxyProviderProps) { const updatedProxy = await proxyApis.updateProxy(proxyId, updatesPayload, organizationId, apimBaseUrl); setProxy(updatedProxy); - // Best-effort: delete the old secret only after the proxy update succeeds. - if (authValue && !isAlreadyPlaceholder) { - const existingAuth = typeof proxy?.provider === 'object' ? proxy?.provider?.auth : undefined; - const oldHandle = existingAuth?.value ? extractSecretHandle(existingAuth.value) : null; - if (oldHandle) { - deleteSecret(oldHandle).catch((err) => { - logger.warn('Could not delete old secret after proxy update', { oldHandle, err }); - }); - } - } - return updatedProxy; } catch (err) { logger.error('Failed to update proxy:', err); throw err; } - }, [proxyId, organizationId, apimBaseUrl, proxy]); + }, [proxyId, organizationId, apimBaseUrl]); const deleteProxy = useCallback(async (): Promise => { if (!proxyId || !organizationId) { diff --git a/portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyNew.tsx b/portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyNew.tsx index 9c28c019f3..7ff509dfc3 100644 --- a/portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyNew.tsx +++ b/portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyNew.tsx @@ -355,22 +355,29 @@ function LLMProxyNewContent({ let providerAuthValue = providerDetail?.upstream?.main?.auth?.value ?? ''; if (selectedProviderRequiresApiKey) { const rawKey = manualApiKeyValue.trim() || selectedProviderApiKeyValue || ''; - const secretHandle = generateSecretHandle(); - const secretResponse = await createSecret({ - id: secretHandle, - displayName: `${generatedId} Provider API Key`, - description: `Auto-generated secret for LLM proxy ${generatedId}`, - value: rawKey, - type: 'GENERIC', - }); - logger.info('Created secret for LLM proxy provider auth', { - secretHandle: secretResponse.id, - proxyId: generatedId, - }); - createdSecretHandle = secretResponse.id; - providerAuthType = 'api-key'; - providerAuthHeader = selectedProviderApiKeyName; - providerAuthValue = buildSecretPlaceholder(secretResponse.id); + const isAlreadyPlaceholder = rawKey.includes('{{ secret '); + if (isAlreadyPlaceholder) { + providerAuthType = 'api-key'; + providerAuthHeader = selectedProviderApiKeyName; + providerAuthValue = rawKey; + } else { + const secretHandle = generateSecretHandle(); + const secretResponse = await createSecret({ + id: secretHandle, + displayName: `${generatedId} Provider API Key`, + description: `Auto-generated secret for LLM proxy ${generatedId}`, + value: rawKey, + type: 'GENERIC', + }); + logger.info('Created secret for LLM proxy provider auth', { + secretHandle: secretResponse.id, + proxyId: generatedId, + }); + createdSecretHandle = secretResponse.id; + providerAuthType = 'api-key'; + providerAuthHeader = selectedProviderApiKeyName; + providerAuthValue = buildSecretPlaceholder(secretResponse.id); + } } const payload: CreateProxyRequest = { diff --git a/portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyOverview.tsx b/portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyOverview.tsx index 21c8e16e47..09d88c6e5f 100644 --- a/portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyOverview.tsx +++ b/portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyOverview.tsx @@ -234,9 +234,9 @@ function ProxyOverviewContent() { setSavedProxy(updatedProxy); showSnackbar('Proxy updated successfully.', 'success'); } catch (err) { - setUpdateError( - err instanceof Error ? err.message : 'Failed to update proxy' - ); + const message = err instanceof Error ? err.message : 'Failed to update proxy'; + setUpdateError(message); + showSnackbar(message, 'error'); } finally { setIsSavingChanges(false); }