Add secret management improvements and add secret management support for REST APIs#2526
Conversation
|
Naduni Pamudika seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds backend validation for missing secret references, server-side cleanup for rotated secrets, upstream auth redaction in API responses, and frontend updates that stop client-side secret deletion, reuse existing placeholders in proxy creation, and add end-to-end coverage for the new flows. ChangesBackend secret validation, cleanup, and redaction
Frontend secret handling and proxy UX
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant UI
participant APIService
participant SecretService
participant APIRepository
UI->>APIService: UpdateAPI(request)
APIService->>SecretService: ValidateSecretRefs(request)
SecretService-->>APIService: ok or ErrSecretRefMissing
APIService->>APIRepository: UpdateAPI(updated model)
APIService->>SecretService: cleanupRotatedSecret(oldAuth, newAuth)
APIService-->>UI: updated API response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a8475c4 to
faac78b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
portals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsx (1)
133-158: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNewly created secret is orphaned if the provider update fails.
If
createSecretsucceeds butupdateLLMProvider(Line 160) then throws, the just-created secret is persisted in the vault but never referenced by any provider, leaking an orphaned secret.LLMProxyNew.tsxhandles this exact case with a best-effort compensating delete; consider mirroring that here for consistency. Note this is distinct from the rotated-away secret cleanup, which is correctly deferred server-side.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsx` around lines 133 - 158, The secret creation path in LLMProviderContext’s update flow can leave an orphaned secret if the subsequent provider update fails. Add best-effort compensating cleanup around the createSecret/updateLLMProvider sequence in LLMProviderContext so that a newly created secret is deleted if updateLLMProvider throws, mirroring the existing handling in LLMProxyNew.tsx; keep the rotated-away secret cleanup unchanged since that is handled server-side.platform-api/internal/service/llm_test.go (1)
1539-1592: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider also asserting
new-handlestays Active.The API-layer equivalent (
TestAPIServiceSecretLifecycle_Integration) checks both the deprecated old secret and that the new one remains active. This test only checks the old secret's status, so a regression that also deprecates/deactivates the newly-referenced secret wouldn't be caught here.♻️ Proposed addition
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.Errorf("expected new secret to remain active, got status=%v", secretRepo.secrets["new-handle"].Status) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/service/llm_test.go` around lines 1539 - 1592, The LLM proxy update cleanup test only verifies that the rotated secret is deprecated, so it can miss a regression where the newly referenced secret is also deactivated. Update TestLLMProxyServiceUpdate_CleansUpRotatedSecret in llm_test.go to also assert that the secret referenced by the new UpstreamAuth value ("new-handle") remains in SecretStatusActive after calling LLMProxyService.Update, alongside the existing check for the old secret becoming deprecated.platform-api/internal/service/llm.go (1)
877-889: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated secret-validation block and rename the misleading helper.
This exact
nil-guard → marshal → ValidateSecretRefsblock (including the copy-pasted"failed to marshal request for secret validation"message) now repeats in provider Create/Update, proxy Create/Update, and MCP proxy Create/Update.marshalUpstreamForValidationalso no longer marshals only the upstream — it takes the whole request asinterface{}, so its name is now misleading.Consider a single
SecretServicemethod that both marshals and validates, replacing all six sites and droppingmarshalUpstreamForValidation:♻️ Proposed consolidation
Add to
secret_service.go:// ValidateRequestSecretRefs marshals req and validates that every // {{ secret "..." }} placeholder references an existing secret. func (s *SecretService) ValidateRequestSecretRefs(orgID string, req interface{}) error { b, err := json.Marshal(req) if err != nil { return fmt.Errorf("failed to marshal request for secret validation: %w", err) } return s.ValidateSecretRefs(orgID, string(b)) }Then each call site becomes:
- 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 - } - } + if s.secretService != nil { + if err := s.secretService.ValidateRequestSecretRefs(orgUUID, req); err != nil { + return nil, err + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/service/llm.go` around lines 877 - 889, The secret-validation logic is duplicated across multiple create/update paths, and the helper name no longer matches what it marshals. Add a single SecretService method like ValidateRequestSecretRefs that marshals the full request and calls ValidateSecretRefs, then replace the repeated nil-guard → marshal → validate blocks in the provider, proxy, and MCP proxy flows with that method. Rename or remove marshalUpstreamForValidation so the name reflects that it handles the whole request object, not just upstream, and keep the existing error message behavior in the new centralized helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/internal/service/api.go`:
- Around line 758-789: The auth backfill logic in preserveAPIUpstreamAuth is too
broad because it restores the stored Value even when the auth Type changes,
which can leave a mismatched secret attached. Update
preserveUpstreamAuthOnAPIUpdate/preserveAPIUpstreamAuth so the existing Value is
only preserved when the incoming UpstreamAuth has the same Type as the stored
one, and otherwise leave the update’s auth untouched. Add a test covering a Type
change with no Value to verify the old secret is not reused.
In `@platform-api/internal/service/secret_service.go`:
- Around line 249-266: cleanupRotatedSecret can panic when delete fails because
it calls logger.Warn unconditionally even though logger is a *slog.Logger that
may be nil. Update cleanupRotatedSecret in SecretService to treat logging as
best-effort: guard the Warn call with a nil check, or fall back to a no-op when
logger is nil, so Delete failures like SecretInUseError do not crash callers.
Keep the behavior otherwise unchanged, and preserve the existing use of
extractSecretHandle and s.Delete.
---
Nitpick comments:
In `@platform-api/internal/service/llm_test.go`:
- Around line 1539-1592: The LLM proxy update cleanup test only verifies that
the rotated secret is deprecated, so it can miss a regression where the newly
referenced secret is also deactivated. Update
TestLLMProxyServiceUpdate_CleansUpRotatedSecret in llm_test.go to also assert
that the secret referenced by the new UpstreamAuth value ("new-handle") remains
in SecretStatusActive after calling LLMProxyService.Update, alongside the
existing check for the old secret becoming deprecated.
In `@platform-api/internal/service/llm.go`:
- Around line 877-889: The secret-validation logic is duplicated across multiple
create/update paths, and the helper name no longer matches what it marshals. Add
a single SecretService method like ValidateRequestSecretRefs that marshals the
full request and calls ValidateSecretRefs, then replace the repeated nil-guard →
marshal → validate blocks in the provider, proxy, and MCP proxy flows with that
method. Rename or remove marshalUpstreamForValidation so the name reflects that
it handles the whole request object, not just upstream, and keep the existing
error message behavior in the new centralized helper.
In `@portals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsx`:
- Around line 133-158: The secret creation path in LLMProviderContext’s update
flow can leave an orphaned secret if the subsequent provider update fails. Add
best-effort compensating cleanup around the createSecret/updateLLMProvider
sequence in LLMProviderContext so that a newly created secret is deleted if
updateLLMProvider throws, mirroring the existing handling in LLMProxyNew.tsx;
keep the rotated-away secret cleanup unchanged since that is handled
server-side.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23eba1b1-8ff3-4b66-b533-dd19fbdf256c
📒 Files selected for processing (16)
platform-api/internal/handler/api.goplatform-api/internal/handler/llm.goplatform-api/internal/server/server.goplatform-api/internal/service/api.goplatform-api/internal/service/api_secret_integration_test.goplatform-api/internal/service/api_test.goplatform-api/internal/service/llm.goplatform-api/internal/service/llm_test.goplatform-api/internal/service/mcp.goplatform-api/internal/service/secret_service.goplatform-api/internal/utils/api.goplatform-api/internal/utils/api_test.goportals/ai-workspace/cypress/e2e/001-providers/003-llm-proxy-secret-management.cy.jsportals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsxportals/ai-workspace/src/contexts/proxy/ProxyContext.tsxportals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyNew.tsx
faac78b to
839d4f5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
platform-api/internal/service/api_secret_integration_test.go (1)
198-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify secret B is protected after rotation.
The rotation test confirms secret A is deprecated and deletable, and secret B remains active — but it never verifies that secret B is now referenced (i.e., protected from deletion). Adding a delete-protection check for secret B would confirm the reference was properly transferred, mirroring the delete-protection assertion for secret A earlier in the test.
🧪 Suggested addition after line 231
if secretB.Status != string(model.SecretStatusActive) { t.Errorf("expected secret B to remain active after rotation, got status=%q", secretB.Status) } + // Secret B is now referenced by the API, so it must be protected from deletion. + if err := secretSvc.Delete(apiSecretITOrgUUID, "it-secret-b", "alice"); err == nil { + t.Fatal("expected secret B deletion to be blocked after rotation, got nil error") + } // Secret A is no longer referenced, so it can now be hard-deleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/service/api_secret_integration_test.go` around lines 198 - 235, The rotation test in api_secret_integration_test.go should also verify that secret B becomes delete-protected after UpdateAPIByHandle switches the API auth from it-secret-a to it-secret-b. Add a delete attempt/check using secretSvc.Delete for it-secret-b after the rotation assertions, and expect it to fail the same way the earlier delete-protection check for secret A does. Use the existing test symbols UpdateAPIByHandle, secretSvc.Get, and secretSvc.Delete to confirm the reference transfer is enforced.platform-api/internal/handler/api.go (1)
136-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
WithLogMessagefor observability consistency.Most error branches in both
CreateAPIandUpdateAPIinclude.WithLogMessage(fmt.Sprintf("... in org %s", orgId))for structured log context. The newErrSecretRefMissingbranches omit this, making it harder to trace which organization encountered the missing secret reference.♻️ Suggested addition for CreateAPI
if errors.Is(err, constants.ErrSecretRefMissing) { - return apperror.ValidationFailed.Wrap(err, "One or more referenced secrets do not exist") + return apperror.ValidationFailed.Wrap(err, "One or more referenced secrets do not exist"). + WithLogMessage(fmt.Sprintf("missing secret reference in org %s", orgId)) }♻️ Suggested addition for UpdateAPI
if errors.Is(err, constants.ErrSecretRefMissing) { - return apperror.ValidationFailed.Wrap(err, "One or more referenced secrets do not exist") + return apperror.ValidationFailed.Wrap(err, "One or more referenced secrets do not exist"). + WithLogMessage(fmt.Sprintf("missing secret reference for API %s in org %s", apiId, orgId)) }Also applies to: 274-276
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/internal/handler/api.go` around lines 136 - 138, The ErrSecretRefMissing branches in CreateAPI and UpdateAPI currently wrap the validation error but omit the standard structured log context used elsewhere. Update the error handling around the relevant checks in api.go to add .WithLogMessage(fmt.Sprintf("... in org %s", orgId)) to the returned apperror.ValidationFailed.Wrap result, matching the existing observability pattern in CreateAPI and UpdateAPI so the organization can be traced consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@platform-api/internal/handler/api.go`:
- Around line 136-138: The ErrSecretRefMissing branches in CreateAPI and
UpdateAPI currently wrap the validation error but omit the standard structured
log context used elsewhere. Update the error handling around the relevant checks
in api.go to add .WithLogMessage(fmt.Sprintf("... in org %s", orgId)) to the
returned apperror.ValidationFailed.Wrap result, matching the existing
observability pattern in CreateAPI and UpdateAPI so the organization can be
traced consistently.
In `@platform-api/internal/service/api_secret_integration_test.go`:
- Around line 198-235: The rotation test in api_secret_integration_test.go
should also verify that secret B becomes delete-protected after
UpdateAPIByHandle switches the API auth from it-secret-a to it-secret-b. Add a
delete attempt/check using secretSvc.Delete for it-secret-b after the rotation
assertions, and expect it to fail the same way the earlier delete-protection
check for secret A does. Use the existing test symbols UpdateAPIByHandle,
secretSvc.Get, and secretSvc.Delete to confirm the reference transfer is
enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05d2f0bf-27c0-435a-88f5-4df985b6e6c8
📒 Files selected for processing (19)
platform-api/internal/handler/api.goplatform-api/internal/handler/llm.goplatform-api/internal/server/server.goplatform-api/internal/service/api.goplatform-api/internal/service/api_secret_integration_test.goplatform-api/internal/service/api_test.goplatform-api/internal/service/llm.goplatform-api/internal/service/llm_test.goplatform-api/internal/service/mcp.goplatform-api/internal/service/mcp_secret_integration_test.goplatform-api/internal/service/secret_service.goplatform-api/internal/service/secret_service_test.goplatform-api/internal/utils/api.goplatform-api/internal/utils/api_test.goportals/ai-workspace/cypress/e2e/001-providers/003-llm-proxy-secret-management.cy.jsportals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsxportals/ai-workspace/src/contexts/proxy/ProxyContext.tsxportals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyNew.tsxportals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyOverview.tsx
💤 Files with no reviewable changes (13)
- portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyOverview.tsx
- portals/ai-workspace/src/pages/appShell/appShellPages/proxies/LLMProxyNew.tsx
- platform-api/internal/utils/api_test.go
- platform-api/internal/service/mcp.go
- portals/ai-workspace/src/contexts/proxy/ProxyContext.tsx
- portals/ai-workspace/src/contexts/llmProvider/LLMProviderContext.tsx
- platform-api/internal/service/secret_service.go
- platform-api/internal/service/secret_service_test.go
- platform-api/internal/service/mcp_secret_integration_test.go
- platform-api/internal/service/llm_test.go
- portals/ai-workspace/cypress/e2e/001-providers/003-llm-proxy-secret-management.cy.js
- platform-api/internal/service/llm.go
- platform-api/internal/utils/api.go
🚧 Files skipped from review as they are similar to previous changes (2)
- platform-api/internal/server/server.go
- platform-api/internal/service/api.go
839d4f5 to
e64a14a
Compare
4 participants
Purpose
Add secret management improvements and add secret management support for REST APIs
Fixes for #2149