Stop showing secret configuration values in the agent catalogue and agent creation form - #1461
Conversation
…r-side on agent creation instead
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughSecret configuration defaults are redacted in service responses and catalog views. Agent creation applies schema defaults, validates environment variables, and stores sensitive values through secret management. Catalog forms track secret metadata and reset environment rows when the version changes. ChangesSecret default protection and agent creation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CatalogUI
participant AgentManagerService
participant SecretStore
participant OpenChoreo
CatalogUI->>AgentManagerService: Submit environment variables
AgentManagerService->>AgentManagerService: Apply schema secret defaults
AgentManagerService->>SecretStore: Store sensitive values
AgentManagerService->>OpenChoreo: Create agent with secret references
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
console/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.test.ts (1)
77-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the configured global Vitest APIs.
These new tests use
describe,it, andexpectfrom thevitestimport. Remove the named test API import and use the configured globals.As per coding guidelines, “Write tests with Vitest using the jsdom environment and global test APIs.”
🤖 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 `@console/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.test.ts` around lines 77 - 208, Update the tests in the buildCatalogAgentPayload and deriveCatalogEnvSeed suites to use the configured global Vitest APIs. Remove the named describe, it, and expect imports from vitest while keeping the existing test behavior unchanged.Source: Coding guidelines
agent-manager-service/services/agent_kind_service.go (1)
565-591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the aliasing consistent: the function mutates the caller's slice.
result := envVarsdoes not copy. Lines 577-579 write through to the caller's backing array. The returned slice therefore aliases the input when no append occurs, and stops aliasing onceappendgrows the slice. The behaviour differs based on the input capacity.The single current caller,
CreateAgent, reassignsreq.Configurations.Env = envVarsimmediately, so this is not a live defect. It is a trap for the next caller, which may keep a reference to the original slice and observe secret values written into it.Copy the input first so the function is a pure transform.
♻️ Proposed fix to avoid mutating the input
result := envVars + if len(envVars) > 0 { + result = make([]spec.EnvironmentVariable, len(envVars)) + copy(result, envVars) + } for _, item := range schema {🤖 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 `@agent-manager-service/services/agent_kind_service.go` around lines 565 - 591, Update ApplySecretConfigDefaults to copy envVars into a new result slice before modifying existing entries or appending defaults. Preserve the current secret-default and sensitivity behavior while ensuring the returned slice never aliases or mutates the caller’s input slice.agent-manager-service/tests/create_agent_test.go (1)
1238-1294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive each nested subtest its own mock clients.
openChoreoClientandsecretMgmtClientare created once at line 1238 and shared by both nested subtests. Line 1292 assertsCreateSecretCalls()has length 1. That assertion holds only because the preceding subtest at line 1266 makes zero calls.The two subtests are coupled through mock call history. If the rejection path ever starts calling the secret client, the second subtest fails with a misleading count mismatch instead of the first subtest failing on its own assertion.
Construct the clients and the app inside each nested subtest, as the sibling top-level subtests already do.
🤖 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 `@agent-manager-service/tests/create_agent_test.go` around lines 1238 - 1294, Move creation of openChoreoClient, secretMgmtClient, testClients, and app into each nested subtest under the shared test block, so every case has isolated mock clients and call history. Preserve each subtest’s existing request setup and assertions, including the successful case’s CreateSecretCalls length check.
🤖 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 `@agent-manager-service/docs/api_v1_openapi.yaml`:
- Line 17201: Update the schema description for the default-value field to state
that secret defaults with a non-empty value are returned as the masked
placeholder `••••••••`, while the field is omitted only when no default or an
empty default exists; preserve the existing plain-text behavior for non-secret
inputs.
In `@agent-manager-service/models/agent_kind.go`:
- Around line 69-96: Update the kind write paths in PublishAgentKind and
AddVersion, before or within toModelConfigSchema, to reject or omit secret
defaultValue values equal to RedactedSecretDefaultPlaceholder. Ensure the
placeholder is never persisted in AgentKindVersion.ConfigSchema or reused by
ApplySecretConfigDefaults, while preserving legitimate secret defaults.
In `@console/workspaces/pages/add-new-agent/src/components/CatalogAgentFlow.tsx`:
- Around line 102-115: The CatalogAgentFlow environment reseeding effect
currently runs whenever catalogEnvSeed changes, including refetches of the same
version. Update the useEffect around catalogEnvSeed to track the last seeded
effectiveVersion and call setFormData only when that version changes, preserving
explicit env and secret overrides during same-version data refreshes.
---
Nitpick comments:
In `@agent-manager-service/services/agent_kind_service.go`:
- Around line 565-591: Update ApplySecretConfigDefaults to copy envVars into a
new result slice before modifying existing entries or appending defaults.
Preserve the current secret-default and sensitivity behavior while ensuring the
returned slice never aliases or mutates the caller’s input slice.
In `@agent-manager-service/tests/create_agent_test.go`:
- Around line 1238-1294: Move creation of openChoreoClient, secretMgmtClient,
testClients, and app into each nested subtest under the shared test block, so
every case has isolated mock clients and call history. Preserve each subtest’s
existing request setup and assertions, including the successful case’s
CreateSecretCalls length check.
In `@console/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.test.ts`:
- Around line 77-208: Update the tests in the buildCatalogAgentPayload and
deriveCatalogEnvSeed suites to use the configured global Vitest APIs. Remove the
named describe, it, and expect imports from vitest while keeping the existing
test behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 35930a54-b4d1-42a6-8a8e-3bcc37c090c6
📒 Files selected for processing (17)
agent-manager-service/docs/api_v1_openapi.yamlagent-manager-service/models/agent_kind.goagent-manager-service/models/agent_kind_test.goagent-manager-service/services/agent_kind_service.goagent-manager-service/services/agent_kind_service_unit_test.goagent-manager-service/services/agent_manager.goagent-manager-service/tests/create_agent_test.goconsole/workspaces/libs/shared-component/src/components/DeploymentConfig.tsxconsole/workspaces/libs/shared-component/src/components/index.tsconsole/workspaces/libs/views/src/component/EnvVariableEditor/EnvVariableEditor.tsxconsole/workspaces/pages/add-new-agent/src/components/CatalogAgentFlow.tsxconsole/workspaces/pages/add-new-agent/src/components/EnvironmentVariable.tsxconsole/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.test.tsconsole/workspaces/pages/add-new-agent/src/utils/buildAgentPayload.tsconsole/workspaces/pages/agent-kind/src/Catalog.KindDetails.tsxconsole/workspaces/pages/agent-kind/src/Publish.VersionDetails.tsxconsole/workspaces/pages/agent-kind/src/RuntimeConfigEditor.tsx
💤 Files with no reviewable changes (2)
- console/workspaces/libs/shared-component/src/components/DeploymentConfig.tsx
- console/workspaces/libs/shared-component/src/components/index.ts
…en publishing a kind version
… of query cache identity
| // secret with a baked-in default apart from one that has none at all (and therefore | ||
| // must be supplied by whoever creates an agent from the kind) without ever learning | ||
| // what the real value is. | ||
| const RedactedSecretDefaultPlaceholder = "••••••••" |
There was a problem hiding this comment.
I don't understand why we need to return this, shouldn't we omit the field and rely on "isSecret" bool in the frontend to decide whether we render a placeholder?
There was a problem hiding this comment.
the reason is isSecret only tells us the field is sensitive, not whether it actually has a default.
If we just omit the value and let the frontend decide purely off isSecret, we can't tell "this secret has a default, safe to leave blank" apart from "this secret has no default, you need to type one." Both would look identical to the frontend. That's a real problem: a required secret with no default would look exactly like one that's already covered, so people would leave it blank thinking it's handled, then hit a confusing error (or worse, silently ship an agent missing a value).
The placeholder is just a fixed, non-secret string, never the real value, that lets the frontend tell those two cases apart: present means "something's there, lock the field," absent means "nothing's there, this needs input."
3 participants
Purpose
Agent Kind runtime configuration values marked as secret (e.g. API keys) were being sent back to the browser in plaintext, and shown in the catalogue and in the agent creation form.
Goals
Approach
models/agent_kind.go:RedactSecretDefaultsreplaces a secret item's real default with a placeholder in every read response, or leaves it empty if no default was ever set, so a client can tell "hidden default" apart from "no default."services/agent_kind_service.go:ApplySecretConfigDefaultsfills in a secret's real default on the server when a create-agent request leaves it untouched, forces every matching env var toisSensitive, and rejects duplicate keys so a value can't slip past the secret handling.Catalog.KindDetails.tsx/Publish.VersionDetails.tsx/RuntimeConfigEditor.tsx: show masked dots only when a default actually exists,—otherwise.CatalogAgentFlow.tsx: reuses the existing locked "existing secret" input instead of a password mask with a working reveal button, and only locks a field when the kind actually has a default for it. Switching the selected Agent Kind Version now fully resets the form so a value can't carry over into a different version.DeploymentConfig.tsxfromshared-component, an unused component with the same bug and no callers.Agent catalogue / kind version details
Create agent from kind
User stories
Release note
N/A
Documentation
N/A
Training
N/A
Certification
N/A
Marketing
N/A
Automation tests
agent_kind_test.gocoversRedactSecretDefaultsmasking a real default, leaving a no-default item empty, and not mutating its source.agent_kind_service_unit_test.gocovers the redaction end to end throughGetKind.buildAgentPayload.test.tscoversderiveCatalogEnvSeedonly locking a field with a real default, andbuildCatalogAgentPayloaddropping whitespace-only values.make test-unit: all packages pass.create_agent_test.go,TestCreateAgentFromKind_SecretConfigDefaults: applies the server-side default when untouched, an explicit override wins and is still stored as sensitive, a duplicate env key is rejected before anything is created, a mandatory secret with no default must be supplied by the caller.make test-integration: all tests pass.Security checks
Samples
N/A
Related PRs
N/A
Migrations (if applicable)
N/A
Test environment
macOS (Darwin), Go 1.26.4, Postgres 16 (postgres:16-alpine via Docker) for the backend. Node.js with Vite and Vitest for the console. Browser-based manual verification wasn't completed (blocked by local sign-in).
Learning
N/A
Summary by CodeRabbit
Security
Bug Fixes
UI Changes