feat: migrate TableVirtualKey.Value from string to SecretVar to support env/vault references - #4504
Merged
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 (34)
📝 WalkthroughSummary by CodeRabbitRelease Notes
Walkthrough
ChangesVirtual Key SecretVar Flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 17, 2026
Contributor
Author
18 tasks
BearTS
changed the title
SecretVar (env/vault) as virtual key value with DB persistence, hash stability, and UI resolution
BearTS
marked this pull request as ready for review
June 17, 2026 19:00
BearTS
marked this pull request as draft
June 17, 2026 19:04
Contributor
Confidence Score: 3/5The in-memory auth path works correctly, but the DB-backed lookup path used by MCP sessions, list-models, and provider queries breaks for any env-sourced VK after its environment variable is rotated — and for API-created env-sourced VKs there is no automatic recovery mechanism. Two distinct defects affect the DB-backed lookup path and the encryption-status metadata written for env/vault refs. The stale-hash issue (rdb.go) means MCP auth and other DB-lookup callers return ErrNotFound after an env var changes value, with no self-healing path for API-created VKs. The encryption-status mismatch (virtualkey.go) risks silent failures in any re-encryption tooling that trusts that column. Neither defect prevents in-memory governance from working, so the service remains functional for the common case, but the edge conditions are reachable in production. framework/configstore/rdb.go (GetVirtualKeyByValue hash/fallback logic for env refs) and framework/configstore/tables/virtualkey.go (EncryptionStatus assignment in BeforeSave). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant Handler
participant DB
participant InMem as In-Memory Store
Note over Handler,DB: Create env-sourced VK
Client->>Handler: "POST value=env.MY_VK"
Handler->>Handler: UnmarshalJSON resolves MY_VK to sk-bf-v1
Handler->>DB: "BeforeSave writes value=env.MY_VK, value_hash=SHA256(sk-bf-v1)"
DB-->>Handler: OK
Note over Client,InMem: Auth — in-memory path
Client->>Handler: x-bf-vk sk-bf-v1
Handler->>InMem: Load(sk-bf-v1) found
Note over DB,InMem: After env var rotation MY_VK=sk-bf-v2
Handler->>DB: GetVirtualKeyByValue(sk-bf-v2)
DB-->>Handler: "SHA256(v2) != SHA256(v1) AND env.MY_VK != sk-bf-v2 — ErrNotFound"
Handler->>InMem: Load(sk-bf-v2)
InMem-->>Handler: found via Scan re-resolution
%%{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"}}}%%
sequenceDiagram
participant Client
participant Handler
participant DB
participant InMem as In-Memory Store
Note over Handler,DB: Create env-sourced VK
Client->>Handler: "POST value=env.MY_VK"
Handler->>Handler: UnmarshalJSON resolves MY_VK to sk-bf-v1
Handler->>DB: "BeforeSave writes value=env.MY_VK, value_hash=SHA256(sk-bf-v1)"
DB-->>Handler: OK
Note over Client,InMem: Auth — in-memory path
Client->>Handler: x-bf-vk sk-bf-v1
Handler->>InMem: Load(sk-bf-v1) found
Note over DB,InMem: After env var rotation MY_VK=sk-bf-v2
Handler->>DB: GetVirtualKeyByValue(sk-bf-v2)
DB-->>Handler: "SHA256(v2) != SHA256(v1) AND env.MY_VK != sk-bf-v2 — ErrNotFound"
Handler->>InMem: Load(sk-bf-v2)
InMem-->>Handler: found via Scan re-resolution
Reviews (9): Last reviewed commit: "feat: vk as secretVar" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/tables/virtualkey.go (1)
362-368:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not decrypt an empty
value_source_refon existing encrypted rows.After the migration, already-encrypted VK rows can have
EncryptionStatusEncryptedwith an empty newvalue_source_ref. Line 385 then attempts to decrypt an empty/plain value and can make VK loads fail. Only encrypt/decrypt the source ref when it is non-empty.Proposed fix
if encrypt.IsEnabled() && vk.Value != "" { if err := encryptString(&vk.Value); err != nil { return fmt.Errorf("failed to encrypt virtual key value: %w", err) } - if err := encryptString(&vk.ValueSourceRef); err != nil { - return fmt.Errorf("failed to encrypt virtual key value source ref: %w", err) + if vk.ValueSourceRef != "" { + if err := encryptString(&vk.ValueSourceRef); err != nil { + return fmt.Errorf("failed to encrypt virtual key value source ref: %w", err) + } } vk.EncryptionStatus = EncryptionStatusEncrypted }switch vk.EncryptionStatus { case EncryptionStatusEncrypted: if err := decryptString(&vk.Value); err != nil { return fmt.Errorf("failed to decrypt virtual key value: %w", err) } - if err := decryptString(&vk.ValueSourceRef); err != nil { - return fmt.Errorf("failed to decrypt virtual key value source ref: %w", err) + if vk.ValueSourceRef != "" { + if err := decryptString(&vk.ValueSourceRef); err != nil { + return fmt.Errorf("failed to decrypt virtual key value source ref: %w", err) + } } }Based on learnings,
AfterFindhooks should guard decrypt calls by checking for empty or placeholder values before decrypting.Also applies to: 380-387
🤖 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 `@framework/configstore/tables/virtualkey.go` around lines 362 - 368, In the encryption block where vk.Value and vk.ValueSourceRef are encrypted, add a non-empty check before encrypting vk.ValueSourceRef to prevent attempting to encrypt empty values. Specifically, modify the encryptString call for vk.ValueSourceRef to only execute when vk.ValueSourceRef is not empty (change from encrypting unconditionally to adding a condition like if vk.ValueSourceRef != "" before the encryptString call). Additionally, apply the same guard in the corresponding AfterFind hook decryption block around lines 380-387 to ensure empty or placeholder values are not decrypted, preventing VK load failures on already-encrypted rows that have an empty value_source_ref.Source: Learnings
🤖 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 `@framework/configstore/clientconfig.go`:
- Around line 812-813: The hash computation for vk.Value and vk.ValueSourceRef
lacks boundary encoding between them, which can cause hash collisions when
different value pairs produce the same byte sequence. Add length-prefixed
encoding or a separator between the hash.Write calls for vk.Value and
vk.ValueSourceRef to disambiguate the hash input and ensure that different
(Value, ValueSourceRef) combinations always produce different hashes, preventing
missed config resync triggers.
In `@framework/configstore/migrations.go`:
- Around line 10835-10840: The Rollback function uses GORM's mg.DropColumn
method to drop the "ValueSourceRef" column from tables.TableVirtualKey, but this
will fail with SQLite foreign key constraint errors during the table rebuild
within a transaction (as documented in the migrationDropVKAccessProfileIDColumn
migration). Replace the mg.DropColumn call with a raw SQL approach using the
dropColumnSQL() helper function, following the same pattern established in
migrationDropVKAccessProfileIDColumn at line 10031, to ensure the column drop
can succeed without triggering FK rebuild failures.
In `@framework/configstore/tables/virtualkey.go`:
- Around line 347-356: The switch statement that sets ValueSourceRef is executed
on every save operation, including when a VirtualKey value is rotated. When a VK
loaded from env or vault is rotated (generating a new value), the ValueSourceRef
should be cleared to empty string to indicate it's no longer sourced from that
external location, rather than re-deriving it from the current ValueSource.
Modify the code to detect when a rotation is occurring (the value is being
changed/generated) and in that rotation path, set ValueSourceRef to an empty
string. The current switch logic that re-derives ValueSourceRef from IsFromEnv()
and IsFromVault() should only execute when the value is NOT being rotated.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 1314-1317: The code currently assigns the resolved value from
req.Value.GetValue() directly to vk.Value without validating that the resolved
value is non-empty. This can cause unresolved env or vault references to
overwrite the generated credential with an empty string, resulting in an
unusable VK. Modify the assignment block to call GetValue() once, validate that
the result is non-empty (not ""), and only assign to vk.Value and vk.ValueSource
if the resolved value is a non-empty plaintext string. This prevents empty
resolved values from replacing the generated key.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2255-2259: When a virtual key Value fails the prefix check and is
replaced via governance.GenerateVirtualKey(), the ValueSourceRef and ValueSource
fields are not being reset, creating a mismatch between the auto-generated value
and its source metadata. Locate all places where governance.GenerateVirtualKey()
is called to replace an invalid Value (in the config validation logic around the
virtual keys handling), and in each of those locations, reset the ValueSourceRef
and ValueSource fields to empty strings immediately after assigning the newly
generated value to maintain metadata consistency across the SecretVar contract.
In `@transports/config.schema.json`:
- Around line 705-706: The object schema in the anyOf array at lines 705-706
declares allowed properties but does not enforce that at least one resolvable
field must be present, allowing invalid objects like {} or {"from_env": true} to
pass validation. Add a "required" constraint to the object schema definition
that mandates at least one of the actual value/source fields ("value",
"env_var", or "vault_var") must be present, ensuring the SecretVar object always
has a usable value or reference as required by the corresponding type definition
in ui/lib/types/schemas.ts.
In `@ui/components/prompts/components/apiKeySelectorView.tsx`:
- Around line 34-35: The virtual key options are being keyed by their resolved
values (o.value) which can cause React rendering issues if two virtual keys
resolve to identical plaintext values. In the rendering code around lines 69 and
80 where the options from vkOpts are rendered, use a stable unique identifier
(vk.id) as the React key instead of o.value. Additionally, add data-testid
attributes to the ComboboxItem elements for each option to enable E2E test
compatibility per the coding guidelines.
---
Outside diff comments:
In `@framework/configstore/tables/virtualkey.go`:
- Around line 362-368: In the encryption block where vk.Value and
vk.ValueSourceRef are encrypted, add a non-empty check before encrypting
vk.ValueSourceRef to prevent attempting to encrypt empty values. Specifically,
modify the encryptString call for vk.ValueSourceRef to only execute when
vk.ValueSourceRef is not empty (change from encrypting unconditionally to adding
a condition like if vk.ValueSourceRef != "" before the encryptString call).
Additionally, apply the same guard in the corresponding AfterFind hook
decryption block around lines 380-387 to ensure empty or placeholder values are
not decrypted, preventing VK load failures on already-encrypted rows that have
an empty value_source_ref.
🪄 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: f5269d9e-f80a-4b28-94ef-3e4a018af1c8
📒 Files selected for processing (16)
.github/workflows/scripts/schemasync/main.goframework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/sqlite.goframework/configstore/tables/virtualkey.goframework/configstore/tables/virtualkey_secretvar_test.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.gotransports/config.schema.jsonui/app/workspace/mcp-registry/views/mcpUsageGuide/commandBuilders.tsui/app/workspace/mcp-registry/views/mcpUsageGuide/mcpUsageGuideSheet.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/components/prompts/components/apiKeySelectorView.tsxui/components/prompts/fragments/settingsPanel.tsxui/lib/types/governance.ts
BearTS
force-pushed
the
06-18-feat_vk_as_secretvar
branch
from
June 18, 2026 05:41
6f0884f to
e2f7200
Compare
BearTS
marked this pull request as ready for review
June 18, 2026 06:25
BearTS
changed the title
SecretVar (env/vault) as virtual key value with DB persistence, hash stability, and UI resolutionTableVirtualKey.Value from string to SecretVar to support env/vault references
BearTS
force-pushed
the
06-18-feat_vk_as_secretvar
branch
from
June 18, 2026 07:09
e2f7200 to
b03fb66
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 `@framework/configstore/tables/virtualkey_secretvar_test.go`:
- Around line 78-79: The test calls BeforeSave with a nil argument on both the
literal and envSourced variables, which can panic if VaultStoreWriteEnabled
returns true since the implementation dereferences tx.Statement.Table. Fix this
by either: (1) using setupTestDB(t) to create a proper test database instance
and passing that database to the BeforeSave calls instead of nil, or (2) if
vault functionality must remain disabled for this test, add an explicit comment
above each BeforeSave call documenting that the test assumes vault store is
disabled and explaining why passing nil is safe in this specific context.
🪄 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: 4958cc03-afbf-4739-9f90-f86ea3f0fef1
📒 Files selected for processing (27)
framework/configstore/clientconfig.goframework/configstore/encryption_test.goframework/configstore/migrations_test.goframework/configstore/rdb_deadlock_postgres_test.goframework/configstore/rdb_mcp_sessions_test.goframework/configstore/rdb_test.goframework/configstore/sqlite.goframework/configstore/tables/encryption_test.goframework/configstore/tables/virtualkey.goframework/configstore/tables/virtualkey_secretvar_test.goframework/logstore/asyncjob_test.goplugins/governance/store.goplugins/governance/store_test.goplugins/governance/test_utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/mcp-registry/views/mcpUsageGuide/commandBuilders.tsui/app/workspace/mcp-registry/views/mcpUsageGuide/mcpUsageGuideSheet.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/components/prompts/components/apiKeySelectorView.tsxui/components/prompts/fragments/settingsPanel.tsxui/lib/types/governance.ts
✅ Files skipped from review due to trivial changes (1)
- transports/config.schema.json
🚧 Files skipped from review as they are similar to previous changes (25)
- plugins/governance/test_utils.go
- framework/configstore/rdb_mcp_sessions_test.go
- framework/logstore/asyncjob_test.go
- ui/components/prompts/components/apiKeySelectorView.tsx
- framework/configstore/sqlite.go
- framework/configstore/encryption_test.go
- transports/bifrost-http/handlers/mcpserver.go
- plugins/governance/store_test.go
- transports/bifrost-http/handlers/governance.go
- framework/configstore/clientconfig.go
- ui/components/prompts/fragments/settingsPanel.tsx
- framework/configstore/migrations_test.go
- plugins/governance/store.go
- ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
- ui/lib/types/governance.ts
- transports/bifrost-http/lib/config.go
- transports/bifrost-http/server/server.go
- ui/app/workspace/mcp-registry/views/mcpUsageGuide/commandBuilders.ts
- transports/bifrost-http/handlers/governance_test.go
- ui/app/workspace/mcp-registry/views/mcpUsageGuide/mcpUsageGuideSheet.tsx
- framework/configstore/rdb_deadlock_postgres_test.go
- framework/configstore/tables/encryption_test.go
- framework/configstore/tables/virtualkey.go
- framework/configstore/rdb_test.go
- transports/bifrost-http/lib/config_test.go
Contributor
|
Got it — you've already flagged this as a breaking change in the PR description, and the UI has been updated to handle both forms via Just worth confirming: for literal-value keys, does |
BearTS
force-pushed
the
06-18-feat_vk_as_secretvar
branch
from
June 18, 2026 09:01
b03fb66 to
1d04c33
Compare
BearTS
force-pushed
the
06-18-feat_vk_as_secretvar
branch
from
June 21, 2026 19:36
1d04c33 to
f5f2c18
Compare
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 21, 2026 19:36
ccadc7c to
46072a4
Compare
BearTS
force-pushed
the
06-18-feat_vk_as_secretvar
branch
from
June 21, 2026 20:41
f5f2c18 to
8788a96
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/plugins.go`:
- Around line 619-624: The condition in the existing value check does not
properly account for vault references. Currently, the condition checks `if
!secretVal.IsFromEnv() && secretVal.IsRedacted()` but this incorrectly treats
vault references as redacted plain strings (since IsRedacted returns true for
both environment and vault references). Update the condition to also exclude
vault references by adding an additional check for `IsFromVault()` so that vault
references like vault.NEW_SECRET are passed through instead of being restored to
the existing value.
🪄 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: 9667828f-4b32-4525-be98-dd8181cf4291
📒 Files selected for processing (34)
core/internal/mcptests/extraheaders_test.gocore/schemas/vault_test.goframework/configstore/clientconfig.goframework/configstore/encryption_test.goframework/configstore/migrations_test.goframework/configstore/rdb_deadlock_postgres_test.goframework/configstore/rdb_mcp_sessions_test.goframework/configstore/rdb_test.goframework/configstore/sqlite.goframework/configstore/tables/encryption.goframework/configstore/tables/encryption_test.goframework/configstore/tables/virtualkey.goframework/configstore/tables/virtualkey_secretvar_test.goframework/logstore/asyncjob_test.goframework/postgresconn/postgresconn_test.goplugins/governance/store.goplugins/governance/store_test.goplugins/governance/test_utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/bifrost-http/handlers/list_models_vk_test.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/plugins.gotransports/bifrost-http/handlers/plugins_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/mcp-registry/views/mcpUsageGuide/commandBuilders.tsui/app/workspace/mcp-registry/views/mcpUsageGuide/mcpUsageGuideSheet.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/components/prompts/components/apiKeySelectorView.tsxui/components/prompts/fragments/settingsPanel.tsxui/lib/types/governance.ts
💤 Files with no reviewable changes (1)
- framework/configstore/tables/encryption.go
✅ Files skipped from review due to trivial changes (3)
- core/schemas/vault_test.go
- transports/config.schema.json
- framework/postgresconn/postgresconn_test.go
🚧 Files skipped from review as they are similar to previous changes (26)
- framework/configstore/sqlite.go
- framework/configstore/encryption_test.go
- plugins/governance/test_utils.go
- ui/components/prompts/fragments/settingsPanel.tsx
- ui/app/workspace/mcp-registry/views/mcpUsageGuide/mcpUsageGuideSheet.tsx
- core/internal/mcptests/extraheaders_test.go
- framework/configstore/clientconfig.go
- plugins/governance/store_test.go
- framework/configstore/rdb_deadlock_postgres_test.go
- framework/configstore/migrations_test.go
- ui/lib/types/governance.ts
- framework/logstore/asyncjob_test.go
- ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx
- transports/bifrost-http/server/server.go
- ui/components/prompts/components/apiKeySelectorView.tsx
- framework/configstore/rdb_mcp_sessions_test.go
- framework/configstore/tables/virtualkey_secretvar_test.go
- transports/bifrost-http/handlers/mcpserver.go
- framework/configstore/tables/virtualkey.go
- transports/bifrost-http/handlers/governance.go
- plugins/governance/store.go
- framework/configstore/tables/encryption_test.go
- ui/app/workspace/mcp-registry/views/mcpUsageGuide/commandBuilders.ts
- framework/configstore/rdb_test.go
- transports/bifrost-http/handlers/governance_test.go
- transports/bifrost-http/lib/config_test.go
BearTS
force-pushed
the
06-18-feat_vk_as_secretvar
branch
from
June 21, 2026 20:56
8788a96 to
11373fa
Compare
This was referenced Jun 21, 2026
Closed
BearTS
force-pushed
the
06-15-chore_update_config_schema_and_helm
branch
from
June 22, 2026 13:18
46072a4 to
58fc774
Compare
BearTS
force-pushed
the
06-18-feat_vk_as_secretvar
branch
from
June 22, 2026 13:18
11373fa to
399ab83
Compare
Contributor
Merge activity
|
akshaydeo
changed the base branch from
06-15-chore_update_config_schema_and_helm
to
graphite-base/4504
June 22, 2026 16:33
akshaydeo
dismissed
coderabbitai[bot]’s stale review
June 22, 2026 16:36
The base branch was changed.
akshaydeo
force-pushed
the
06-18-feat_vk_as_secretvar
branch
from
June 22, 2026 16:36
399ab83 to
55c4860
Compare
akshaydeo
pushed a commit
that referenced
this pull request
Jun 24, 2026
… support env/vault references (#4504) ## Summary Virtual key values can now be sourced from environment variables or vault references (`env.X` / `vault.X`) rather than only from literal strings. The resolved plaintext is stored in the existing `Value` column (preserving hash-based lookup and auth), while a new `value_source_ref` column persists the original reference so the `SecretVar` shape survives a DB round-trip. ## Changes - **`TableVirtualKey` struct** — `Value` is now `json:"-"` (excluded from direct marshalling). Two new fields are added: `ValueSourceRef` (persisted, encrypted) stores the `env.X`/`vault.X` reference; `ValueSource` (transient, `gorm:"-"`) holds the runtime `SecretVar`. Custom `MarshalJSON`/`UnmarshalJSON` handle both bare strings and `SecretVar` objects for the `value` JSON field, redacting resolved secrets for env/vault sources. - **`BeforeSave` / `AfterFind`** — `ValueSourceRef` is encrypted alongside `Value` on write and decrypted on read; `AfterFind` reconstructs `ValueSource` from the stored plaintext and reference without re-resolving the live environment, keeping it consistent with the hashed value. - **Database migration** — `add_virtual_key_value_source_ref_column` adds the `value_source_ref` column to `governance_virtual_keys`. Existing rows default to `NULL`/`""` (literal value); no backfill is needed. - **`UpdateVirtualKey`** — `value_source_ref` is included in the explicit `Select` column list so updates persist the reference. - **`GenerateVirtualKeyHash`** — `ValueSourceRef` is now included in the hash so a changed env/vault reference triggers a config resync. - **`mergeGovernanceConfig`** — Removed the inline `env.` prefix resolution logic; env/vault references are now resolved during `TableVirtualKey.UnmarshalJSON`, so `Value` already holds the plaintext by the time the merge runs. `ValueSourceRef` and `ValueSource` are propagated from the existing DB record when the value is carried forward. - **`CreateVirtualKeyRequest`** — Accepts an optional `value` field (literal, reference string, or `SecretVar` object). When omitted, a value is generated server-side as before. - **Config schema** — `value` under `virtual_keys` is updated from `type: string` to `anyOf: [string, object]` to accept `SecretVar` objects. - **SQLite store** — `RegisterVaultCallbacks` is now called on the SQLite DB at init, consistent with the Postgres path. - **`schemasync` ignore list** — The `value` property is added to the ignore list with an explanation of the custom marshalling. - **UI (`governance.ts`)** — `VirtualKey.value` is typed as `string | SecretVar`. A `resolveVirtualKeyValue` helper extracts the usable string from either form. All UI call sites (virtual keys table, MCP usage guide, prompt settings panel, API key selector) use this helper instead of accessing `.value` directly. - **Tests** — `virtualkey_secretvar_test.go` covers env-sourced, vault-sourced, and literal round-trips; hash stability across sources; `MarshalJSON` redaction; and `UnmarshalJSON` accepting bare strings, reference strings, and `SecretVar` objects. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./framework/configstore/... ./framework/configstore/tables/... ./transports/bifrost-http/... # UI cd ui pnpm i pnpm build ``` To exercise env-sourced virtual keys end-to-end: 1. Set an environment variable, e.g. `export MY_VK=sk-bf-test-abc123`. 2. Create a virtual key via the API with `"value": "env.MY_VK"` or `"value": {"from_env": true, "env_var": "env.MY_VK"}`. 3. Confirm the response shows `"value": {"from_env": true, "env_var": "env.MY_VK"}` with the resolved secret redacted. 4. Confirm the `x-bf-vk` auth path still works with the resolved plaintext value. 5. Restart the service and confirm the virtual key is reconstructed correctly from the DB without re-reading the environment. ## Breaking changes - [x] Yes - [ ] No The `value` field on `VirtualKey` in the API response changes from a bare string to a `SecretVar` object for env/vault-sourced keys. Consumers that assumed `value` is always a string will need to handle the object form. Literal-value keys continue to emit a plain string in `value.value` with no other fields set, so the impact is limited to env/vault-sourced keys. The UI is updated accordingly. ## Security considerations - Resolved plaintext virtual key values are never emitted in API responses for env/vault-sourced keys; only the reference and source flags are returned. - `ValueSourceRef` is encrypted at rest alongside `Value` using the same encryption path. - `AfterFind` does not re-resolve the live environment on read, preventing a class of TOCTOU issues where the env var changes after the key is created. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
This was referenced Jun 24, 2026
Merged
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
Virtual key values can now be sourced from environment variables or vault references (
env.X/vault.X) rather than only from literal strings. The resolved plaintext is stored in the existingValuecolumn (preserving hash-based lookup and auth), while a newvalue_source_refcolumn persists the original reference so theSecretVarshape survives a DB round-trip.Changes
TableVirtualKeystruct —Valueis nowjson:"-"(excluded from direct marshalling). Two new fields are added:ValueSourceRef(persisted, encrypted) stores theenv.X/vault.Xreference;ValueSource(transient,gorm:"-") holds the runtimeSecretVar. CustomMarshalJSON/UnmarshalJSONhandle both bare strings andSecretVarobjects for thevalueJSON field, redacting resolved secrets for env/vault sources.BeforeSave/AfterFind—ValueSourceRefis encrypted alongsideValueon write and decrypted on read;AfterFindreconstructsValueSourcefrom the stored plaintext and reference without re-resolving the live environment, keeping it consistent with the hashed value.add_virtual_key_value_source_ref_columnadds thevalue_source_refcolumn togovernance_virtual_keys. Existing rows default toNULL/""(literal value); no backfill is needed.UpdateVirtualKey—value_source_refis included in the explicitSelectcolumn list so updates persist the reference.GenerateVirtualKeyHash—ValueSourceRefis now included in the hash so a changed env/vault reference triggers a config resync.mergeGovernanceConfig— Removed the inlineenv.prefix resolution logic; env/vault references are now resolved duringTableVirtualKey.UnmarshalJSON, soValuealready holds the plaintext by the time the merge runs.ValueSourceRefandValueSourceare propagated from the existing DB record when the value is carried forward.CreateVirtualKeyRequest— Accepts an optionalvaluefield (literal, reference string, orSecretVarobject). When omitted, a value is generated server-side as before.valueundervirtual_keysis updated fromtype: stringtoanyOf: [string, object]to acceptSecretVarobjects.RegisterVaultCallbacksis now called on the SQLite DB at init, consistent with the Postgres path.schemasyncignore list — Thevalueproperty is added to the ignore list with an explanation of the custom marshalling.governance.ts) —VirtualKey.valueis typed asstring | SecretVar. AresolveVirtualKeyValuehelper extracts the usable string from either form. All UI call sites (virtual keys table, MCP usage guide, prompt settings panel, API key selector) use this helper instead of accessing.valuedirectly.virtualkey_secretvar_test.gocovers env-sourced, vault-sourced, and literal round-trips; hash stability across sources;MarshalJSONredaction; andUnmarshalJSONaccepting bare strings, reference strings, andSecretVarobjects.Type of change
Affected areas
How to test
To exercise env-sourced virtual keys end-to-end:
export MY_VK=sk-bf-test-abc123."value": "env.MY_VK"or"value": {"from_env": true, "env_var": "env.MY_VK"}."value": {"from_env": true, "env_var": "env.MY_VK"}with the resolved secret redacted.x-bf-vkauth path still works with the resolved plaintext value.Breaking changes
The
valuefield onVirtualKeyin the API response changes from a bare string to aSecretVarobject for env/vault-sourced keys. Consumers that assumedvalueis always a string will need to handle the object form. Literal-value keys continue to emit a plain string invalue.valuewith no other fields set, so the impact is limited to env/vault-sourced keys. The UI is updated accordingly.Security considerations
ValueSourceRefis encrypted at rest alongsideValueusing the same encryption path.AfterFinddoes not re-resolve the live environment on read, preventing a class of TOCTOU issues where the env var changes after the key is created.Checklist
docs/contributing/README.mdand followed the guidelines