Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions transports/bifrost-http/handlers/governance.go
Original file line number Diff line number Diff line change
Expand Up @@ -4028,6 +4028,9 @@ func (h *GovernanceHandler) createRoutingRule(ctx *fasthttp.RequestCtx) {
} else if req.ScopeID == nil || *req.ScopeID == "" {
SendError(ctx, 400, "scope_id field is required when scope is not global")
return
} else if err := h.validateRoutingScopeID(ctx, scope, *req.ScopeID); err != nil {
sendRoutingScopeIDValidationError(ctx, err)
return
Comment thread
greptile-apps[bot] marked this conversation as resolved.
}

// Build targets
Expand Down Expand Up @@ -4180,6 +4183,13 @@ func (h *GovernanceHandler) updateRoutingRule(ctx *fasthttp.RequestCtx) {
} else if rule.ScopeID == nil || *rule.ScopeID == "" {
SendError(ctx, 400, "scope_id field is required when scope is not global")
return
} else if req.Scope != nil || req.ScopeID != nil {
// Only re-validate when scope or scope_id actually changed in this request;
// avoids re-checking on every unrelated update (e.g. toggling enabled).
if err := h.validateRoutingScopeID(ctx, rule.Scope, *rule.ScopeID); err != nil {
sendRoutingScopeIDValidationError(ctx, err)
return
}
}

// Update in database
Expand Down Expand Up @@ -4602,6 +4612,55 @@ var validRoutingScopes = map[string]bool{
"virtual_key": true,
}

// errRoutingScopeIDNotFound marks a validateRoutingScopeID failure as a genuine
// "entity doesn't exist" rejection, as opposed to a store error (DB down, timeout,
// context cancellation). Callers use errors.Is to tell the two apart: the former
// is a 400 (bad request data), the latter a 500 (server couldn't verify).
var errRoutingScopeIDNotFound = errors.New("routing rule scope_id not found")

// validateRoutingScopeID checks that scopeID resolves to an existing entity of the
// given scope type. A rule whose scope_id doesn't resolve silently matches zero
// requests (the routing engine caches rules keyed by the real entity ID), so this
// must be rejected at write time rather than left to fail invisibly at eval time.
func (h *GovernanceHandler) validateRoutingScopeID(ctx context.Context, scope string, scopeID string) error {
switch scope {
case "virtual_key":
if _, err := h.configStore.GetVirtualKey(ctx, scopeID); err != nil {
if errors.Is(err, configstore.ErrNotFound) {
return fmt.Errorf("virtual key '%s' not found: %w", scopeID, errRoutingScopeIDNotFound)
}
return fmt.Errorf("failed to verify virtual key: %w", err)
}
case "team":
if _, err := h.configStore.GetTeam(ctx, scopeID); err != nil {
if errors.Is(err, configstore.ErrNotFound) {
return fmt.Errorf("team '%s' not found: %w", scopeID, errRoutingScopeIDNotFound)
}
return fmt.Errorf("failed to verify team: %w", err)
}
case "customer":
if _, err := h.configStore.GetCustomer(ctx, scopeID); err != nil {
if errors.Is(err, configstore.ErrNotFound) {
return fmt.Errorf("customer '%s' not found: %w", scopeID, errRoutingScopeIDNotFound)
}
return fmt.Errorf("failed to verify customer: %w", err)
}
}
return nil
}

// sendRoutingScopeIDValidationError maps a validateRoutingScopeID error to the
// right HTTP status: 400 when scope_id genuinely doesn't resolve, 500 when the
// store itself failed and existence couldn't be determined.
func sendRoutingScopeIDValidationError(ctx *fasthttp.RequestCtx, err error) {
if errors.Is(err, errRoutingScopeIDNotFound) {
SendError(ctx, 400, err.Error())
return
}
logger.Error("failed to validate routing rule scope_id: %v", err)
SendError(ctx, 500, "Failed to verify scope_id")
}

// validateRoutingScope validates that the scope value is one of the allowed values
func validateRoutingScope(scope string) error {
if scope == "" {
Expand Down
73 changes: 73 additions & 0 deletions transports/bifrost-http/lib/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -2335,6 +2335,20 @@ func resolveGovernanceKeyReferences(ctx context.Context, config *Config, governa
}

// mergeGovernanceConfig merges governance config from file with store
// entityIDSet builds a lookup set of IDs from an already-synced slice plus a
// slice of entries still pending insertion, so newly-added-in-this-file entities
// are valid scope_id targets even before they're persisted.
func entityIDSet[T any](existing []T, id func(T) string, toAdd []T) map[string]bool {
set := make(map[string]bool, len(existing)+len(toAdd))
for _, e := range existing {
set[id(e)] = true
}
for _, e := range toAdd {
set[id(e)] = true
}
return set
}

func mergeGovernanceConfig(ctx context.Context, config *Config, configData *ConfigData, governanceConfig *configstore.GovernanceConfig) {
logger.Debug("merging governance config from config file with store")
// When config.json is the source of truth, file-present entities must be
Expand Down Expand Up @@ -2552,6 +2566,65 @@ func mergeGovernanceConfig(ctx context.Context, config *Config, configData *Conf
virtualKeysToAdd = append(virtualKeysToAdd, configData.Governance.VirtualKeys[i])
}
}
// Build the set of entity IDs each non-global scope may reference, so a routing
// rule whose scope_id doesn't resolve (e.g. a name typed in place of an ID) is
// rejected here instead of silently matching zero requests at eval time.
//
// Under source_of_truth=config.json, entities absent from the file are deleted
// by the later prune step, so a currently-persisted entity that isn't in the
// file is NOT a valid target — only file-declared IDs survive. Otherwise
// (default merge mode, or the section is simply absent from this file) anything
// already persisted, or being added by this same file, remains valid.
teamScopeIDs := entityIDSet(governanceConfig.Teams, func(t configstoreTables.TableTeam) string { return t.ID }, teamsToAdd)
if configData.isConfigJSONSourceOfTruth() && configData.governanceSectionPresent("teams") {
teamScopeIDs = entityIDSet(configData.Governance.Teams, func(t configstoreTables.TableTeam) string { return t.ID }, nil)
}
customerScopeIDs := entityIDSet(governanceConfig.Customers, func(c configstoreTables.TableCustomer) string { return c.ID }, customersToAdd)
if configData.isConfigJSONSourceOfTruth() && configData.governanceSectionPresent("customers") {
customerScopeIDs = entityIDSet(configData.Governance.Customers, func(c configstoreTables.TableCustomer) string { return c.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)
}
Comment on lines +2586 to +2589

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

validRoutingScopeIDs := map[string]map[string]bool{
"team": teamScopeIDs,
"customer": customerScopeIDs,
"virtual_key": vkScopeIDs,
}

// Sanitize routing rules with an unresolvable scope_id BEFORE the merge loop
// and before pruneGovernanceConfigToFile treats configData.Governance.RoutingRules
// as the new authoritative snapshot (it both computes the prune keep-set from it
// and assigns it verbatim to config.GovernanceConfig.RoutingRules). A brand-new
// invalid rule is dropped outright; an invalid edit to an existing rule reverts
// to the last persisted version, so the in-memory snapshot and the prune
// keep-set never reflect data that was rejected and never written to the DB.
existingRoutingRulesByID := make(map[string]configstoreTables.TableRoutingRule, len(governanceConfig.RoutingRules))
for _, r := range governanceConfig.RoutingRules {
existingRoutingRulesByID[r.ID] = r
}
sanitizedRoutingRules := make([]configstoreTables.TableRoutingRule, 0, len(configData.Governance.RoutingRules))
for _, rule := range configData.Governance.RoutingRules {
if rule.Scope != "" && rule.Scope != "global" {
scopeID := ""
if rule.ScopeID != nil {
scopeID = *rule.ScopeID
}
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Fallback preserves dangling rule When an authoritative file removes team A and changes an existing team-scoped rule to an invalid ID, this branch restores the persisted rule that still points to team A. The later prune deletes team A but keeps the restored rule, leaving it with a nonexistent 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.

} else {
logger.Warn("routing rule %s: scope_id %q does not resolve to an existing %s (use the entity's id, not its name); skipping new rule", rule.ID, scopeID, rule.Scope)
}
continue
}
}
sanitizedRoutingRules = append(sanitizedRoutingRules, rule)
}
configData.Governance.RoutingRules = sanitizedRoutingRules

// Merge RoutingRules by ID with hash comparison
routingRulesToAdd := make([]configstoreTables.TableRoutingRule, 0)
routingRulesToUpdate := make([]configstoreTables.TableRoutingRule, 0)
Expand Down
Loading