feat: validate routing rule scope_id resolves to an existing entity at write time and during config.json sync - #5371
Merged
akshaydeo merged 1 commit intoJul 20, 2026
Conversation
|
|
This was referenced Jul 20, 2026
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughRouting-rule configuration reconciliation now filters unresolved non-global scopes. Governance HTTP create and update handlers validate scope IDs against the config store, with conditional update validation and distinct client-error versus infrastructure-error responses. ChangesRouting scope validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GovernanceHandler
participant ConfigStore
Client->>GovernanceHandler: Create or update routing rule
GovernanceHandler->>ConfigStore: Validate scope_id for scope type
ConfigStore-->>GovernanceHandler: Matching entity or error
GovernanceHandler-->>Client: Success, HTTP 400, or HTTP 500
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" Comment |
18 tasks
Collaborator
Author
18 tasks
Contributor
Confidence Score: 4/5A contained config reconciliation issue should be fixed before merging.
transports/bifrost-http/lib/config.go Important Files Changed
Reviews (3): Last reviewed commit: "fix: validate routing rule scope_id refe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Pratham-Mishra04
force-pushed
the
07-20-fix_validate_routing_rule_scope_id_references_a_real_entity
branch
from
July 20, 2026 07:17
0135626 to
378f2de
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Pratham-Mishra04
marked this pull request as draft
July 20, 2026 07:24
Pratham-Mishra04
changed the base branch from
07-18-fix_response_message_output_struct_fixes_in_api_reference
to
graphite-base/5371
July 20, 2026 07:51
Pratham-Mishra04
force-pushed
the
graphite-base/5371
branch
from
July 20, 2026 07:54
efe7704 to
f53936d
Compare
Pratham-Mishra04
force-pushed
the
07-20-fix_validate_routing_rule_scope_id_references_a_real_entity
branch
from
July 20, 2026 07:54
378f2de to
80d1e92
Compare
graphite-app
Bot
dismissed
coderabbitai[bot]’s stale review
July 20, 2026 07:54
The base branch was changed.
Pratham-Mishra04
force-pushed
the
07-20-fix_validate_routing_rule_scope_id_references_a_real_entity
branch
from
July 20, 2026 07:54
80d1e92 to
c7c9565
Compare
akshaydeo
marked this pull request as ready for review
July 20, 2026 08:35
Contributor
Merge activity
|
akshaydeo
deleted the
07-20-fix_validate_routing_rule_scope_id_references_a_real_entity
branch
July 20, 2026 08:36
| if scopeID == "" || !validRoutingScopeIDs[rule.Scope][scopeID] { | ||
| if existing, ok := existingRoutingRulesByID[rule.ID]; ok { | ||
| logger.Warn("routing rule %s: scope_id %q does not resolve to an existing %s (use the entity's id, not its name); keeping last persisted version", rule.ID, scopeID, rule.Scope) | ||
| sanitizedRoutingRules = append(sanitizedRoutingRules, existing) |
Contributor
There was a problem hiding this comment.
scope_id. The rule then remains active in storage but silently matches no traffic. Only restore the persisted rule when its scope target will survive this sync; otherwise remove or reject the rule.
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 2586-2589: Update the virtual-key merge flow and the
source-of-truth branch around vkScopeIDs to track IDs of virtual keys skipped
when secret-backed Value resolution fails, then exclude those IDs when
rebuilding vkScopeIDs from configData.Governance.VirtualKeys. Keep persisted
virtual keys included while ensuring routing-rule scope validation cannot treat
skipped, non-persisted virtual keys as valid targets.
🪄 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: 3e05856e-bb0c-424e-b78c-89e074b80f15
📒 Files selected for processing (2)
transports/bifrost-http/handlers/governance.gotransports/bifrost-http/lib/config.go
Comment on lines
+2586
to
+2589
| vkScopeIDs := entityIDSet(governanceConfig.VirtualKeys, func(vk configstoreTables.TableVirtualKey) string { return vk.ID }, virtualKeysToAdd) | ||
| if configData.isConfigJSONSourceOfTruth() && configData.governanceSectionPresent("virtual_keys") { | ||
| vkScopeIDs = entityIDSet(configData.Governance.VirtualKeys, func(vk configstoreTables.TableVirtualKey) string { return vk.ID }, nil) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skipped virtual keys (unresolved secret refs) are still counted as valid routing-rule scope targets in source-of-truth mode.
When source_of_truth=config.json and the virtual_keys section is present, vkScopeIDs is rebuilt from the raw configData.Governance.VirtualKeys slice. However, that slice can contain entries that were skipped from persistence — the not-found branch (around line 2555-2557) assigns the VK an ID and then continues without appending to virtualKeysToAdd when a secret-backed Value fails to resolve. That skipped VK's ID still ends up counted as "resolvable" here, so a routing rule scoped to it will pass sanitization and get persisted, even though the VK itself will never exist in the DB — reproducing exactly the "silently matches zero requests" failure this PR is meant to prevent. Teams/Customers don't have this asymmetry since their merge loops have no comparable skip path; this is virtual-key specific.
🐛 Proposed fix: track skipped VK IDs and exclude them from the scope-ID set
+ skippedVirtualKeyIDs := make(map[string]bool)
for i, newVirtualKey := range configData.Governance.VirtualKeys {
...
if !found {
...
resolvedVal := configData.Governance.VirtualKeys[i].Value.GetValue()
if resolvedVal == "" && configData.Governance.VirtualKeys[i].Value.IsFromSecret() {
logger.Warn("virtual key %s: env/vault ref %q could not be resolved, skipping", newVirtualKey.ID, configData.Governance.VirtualKeys[i].Value.GetRawRef())
+ skippedVirtualKeyIDs[configData.Governance.VirtualKeys[i].ID] = true
continue
}
...
}
}
...
vkScopeIDs := entityIDSet(governanceConfig.VirtualKeys, func(vk configstoreTables.TableVirtualKey) string { return vk.ID }, virtualKeysToAdd)
if configData.isConfigJSONSourceOfTruth() && configData.governanceSectionPresent("virtual_keys") {
vkScopeIDs = entityIDSet(configData.Governance.VirtualKeys, func(vk configstoreTables.TableVirtualKey) string { return vk.ID }, nil)
+ for id := range skippedVirtualKeyIDs {
+ delete(vkScopeIDs, id)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| vkScopeIDs := entityIDSet(governanceConfig.VirtualKeys, func(vk configstoreTables.TableVirtualKey) string { return vk.ID }, virtualKeysToAdd) | |
| if configData.isConfigJSONSourceOfTruth() && configData.governanceSectionPresent("virtual_keys") { | |
| vkScopeIDs = entityIDSet(configData.Governance.VirtualKeys, func(vk configstoreTables.TableVirtualKey) string { return vk.ID }, nil) | |
| } | |
| vkScopeIDs := entityIDSet(governanceConfig.VirtualKeys, func(vk configstoreTables.TableVirtualKey) string { return vk.ID }, virtualKeysToAdd) | |
| if configData.isConfigJSONSourceOfTruth() && configData.governanceSectionPresent("virtual_keys") { | |
| vkScopeIDs = entityIDSet(configData.Governance.VirtualKeys, func(vk configstoreTables.TableVirtualKey) string { return vk.ID }, nil) | |
| for id := range skippedVirtualKeyIDs { | |
| delete(vkScopeIDs, id) | |
| } | |
| } |
🤖 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 2586 - 2589, Update the
virtual-key merge flow and the source-of-truth branch around vkScopeIDs to track
IDs of virtual keys skipped when secret-backed Value resolution fails, then
exclude those IDs when rebuilding vkScopeIDs from
configData.Governance.VirtualKeys. Keep persisted virtual keys included while
ensuring routing-rule scope validation cannot treat skipped, non-persisted
virtual keys as valid targets.
Source: Path instructions
18 tasks
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
… at write time and during config.json sync (maximhq#5371) ## Summary Routing rules with a non-global `scope` can reference a `scope_id` that doesn't correspond to any real entity (e.g. a name typed in place of an ID). When this happens, the routing engine silently matches zero requests because it caches rules keyed by the real entity ID. This PR rejects invalid `scope_id` values at write time — both via the HTTP API and during config file ingestion — so misconfigured rules fail loudly instead of silently doing nothing. ## Changes - Added `validateRoutingScopeID` to the governance HTTP handler, which looks up the referenced entity (`virtual_key`, `team`, or `customer`) in the config store and returns a 400 error if it doesn't exist. This is called on `createRoutingRule` unconditionally and on `updateRoutingRule` only when `scope` or `scope_id` is part of the request payload. - Added `entityIDSet` helper in the config file merge path that builds a lookup set from already-persisted entities plus those pending insertion in the same sync, so a routing rule can validly reference an entity introduced in the same config file. - During `mergeGovernanceConfig`, routing rules whose `scope_id` doesn't resolve to a known entity of the declared scope type are now skipped with a warning log rather than being written to the store. ## Type of change - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) ## How to test ```sh go test ./... ``` 1. Create a routing rule via the API with a `scope` of `virtual_key` and a `scope_id` that does not exist — expect a 400 response with a message indicating the virtual key was not found. 2. Create a routing rule with a valid `scope_id` — expect success. 3. Update a routing rule without changing `scope` or `scope_id` (e.g. toggle `enabled`) — expect no re-validation call and a successful response. 4. Load a config file containing a routing rule whose `scope_id` is a name rather than an ID — expect a warning log and the rule to be skipped. ## Breaking changes - [x] No ## Security considerations Prevents routing rules from being written in a state where they silently match no traffic, which could mask misconfiguration or be exploited to create rules that appear active but are effectively no-ops. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
… at write time and during config.json sync (maximhq#5371) ## Summary Routing rules with a non-global `scope` can reference a `scope_id` that doesn't correspond to any real entity (e.g. a name typed in place of an ID). When this happens, the routing engine silently matches zero requests because it caches rules keyed by the real entity ID. This PR rejects invalid `scope_id` values at write time — both via the HTTP API and during config file ingestion — so misconfigured rules fail loudly instead of silently doing nothing. ## Changes - Added `validateRoutingScopeID` to the governance HTTP handler, which looks up the referenced entity (`virtual_key`, `team`, or `customer`) in the config store and returns a 400 error if it doesn't exist. This is called on `createRoutingRule` unconditionally and on `updateRoutingRule` only when `scope` or `scope_id` is part of the request payload. - Added `entityIDSet` helper in the config file merge path that builds a lookup set from already-persisted entities plus those pending insertion in the same sync, so a routing rule can validly reference an entity introduced in the same config file. - During `mergeGovernanceConfig`, routing rules whose `scope_id` doesn't resolve to a known entity of the declared scope type are now skipped with a warning log rather than being written to the store. ## Type of change - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) ## How to test ```sh go test ./... ``` 1. Create a routing rule via the API with a `scope` of `virtual_key` and a `scope_id` that does not exist — expect a 400 response with a message indicating the virtual key was not found. 2. Create a routing rule with a valid `scope_id` — expect success. 3. Update a routing rule without changing `scope` or `scope_id` (e.g. toggle `enabled`) — expect no re-validation call and a successful response. 4. Load a config file containing a routing rule whose `scope_id` is a name rather than an ID — expect a warning log and the rule to be skipped. ## Breaking changes - [x] No ## Security considerations Prevents routing rules from being written in a state where they silently match no traffic, which could mask misconfiguration or be exploited to create rules that appear active but are effectively no-ops. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
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
Routing rules with a non-global
scopecan reference ascope_idthat doesn't correspond to any real entity (e.g. a name typed in place of an ID). When this happens, the routing engine silently matches zero requests because it caches rules keyed by the real entity ID. This PR rejects invalidscope_idvalues at write time — both via the HTTP API and during config file ingestion — so misconfigured rules fail loudly instead of silently doing nothing.Changes
validateRoutingScopeIDto the governance HTTP handler, which looks up the referenced entity (virtual_key,team, orcustomer) in the config store and returns a 400 error if it doesn't exist. This is called oncreateRoutingRuleunconditionally and onupdateRoutingRuleonly whenscopeorscope_idis part of the request payload.entityIDSethelper in the config file merge path that builds a lookup set from already-persisted entities plus those pending insertion in the same sync, so a routing rule can validly reference an entity introduced in the same config file.mergeGovernanceConfig, routing rules whosescope_iddoesn't resolve to a known entity of the declared scope type are now skipped with a warning log rather than being written to the store.Type of change
Affected areas
How to test
go test ./...scopeofvirtual_keyand ascope_idthat does not exist — expect a 400 response with a message indicating the virtual key was not found.scope_id— expect success.scopeorscope_id(e.g. toggleenabled) — expect no re-validation call and a successful response.scope_idis a name rather than an ID — expect a warning log and the rule to be skipped.Breaking changes
Security considerations
Prevents routing rules from being written in a state where they silently match no traffic, which could mask misconfiguration or be exploited to create rules that appear active but are effectively no-ops.
Checklist
docs/contributing/README.mdand followed the guidelines