feat: add expiry field to virtual keys - #4887
Merged
Merged
Conversation
|
|
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Jul 3, 2026
Merged
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (15)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds virtual key expiry support across storage, governance enforcement, HTTP request handling, schema/types, and UI creation and display flows. ChangesVirtual Key Expiry
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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" 🔧 ast-grep (0.44.0)transports/bifrost-http/lib/config_test.goast-grep timed out on this file Comment |
Contributor
![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.
🧹 Nitpick comments (3)
plugins/governance/main.go (1)
1443-1473: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the fail-closed short-circuit blocks into a helper.
The not-found, inactive, and now expired blocks in
PreMCPHookare structurally identical (setgovernanceRejectedContextKey, build the sameMCPPluginShortCircuit/BifrostErrorshape, differing only inDecision/message). Extracting a small helper (e.g.rejectMCPVK(ctx, decision, message)) would reduce triplication and keep future additions consistent.♻️ Proposed helper extraction
+func (p *GovernancePlugin) rejectMCPVK(ctx *schemas.BifrostContext, decision Decision, message string) *schemas.MCPPluginShortCircuit { + ctx.SetValue(governanceRejectedContextKey, true) + return &schemas.MCPPluginShortCircuit{Error: &schemas.BifrostError{ + Type: bifrost.Ptr(string(decision)), + StatusCode: bifrost.Ptr(403), + Error: &schemas.ErrorField{ + Message: message, + }, + }} +} + vk, ok := p.store.GetVirtualKey(ctx, virtualKeyValue) if !ok || vk == nil { - // VK became invalid after initial check - fail closed for security - ctx.SetValue(governanceRejectedContextKey, true) - return req, &schemas.MCPPluginShortCircuit{Error: &schemas.BifrostError{ - Type: bifrost.Ptr(string(DecisionVirtualKeyNotFound)), - StatusCode: bifrost.Ptr(403), - Error: &schemas.ErrorField{ - Message: "Virtual key not found", - }, - }}, nil + // VK became invalid after initial check - fail closed for security + return req, p.rejectMCPVK(ctx, DecisionVirtualKeyNotFound, "Virtual key not found"), nil } if !vk.IsActiveValue() { - ctx.SetValue(governanceRejectedContextKey, true) - return req, &schemas.MCPPluginShortCircuit{Error: &schemas.BifrostError{ - Type: bifrost.Ptr(string(DecisionVirtualKeyBlocked)), - StatusCode: bifrost.Ptr(403), - Error: &schemas.ErrorField{ - Message: "Virtual key is inactive", - }, - }}, nil + return req, p.rejectMCPVK(ctx, DecisionVirtualKeyBlocked, "Virtual key is inactive"), nil } if vk.IsExpiredAt(time.Now().UTC()) { - ctx.SetValue(governanceRejectedContextKey, true) - return req, &schemas.MCPPluginShortCircuit{Error: &schemas.BifrostError{ - Type: bifrost.Ptr(string(DecisionVirtualKeyBlocked)), - StatusCode: bifrost.Ptr(403), - Error: &schemas.ErrorField{ - Message: "Virtual key has expired", - }, - }}, nil + return req, p.rejectMCPVK(ctx, DecisionVirtualKeyBlocked, "Virtual key has expired"), nil }🤖 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 `@plugins/governance/main.go` around lines 1443 - 1473, The fail-closed short-circuit handling in PreMCPHook is duplicated across the not-found, inactive, and expired virtual key checks. Extract the repeated ctx.SetValue(governanceRejectedContextKey, true) plus MCPPluginShortCircuit/BifrostError construction into a small helper such as rejectMCPVK, and pass in the decision type and error message so all three branches reuse the same path. Keep the existing behavior intact while making future governance rejections consistent and easier to extend.ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx (1)
83-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
isExpiredcomputation — extract a shared helper.
const isExpired = !!vk.expires_at && Date.now() >= new Date(vk.expires_at).getTime();is duplicated verbatim in the CSV export (line 83), the row-render logic (line 849), and again invirtualKeyDetailsSheet.tsx(line 140). A singleisVirtualKeyExpired(expiresAt?: string | null): booleanhelper (e.g. inui/lib/utils) would keep this consistent if the expiry semantics ever change.♻️ Suggested helper
export function isVirtualKeyExpired(expiresAt?: string | null): boolean { return !!expiresAt && Date.now() >= new Date(expiresAt).getTime(); }As per path instructions, "reuse existing constants, page structure, shared components... before introducing one-off UI conventions."
Also applies to: 849-850
🤖 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/virtual-keys/views/virtualKeysTable.tsx` around lines 83 - 84, The expiry check is duplicated across the virtual keys table and details sheet, so extract it into a shared helper and reuse it everywhere. Add a reusable isVirtualKeyExpired(expiresAt?: string | null) utility in the shared utils area (for example ui/lib/utils), then replace the inline Date.now/new Date logic in virtualKeysTable.tsx and virtualKeyDetailsSheet.tsx with that helper in both the CSV export and row-render paths. Keep the existing status logic in VirtualKeysTable and the details sheet behavior unchanged except for calling the shared helper.Source: Path instructions
transports/bifrost-http/handlers/governance.go (1)
168-169: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
expires_attype asymmetry causes inconsistent error UX between create and update.
CreateVirtualKeyRequest.ExpiresAtis a*time.Time, so malformed date strings fail during the top-leveljson.Unmarshal(line 1243) and only ever surface as the generic"Invalid JSON"error.UpdateVirtualKeyRequest.ExpiresAtis a*stringthat is explicitly parsed and yields the much more actionable"expires_at must be an RFC3339 timestamp"message. A client sending a badexpires_aton create gets no hint about which field is wrong.♻️ Suggested fix: mirror the update path's explicit-string-parse pattern on create
- ExpiresAt *time.Time `json:"expires_at,omitempty"` // Optional expiry; nil means never expires + ExpiresAt *string `json:"expires_at,omitempty"` // Optional expiry (RFC3339); nil means never expiresThen parse it the same way
updateVirtualKeydoes, converting to*time.Timebefore persisting.Also applies to: 1276-1283
🤖 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/handlers/governance.go` around lines 168 - 169, CreateVirtualKeyRequest.ExpiresAt is using *time.Time, which makes malformed expires_at values fail as generic “Invalid JSON” during json.Unmarshal instead of a field-specific message. Change the create flow to mirror UpdateVirtualKeyRequest and updateVirtualKey by using an explicit string field for expires_at in the request struct, then parse it with the same RFC3339 validation and convert to *time.Time before persistence. Keep the change aligned in governance.go for both the request type and the create handler so bad input returns the same actionable error UX as update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/governance/main.go`:
- Around line 1443-1473: The fail-closed short-circuit handling in PreMCPHook is
duplicated across the not-found, inactive, and expired virtual key checks.
Extract the repeated ctx.SetValue(governanceRejectedContextKey, true) plus
MCPPluginShortCircuit/BifrostError construction into a small helper such as
rejectMCPVK, and pass in the decision type and error message so all three
branches reuse the same path. Keep the existing behavior intact while making
future governance rejections consistent and easier to extend.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 168-169: CreateVirtualKeyRequest.ExpiresAt is using *time.Time,
which makes malformed expires_at values fail as generic “Invalid JSON” during
json.Unmarshal instead of a field-specific message. Change the create flow to
mirror UpdateVirtualKeyRequest and updateVirtualKey by using an explicit string
field for expires_at in the request struct, then parse it with the same RFC3339
validation and convert to *time.Time before persistence. Keep the change aligned
in governance.go for both the request type and the create handler so bad input
returns the same actionable error UX as update.
In `@ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx`:
- Around line 83-84: The expiry check is duplicated across the virtual keys
table and details sheet, so extract it into a shared helper and reuse it
everywhere. Add a reusable isVirtualKeyExpired(expiresAt?: string | null)
utility in the shared utils area (for example ui/lib/utils), then replace the
inline Date.now/new Date logic in virtualKeysTable.tsx and
virtualKeyDetailsSheet.tsx with that helper in both the CSV export and
row-render paths. Keep the existing status logic in VirtualKeysTable and the
details sheet behavior unchanged except for calling the shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf86fcd9-9fb6-4f36-b90b-11a6dc074b72
📒 Files selected for processing (17)
framework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/virtualkey.goplugins/governance/main.goplugins/governance/resolver.gotests/cmd/e2eseed/go.modtests/cmd/seed/go.modtests/cmd/seedvks/go.modtransports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.jsonui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/app/workspace/virtual-keys/views/virtualKeysTable.tsxui/components/ui/datePickerWithRange.tsxui/lib/types/governance.ts
Pratham-Mishra04
force-pushed
the
07-03-feat_add_expiration_support_to_virtual_keys
branch
from
July 3, 2026 14:53
21eac0c to
041a823
Compare
Contributor
Merge activity
|
yangtuooc
added a commit
to yangtuooc/bifrost
that referenced
this pull request
Jul 4, 2026
* 'dev' of https://github.com/maximhq/bifrost: ipv6 support (maximhq#4895) docs: add virtual key expiry support docs (maximhq#4889) test: add Postman e2e collection and runner for virtual key expiry validation and enforcement (maximhq#4888) feat: add expiry field to virtual keys (maximhq#4887) fix: converts thinking to disabled if tool choice is required for deepseek (maximhq#4861) chore: adds docs for deepseek provider (maximhq#4854) chore: adds tests for deepseek provider (maximhq#4853) feat: adds deepseek provider (maximhq#4852) fix: cost for image generation or image edit streaming (maximhq#4802) feat: add `BedrockMantleKeyConfig` support to key hashing, schema/table mapping, and sensitive field clearing (maximhq#4886) fix: skip O(N) reference refresh on request-time rate-limit/budget reset (maximhq#4883) refactor: simplify Responses lifecycle permissions to require explicit per-verb flags and expose them in UI (maximhq#4880) fix: append datasheet models for incomplete list models call (maximhq#4879) # Conflicts: # ui/app/workspace/providers/fragments/allowedRequestsFields.tsx # ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx # ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx # ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx # ui/components/ui/datePickerWithRange.tsx
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
3 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
This PR adds optional expiry support for virtual keys. A virtual key can now be given an
expires_attimestamp; once that time passes, all requests using the key are rejected with a 403. Keys with no expiry continue to work indefinitely as before.Changes
ExpiresAt *time.Timefield toTableVirtualKeywith a corresponding database migration (add_virtual_key_expires_at_column) that adds a nullableexpires_atcolumn togovernance_virtual_keys.IsExpiredAt(now time.Time) boolmethod onTableVirtualKeythat treatsnow >= expires_atas expired andnilas never-expires.ExpiresAtin theGenerateVirtualKeyHashfunction (only when set, so existing rows without an expiry retain their current hash).PreRequestHook,PreMCPHook, andEvaluateVirtualKeyRequestin the governance plugin and resolver, returning a 403 with a clear "Virtual key has expired" message.UpdateVirtualKeyto persist theexpires_atcolumn. Sending a timestamp sets a new expiry; sending""clears it; omitting the field leaves it unchanged.expires_atvalidation in the HTTP handler for both create (must be a future timestamp) and update (RFC3339 parse + future check).expires_atfield.ExpiryPickerFieldcomponent in the virtual key form with "Never", preset buttons (30 min, 1 h, 24 h, 7 days), and a custom date-time picker. The form only sends the expiry field when it has been explicitly changed, preventing an already-expired key's old timestamp from being re-submitted and rejected during an unrelated edit.DateTimePickercomponent to accept abuttonVariantprop so the expiry picker can reflect selection state visually.Type of change
Affected areas
How to test
Create a key with an expiry:
POST /governance/virtual-keys { "name": "expiring-key", "expires_at": "2025-12-31T23:59:59Z", ... }Requests using this key after the expiry timestamp should receive a 403 with
"Virtual key has expired".Clear an expiry:
PATCH /governance/virtual-keys/{id} { "expires_at": "" }The key should now work indefinitely.
Verify hash stability: A virtual key with no
ExpiresAtmust produce the same hash before and after this migration is applied.Screenshots/Recordings
The virtual key form now includes an expiry picker row with preset buttons and a custom date-time picker. The keys table and detail sheet show an "Expired" badge with a relative time label when a key has passed its expiry.
Breaking changes
Related issues
Security considerations
Expired keys are rejected at the earliest enforcement point in both the HTTP request path (
PreRequestHook) and the MCP execution path (PreMCPHook), failing closed. The expiry is checked in-memory against the already-loaded virtual key record, so no additional database queries are introduced on the hot path.Checklist
docs/contributing/README.mdand followed the guidelines