feat: enforce routing allowlist in core and propagate VK provider constraints via context key - #3936
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR threads a governance-provided provider allowlist through request context, has the model catalog resolver intersect catalog candidates with that allowlist, and enforces the allowlist in core handlers by rejecting disallowed primaries and filtering/persisting allowed fallbacks (fail-closed when empty). ChangesRouting Governance Allowlist Enforcement
Sequence DiagramsequenceDiagram
participant Client
participant GovernancePlugin
participant BifrostContext
participant ModelCatalogResolver
participant CoreHandler
Client->>GovernancePlugin: request (PreRequestHook)
GovernancePlugin->>BifrostContext: publish allowed providers
Client->>ModelCatalogResolver: ResolveProviderFromCatalog (reads allowlist)
ModelCatalogResolver->>BifrostContext: read allowlist, filter candidates
ModelCatalogResolver->>CoreHandler: selected provider (may be empty)
CoreHandler->>BifrostContext: enforce allowlist on primary + filter fallbacks
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
Confidence Score: 5/5Safe to merge; both streaming and non-streaming enforcement paths correctly reject disallowed providers with HTTP 400, the cooperative filtering in the catalog resolver is wired correctly, and fallback pruning is consistent with the hard enforcement. The core enforcement logic is correct, the StatusCode is set on both paths via the shared helper, and allowlist semantics (fail-closed on empty, no-op when key absent) are implemented correctly. The two findings are limited to an informational data-exposure nit in the error message and a comment accuracy issue in the governance plugin; neither affects runtime behaviour. The error message construction in Important Files Changed
Reviews (14): Last reviewed commit: "feat: add governance allowlist support f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
cf73a90 to
565ee32
Compare
63d908a to
50e8ded
Compare
565ee32 to
349d846
Compare
50e8ded to
7ad668e
Compare
349d846 to
7a1013e
Compare
7ad668e to
53da3d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
53da3d4 to
fff3d21
Compare
7a1013e to
b03005e
Compare
4edbac5 to
9f40efe
Compare
694970e to
a2384cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bifrost.go`:
- Around line 4681-4684: The error currently builds a message including the full
allowed slice (using newBifrostErrorFromMsg and PopulateExtraFields with
variables provider and allowed) which gets serialized back to clients; change
the generated error message to a generic statement like "provider is not
permitted for this request" (do not include the contents of allowed) and set
bifrostErr.StatusCode as before, and separately log the detailed allowlist and
context (req.RequestType, model, provider, allowed) to your internal logger for
audit/debugging; apply the same change to the duplicate block that uses the same
pattern around PopulateExtraFields at the other location.
- Around line 4169-4190: RunPreRequestHooks currently runs PreRequestHook
plugins but does not enforce the routing allowlist, letting bypass callers use
providers/fallbacks set by plugins; update RunPreRequestHooks to enforce
BifrostContextKeyRoutingAllowedProviders after pipeline.RunPreRequestHooks by
validating and filtering req.Provider and req.Fallbacks (schemas.BifrostRequest)
against the allowed providers list stored on ctx, clearing or replacing any
disallowed provider entries and logging/wrapping as a schemas.BifrostError when
no allowed provider remains; use
bifrost.getPluginPipeline()/bifrost.releasePluginPipeline() unchanged but ensure
the allowlist check is done before returning so callers cannot skip enforcement.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 746cb507-3e94-4a5d-9307-e77294953639
📒 Files selected for processing (5)
core/bifrost.gocore/schemas/bifrost.godocs/providers/provider-routing.mdxplugins/governance/main.goplugins/modelcatalogresolver/main.go
💤 Files with no reviewable changes (2)
- plugins/governance/main.go
- plugins/modelcatalogresolver/main.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/bifrost.go`:
- Around line 4681-4684: The error currently builds a message including the full
allowed slice (using newBifrostErrorFromMsg and PopulateExtraFields with
variables provider and allowed) which gets serialized back to clients; change
the generated error message to a generic statement like "provider is not
permitted for this request" (do not include the contents of allowed) and set
bifrostErr.StatusCode as before, and separately log the detailed allowlist and
context (req.RequestType, model, provider, allowed) to your internal logger for
audit/debugging; apply the same change to the duplicate block that uses the same
pattern around PopulateExtraFields at the other location.
- Around line 4169-4190: RunPreRequestHooks currently runs PreRequestHook
plugins but does not enforce the routing allowlist, letting bypass callers use
providers/fallbacks set by plugins; update RunPreRequestHooks to enforce
BifrostContextKeyRoutingAllowedProviders after pipeline.RunPreRequestHooks by
validating and filtering req.Provider and req.Fallbacks (schemas.BifrostRequest)
against the allowed providers list stored on ctx, clearing or replacing any
disallowed provider entries and logging/wrapping as a schemas.BifrostError when
no allowed provider remains; use
bifrost.getPluginPipeline()/bifrost.releasePluginPipeline() unchanged but ensure
the allowlist check is done before returning so callers cannot skip enforcement.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 746cb507-3e94-4a5d-9307-e77294953639
📒 Files selected for processing (5)
core/bifrost.gocore/schemas/bifrost.godocs/providers/provider-routing.mdxplugins/governance/main.goplugins/modelcatalogresolver/main.go
💤 Files with no reviewable changes (2)
- plugins/governance/main.go
- plugins/modelcatalogresolver/main.go
🛑 Comments failed to post (2)
core/bifrost.go (2)
4169-4190:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRunPreRequestHooks leaves bypass callers outside the hard allowlist check.
This helper is explicitly for entry points that do not go through
handleRequest/handleStreamRequest, but it only runsPreRequestHooks and returns. Any caller that routes immediately after this method can still use a provider or fallback set onreqwithout ever enforcingBifrostContextKeyRoutingAllowedProviders, which breaks the PR's "core hard guarantee" for those paths. Fold the same allowlist validation/filtering into this API, or make it return a validated request /*schemas.BifrostErrorso bypass callers cannot forget the check.🤖 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/bifrost.go` around lines 4169 - 4190, RunPreRequestHooks currently runs PreRequestHook plugins but does not enforce the routing allowlist, letting bypass callers use providers/fallbacks set by plugins; update RunPreRequestHooks to enforce BifrostContextKeyRoutingAllowedProviders after pipeline.RunPreRequestHooks by validating and filtering req.Provider and req.Fallbacks (schemas.BifrostRequest) against the allowed providers list stored on ctx, clearing or replacing any disallowed provider entries and logging/wrapping as a schemas.BifrostError when no allowed provider remains; use bifrost.getPluginPipeline()/bifrost.releasePluginPipeline() unchanged but ensure the allowlist check is done before returning so callers cannot skip enforcement.
4681-4684:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't echo the full routing allowlist back to clients.
The rejection message includes the entire
allowedslice, and the transport layer serializes that message into the JSON error body. That turns an authorization failure into a provider-policy enumeration endpoint for the virtual key. Keep the detailed allowlist in routing logs, but return a generic 400 message to callers.Also applies to: 4794-4797
🤖 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/bifrost.go` around lines 4681 - 4684, The error currently builds a message including the full allowed slice (using newBifrostErrorFromMsg and PopulateExtraFields with variables provider and allowed) which gets serialized back to clients; change the generated error message to a generic statement like "provider is not permitted for this request" (do not include the contents of allowed) and set bifrostErr.StatusCode as before, and separately log the detailed allowlist and context (req.RequestType, model, provider, allowed) to your internal logger for audit/debugging; apply the same change to the duplicate block that uses the same pattern around PopulateExtraFields at the other location.
a2384cb to
c0c727c
Compare
9f40efe to
9b1c18b
Compare
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 `@core/bifrost.go`:
- Around line 4676-4688: Extract the duplicated allowlist gating logic into a
single helper (e.g., enforceRoutingAllowlist) that accepts the context,
provider, req, model and current fallbacks and returns either the filtered
fallbacks or a prepared *schemas.BifrostError; move the block that checks
ctx.Value(schemas.BifrostContextKeyRoutingAllowedProviders), constructs
newBifrostErrorFromMsg, calls PopulateExtraFields, sets StatusCode, calls
filterFallbacksByAllowlist and req.SetFallbacks into that helper and update both
the sync and streaming callers to call the helper and handle the returned error
or use the returned fallbacks.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ebfdb73f-0d04-4731-ad12-8573f5e94cb0
📒 Files selected for processing (5)
core/bifrost.gocore/schemas/bifrost.godocs/providers/provider-routing.mdxplugins/governance/main.goplugins/modelcatalogresolver/main.go
c0c727c to
c6758e2
Compare
0404470 to
252041c
Compare
c6758e2 to
0cb1e9f
Compare
0cb1e9f to
c6758e2
Compare
252041c to
0404470
Compare
c6758e2 to
8be1116
Compare
0404470 to
2aef19b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bifrost.go`:
- Around line 4708-4714: The routing-allowlist check (enforceRoutingAllowlist)
currently runs before RunLLMPreHooks, allowing PreLLMHook code to call
BifrostRequest.UpdateProvider(...) and change the final provider/fallbacks
without re-checking; move or duplicate the allowlist enforcement so it runs
after RunLLMPreHooks and immediately before dispatching to tryRequest /
tryStreamRequest (or add a final enforceRoutingAllowlist call inside tryRequest
and tryStreamRequest right before queue dispatch), ensuring the final provider
set from preReq.GetRequestFields() is validated against
BifrostContextKeyRoutingAllowedProviders; update all similar sites referenced
(around lines for 4817-4820, 4924-4963, 5175-5302) to perform the post-hook
allowlist enforcement.
In `@plugins/modelcatalogresolver/main.go`:
- Around line 88-95: The current logic uses len(existingFallbacks) == 0 to
decide whether to auto-populate fallbacks, which treats an explicitly empty
fallbacks slice the same as an omitted field; update the request plumbing to
carry a presence signal (either make BifrostRequest.Fallbacks a pointer/slice
pointer and treat nil as “not provided”, or add a boolean like
BifrostRequest.FallbacksProvided) and change the condition in the block that
references existingFallbacks/candidates/GetProvidersForModel to check that
presence signal (e.g. existingFallbacks == nil or !FallbacksProvided) instead of
len(existingFallbacks) == 0 so an explicit empty fallback list is respected.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5bba71b5-4d48-46bb-a5bd-1d04f1a4589c
📒 Files selected for processing (5)
core/bifrost.gocore/schemas/bifrost.godocs/providers/provider-routing.mdxplugins/governance/main.goplugins/modelcatalogresolver/main.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/bifrost.go`:
- Around line 4708-4714: The routing-allowlist check (enforceRoutingAllowlist)
currently runs before RunLLMPreHooks, allowing PreLLMHook code to call
BifrostRequest.UpdateProvider(...) and change the final provider/fallbacks
without re-checking; move or duplicate the allowlist enforcement so it runs
after RunLLMPreHooks and immediately before dispatching to tryRequest /
tryStreamRequest (or add a final enforceRoutingAllowlist call inside tryRequest
and tryStreamRequest right before queue dispatch), ensuring the final provider
set from preReq.GetRequestFields() is validated against
BifrostContextKeyRoutingAllowedProviders; update all similar sites referenced
(around lines for 4817-4820, 4924-4963, 5175-5302) to perform the post-hook
allowlist enforcement.
In `@plugins/modelcatalogresolver/main.go`:
- Around line 88-95: The current logic uses len(existingFallbacks) == 0 to
decide whether to auto-populate fallbacks, which treats an explicitly empty
fallbacks slice the same as an omitted field; update the request plumbing to
carry a presence signal (either make BifrostRequest.Fallbacks a pointer/slice
pointer and treat nil as “not provided”, or add a boolean like
BifrostRequest.FallbacksProvided) and change the condition in the block that
references existingFallbacks/candidates/GetProvidersForModel to check that
presence signal (e.g. existingFallbacks == nil or !FallbacksProvided) instead of
len(existingFallbacks) == 0 so an explicit empty fallback list is respected.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 5bba71b5-4d48-46bb-a5bd-1d04f1a4589c
📒 Files selected for processing (5)
core/bifrost.gocore/schemas/bifrost.godocs/providers/provider-routing.mdxplugins/governance/main.goplugins/modelcatalogresolver/main.go
🛑 Comments failed to post (2)
core/bifrost.go (1)
4708-4714:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRe-check the allowlist after
RunLLMPreHooks.This gate runs before
tryRequest/tryStreamRequest, but both paths still executeRunLLMPreHooksand then route using the provider frompreReq.GetRequestFields(). A later LLM plugin can therefore callUpdateProvider(...)and move the attempt — including fallback attempts — onto a provider that was never checked againstBifrostContextKeyRoutingAllowedProviders. The hard deny needs to run on the final provider/fallback set after pre-hooks, or insidetryRequest/tryStreamRequestbefore queue dispatch.Based on learnings:
PreLLMHookprovider switching is intentionally supported viaBifrostRequest.UpdateProvider(...), and as per coding guidelines the routing allowlist is the fail-closed contract for final provider selection.Also applies to: 4817-4820, 4924-4963, 5175-5302
🤖 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/bifrost.go` around lines 4708 - 4714, The routing-allowlist check (enforceRoutingAllowlist) currently runs before RunLLMPreHooks, allowing PreLLMHook code to call BifrostRequest.UpdateProvider(...) and change the final provider/fallbacks without re-checking; move or duplicate the allowlist enforcement so it runs after RunLLMPreHooks and immediately before dispatching to tryRequest / tryStreamRequest (or add a final enforceRoutingAllowlist call inside tryRequest and tryStreamRequest right before queue dispatch), ensuring the final provider set from preReq.GetRequestFields() is validated against BifrostContextKeyRoutingAllowedProviders; update all similar sites referenced (around lines for 4817-4820, 4924-4963, 5175-5302) to perform the post-hook allowlist enforcement.Sources: Coding guidelines, Learnings
plugins/modelcatalogresolver/main.go (1)
88-95:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't overwrite an explicit
fallbacks: []with catalog fallbacks.Line 95 only checks
len(existingFallbacks) == 0, so omitted fallbacks and an explicitly empty slice are treated the same. That means a caller trying to disable failover can still get cross-provider fallbacks injected here, which changes routing behavior and may forward prompts to providers they intentionally opted out of. This needs a field-presence signal fromBifrostRequestinstead of a length check.Also applies to: 95-104
🤖 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/modelcatalogresolver/main.go` around lines 88 - 95, The current logic uses len(existingFallbacks) == 0 to decide whether to auto-populate fallbacks, which treats an explicitly empty fallbacks slice the same as an omitted field; update the request plumbing to carry a presence signal (either make BifrostRequest.Fallbacks a pointer/slice pointer and treat nil as “not provided”, or add a boolean like BifrostRequest.FallbacksProvided) and change the condition in the block that references existingFallbacks/candidates/GetProvidersForModel to check that presence signal (e.g. existingFallbacks == nil or !FallbacksProvided) instead of len(existingFallbacks) == 0 so an explicit empty fallback list is respected.
Merge activity
|
The base branch was changed.
3 participants
Summary
Introduces a fail-closed routing allowlist enforcement mechanism so that Virtual Key provider restrictions cannot be bypassed by user-specified provider prefixes, downstream routing plugins, or any other layer that runs after governance has evaluated the request.
Changes
BifrostContextKeyRoutingAllowedProviderscontext key ([]ModelProvider) that plugins can set to declare which providers are valid for a given request.ProviderConfigsproviders to this context key after evaluating a Virtual Key. An empty slice means no provider is permitted (fail-closed).handleRequestandhandleStreamRequestin core now read this context key after allPreRequestHookplugins have run and enforce it as a hard constraint: if the resolved primary provider is not in the allowlist, the request is rejected with HTTP 400. Fallbacks targeting non-allowed providers are silently filtered out via the newfilterFallbacksByAllowlisthelper.The two-level design is intentional: cooperative filtering in the catalog resolver and enterprise LB surfaces routing decisions in observability logs, while core enforcement makes the constraint a guarantee that no plugin or user input can circumvent.
Type of change
Affected areas
How to test
go test ./...provider_configsrestricted to a specific provider (e.g.,openai).provider/modelprefix targeting a non-allowed provider (e.g.,anthropic/claude-3). Expect HTTP 400 with a message indicating the provider is not permitted.BifrostContextKeyRoutingAllowedProvidersto an empty slice from a custom plugin and confirm the request fails closed with HTTP 400.Breaking changes
Requests that previously succeeded by specifying a provider prefix not covered by the Virtual Key's
provider_configswill now be rejected with HTTP 400. Any deployment relying on user-supplied provider prefixes overriding VK restrictions must update those requests to use a permitted provider.Security considerations
This change closes a governance bypass where a user or downstream plugin could route a request to a provider not permitted by the Virtual Key's configuration. The enforcement is fail-closed: an empty allowlist denies all providers rather than allowing all. Custom plugins that set
BifrostContextKeyRoutingAllowedProvidersshould be aware that an empty slice will block the request entirely.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
New Features
Documentation