refactor: encapsulate SecretVar internals behind typed SecretType enum and accessor methods#4599
Closed
BearTS wants to merge 3 commits into
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 (18)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesSecretVar struct refactor and API surface change
Vault storage and removal integration with SecretType
Configstore hashing, storage, and redaction
Transport handlers and HTTP config flow
Provider utilities and schema extension
Encryption and persistence tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 22, 2026
Contributor
Author
18 tasks
BearTS
changed the title
refactor: encapsulate SecretVar fields and expose via Ref()/IsFromSecret() accessors with explicit MarshalJSON
Contributor
Confidence Score: 4/5Safe to merge after restoring the early-return fail-closed guards in The transports/bifrost-http/lib/config.go — the Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class SecretVar {
+string Val
-string secretRef
+SecretType SecretType
+GetSecretRef() string
+IsFromSecret() bool
+IsFromVault() bool
+IsFromEnv() bool
+IsSet() bool
+IsRedacted() bool
+Redacted() SecretVar
+FullyRedacted() SecretVar
+MarshalJSON() []byte
+UnmarshalJSON(data []byte) error
+Scan(value any) error
+Value() driver.Value
}
class SecretType {
<<enumeration>>
plain_text
env
vault
}
class NewSecretVar {
<<constructor>>
+NewSecretVar(value string) SecretVar
}
SecretVar --> SecretType
NewSecretVar --> SecretVar
%%{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"}}}%%
classDiagram
class SecretVar {
+string Val
-string secretRef
+SecretType SecretType
+GetSecretRef() string
+IsFromSecret() bool
+IsFromVault() bool
+IsFromEnv() bool
+IsSet() bool
+IsRedacted() bool
+Redacted() SecretVar
+FullyRedacted() SecretVar
+MarshalJSON() []byte
+UnmarshalJSON(data []byte) error
+Scan(value any) error
+Value() driver.Value
}
class SecretType {
<<enumeration>>
plain_text
env
vault
}
class NewSecretVar {
<<constructor>>
+NewSecretVar(value string) SecretVar
}
SecretVar --> SecretType
NewSecretVar --> SecretVar
Reviews (4): Last reviewed commit: "feat: add secret type instead of from se..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/schemas/vault_test.go (1)
174-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen the env-map assertion to fail on unintended mutation (Line 174).
This check only verifies “not stored” when metadata still equals
env.X; if the entry is mutated, the test can pass silently.Suggested fix
- if env := m.Headers["X-Env"]; env.IsFromSecret() && env.Ref() == "env.X" { - if stored["bifrost/m/1/headers/X-Env"] != "" { - t.Error("env-sourced header should not be vault-stored") - } - } + env := m.Headers["X-Env"] + if !env.IsFromSecret() || env.Ref() != "env.X" { + t.Errorf("env entry metadata mutated: val=%q ref=%q fromSecret=%v", env.Val, env.Ref(), env.IsFromSecret()) + } + if stored["bifrost/m/1/headers/X-Env"] != "" { + t.Error("env-sourced header should not be vault-stored") + }🤖 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/vault_test.go` around lines 174 - 178, The conditional check in the headers validation block only performs the vault storage assertion when env.Ref() equals "env.X", which means if the reference value gets mutated unexpectedly, the condition becomes false and the assertion never runs, allowing the bug to pass silently. Strengthen this test by either adding an explicit assertion that verifies env.Ref() must equal "env.X" before performing the storage check, or by moving the vault storage check outside of the conditional logic to ensure it always validates that env-sourced headers are not stored in the vault, regardless of how the metadata reference might change.
🤖 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 258-270: The SecretVar.MarshalJSON method is calling sonic.Marshal
directly instead of using the package-level Marshal() wrapper function for
consistency with other MarshalJSON implementations in the same package (such as
OptionalJSON.MarshalJSON). Replace the sonic.Marshal call with Marshal on the
line that returns the marshaled struct in the SecretVar.MarshalJSON method to
ensure consistency across the package.
---
Outside diff comments:
In `@core/schemas/vault_test.go`:
- Around line 174-178: The conditional check in the headers validation block
only performs the vault storage assertion when env.Ref() equals "env.X", which
means if the reference value gets mutated unexpectedly, the condition becomes
false and the assertion never runs, allowing the bug to pass silently.
Strengthen this test by either adding an explicit assertion that verifies
env.Ref() must equal "env.X" before performing the storage check, or by moving
the vault storage check outside of the conditional logic to ensure it always
validates that env-sourced headers are not stored in the vault, regardless of
how the metadata reference might change.
🪄 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: 7e2139b3-7596-4a90-a84f-bde2e848dcc7
📒 Files selected for processing (11)
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/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.go
BearTS
force-pushed
the
06-22-feat_make_secretref_and_fromsecret_non_global_fields
branch
from
983be90 to
6ccf345
Compare
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/lib/config.go`:
- Around line 3694-3700: In the config loading path around
authConfig.AdminUserName and authConfig.AdminPassword validation, when either
credential has IsFromSecret() returning true and GetValue() returning empty, add
an early return or skip mechanism to prevent persistence rather than just
logging a warning. Currently the code logs a warning but continues to persist
the empty credentials at line 3755, which overwrites valid passwords in the
database. Instead, when authConfig.AdminUserName or authConfig.AdminPassword
meets the condition (IsFromSecret() is true and GetValue() is empty), return
early from the function or set a flag to skip the persistence step entirely,
matching the error-handling behavior in the HTTP handler 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: a2634024-30f0-4713-b00d-193788c61971
📒 Files selected for processing (11)
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/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.go
✅ Files skipped from review due to trivial changes (2)
- framework/configstore/tables/mcp.go
- core/providers/utils/utils.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/schemas/utils.go
- framework/configstore/rdb.go
- core/schemas/vault.go
- framework/configstore/clientconfig.go
- core/schemas/vault_test.go
- core/schemas/secretvar.go
- core/schemas/secretvar_test.go
Comment on lines
3694
to
3700
| // Fail-closed: if env/vault reference is unresolved, don't persist empty credentials. | ||
| if authConfig.AdminUserName != nil && authConfig.AdminUserName.GetValue() == "" && authConfig.AdminUserName.IsFromSecret() { | ||
| logger.Warn("username set with external reference but value is empty: %s — skipping auth config", authConfig.AdminUserName.SecretRef) | ||
| if dbAuthConfig != nil { | ||
| config.GovernanceConfig.AuthConfig = dbAuthConfig | ||
| } | ||
| return | ||
| logger.Warn("username set with external reference but value is empty: %s", authConfig.AdminUserName.Ref()) | ||
| } | ||
| if authConfig.AdminPassword != nil && authConfig.AdminPassword.GetValue() == "" && authConfig.AdminPassword.IsFromSecret() { | ||
| logger.Warn("password set with external reference but value is empty: %s — skipping auth config", authConfig.AdminPassword.SecretRef) | ||
| if dbAuthConfig != nil { | ||
| config.GovernanceConfig.AuthConfig = dbAuthConfig | ||
| } | ||
| return | ||
| logger.Warn("password set with external reference but value is empty: %s", authConfig.AdminPassword.Ref()) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any early-return or persistence skip logic for unresolved secrets
# after the warning logs in loadAuthConfig
# Search for the full loadAuthConfig function to see if there's protection downstream
ast-grep --pattern $'func loadAuthConfig($_, $_, $_) {
$$$
}'Repository: maximhq/bifrost
Length of output: 8787
Skip persistence when external secret references are unresolved, not just warn.
The comment claims "Fail-closed: if env/vault reference is unresolved, don't persist empty credentials," but the code only logs a warning (lines 3694–3700) and continues processing. When an unresolved environment variable causes GetValue() to return empty:
- Line 3705:
filePasswordbecomes"" - If the DB has an existing valid password,
passwordMatchwill be false (line 3712) - Code continues past line 3725 without early return
- Empty password is hashed and persisted at line 3755, overwriting the valid DB password
The HTTP handler path (handlers/config.go) correctly returns an error for unresolved secrets. The config-file loading path should similarly return early or skip persistence when IsFromSecret() is true and the value is empty, to prevent credential loss during transient env var unavailability.
🤖 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 `@transports/bifrost-http/lib/config.go` around lines 3694 - 3700, In the
config loading path around authConfig.AdminUserName and authConfig.AdminPassword
validation, when either credential has IsFromSecret() returning true and
GetValue() returning empty, add an early return or skip mechanism to prevent
persistence rather than just logging a warning. Currently the code logs a
warning but continues to persist the empty credentials at line 3755, which
overwrites valid passwords in the database. Instead, when
authConfig.AdminUserName or authConfig.AdminPassword meets the condition
(IsFromSecret() is true and GetValue() is empty), return early from the function
or set a flag to skip the persistence step entirely, matching the error-handling
behavior in the HTTP handler path.
Comment on lines
+322
to
+323
| a: NewSecretVarFromRef("env.TEST", "test"), | ||
| b: NewSecretVarFromRef("env.TEST", "test"), |
Contributor
There was a problem hiding this comment.
NewSecretVarFromRef is undefined — test package will not compile
The function NewSecretVarFromRef is called throughout secretvar_test.go and vault_test.go (24+ call sites) but is not defined anywhere in the codebase. The PR description lists it as a new constructor to be added, and the test migration from direct struct literals to NewSecretVarFromRef is the entire point of the change, but the function definition is missing from secretvar.go (or any other file in core/schemas). Running go test ./core/schemas/... will fail with undefined: NewSecretVarFromRef.
18 tasks
BearTS
changed the title
refactor: encapsulate SecretVar fields and expose via Ref()/IsFromSecret() accessors with explicit MarshalJSON
Comment on lines
3694
to
3700
| // Fail-closed: if env/vault reference is unresolved, don't persist empty credentials. | ||
| if authConfig.AdminUserName != nil && authConfig.AdminUserName.GetValue() == "" && authConfig.AdminUserName.IsFromSecret() { | ||
| logger.Warn("username set with external reference but value is empty: %s — skipping auth config", authConfig.AdminUserName.SecretRef) | ||
| if dbAuthConfig != nil { | ||
| config.GovernanceConfig.AuthConfig = dbAuthConfig | ||
| } | ||
| return | ||
| logger.Warn("username set with external reference but value is empty: %s", authConfig.AdminUserName.GetSecretRef()) | ||
| } | ||
| if authConfig.AdminPassword != nil && authConfig.AdminPassword.GetValue() == "" && authConfig.AdminPassword.IsFromSecret() { | ||
| logger.Warn("password set with external reference but value is empty: %s — skipping auth config", authConfig.AdminPassword.SecretRef) | ||
| if dbAuthConfig != nil { | ||
| config.GovernanceConfig.AuthConfig = dbAuthConfig | ||
| } | ||
| return | ||
| logger.Warn("password set with external reference but value is empty: %s", authConfig.AdminPassword.GetSecretRef()) | ||
| } |
Contributor
There was a problem hiding this comment.
The refactor removed the return (and config.GovernanceConfig.AuthConfig = dbAuthConfig) that followed each warning when an admin credential backed by an unresolved env/vault reference was detected. The comment still says "Fail-closed" but the code no longer is.
When AdminUserName or AdminPassword resolves to an empty string (e.g., the env var is missing at startup), the new code logs a warning and falls through to the matching logic. There, filePassword is "", the bcrypt comparison against the stored hash fails (passwordMatch = false), FlushSessions is called (dropping all active sessions), and then an empty-string password is hashed and written to the database — overwriting the valid stored credential. This is the opposite of the intended fail-closed behavior.
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
1 participant
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
SecretVar's internal fieldsSecretRefandFromSecretwere previously exported, allowing callers to read and write them directly. This PR encapsulates those fields as unexported (secretRef,SecretType) and exposes them through a controlled public API (GetSecretRef(),IsFromSecret(),IsFromVault(),IsFromEnv()). A newSecretTypeenum (plain_text,env,vault) replaces the booleanFromSecretflag, making the source of a secret explicit. JSON serialization is handled via an explicitMarshalJSONmethod that emitssecret_refandtypefields, with full backward compatibility for the legacyfrom_secret,env_var, andfrom_envshapes on unmarshal.Changes
SecretRefandFromSecretfields onSecretVarare now unexported (secretRef).FromSecret boolis replaced bySecretType SecretTypewith constantsSecretTypePlainText,SecretTypeEnv, andSecretTypeVault.GetSecretRef() stringaccessor exposes the secret reference string.Type() SecretTypeaccessor exposes the secret type.IsFromEnv() boolmethod is added alongside the existingIsFromSecret()andIsFromVault().MarshalJSONmethod is added so JSON serialization emitssecret_refandtypefields correctly despite the fields being unexported.inferSecretTypeis introduced to derive theSecretTypefrom a reference string prefix when reading legacy formats.SecretRefandFromSecretacross proxy/TLS configuration, config handlers, hash generation, MCP table serialization, vault operations, and config store are updated to use the new accessor methods.GetSecretRef(),IsFromSecret(),IsFromVault(), andIsFromEnv()instead of direct field access, and test fixtures constructingSecretVarstruct literals are migrated toNewSecretVaror the new typed field form.secret_refandtypealongside the legacyenv_var/from_envfields.Type of change
Affected areas
How to test
go test ./core/schemas/... ./core/providers/utils/... ./framework/configstore/... ./transports/bifrost-http/...Verify that secret references (env and vault) round-trip correctly through JSON marshal/unmarshal, that
GetSecretRef()returns the expected reference string, and thatIsFromSecret(),IsFromVault(), andIsFromEnv()behave as expected. Confirm that legacy JSON payloads usingfrom_secret,env_var, andfrom_envfields continue to deserialize correctly.Breaking changes
Any code outside this repository that directly accessed
SecretVar.SecretReforSecretVar.FromSecretwill no longer compile. Callers must migrate toGetSecretRef()andIsFromSecret()respectively. Code constructingSecretVarstruct literals with those fields must be updated to useNewSecretVaror the newSecretType-based form. The JSON serialization shape changes from emittingfrom_secret/env_var/from_envto emittingsecret_refandtype; consumers reading that JSON must handle the new format.Security considerations
Encapsulating
secretRefas an unexported field and replacing the ambiguousFromSecret boolwith an explicitSecretTypeenum prevents external packages from constructing aSecretVarin an inconsistent state (e.g., settingFromSecret: truewithout a valid reference, or bypassing the constructor's resolution logic). This reduces the risk of accidentally leaking or misrepresenting secret reference metadata in API responses.Checklist
docs/contributing/README.mdand followed the guidelines