refactor: unify SecretVar secret source into typed ref,SecretType fields, replacing FromEnv, EnvVar, FromVault, VaultRef#4595
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (42)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughSecretVar transitions from separate environment and vault source fields ( ChangesSecretVar struct and schema unification
Config validation and error handling
Storage, encryption, and persistence logic
Redaction and reference restoration for plugin/handler use
UI schema definitions and type constants
UI component updates for new SecretVar representation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
This was referenced Jun 21, 2026
Contributor
Author
This was referenced Jun 21, 2026
BearTS
changed the title
fix: extend empty-value validation for proxy and auth config to include Vault references
Contributor
Confidence Score: 3/5The core SecretVar refactor is internally consistent and the UI migration is complete, but two unresolved issues from a prior review carry production risk: env/vault-backed admin passwords flush all active sessions on every config save, and stale inline SecretVar schemas in config.schema.json will reject the new ref/type fields via additionalProperties:false. The auth handler now triggers FlushSessions on every save whenever admin credentials are env/vault-backed, silently logging out dashboard users. Additionally, multiple inline SecretVar object definitions throughout config.schema.json still only allow the old env_var/from_env keys, which means any config file using the new ref/type format for provider credentials will fail schema validation. Both issues are present in the current diff. transports/bifrost-http/handlers/config.go (passwordChanged logic for env/vault-backed auth) and transports/config.schema.json (inline SecretVar definitions with additionalProperties:false still listing only the old env_var/from_env fields). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["SecretVar (incoming JSON / DB / string)"] --> B{Source format?}
B -->|"new: ref + type fields"| C["Set ref = raw.Ref\nSet SecretType = raw.SecretType"]
B -->|"new: ref only"| D["Set ref = raw.Ref\nInfer SecretType from prefix"]
B -->|"legacy: from_env + env_var"| E["Rewrite to env.VAR prefix\nSet SecretType = env"]
B -->|"plain string with env. prefix"| F["Set ref = value\nSet SecretType = env"]
B -->|"plain string with vault. prefix"| G["Set ref = value\nSet SecretType = vault"]
B -->|"literal value"| H["Set Val = value\nSecretType = plain_text"]
C --> I{Resolve value}
D --> I
E --> J["os.LookupEnv(key)"]
F --> J
G --> K["LookupVault(ref)"]
I -->|SecretTypeEnv| J
I -->|SecretTypeVault| K
J --> L["SecretVar.Val = resolved"]
K --> L
H --> L
L --> M["IsFromSecret()\nGetRef() / GetRawRef()\nValue() / MarshalJSON()"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["SecretVar (incoming JSON / DB / string)"] --> B{Source format?}
B -->|"new: ref + type fields"| C["Set ref = raw.Ref\nSet SecretType = raw.SecretType"]
B -->|"new: ref only"| D["Set ref = raw.Ref\nInfer SecretType from prefix"]
B -->|"legacy: from_env + env_var"| E["Rewrite to env.VAR prefix\nSet SecretType = env"]
B -->|"plain string with env. prefix"| F["Set ref = value\nSet SecretType = env"]
B -->|"plain string with vault. prefix"| G["Set ref = value\nSet SecretType = vault"]
B -->|"literal value"| H["Set Val = value\nSecretType = plain_text"]
C --> I{Resolve value}
D --> I
E --> J["os.LookupEnv(key)"]
F --> J
G --> K["LookupVault(ref)"]
I -->|SecretTypeEnv| J
I -->|SecretTypeVault| K
J --> L["SecretVar.Val = resolved"]
K --> L
H --> L
L --> M["IsFromSecret()\nGetRef() / GetRawRef()\nValue() / MarshalJSON()"]
Reviews (14): Last reviewed commit: "feat: add secret type instead of from se..." | Re-trigger Greptile |
BearTS
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
from
881d484 to
9099811
Compare
18 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transports/bifrost-http/lib/config.go`:
- Around line 3693-3706: The preserveSecretVar function currently only preserves
EnvVar and FromEnv metadata but strips VaultRef and FromVault metadata when
persisting hashed passwords, causing vault provenance to be lost during later
processing. Update the preserveSecretVar function to also preserve the vault
reference metadata (VaultRef and FromVault fields) alongside the existing
environment variable metadata so that subsequent reads can reliably identify and
apply vault-specific diagnostics and behavior, matching the vault-aware checks
now present in the AdminUserName and AdminPassword validation blocks.
🪄 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: f3290fef-ab61-45be-89a2-c138b9c2f86c
📒 Files selected for processing (3)
core/providers/utils/utils.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.go
BearTS
changed the title
fix: extend empty-value validation for proxy and auth config to include Vault referencessecret_ref\/\from_secret\ representation
BearTS
changed the title
secret_ref\/\from_secret\ representationsecret_ref\/\from_secret\ representation
BearTS
changed the title
secret_ref\/\from_secret\ representationsecret_ref to from_secret representation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
BearTS
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
from
feaa595 to
30c2b3f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/app/workspace/mcp-registry/views/mcpClientForm.tsx (1)
168-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccept
secret_refwhen validating connection URLs.With the unified SecretVar model, env/vault URL references live in
secret_refwith an emptyvalue; Line 169 treats them as missing and blocks creating HTTP/SSE clients from external refs.Proposed fix
if (connectionType === "http" || connectionType === "sse") { - const connVal = data.connection_string?.value || ""; - if (!connVal.trim()) { + const connVal = data.connection_string?.value?.trim() || ""; + const connRef = data.connection_string?.secret_ref?.trim() || ""; + if (!connVal && !connRef) { setError("connection_string", { message: "Connection URL is required" }); hasErrors = true; - } else if (!/^((https?:\/\/.+)|(env\.[A-Z_]+))$/.test(connVal)) { + } else if (connVal && !/^https?:\/\/.+$/.test(connVal)) { setError("connection_string", { - message: "Connection URL must start with http://, https://, or be an environment variable (env.VAR_NAME)", + message: "Connection URL must start with http://, https://, or be an external reference", }); hasErrors = true; } }🤖 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 `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx` around lines 168 - 178, The validation logic in the connectionType check for "http" or "sse" only validates data.connection_string?.value but needs to also accept secret_ref references. Modify the validation condition at line 169 to check if either the value property contains a non-empty string OR the secret_ref property is populated, since external references now use the secret_ref field with an empty value. The regex validation that follows should only apply when validating the actual connection_string value, not when secret_ref is being used for the external reference.
🤖 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 `@core/schemas/secretvar.go`:
- Around line 30-44: The guard condition in the UnmarshalJSON method currently
requires both valueNode to exist AND either envNode or secretRefNode to exist,
causing value-only objects like {"value":"sk"} to fall through unparsed. Relax
the guard condition to only check valueNode.Exists() without requiring the other
nodes. Additionally, the secretVarCompat struct only handles legacy env metadata
(EnvVar and FromEnv fields) but lacks support for legacy vault metadata. Add
VaultRef and FromVault fields to the secretVarCompat struct to capture the old
vault model. After unmarshaling into the raw secretVarCompat variable, map both
legacy models into the SecretRef and FromSecret fields: if FromVault is true and
VaultRef is not empty, set SecretRef to VaultRef and FromSecret to true before
returning the parsed SecretVar. This ensures all object shapes including legacy
vault payloads are properly preserved during the migration.
- Around line 47-49: The issue is that `FromSecret=true` can be set with an
empty `SecretRef`, creating an inconsistent state where `Value()` silently drops
the configured value. In the `UnmarshalJSON` method at lines 47-49, add
validation to reject cases where `FromSecret` is true but `SecretRef` is empty
by returning an error. In the `NewSecretVar` function, only set
`FromSecret=true` when `SecretRef` is non-empty; if `SecretRef` is empty, fall
back to creating a plain `SecretVar{Val: raw.Val}` instead. Apply this same
validation pattern to the other occurrences mentioned at lines 258-260 and
381-384 to ensure consistency throughout the file.
In `@core/schemas/vault.go`:
- Around line 121-125: The removeOwnedVaultSecretVar function currently only
checks if SecretRef has the vault prefix before deleting owned vault entries,
but it should also verify that the FromSecret field is true to ensure the field
is actively secret-backed. Add an additional condition to check field.FromSecret
in the guard clause alongside the existing SecretRef validation to prevent
deletion of vault paths when the field has a stale or malformed SecretRef with
FromSecret set to false.
In `@framework/configstore/clientconfig.go`:
- Around line 1326-1332: The hash function for ConnectionString needs to
distinguish between secret and literal values to prevent configuration
reconciliation from being skipped when switching between a secret reference and
a literal value with the same content. In the conditional block checking
IsFromSecret(), prefix the hash input with a source indicator (such as "ref:")
before writing the SecretRef value in the true branch, and prefix with a
different indicator (such as "val:") before writing the Val value in the false
branch. This matches the convention used for key values and MCP headers to
ensure different sources produce different hashes.
In `@transports/bifrost-http/handlers/config.go`:
- Around line 139-147: The issue is that when a password is secret-backed and
`IsFromSecret()` returns true, the code is setting `Val: ""` (empty string) in
the `SecretVar` struct, which causes validation failures when the config is
later updated. Replace the empty string placeholder with a redacted sentinel
value (matching the pattern used in the else branch for non-secret passwords)
while keeping the `SecretRef` field intact. This same fix needs to be applied in
both locations mentioned: the primary location in the password handling block
and the similar occurrence around line 697-702.
In `@transports/bifrost-http/lib/config.go`:
- Around line 3693-3698: The current code warns when AdminUserName or
AdminPassword are set with external secret references but resolve to empty
values, but continues processing and allows these empty values to be persisted
to config.GovernanceConfig.AuthConfig. Instead of just logging a warning and
continuing, add logic to return early or skip the auth config persistence after
detecting an empty secret-resolved value. Check for the same conditions in the
existing if statements (when the field is not nil, GetValue() returns empty
string, and IsFromSecret() is true) and when found, either return early or fall
back to the DB config instead of persisting the empty auth configuration,
ensuring a fail-closed validation approach similar to the handler's behavior.
In `@ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx`:
- Line 40: The current check for hasCACert at line 40 does not validate that
SecretVar payloads contain actual non-empty values or references. The condition
`tls.ca_cert_pem?.value || tls.ca_cert_pem?.from_secret` allows `from_secret:
true` without a meaningful value or reference, and does not handle
whitespace-only values. Create a trimmed presence helper function that validates
both the value and from_secret properties by checking that either the value is a
non-empty trimmed string OR the from_secret is a non-empty trimmed string, then
apply this helper consistently to the hasCACert check and the other similar
SecretVar validations at lines 221 and 301-303 throughout the
mcpLibraryInstallSheet.tsx file.
In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx`:
- Line 42: The SecretVar presence check for hasCACert (and similar checks
elsewhere in the file at lines 142 and 248-250) uses from_secret to validate if
a reference exists, but this is insufficient and doesn't account for
whitespace-only values. Replace the from_secret check with a proper validation
using trimmed value or secret_ref checks to ensure that SecretVar presence is
consistently validated across all similar checks in the mcpClientForm component.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 509-510: At the point where displaying the connection_string
secret reference in mcpClientSheet.tsx, remove the hardcoded `env.` prefix that
is being concatenated with mcpClient.config.connection_string.secret_ref. Since
from_secret is generic and secret_ref can reference different types of secrets
(not just environment variables), display the secret_ref value directly without
forcing an env prefix. This will prevent mislabeling non-env references and
avoid rendering issues when secret_ref is empty.
In `@ui/app/workspace/observability/fragments/otelFormFragment.tsx`:
- Line 314: The collectorPreview assignment in the otelFormFragment.tsx file
incorrectly assumes collectorUrl always has a .value property, but since
collector_url can be a string or a SecretVar object, plain string values fail to
render a preview. Update the collectorPreview constant to check whether
collectorUrl is a plain string and use it directly, or if it's a SecretVar
object with from_secret true use collectorUrl.secret_ref, otherwise use
collectorUrl.value. This ensures string-backed URLs display properly in the
collapsed preview.
In `@ui/app/workspace/providers/views/modelProviderKeysTableView.tsx`:
- Around line 235-240: The detection logic now checks for from_secret across
multiple key configurations, which is source-agnostic and not limited to
environment variables. Update the warning message that gets displayed when
from_secret is detected to use source-agnostic language instead of referring
specifically to "env var" or "environment variable". Replace the environment
variable-specific warning copy with generic language that describes secrets
being configured from external sources, and apply this change to all locations
where this pattern occurs throughout the file (including lines 241-258 as
mentioned in the comment).
In `@ui/components/ui/secretVarInput.tsx`:
- Around line 103-107: The onChange handler in secretVarInput.tsx is setting
value to the newValue (the reference string) when detecting a secret reference,
but this is inconsistent with how toSecretVarFormValue in secretVarForm.ts
handles secret references by setting value to an empty string. To align these
behaviors and avoid subtle bugs, modify the onChange handler so that when
newValue starts with "env." or "vault.", set the value field to an empty string
instead of newValue, ensuring the form state is consistent regardless of whether
the secret reference comes from form initialization or user input.
---
Outside diff comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx`:
- Around line 168-178: The validation logic in the connectionType check for
"http" or "sse" only validates data.connection_string?.value but needs to also
accept secret_ref references. Modify the validation condition at line 169 to
check if either the value property contains a non-empty string OR the secret_ref
property is populated, since external references now use the secret_ref field
with an empty value. The regex validation that follows should only apply when
validating the actual connection_string value, not when secret_ref is being used
for the external reference.
🪄 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: 843bdf8f-e968-4f5a-89fb-73f06fd782f5
📒 Files selected for processing (34)
core/providers/utils/utils.gocore/schemas/secretvar.gocore/schemas/secretvar_test.gocore/schemas/utils.gocore/schemas/vault.gocore/schemas/vault_test.goframework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/tables/encryption.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goplugins/otel/main.goplugins/telemetry/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/lib/config.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/securityView.tsxui/app/workspace/mcp-registry/library/page.tsxui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/observability/fragments/otelFormFragment.tsxui/app/workspace/observability/fragments/prometheusFormFragment.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/deploymentsTable.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/components/onboardingWidget.tsxui/components/ui/headersTable.tsxui/components/ui/secretVarInput.tsxui/lib/constants/config.tsui/lib/types/config.tsui/lib/types/schemas.tsui/lib/utils/secretVarForm.ts
BearTS
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
2 times, most recently
from
62aa43e to
8b61327
Compare
Closed
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
core/schemas/secretvar.go (2)
30-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParse value-only objects and legacy vault metadata before falling back.
{"value":"sk"}is the default JSON shape for a literalSecretVar, but the guard requiresenv_varorsecret_ref, so it becomes a raw JSON string. The compat struct also still omits legacy vault fields, so old vault-backed payloads are not migrated. This is the same migration hole flagged earlier and still appears in the current snippet.Suggested direction
if sonic.Valid([]byte(value)) { valueNode, _ := sonic.Get([]byte(val), "value") - envNode, _ := sonic.Get([]byte(val), "env_var") - secretRefNode, _ := sonic.Get([]byte(val), "secret_ref") - if valueNode.Exists() && (envNode.Exists() || secretRefNode.Exists()) { + if valueNode.Exists() { type secretVarCompat struct { Val string `json:"value"` SecretRef string `json:"secret_ref"` FromSecret bool `json:"from_secret"` // backward compat: env_var/from_env (shipped) EnvVar string `json:"env_var"` FromEnv bool `json:"from_env"` + // backward compat: vault_ref/from_vault + VaultRef string `json:"vault_ref"` + FromVault bool `json:"from_vault"` }Mirror the same guard and compat fields in
UnmarshalJSON, then mapFromVault && VaultRef != ""intoSecretRef/FromSecret.Also applies to: 248-260
🤖 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 `@core/schemas/secretvar.go` around lines 30 - 44, The secretVarCompat struct is missing legacy vault fields and the guard condition does not accept value-only JSON objects like {"value":"sk"}. Add the FromVault and VaultRef fields to the secretVarCompat struct to capture legacy vault metadata, update the guard condition to also accept objects with only a value field (by checking if valueNode exists and at least one of envNode, secretRefNode, or the vault fields exist, or just valueNode alone), and after unmarshaling into the raw variable, add migration logic that maps FromVault && VaultRef != "" into SecretRef and FromSecret to ensure backward compatibility with vault-backed payloads.
47-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
SecretRefandFromSecretnormalized before persistence.
from_secret:truewith an emptysecret_refstill creates an invalid reference state thatValue()persists as"". Conversely, a non-emptysecret_refwith omitted/falsefrom_secretis treated as non-secret. Reject the empty-reference case and inferFromSecret=truefrom any non-emptySecretRef.Suggested direction
- if raw.SecretRef != "" || raw.FromSecret { + if raw.SecretRef != "" { e.SecretRef = raw.SecretRef - e.FromSecret = raw.FromSecret + e.FromSecret = true + } else if raw.FromSecret { + // NewSecretVar cannot return an error; keep this malformed object as a literal. + return e- if raw.SecretRef != "" || raw.FromSecret { + if raw.FromSecret && raw.SecretRef == "" { + return fmt.Errorf("secret_ref is required when from_secret is true") + } + if raw.SecretRef != "" { e.SecretRef = raw.SecretRef - e.FromSecret = raw.FromSecret + e.FromSecret = truefunc (e SecretVar) Value() (driver.Value, error) { if e.FromSecret { + if e.SecretRef == "" { + return nil, fmt.Errorf("secret_ref is required when from_secret is true") + } return e.SecretRef, nil }Also applies to: 266-268, 389-392
🤖 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 `@core/schemas/secretvar.go` around lines 47 - 49, The code currently assigns `SecretRef` and `FromSecret` from raw input without validation or normalization. Add validation logic to reject cases where `FromSecret` is true but `SecretRef` is empty, and infer `FromSecret=true` whenever a non-empty `SecretRef` is provided. Apply this normalized assignment logic in the three locations mentioned (lines 47-49, 266-268, and 389-392) to ensure consistent handling of secret references before persistence. The normalization should occur before assigning to the `e.SecretRef` and `e.FromSecret` fields to guarantee that invalid reference states are caught and that the state is always consistent.
🤖 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 `@core/schemas/secretvar.go`:
- Around line 150-152: The change on line 151 now treats external secret
references (FromSecret) as redacted by returning true. This breaks existing
callers that use IsRedacted() to determine whether to preserve stored
credentials, causing env.*/vault.* updates to be dropped unintentionally. Update
all call sites that use IsRedacted() to check whether to preserve stored
credentials to instead use ShouldPreserveStored(). This specifically includes
the AdminPassword handling in the auth-config path and any other
persistence/update gates in the secretvar package (around lines 396-407 as
noted).
- Around line 77-82: Remove the assignment of e.SecretRef to e.Val before the
LookupVault call in the vault reference resolution block. Instead, only assign
to e.Val when the LookupVault call succeeds (inside the if condition that checks
ok is true). This ensures Val remains empty when the vault lookup fails,
allowing downstream empty value checks to properly detect unresolved vault
references. Apply this same fix to all other occurrences mentioned in the
comment at the specified line ranges.
- Around line 357-380: The issue is in the string case block where
strings.Trim(v, "\"") unconditionally removes all leading and trailing quotes
before checking for env.* and vault.* prefixes, which can corrupt or misclassify
quoted literal secrets. Remove the early quote trimming and instead check for
the env.* and vault.* prefixes on the original untrimmed string value. Only
strip quotes from literal string values that are not env.* or vault.*
references, preserving the raw string behavior expected by the scan operation.
---
Duplicate comments:
In `@core/schemas/secretvar.go`:
- Around line 30-44: The secretVarCompat struct is missing legacy vault fields
and the guard condition does not accept value-only JSON objects like
{"value":"sk"}. Add the FromVault and VaultRef fields to the secretVarCompat
struct to capture legacy vault metadata, update the guard condition to also
accept objects with only a value field (by checking if valueNode exists and at
least one of envNode, secretRefNode, or the vault fields exist, or just
valueNode alone), and after unmarshaling into the raw variable, add migration
logic that maps FromVault && VaultRef != "" into SecretRef and FromSecret to
ensure backward compatibility with vault-backed payloads.
- Around line 47-49: The code currently assigns `SecretRef` and `FromSecret`
from raw input without validation or normalization. Add validation logic to
reject cases where `FromSecret` is true but `SecretRef` is empty, and infer
`FromSecret=true` whenever a non-empty `SecretRef` is provided. Apply this
normalized assignment logic in the three locations mentioned (lines 47-49,
266-268, and 389-392) to ensure consistent handling of secret references before
persistence. The normalization should occur before assigning to the
`e.SecretRef` and `e.FromSecret` fields to guarantee that invalid reference
states are caught and that the state is always consistent.
🪄 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: 2df68392-1806-478e-aaab-3abb23fabfd4
📒 Files selected for processing (34)
core/providers/utils/utils.gocore/schemas/secretvar.gocore/schemas/secretvar_test.gocore/schemas/utils.gocore/schemas/vault.gocore/schemas/vault_test.goframework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/tables/encryption.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goplugins/otel/main.goplugins/telemetry/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/lib/config.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/securityView.tsxui/app/workspace/mcp-registry/library/page.tsxui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/observability/fragments/otelFormFragment.tsxui/app/workspace/observability/fragments/prometheusFormFragment.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/deploymentsTable.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/components/onboardingWidget.tsxui/components/ui/headersTable.tsxui/components/ui/secretVarInput.tsxui/lib/constants/config.tsui/lib/types/config.tsui/lib/types/schemas.tsui/lib/utils/secretVarForm.ts
🚧 Files skipped from review as they are similar to previous changes (33)
- ui/app/workspace/providers/fragments/deploymentsTable.tsx
- framework/configstore/tables/encryption.go
- framework/configstore/tables/mcp.go
- ui/app/workspace/mcp-registry/library/page.tsx
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- framework/configstore/rdb.go
- plugins/telemetry/main.go
- ui/components/ui/headersTable.tsx
- core/schemas/utils.go
- core/providers/utils/utils.go
- ui/lib/constants/config.ts
- ui/app/workspace/config/views/mcpView.tsx
- plugins/otel/main.go
- ui/app/workspace/observability/fragments/prometheusFormFragment.tsx
- ui/app/workspace/config/views/securityView.tsx
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- framework/configstore/tables/oauth.go
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- core/schemas/vault.go
- ui/components/ui/secretVarInput.tsx
- transports/bifrost-http/lib/config.go
- ui/components/onboardingWidget.tsx
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- ui/app/workspace/observability/fragments/otelFormFragment.tsx
- core/schemas/vault_test.go
- ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx
- ui/lib/types/schemas.ts
- ui/lib/types/config.ts
- transports/bifrost-http/handlers/config.go
- framework/configstore/clientconfig.go
- ui/lib/utils/secretVarForm.ts
- transports/bifrost-http/handlers/plugins.go
- core/schemas/secretvar_test.go
18 tasks
BearTS
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
from
8b61327 to
cc29fc9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transports/bifrost-http/handlers/config.go`:
- Around line 697-702: The authChanged variable is computed using
!AdminPassword.IsRedacted() to determine if the password was modified, but this
check always returns false for FromSecret passwords since IsRedacted() returns
true for all secret-backed values, causing the session flush logic at lines
759-766 to be skipped for secret-backed password rotations. Update the
authChanged computation to use the same predicate as the validation checks shown
(checking if AdminPassword.IsFromSecret() and AdminPassword.GetValue() is not
empty) instead of relying on IsRedacted(), so that session flushing is triggered
when secret-backed passwords are actually updated.
🪄 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: 53958461-90ca-40d7-942b-294726c283c6
📒 Files selected for processing (34)
core/providers/utils/utils.gocore/schemas/secretvar.gocore/schemas/secretvar_test.gocore/schemas/utils.gocore/schemas/vault.gocore/schemas/vault_test.goframework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/tables/encryption.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goplugins/otel/main.goplugins/telemetry/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/lib/config.goui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/securityView.tsxui/app/workspace/mcp-registry/library/page.tsxui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/observability/fragments/otelFormFragment.tsxui/app/workspace/observability/fragments/prometheusFormFragment.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/deploymentsTable.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/components/onboardingWidget.tsxui/components/ui/headersTable.tsxui/components/ui/secretVarInput.tsxui/lib/constants/config.tsui/lib/types/config.tsui/lib/types/schemas.tsui/lib/utils/secretVarForm.ts
✅ Files skipped from review due to trivial changes (2)
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/app/workspace/mcp-registry/library/page.tsx
🚧 Files skipped from review as they are similar to previous changes (31)
- ui/app/workspace/observability/fragments/otelFormFragment.tsx
- ui/components/onboardingWidget.tsx
- ui/app/workspace/observability/fragments/prometheusFormFragment.tsx
- ui/lib/constants/config.ts
- plugins/otel/main.go
- core/schemas/utils.go
- plugins/telemetry/main.go
- ui/components/ui/headersTable.tsx
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- ui/app/workspace/config/views/mcpView.tsx
- framework/configstore/rdb.go
- framework/configstore/tables/mcp.go
- core/schemas/vault.go
- ui/app/workspace/providers/fragments/deploymentsTable.tsx
- transports/bifrost-http/handlers/plugins.go
- framework/configstore/tables/encryption.go
- framework/configstore/tables/oauth.go
- ui/components/ui/secretVarInput.tsx
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- ui/app/workspace/config/views/securityView.tsx
- ui/lib/utils/secretVarForm.ts
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- ui/lib/types/config.ts
- transports/bifrost-http/lib/config.go
- ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx
- ui/lib/types/schemas.ts
- core/providers/utils/utils.go
- core/schemas/vault_test.go
- framework/configstore/clientconfig.go
- core/schemas/secretvar.go
- core/schemas/secretvar_test.go
18 tasks
BearTS
changed the title
secret_ref to from_secret representationsecret_ref\/\from_secret\ representation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
core/schemas/secretvar.go (1)
396-396:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve literal quote characters when scanning plain values.
Line 376 says quoted ref-like values are literals, but Line 396 strips all leading/trailing quotes from every plain literal. Since
Value()stores raw strings, scanning should round-trip the raw DB value unless a dedicated legacy JSON path is being parsed.Proposed fix
- e.Val = strings.Trim(v, "\"") + e.Val = v🤖 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 `@core/schemas/secretvar.go` at line 396, In the secretvar.go file where `e.Val = strings.Trim(v, "\"")` appears, remove the strings.Trim call and instead directly assign the raw value to preserve literal quote characters in plain values. Change it to simply `e.Val = v` so that the raw database string is preserved during scanning, as the Value() method is intended to store raw strings without quote stripping unless explicitly parsing a legacy JSON path.
🤖 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 `@core/schemas/secretvar.go`:
- Around line 63-67: The validation in the explicit-type format branch (where
raw.SecretType is checked) must ensure that both type and secret_ref are
provided together as a valid pair, not just checking if SecretType is non-empty.
Add validation to reject cases where SecretType is specified but SecretRef is
empty, and also validate that the SecretType matches the expected backend for
the given SecretRef to prevent mismatched pairs like type:"env" with
secret_ref:"vault.x". Apply this same validation logic at the location marked
"Also applies to: 280-284" to ensure consistency across all code paths that
handle the new format with explicit type fields.
- Around line 52-59: The secretVarCompat struct is missing the legacy vault
fields vault_ref and from_vault, which causes persisted vault reference objects
to be decoded as plain values and lose their vault reference during migration.
Add vault_ref (string) and from_vault (bool) JSON tags to the secretVarCompat
struct, then in both the NewSecretVar and UnmarshalJSON methods, check if
from_vault is true and map vault_ref to secretRef while setting the type to
SecretTypeVault, handling this vault mapping before the fallback logic for plain
values. Apply the same fix to the other compat struct referenced at lines
269-276.
- Line 61: The issue is that the code detects whether the unquoted `val` is
object-shaped on line 50, but then attempts to unmarshal the original quoted
`value` on line 61 using sonic.Unmarshal. This causes a mismatch where quoted
SecretVar object strings are detected as objects but fail to unmarshal. Fix this
by changing the sonic.Unmarshal call to unmarshal the unquoted `val` variable
instead of the original `value`, so that the buffer being unmarshaled matches
the buffer that was inspected for object structure.
---
Duplicate comments:
In `@core/schemas/secretvar.go`:
- Line 396: In the secretvar.go file where `e.Val = strings.Trim(v, "\"")`
appears, remove the strings.Trim call and instead directly assign the raw value
to preserve literal quote characters in plain values. Change it to simply `e.Val
= v` so that the raw database string is preserved during scanning, as the
Value() method is intended to store raw strings without quote stripping unless
explicitly parsing a legacy JSON path.
🪄 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: 4d81c717-b4f6-44b1-adb7-75bdc5df9d35
📒 Files selected for processing (7)
core/schemas/secretvar.gocore/schemas/secretvar_test.gocore/schemas/vault.gocore/schemas/vault_test.goframework/configstore/clientconfig_redaction_test.gotransports/bifrost-http/handlers/plugins.gotransports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (6)
- core/schemas/vault.go
- framework/configstore/clientconfig_redaction_test.go
- transports/config.schema.json
- transports/bifrost-http/handlers/plugins.go
- core/schemas/vault_test.go
- core/schemas/secretvar_test.go
BearTS
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
from
9fe89f4 to
f4157a4
Compare
BearTS
changed the title
refactor: unify SecretVar secret source into typed \ref\/\SecretType\ fields, replacing \FromEnv\/\EnvVar\/\FromVault\/\VaultRef\``
BearTS
changed the title
refactor: unify SecretVar secret source into typed \ref\/\SecretType\ fields, replacing \FromEnv\/\EnvVar\/\FromVault\/\VaultRef\``ref\/\SecretType\ fields, replacing \FromEnv\/\EnvVar\/\FromVault\/\VaultRef\
BearTS
changed the title
ref\/\SecretType\ fields, replacing \FromEnv\/\EnvVar\/\FromVault\/\VaultRef\ref,SecretType fields, replacing FromEnv, EnvVar, FromVault, VaultRef
BearTS
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
2 times, most recently
from
d317296 to
c4c798c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@ui/app/workspace/providers/fragments/deploymentsTable.tsx`:
- Around line 58-59: The isEmptySecretVar function currently does not account
for whitespace-only values in the value and ref fields of a SecretVar object.
When users input only whitespace characters, these are treated as valid entries
instead of empty. Modify the isEmptySecretVar function to trim the value and ref
properties before checking if they are empty, ensuring that whitespace-only
inputs are properly recognized as empty and treated as undefined.
In `@ui/lib/types/schemas.ts`:
- Line 502: The condition checking `data.url?.from_secret || data.url?.ref`
allows validation to pass when from_secret is true but ref is empty, which
results in an empty proxy URL being serialized. Instead of checking truthiness
of ref directly, use the existing trimmed presence helper function that is
already defined in this file to validate that ref contains actual non-empty
content after trimming whitespace. Update the return statement to combine the
from_secret check with a proper call to the trimmed presence helper for the ref
value to ensure a required proxy URL is not left empty.
- Around line 62-63: The secretVarSchema in ui/lib/types/schemas.ts uses
from_secret as a boolean field, but the backend contract expects a type field
with enum values ("plain_text", "env", or "vault") to properly distinguish
secret sources. Replace the from_secret field in secretVarSchema with a type
enum field matching the backend contract, then update the
toOptionalSecretVarPayload function to emit type instead of from_secret when
serializing to the backend. Additionally, add validation to ensure that when
type is "env" or "vault", the ref field must be non-empty, as the backend
requires a non-empty reference for secret-backed fields to have a resolvable
value.
In `@ui/lib/utils/secretVarForm.ts`:
- Around line 17-20: The issue is that when normalizing form state in the return
statement, if from_secret is optional and an object only has a ref property like
{ ref: "env.FOO" }, the current logic sets from_secret: false, which causes the
ref to be treated as a literal/empty value and potentially dropped on save. Fix
this by modifying the from_secret assignment to infer true when a ref is present
and from_secret is undefined, rather than defaulting to false. Use the nullish
coalescing operator to check if field.from_secret is explicitly provided, and if
not, default to true when field.ref has a value and false otherwise.
🪄 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: be614fe6-0aa9-4add-9cd2-2187cafd7094
📒 Files selected for processing (35)
core/providers/utils/utils.gocore/schemas/secretvar.gocore/schemas/secretvar_test.gocore/schemas/utils.gocore/schemas/vault.gocore/schemas/vault_test.goframework/configstore/clientconfig.goframework/configstore/clientconfig_redaction_test.goframework/configstore/rdb.goframework/configstore/tables/encryption_test.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey_secretvar_test.goframework/configstore/vault_callbacks_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.jsonui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/securityView.tsxui/app/workspace/mcp-registry/library/page.tsxui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/observability/fragments/otelFormFragment.tsxui/app/workspace/observability/fragments/prometheusFormFragment.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/deploymentsTable.tsxui/components/onboardingWidget.tsxui/components/ui/headersTable.tsxui/components/ui/secretVarInput.tsxui/lib/constants/config.tsui/lib/types/config.tsui/lib/types/schemas.tsui/lib/utils/secretVarForm.ts
✅ Files skipped from review due to trivial changes (2)
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- framework/configstore/tables/virtualkey_secretvar_test.go
🚧 Files skipped from review as they are similar to previous changes (25)
- core/schemas/utils.go
- ui/app/workspace/config/views/securityView.tsx
- ui/app/workspace/mcp-registry/library/page.tsx
- ui/app/workspace/observability/fragments/otelFormFragment.tsx
- ui/app/workspace/observability/fragments/prometheusFormFragment.tsx
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- ui/components/onboardingWidget.tsx
- transports/config.schema.json
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- ui/app/workspace/config/views/mcpView.tsx
- framework/configstore/rdb.go
- ui/components/ui/headersTable.tsx
- transports/bifrost-http/handlers/plugins.go
- framework/configstore/clientconfig_redaction_test.go
- framework/configstore/tables/mcp.go
- core/schemas/vault.go
- transports/bifrost-http/lib/config.go
- ui/lib/types/config.ts
- core/schemas/vault_test.go
- framework/configstore/clientconfig.go
- core/providers/utils/utils.go
- framework/configstore/tables/encryption_test.go
- transports/bifrost-http/handlers/config.go
- core/schemas/secretvar_test.go
- core/schemas/secretvar.go
BearTS
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
from
c4c798c to
4c6ac61
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/components/ui/headersTable.tsx (1)
46-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecognize ref-only SecretVars in the table guard.
The new schema allows SecretVars with
refbut novalue; this guard rejects those objects, so persisted external-reference headers can render through the plain input path instead ofSecretVarInput.Proposed fix
const isSecretVar = (val: HeaderValue): val is SecretVar => { - return typeof val === "object" && val !== null && "value" in val; + return typeof val === "object" && val !== null && ("value" in val || "ref" in val || "type" in val); }; @@ const isValueEmpty = (val: HeaderValue): boolean => { if (isSecretVar(val)) { - return !val.value && !val.ref; + return !val.value?.trim() && !val.ref?.trim(); } return !val; };This matches the shared SecretVar contract where
valueandrefare both optional fields.🤖 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 `@ui/components/ui/headersTable.tsx` around lines 46 - 61, The isSecretVar type guard function currently only checks for the presence of a "value" field using the "in" operator, but according to the schema, SecretVars can have only a "ref" field without a "value". This causes ref-only SecretVars to not be recognized as SecretVar objects, causing them to render through the plain input path instead of SecretVarInput. Update the isSecretVar guard to check for the presence of either "value" OR "ref" fields (both are optional), so that SecretVar objects containing only a ref are properly recognized as SecretVar types.
♻️ Duplicate comments (1)
ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx (1)
218-222:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim header SecretVar presence before allowing install.
" "currently passes this validation because the check uses raw strings, while TLS and OAuth checks already require trimmedvalue/refcontent.Proposed fix
const emptySecretVar: SecretVar = { value: "", ref: "" }; +const isSecretVarSet = (field?: SecretVar): boolean => Boolean(field?.value?.trim() || field?.ref?.trim()); + /** Strips empty TLS config so we don't send `{}` to the server. */ function buildTLSConfigPayload(tls: MCPTLSConfig | undefined): MCPTLSConfig | undefined { if (!tls) return undefined; const hasSkipVerify = tls.insecure_skip_verify === true; - const hasCACert = tls.ca_cert_pem?.value?.trim() || tls.ca_cert_pem?.ref?.trim(); + const hasCACert = isSecretVarSet(tls.ca_cert_pem); if (!hasSkipVerify && !hasCACert) return undefined; return { insecure_skip_verify: tls.insecure_skip_verify, ca_cert_pem: hasCACert ? tls.ca_cert_pem : undefined }; } @@ const headersValidationError = useMemo(() => { if ((authType !== "headers" && authType !== "per_user_headers") || !headers) return null; for (const [key, secretVar] of Object.entries(headers)) { - if (!secretVar.value && !secretVar.ref) { + if (!isSecretVarSet(secretVar)) { return `Header "${key}" must have a value`; } } @@ client_id: data.oauth_config?.client_id ?? emptySecretVar, client_secret: - data.oauth_config?.client_secret?.value?.trim() || data.oauth_config?.client_secret?.ref?.trim() + isSecretVarSet(data.oauth_config?.client_secret) ? data.oauth_config.client_secret : undefined,🤖 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 `@ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx` around lines 218 - 222, The validation check in the headersValidationError useMemo function currently accepts whitespace-only strings as valid values because it checks the raw secretVar.value without trimming. Update the condition where you check if secretVar.value exists to trim the value first using trim() method before validating, ensuring that strings containing only whitespace are rejected as invalid, consistent with the existing TLS and OAuth validation behavior.
🤖 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 `@ui/lib/types/config.ts`:
- Around line 207-208: The DefaultModelProviderKey object in the config file is
missing the `value: ""` field that is present in other SecretVar default
objects. Add `value: ""` to the DefaultModelProviderKey definition to match the
pattern used in emptySecretVar() and maintain consistency with other similar
configuration objects that define both `value` and `ref` fields.
In `@ui/lib/types/schemas.ts`:
- Line 955: The conditional logic at line 955 has an operator precedence issue
where the OR operator is evaluated before AND, causing validation to always
execute when type is undefined regardless of the url value. Fix this by adding
parentheses around the two type-checking conditions (data.push_gateway_url?.type
=== "plain_text" || !data.push_gateway_url?.type) to ensure the url check
applies to both cases, so validation only runs when url exists AND either the
type is plain_text OR the type is unset.
In `@ui/lib/utils/secretVarForm.ts`:
- Around line 17-20: The issue is that SecretVar.type is optional in the schema,
so when a valid object like { ref: "env.FOO" } exists without an explicit type
field, the type remains undefined, causing functions like toEnvRefString() and
toOptionalSecretVarPayload() to drop the external reference on save. Infer the
secret type from the ref field format (e.g., if ref starts with "env." it's an
environment variable type) before returning the flattened object in the return
statement at lines 17-20, and apply the same logic to the other affected
locations mentioned in the comment (31-34 and 56-63) where type is being used
without inferring it from ref when needed.
---
Outside diff comments:
In `@ui/components/ui/headersTable.tsx`:
- Around line 46-61: The isSecretVar type guard function currently only checks
for the presence of a "value" field using the "in" operator, but according to
the schema, SecretVars can have only a "ref" field without a "value". This
causes ref-only SecretVars to not be recognized as SecretVar objects, causing
them to render through the plain input path instead of SecretVarInput. Update
the isSecretVar guard to check for the presence of either "value" OR "ref"
fields (both are optional), so that SecretVar objects containing only a ref are
properly recognized as SecretVar types.
---
Duplicate comments:
In `@ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx`:
- Around line 218-222: The validation check in the headersValidationError
useMemo function currently accepts whitespace-only strings as valid values
because it checks the raw secretVar.value without trimming. Update the condition
where you check if secretVar.value exists to trim the value first using trim()
method before validating, ensuring that strings containing only whitespace are
rejected as invalid, consistent with the existing TLS and OAuth validation
behavior.
🪄 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: bdfb363f-66c5-4437-89a9-2b1d1a291c2f
📒 Files selected for processing (36)
core/providers/utils/utils.gocore/schemas/secretvar.gocore/schemas/secretvar_test.gocore/schemas/utils.gocore/schemas/vault.gocore/schemas/vault_test.goframework/configstore/clientconfig.goframework/configstore/clientconfig_redaction_test.goframework/configstore/rdb.goframework/configstore/tables/encryption_test.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey_secretvar_test.goframework/configstore/vault_callbacks_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.jsonui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/securityView.tsxui/app/workspace/mcp-registry/library/page.tsxui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/observability/fragments/otelFormFragment.tsxui/app/workspace/observability/fragments/prometheusFormFragment.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/deploymentsTable.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/components/onboardingWidget.tsxui/components/ui/headersTable.tsxui/components/ui/secretVarInput.tsxui/lib/constants/config.tsui/lib/types/config.tsui/lib/types/schemas.tsui/lib/utils/secretVarForm.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- framework/configstore/vault_callbacks_test.go
- ui/app/workspace/mcp-registry/library/page.tsx
- ui/lib/constants/config.ts
- core/schemas/utils.go
- ui/components/onboardingWidget.tsx
- ui/app/workspace/providers/fragments/deploymentsTable.tsx
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- transports/config.schema.json
- framework/configstore/tables/virtualkey_secretvar_test.go
- core/schemas/vault.go
- ui/app/workspace/observability/fragments/otelFormFragment.tsx
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- transports/bifrost-http/handlers/plugins.go
- framework/configstore/clientconfig_redaction_test.go
- ui/app/workspace/mcp-registry/views/mcpClientForm.tsx
- transports/bifrost-http/lib/config.go
- framework/configstore/rdb.go
- core/providers/utils/utils.go
- core/schemas/vault_test.go
- transports/bifrost-http/handlers/config.go
- framework/configstore/tables/mcp.go
- framework/configstore/clientconfig.go
- transports/bifrost-http/lib/config_test.go
- core/schemas/secretvar.go
- framework/configstore/tables/encryption_test.go
- core/schemas/secretvar_test.go
BearTS
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
2 times, most recently
from
be5b2a3 to
e2329b8
Compare
BearTS
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
from
e2329b8 to
0275a48
Compare
BearTS
force-pushed
the
06-18-feat_vk_as_secretvar
branch
from
11373fa to
399ab83
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ui/app/workspace/mcp-registry/views/mcpClientForm.tsx (1)
42-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHarden SecretVar “is set” checks (trim + real ref) instead of type-only truthiness.
Line 42 and Line 250 treat
typeas sufficient even whenrefis empty, and Line 142 accepts whitespace-only values. Use a single trimmed presence helper (valueorref) for these gates.Proposed fix
const emptySecretVar: SecretVar = { value: "", ref: "" }; +const isSecretVarSet = (field?: SecretVar): boolean => Boolean(field?.value?.trim() || field?.ref?.trim()); /** Strips empty TLS config so we don't send `{}` to the server. */ function buildTLSConfigPayload(tls: MCPTLSConfig | undefined): MCPTLSConfig | undefined { if (!tls) return undefined; const hasSkipVerify = tls.insecure_skip_verify === true; - const hasCACert = tls.ca_cert_pem?.value || (tls.ca_cert_pem?.type === "env" || tls.ca_cert_pem?.type === "vault"); + const hasCACert = isSecretVarSet(tls.ca_cert_pem); if (!hasSkipVerify && !hasCACert) return undefined; return { insecure_skip_verify: tls.insecure_skip_verify, ca_cert_pem: hasCACert ? tls.ca_cert_pem : undefined }; } @@ - for (const [key, secretVar] of Object.entries(headers)) { - if (!secretVar.value && !secretVar.ref) { + for (const [key, secretVar] of Object.entries(headers)) { + if (!isSecretVarSet(secretVar)) { headersValidationError = `Header "${key}" must have a value`; break; } } @@ client_id: data.oauth_config?.client_id ?? emptySecretVar, client_secret: - data.oauth_config?.client_secret?.value || (data.oauth_config?.client_secret?.type === "env" || data.oauth_config?.client_secret?.type === "vault") + isSecretVarSet(data.oauth_config?.client_secret) ? data.oauth_config.client_secret : undefined,Also applies to: 141-142, 250-250
🤖 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 `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx` at line 42, The current SecretVar "is set" checks on lines 42, 141-142, and 250 are insufficient because they rely only on checking if the type field equals "env" or "vault" without verifying that the actual ref value is populated and non-empty. Create a single helper function that properly determines if a SecretVar is set by checking for either a non-empty trimmed value field or a non-empty trimmed ref field, ensuring whitespace-only values are not treated as valid. Replace the type-only logic in hasCACert (line 42), the cert chain check (lines 141-142), and any similar checks around line 250 with calls to this helper function to ensure secrets are only considered "set" when they actually contain valid references.
🤖 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 `@core/schemas/secretvar_test.go`:
- Around line 65-66: Replace all instances of os.Setenv() followed by defer
os.Unsetenv() with the test helper t.Setenv() to prevent environment variable
leakage across tests. Specifically, at all occurrences mentioned (lines 65-66,
114-115, 175-176, 260-261), remove the os.Setenv("TEST_API_KEY",
"actual-api-key-value") and defer os.Unsetenv("TEST_API_KEY") pattern and
replace with a single call to t.Setenv("TEST_API_KEY", "actual-api-key-value").
The t.Setenv helper automatically restores the original environment variable
value after the test completes, ensuring no state leakage between tests.
- Around line 577-579: The assertion for the Val field in the test is guarded by
a condition that checks if tt.wantVal is not empty, which causes the test to
skip validation when the expected value is an empty string. Remove the
conditional check `tt.wantVal != ""` and always execute the assertion that
compares e.Val with tt.wantVal, so that the test can catch regressions where
Scan incorrectly sets Val to a non-empty string when it should be empty.
---
Duplicate comments:
In `@ui/app/workspace/mcp-registry/views/mcpClientForm.tsx`:
- Line 42: The current SecretVar "is set" checks on lines 42, 141-142, and 250
are insufficient because they rely only on checking if the type field equals
"env" or "vault" without verifying that the actual ref value is populated and
non-empty. Create a single helper function that properly determines if a
SecretVar is set by checking for either a non-empty trimmed value field or a
non-empty trimmed ref field, ensuring whitespace-only values are not treated as
valid. Replace the type-only logic in hasCACert (line 42), the cert chain check
(lines 141-142), and any similar checks around line 250 with calls to this
helper function to ensure secrets are only considered "set" when they actually
contain valid references.
🪄 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: 1aa22525-1c92-4416-90b1-5ec190232b0d
📒 Files selected for processing (42)
core/providers/utils/utils.gocore/schemas/secretvar.gocore/schemas/secretvar_test.gocore/schemas/utils.gocore/schemas/vault.gocore/schemas/vault_test.goframework/configstore/clientconfig.goframework/configstore/clientconfig_redaction_test.goframework/configstore/encryption_test.goframework/configstore/rdb.goframework/configstore/tables/encryption.goframework/configstore/tables/encryption_test.goframework/configstore/tables/mcp.goframework/configstore/tables/oauth.goframework/configstore/tables/virtualkey_secretvar_test.goframework/configstore/vault_callbacks_test.goplugins/otel/main.goplugins/telemetry/main.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/ctx_test.gotransports/config.schema.jsonui/app/workspace/config/views/mcpView.tsxui/app/workspace/config/views/securityView.tsxui/app/workspace/mcp-registry/library/page.tsxui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsxui/app/workspace/mcp-registry/views/mcpClientForm.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/observability/fragments/otelFormFragment.tsxui/app/workspace/observability/fragments/prometheusFormFragment.tsxui/app/workspace/providers/fragments/apiKeysFormFragment.tsxui/app/workspace/providers/fragments/deploymentsTable.tsxui/app/workspace/providers/views/modelProviderKeysTableView.tsxui/components/onboardingWidget.tsxui/components/ui/headersTable.tsxui/components/ui/secretVarInput.tsxui/lib/constants/config.tsui/lib/types/config.tsui/lib/types/schemas.tsui/lib/utils/secretVarForm.ts
💤 Files with no reviewable changes (1)
- transports/bifrost-http/lib/config_test.go
✅ Files skipped from review due to trivial changes (1)
- transports/bifrost-http/lib/ctx_test.go
🚧 Files skipped from review as they are similar to previous changes (37)
- ui/app/workspace/mcp-registry/library/page.tsx
- ui/app/workspace/providers/views/modelProviderKeysTableView.tsx
- ui/app/workspace/observability/fragments/otelFormFragment.tsx
- framework/configstore/tables/encryption.go
- ui/components/onboardingWidget.tsx
- core/schemas/utils.go
- framework/configstore/vault_callbacks_test.go
- framework/configstore/tables/oauth.go
- transports/config.schema.json
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- plugins/telemetry/main.go
- transports/bifrost-http/handlers/plugins.go
- ui/app/workspace/config/views/securityView.tsx
- framework/configstore/encryption_test.go
- framework/configstore/clientconfig_redaction_test.go
- core/schemas/vault.go
- ui/app/workspace/observability/fragments/prometheusFormFragment.tsx
- ui/lib/utils/secretVarForm.ts
- plugins/otel/main.go
- transports/bifrost-http/handlers/config.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/app/workspace/providers/fragments/deploymentsTable.tsx
- framework/configstore/tables/virtualkey_secretvar_test.go
- core/schemas/vault_test.go
- ui/components/ui/secretVarInput.tsx
- ui/app/workspace/mcp-registry/library/views/mcpLibraryInstallSheet.tsx
- ui/lib/types/schemas.ts
- ui/components/ui/headersTable.tsx
- core/providers/utils/utils.go
- core/schemas/secretvar.go
- ui/app/workspace/config/views/mcpView.tsx
- transports/bifrost-http/lib/config.go
- framework/configstore/tables/encryption_test.go
- framework/configstore/tables/mcp.go
- framework/configstore/rdb.go
- ui/lib/types/config.ts
- framework/configstore/clientconfig.go
Comment on lines
65
to
66
| os.Setenv("TEST_API_KEY", "actual-api-key-value") | ||
| defer os.Unsetenv("TEST_API_KEY") |
Contributor
There was a problem hiding this comment.
Use t.Setenv to avoid env leakage across tests.
These tests mutate process-wide env and then always Unsetenv, which can clobber a pre-existing value and leak state between tests. Prefer t.Setenv(...) so original values are restored automatically.
Suggested fix
- os.Setenv("TEST_API_KEY", "actual-api-key-value")
- defer os.Unsetenv("TEST_API_KEY")
+ t.Setenv("TEST_API_KEY", "actual-api-key-value")
...
- os.Setenv("MY_KEY", "resolved-value")
- defer os.Unsetenv("MY_KEY")
+ t.Setenv("MY_KEY", "resolved-value")
...
- os.Setenv("TEST_NEW_ENVVAR_KEY", "resolved-value")
- defer os.Unsetenv("TEST_NEW_ENVVAR_KEY")
+ t.Setenv("TEST_NEW_ENVVAR_KEY", "resolved-value")
...
- os.Setenv("TEST_PROJECT_ID", "env-project-id")
- defer os.Unsetenv("TEST_PROJECT_ID")
+ t.Setenv("TEST_PROJECT_ID", "env-project-id")Also applies to: 114-115, 175-176, 260-261
🤖 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 `@core/schemas/secretvar_test.go` around lines 65 - 66, Replace all instances
of os.Setenv() followed by defer os.Unsetenv() with the test helper t.Setenv()
to prevent environment variable leakage across tests. Specifically, at all
occurrences mentioned (lines 65-66, 114-115, 175-176, 260-261), remove the
os.Setenv("TEST_API_KEY", "actual-api-key-value") and defer
os.Unsetenv("TEST_API_KEY") pattern and replace with a single call to
t.Setenv("TEST_API_KEY", "actual-api-key-value"). The t.Setenv helper
automatically restores the original environment variable value after the test
completes, ensuring no state leakage between tests.
Contributor
Merge activity
|
akshaydeo
changed the base branch from
06-18-feat_vk_as_secretvar
to
graphite-base/4595
akshaydeo
force-pushed
the
06-22-fix_add_missing_check_for_vault
branch
from
0275a48 to
69092c2
Compare
Comment on lines
453
to
461
| func (e *SecretVar) ShouldPreserveStored() bool { | ||
| if e == nil { | ||
| return true | ||
| } | ||
| if e.IsFromEnv() || e.IsFromVault() { | ||
| if e.IsFromSecret() { | ||
| return false | ||
| } | ||
| return e.GetValue() == "" || e.IsRedacted() | ||
| } |
Contributor
There was a problem hiding this comment.
ShouldPreserveStored returns false unconditionally for every secret reference
The doc comment says "Returns false for secret references (always intentional)", but this causes a concrete regression in the auth handler: every UI save that echoes back an env/vault-backed admin_password (which GET /api/config always does via FullyRedacted) evaluates passwordChanged = true and triggers FlushSessions, logging out all active dashboard sessions even when nothing changed. The old !IsRedacted() guard was false for env/vault values, so no flush occurred unless the literal hash changed. Consider returning true when the ref matches the already-stored ref, or document that callers doing auth-change detection must compare refs explicitly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates the
SecretVartype's dual-source tracking fields (FromEnv/EnvVarandFromVault/VaultRef) into a unifiedSecretTypeenum with a singlerefstring field. Both environment variable references (env.VAR) and vault references (vault.path) are now represented identically, with the reference string stored inrefand the source type inSecretType. Backward compatibility is preserved for the oldenv_var/from_envJSON format.Changes
SecretVarstruct fieldsFromEnv,EnvVar,FromVault, andVaultRefreplaced with aSecretTypeenum (plain_text,env,vault) and a single unexportedrefstring.IsFromEnv()andIsFromVault()methods supplemented with a unifiedIsFromSecret()method;GetRawSecretRef(),GetRef(),EnvKey(),VaultPath(), andType()accessors added.MarshalJSONadded to emitrefandtypefields;UnmarshalJSON,Scan,Value,Redacted,FullyRedacted,Equals,IsSet, andShouldPreserveStoredupdated to use the unified fields.env_var/from_envJSON payloads are transparently migrated toref/typeon deserialization.StoreVaultSecretVarnow setsrefto"vault." + pathandSecretTypetoSecretTypeVaultinstead of populating the oldVaultReffield.removeOwnedVaultSecretVarupdated to useGetRef()instead of manually stripping thevault.prefix fromVaultRef.IsFromSecret()andGetRawSecretRef().SecretVartype updated:env_var,from_env,vault_var, andfrom_vaultfields replaced withrefandfrom_secret; Zod schemas, form utilities, default constants, and all component logic updated accordingly.config.schema.jsonupdated to includerefandtypefields alongside the legacyenv_var/from_envfields for backward compatibility.env_var/from_envJSON format.Type of change
Affected areas
How to test
Verify that existing
env.VARandvault.pathreferences in stored configs continue to resolve correctly after the migration, including configs written in the oldenv_var/from_envformat.Breaking changes
The
SecretVarJSON representation changes fromenv_var/from_env/vault_var/from_vaulttoref/type. Deserialization of the old format remains supported, but any client or config consuming API responses will receive the new format. The UI has been updated accordingly; external API consumers producing the old format will continue to work on ingest.Security considerations
Secret source metadata previously exposed separate env var names and vault paths in distinct fields. These are now unified under
ref, which is preserved in redacted responses so references remain visible to operators without leaking resolved values. TheScanmethod no longer strips quotes before checking forvault.prefixes, preventing a quoted literal string such as"vault.x"from being misidentified as a vault reference.Checklist
docs/contributing/README.mdand followed the guidelines