refactor: replace string slices with WhiteList for allowlist fields#2125
Merged
Pratham-Mishra04 merged 1 commit intov1.5.0from Mar 18, 2026
Merged
refactor: replace string slices with WhiteList for allowlist fields#2125Pratham-Mishra04 merged 1 commit intov1.5.0from
Pratham-Mishra04 merged 1 commit intov1.5.0from
Conversation
18 tasks
Collaborator
Author
This was referenced Mar 17, 2026
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (31)
📝 WalkthroughWalkthroughReplaces ad-hoc string-slice allowlists with a new public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
framework/modelcatalog/main.go (1)
542-557:⚠️ Potential issue | 🟠 MajorUse the
WhiteListmembership helpers all the way through this check.Line 544 and Line 553 are still exact-case string comparisons, so configs like
["GPT-4O"]or["OpenAI/GPT-4O"]will be rejected even though the new whitelist contract is case-insensitive. Routing the direct-match path throughallowedModels.IsAllowed(model)and the prefixed path throughallowedModels.Contains(providerModel)/strings.EqualFoldkeeps this aligned with the rest of the refactor.Proposed fix
- for _, allowedModel := range allowedModels { - // Direct match: "gpt-4o" == "gpt-4o" - if allowedModel == model { - return true - } - - // Provider-prefixed match: verify it exists in provider's catalog first - // This ensures we only allow provider-specific model combinations that are actually supported - if strings.Contains(allowedModel, "/") { - // Check if this exact prefixed model exists in the provider's catalog - // e.g., for openrouter, check if "openai/gpt-4o" is in its catalog - if slices.Contains(providerCatalogModels, allowedModel) { - // Extract the model part and compare with request - _, modelPart := schemas.ParseModelString(allowedModel, "") - if modelPart == model { - return true - } - } - } - } + if allowedModels.IsAllowed(model) { + return true + } + + for _, providerModel := range providerCatalogModels { + if !allowedModels.Contains(providerModel) { + continue + } + + _, modelPart := schemas.ParseModelString(providerModel, "") + if strings.EqualFold(modelPart, model) { + return true + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 542 - 557, The direct equality checks in the allowed-model loop use case-sensitive string comparison; replace them with the Whitelist helpers so checks honor the case-insensitive contract: call allowedModels.IsAllowed(model) instead of comparing allowedModel == model for the direct-match path, and for provider-prefixed entries use allowedModels.Contains(providerModel) (or allowedModels.Contains(allowedModel) as appropriate) and compare the model parts with strings.EqualFold (use schemas.ParseModelString to extract the modelPart) so providerCatalogModels membership and model equality both use case-insensitive helpers.transports/bifrost-http/handlers/providers.go (1)
747-780:⚠️ Potential issue | 🟠 MajorKeep
filterModelsByKeysonWhiteListsemantics.Line 773 now turns a matched key with
Models: []into “allow everything”, which is the opposite of the new deny-by-default contract, and Line 779 reintroduces exact-case matching by looking upallowedModels[model]. Track whether any requested key actually matched, and useWhiteList.IsAllowed()(or equivalent normalization) for the final membership check.Proposed fix
- allowedModels := make(map[string]bool) - hasRestrictedKey := false - hasUnrestrictedKey := false + allowedModels := make(schemas.WhiteList, 0) + hasRestrictedKey := false + hasUnrestrictedKey := false + matchedKey := false for _, keyID := range keyIDs { for _, key := range config.Keys { if key.ID == keyID { + matchedKey = true if key.Models.IsUnrestricted() { // Key allows all models (wildcard) hasUnrestrictedKey = true } else if !key.Models.IsEmpty() { // Key has specific model restrictions - add them to allowedModels hasRestrictedKey = true - for _, model := range key.Models { - allowedModels[model] = true - } + allowedModels = append(allowedModels, key.Models...) } // else: empty Models = deny all — key contributes nothing break } } } // If any key is unrestricted, return all models (union of "all" and restricted subsets is "all") if hasUnrestrictedKey { return models } - // If no keys have model restrictions (e.g., unknown key IDs), return all models - if !hasRestrictedKey { + // Unknown key IDs should not change the result. + if !matchedKey { return models } + // At least one requested key matched and all matched keys deny all. + if !hasRestrictedKey { + return []string{} + } // Filter models based on restrictions from restricted keys only filtered := []string{} for _, model := range models { - if allowedModels[model] { + if allowedModels.IsAllowed(model) { filtered = append(filtered, model) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/providers.go` around lines 747 - 780, The current filterModelsByKeys logic wrongly treats a matched key with Models: [] as “allow all” and does exact-case map lookups; instead implement deny-by-default WhiteList semantics: in filterModelsByKeys, track whether any requested keyID actually matched a config key (e.g., matchedKey flag), treat key.Models.IsEmpty() as “deny all” (do not add anything to allowedModels), if key.Models.IsUnrestricted() short-circuit to return models, and when building/filtering the final list use the WhiteList.IsAllowed(model) (or the equivalent normalization helper) for membership checks rather than direct allowedModels[model] lookups; if no requested keyIDs matched, fall back to returning models. Ensure you reference and update the code paths around allowedModels, hasRestrictedKey/hasUnrestrictedKey, and the final membership check so they follow WhiteList.IsAllowed semantics.core/bifrost.go (1)
6127-6153:⚠️ Potential issue | 🟡 MinorUpdate the stale model-filtering comment to match current behavior.
The inline comment says empty model lists are supported for all models, but the new
key.Models.IsAllowed(model)logic enforces deny-by-default for empty lists. This can mislead future edits.✏️ Suggested comment fix
- // filter out keys which don't support the model, if the key has no models, it is supported for all models + // filter out keys that don't support the model; an empty whitelist denies all models🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 6127 - 6153, The comment about model-list semantics is inaccurate: update the inline comment near the model-filtering logic (around supportedKeys, skipModelCheck, and the branch that uses key.Models.IsAllowed(model)) to state that an empty key.Models now means deny-by-default (not “supported for all models”), while ["*"] allows all models and specific lists allow only listed models; also note that hasValue and CanProviderKeyValueBeEmpty(baseProviderType) are still required for a key to be considered before calling key.Models.IsAllowed(model). Ensure the comment clearly references key.Models.IsAllowed(model), skipModelCheck, and the supportedKeys accumulation so future editors understand current behavior.transports/bifrost-http/handlers/mcp.go (1)
381-383:⚠️ Potential issue | 🔴 CriticalFix type mismatch: use
schemas.WhiteList{}instead of[]string{}for clearing auto-exec tools.At line 382,
req.ToolsToAutoExecute(typed asschemas.WhiteList) is assigned[]string{}. For type consistency and to prevent breakage if the field type is migrated, use the explicit type. Additionally, the check can be simplified using theIsEmpty()method already available onWhiteList.🛠️ Proposed fix
- if len(req.ToolsToExecute) == 0 { - req.ToolsToAutoExecute = []string{} - } + if req.ToolsToExecute.IsEmpty() { + req.ToolsToAutoExecute = schemas.WhiteList{} + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcp.go` around lines 381 - 383, The code assigns a []string{} to req.ToolsToAutoExecute which is typed as schemas.WhiteList; change the assignment to use schemas.WhiteList{} to fix the type mismatch and avoid future breakage, and simplify the conditional by using req.ToolsToExecute.IsEmpty() or req.ToolsToAutoExecute.IsEmpty() as appropriate (use the WhiteList.IsEmpty() method) so the branch becomes: if the white list is empty then set req.ToolsToAutoExecute = schemas.WhiteList{}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/account.go`:
- Around line 59-64: The duplicate-detection in the whitelist loop uses
case-sensitive keys so entries like "gpt-4" and "GPT-4" slip through even though
Contains()/IsAllowed() are case-insensitive; in the validation loop (where seen
:= make(map[string]struct{}, len(wl)) and the for _, v := range wl { ... }
resides) normalize each entry (e.g., strings.ToLower(v) or strings.ToUpper(v))
before checking/inserting into seen and use that normalized string for the
duplicate check and map key so case-variant duplicates are caught by Validate().
In `@framework/configstore/tables/virtualkey.go`:
- Line 31: Add GORM BeforeSave hooks that validate the WhiteList before
persistence: implement BeforeSave(tx *gorm.DB) error on the structs that contain
the AllowedModels fields (e.g., the VirtualKey-related structs with
AllowedModels schemas.WhiteList) and inside each hook call the
WhiteList.Validate() and return the validation error so invalid lists
(duplicates or mixed ["*","..."]) are rejected before GORM writes them; use the
gorm hook signature (BeforeSave(tx *gorm.DB) error) and ensure any validation
error is returned to abort the save.
---
Outside diff comments:
In `@core/bifrost.go`:
- Around line 6127-6153: The comment about model-list semantics is inaccurate:
update the inline comment near the model-filtering logic (around supportedKeys,
skipModelCheck, and the branch that uses key.Models.IsAllowed(model)) to state
that an empty key.Models now means deny-by-default (not “supported for all
models”), while ["*"] allows all models and specific lists allow only listed
models; also note that hasValue and CanProviderKeyValueBeEmpty(baseProviderType)
are still required for a key to be considered before calling
key.Models.IsAllowed(model). Ensure the comment clearly references
key.Models.IsAllowed(model), skipModelCheck, and the supportedKeys accumulation
so future editors understand current behavior.
In `@framework/modelcatalog/main.go`:
- Around line 542-557: The direct equality checks in the allowed-model loop use
case-sensitive string comparison; replace them with the Whitelist helpers so
checks honor the case-insensitive contract: call allowedModels.IsAllowed(model)
instead of comparing allowedModel == model for the direct-match path, and for
provider-prefixed entries use allowedModels.Contains(providerModel) (or
allowedModels.Contains(allowedModel) as appropriate) and compare the model parts
with strings.EqualFold (use schemas.ParseModelString to extract the modelPart)
so providerCatalogModels membership and model equality both use case-insensitive
helpers.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 381-383: The code assigns a []string{} to req.ToolsToAutoExecute
which is typed as schemas.WhiteList; change the assignment to use
schemas.WhiteList{} to fix the type mismatch and avoid future breakage, and
simplify the conditional by using req.ToolsToExecute.IsEmpty() or
req.ToolsToAutoExecute.IsEmpty() as appropriate (use the WhiteList.IsEmpty()
method) so the branch becomes: if the white list is empty then set
req.ToolsToAutoExecute = schemas.WhiteList{}.
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 747-780: The current filterModelsByKeys logic wrongly treats a
matched key with Models: [] as “allow all” and does exact-case map lookups;
instead implement deny-by-default WhiteList semantics: in filterModelsByKeys,
track whether any requested keyID actually matched a config key (e.g.,
matchedKey flag), treat key.Models.IsEmpty() as “deny all” (do not add anything
to allowedModels), if key.Models.IsUnrestricted() short-circuit to return
models, and when building/filtering the final list use the
WhiteList.IsAllowed(model) (or the equivalent normalization helper) for
membership checks rather than direct allowedModels[model] lookups; if no
requested keyIDs matched, fall back to returning models. Ensure you reference
and update the code paths around allowedModels,
hasRestrictedKey/hasUnrestrictedKey, and the final membership check so they
follow WhiteList.IsAllowed semantics.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0fe5874d-e8f7-44ef-9884-baf19517c376
📒 Files selected for processing (14)
core/bifrost.gocore/mcp/agent.gocore/mcp/utils.gocore/schemas/account.gocore/schemas/mcp.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey.goframework/modelcatalog/main.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/providers.go
Pratham-Mishra04
force-pushed
the
03-16-feat_standardize_empty_array_conventions_in_bifrost
branch
from
0da4bed to
13d4479
Compare
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
ea850ee to
189d310
Compare
|
|
1 similar comment
|
|
12 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
framework/configstore/tables/mcp.go (1)
71-85:⚠️ Potential issue | 🟠 Major
BeforeSavestill letstools_to_auto_executedrift outsidetools_to_execute.The new
Validate()calls only check eachWhiteListin isolation.transports/bifrost-http/handlers/mcp.goalready enforces the cross-field rule, but config sync / direct store writes can still persistToolsToAutoExecute=["foo"]withToolsToExecute=[]or without"*"on the execute list. That leaves runtime to silently skip the tool instead of rejecting the config at save time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/mcp.go` around lines 71 - 85, The BeforeSave logic currently validates each WhiteList in isolation; add a cross-field check after validating c.ToolsToExecute and c.ToolsToAutoExecute (and before marshalling to JSON) to ensure c.ToolsToAutoExecute is a subset of c.ToolsToExecute or that c.ToolsToExecute contains the wildcard "*"—if not, return a descriptive error (e.g., "tools_to_auto_execute must be subset of tools_to_execute or tools_to_execute must contain \"*\""). Use the existing identifiers (c.ToolsToExecute, c.ToolsToAutoExecute, Validate()) and perform the subset check by iterating c.ToolsToAutoExecute entries and confirming each exists in c.ToolsToExecute (or short-circuit on "*"); keep the current JSON assignment behavior when the check passes.transports/bifrost-http/handlers/providers.go (1)
753-760:⚠️ Potential issue | 🟠 MajorEmpty key model allowlists now leak every model through this filter.
WhiteList.IsEmpty()means “deny all”, but this branch still only markshasRestrictedKeyfor non-empty lists. When a requested key matches withModels: [], the fallback on Line 773 returns the full model set instead of[]. This block also feedsallowedModelswith raw strings, so the lookup on Line 779 still bypasses the new case-insensitive whitelist semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/providers.go` around lines 753 - 760, The whitelist handling incorrectly treats an empty key.Models (WhiteList.IsEmpty()) as permissive; instead, when key.Models.IsEmpty() is present you must mark hasRestrictedKey = true and NOT populate allowedModels with any models (so an explicit empty allowlist denies all). Normalize model names to a canonical form (e.g., strings.ToLower) when inserting into allowedModels and ensure the runtime lookup also lowercases requested model names so matching is case-insensitive; finally, change the fallback logic so that if hasRestrictedKey is true but allowedModels is empty you return an empty model set (not the full set).framework/modelcatalog/main.go (1)
532-562:⚠️ Potential issue | 🟡 MinorUse case-insensitive comparisons in the explicit whitelist branch.
The explicit matching path still uses case-sensitive equality/containment, so mixed-case whitelist entries can be incorrectly denied.
🔧 Proposed fix
func (mc *ModelCatalog) IsModelAllowedForProvider(provider schemas.ModelProvider, model string, allowedModels schemas.WhiteList) bool { @@ for _, allowedModel := range allowedModels { // Direct match: "gpt-4o" == "gpt-4o" - if allowedModel == model { + if strings.EqualFold(allowedModel, model) { return true } @@ - if slices.Contains(providerCatalogModels, allowedModel) { + if slices.ContainsFunc(providerCatalogModels, func(candidate string) bool { + return strings.EqualFold(candidate, allowedModel) + }) { // Extract the model part and compare with request _, modelPart := schemas.ParseModelString(allowedModel, "") - if modelPart == model { + if strings.EqualFold(modelPart, model) { return true } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 532 - 562, In IsModelAllowedForProvider, the explicit-whitelist branch currently does case-sensitive checks; update the direct-match and provider-prefixed checks to be case-insensitive by using case-insensitive comparisons: replace the direct equality check of allowedModel == model with a case-insensitive comparison (e.g., strings.EqualFold), and when verifying prefixed entries first against providerCatalogModels, perform a case-insensitive contains (looping providerCatalogModels and using EqualFold against allowedModel) and then compare the extracted modelPart to model with a case-insensitive comparison (EqualFold) as well; ensure you apply this change to allowedModel, providerCatalogModels and modelPart comparisons in the IsModelAllowedForProvider function.
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/governance.go (1)
468-483: Extract shared provider-config whitelist/key resolution into one helper.The same validation + key-fetch logic is repeated in create and update branches; centralizing it will reduce drift when whitelist semantics evolve.
Also applies to: 819-839, 897-920
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 468 - 483, The create/update branches duplicate provider-config validation and key-fetch logic (calls to pc.AllowedModels.Validate, pc.KeyIDs.Validate, checks of pc.KeyIDs.IsUnrestricted/IsEmpty and h.configStore.GetKeysByIDs) — extract that into a single helper (e.g., resolveProviderKeys or validateAndFetchProviderKeys) that accepts context, the provider config (pc) and configStore, performs the two Validate() calls, returns (allowAllKeys bool, keys []configstoreTables.TableKey, error) and use it from both create and update paths (replace the duplicated blocks around AllowedModels/KeyIDs and GetKeysByIDs in the code regions referenced).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/account.go`:
- Line 76: The change of the Key.Models field from []string to the defined type
WhiteList is source-breaking because all composite literals that set Models:
[]string{...} now fail; fix this by restoring source compatibility—either change
WhiteList to be a type alias (type WhiteList = []string) or add a simple
conversion helper and a Key constructor to centralize conversions (e.g., a
function like NewKeyWithModels or ToWhiteList used by callers); update the
schemas.Key declaration (Models) and provide the helper named ToWhiteList or
NewKeyWithModels so existing call sites can be updated easily without widespread
edits.
In `@plugins/governance/main.go`:
- Line 616: Define two typed context-key constants of type
schemas.BifrostContextKey (e.g., governanceModelContextKey and
governanceModelIDContextKey) and replace all raw string context keys "model" and
"modelId" with these constants in the governance plugin; specifically update
ctx.SetValue and any ctx.Value/ctx.Get usages in loadBalanceProvider and
applyRoutingRules to use governanceModelContextKey and
governanceModelIDContextKey so the keys are strongly typed and avoid collisions
with other middleware.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 179-180: The PUT handler contains a type mismatch: change any
assignment of req.ToolsToAutoExecute from []string{} to the correct type
schemas.WhiteList (e.g., replace the occurrence that sets req.ToolsToAutoExecute
= []string{} with req.ToolsToAutoExecute = schemas.WhiteList{}), and scan the
handler for any other assignments or zero-values for ToolsToAutoExecute to
ensure they all use schemas.WhiteList instead of []string.
---
Outside diff comments:
In `@framework/configstore/tables/mcp.go`:
- Around line 71-85: The BeforeSave logic currently validates each WhiteList in
isolation; add a cross-field check after validating c.ToolsToExecute and
c.ToolsToAutoExecute (and before marshalling to JSON) to ensure
c.ToolsToAutoExecute is a subset of c.ToolsToExecute or that c.ToolsToExecute
contains the wildcard "*"—if not, return a descriptive error (e.g.,
"tools_to_auto_execute must be subset of tools_to_execute or tools_to_execute
must contain \"*\""). Use the existing identifiers (c.ToolsToExecute,
c.ToolsToAutoExecute, Validate()) and perform the subset check by iterating
c.ToolsToAutoExecute entries and confirming each exists in c.ToolsToExecute (or
short-circuit on "*"); keep the current JSON assignment behavior when the check
passes.
In `@framework/modelcatalog/main.go`:
- Around line 532-562: In IsModelAllowedForProvider, the explicit-whitelist
branch currently does case-sensitive checks; update the direct-match and
provider-prefixed checks to be case-insensitive by using case-insensitive
comparisons: replace the direct equality check of allowedModel == model with a
case-insensitive comparison (e.g., strings.EqualFold), and when verifying
prefixed entries first against providerCatalogModels, perform a case-insensitive
contains (looping providerCatalogModels and using EqualFold against
allowedModel) and then compare the extracted modelPart to model with a
case-insensitive comparison (EqualFold) as well; ensure you apply this change to
allowedModel, providerCatalogModels and modelPart comparisons in the
IsModelAllowedForProvider function.
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 753-760: The whitelist handling incorrectly treats an empty
key.Models (WhiteList.IsEmpty()) as permissive; instead, when
key.Models.IsEmpty() is present you must mark hasRestrictedKey = true and NOT
populate allowedModels with any models (so an explicit empty allowlist denies
all). Normalize model names to a canonical form (e.g., strings.ToLower) when
inserting into allowedModels and ensure the runtime lookup also lowercases
requested model names so matching is case-insensitive; finally, change the
fallback logic so that if hasRestrictedKey is true but allowedModels is empty
you return an empty model set (not the full set).
---
Nitpick comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 468-483: The create/update branches duplicate provider-config
validation and key-fetch logic (calls to pc.AllowedModels.Validate,
pc.KeyIDs.Validate, checks of pc.KeyIDs.IsUnrestricted/IsEmpty and
h.configStore.GetKeysByIDs) — extract that into a single helper (e.g.,
resolveProviderKeys or validateAndFetchProviderKeys) that accepts context, the
provider config (pc) and configStore, performs the two Validate() calls, returns
(allowAllKeys bool, keys []configstoreTables.TableKey, error) and use it from
both create and update paths (replace the duplicated blocks around
AllowedModels/KeyIDs and GetKeysByIDs in the code regions referenced).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b959f7a4-e9a3-49f1-8a62-0dfd1014572e
📒 Files selected for processing (14)
core/bifrost.gocore/mcp/agent.gocore/mcp/utils.gocore/schemas/account.gocore/schemas/mcp.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey.goframework/modelcatalog/main.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/providers.go
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/governance/resolver.go
- transports/bifrost-http/handlers/mcpserver.go
- core/mcp/utils.go
- core/bifrost.go
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
189d310 to
cb27490
Compare
Pratham-Mishra04
force-pushed
the
03-16-feat_standardize_empty_array_conventions_in_bifrost
branch
from
13d4479 to
2ee4667
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/modelcatalog/main.go (1)
547-562:⚠️ Potential issue | 🟠 MajorKeep catalog-backed whitelist checks case-insensitive.
WhiteListmatching is supposed to be case-insensitive, but this primary path still uses==andslices.Contains(). Mixed-case entries likeOpenAI/GPT-4Owill still be rejected whenever the model catalog is present, even though the fallbackpc.AllowedModels.IsAllowed(model)path inplugins/governance/resolver.gowould allow them.🛠️ Suggested fix
for _, allowedModel := range allowedModels { // Direct match: "gpt-4o" == "gpt-4o" - if allowedModel == model { + if strings.EqualFold(allowedModel, model) { return true } @@ if strings.Contains(allowedModel, "/") { // Check if this exact prefixed model exists in the provider's catalog // e.g., for openrouter, check if "openai/gpt-4o" is in its catalog - if slices.Contains(providerCatalogModels, allowedModel) { - // Extract the model part and compare with request - _, modelPart := schemas.ParseModelString(allowedModel, "") - if modelPart == model { - return true - } + for _, catalogModel := range providerCatalogModels { + if !strings.EqualFold(catalogModel, allowedModel) { + continue + } + // Extract the model part and compare with request + _, modelPart := schemas.ParseModelString(allowedModel, "") + if strings.EqualFold(modelPart, model) { + return true + } + break } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 547 - 562, The whitelist/catalog checks are currently case-sensitive; update the logic in the allowedModels loop to perform case-insensitive comparisons: use case-insensitive equality for the direct match (replace the == check with strings.EqualFold or equivalent), when checking slices.Contains(providerCatalogModels, allowedModel) compare against a lowercased/normalized providerCatalogModels set (or perform a case-insensitive contains), and compare modelPart to model with a case-insensitive comparison (strings.EqualFold) after schemas.ParseModelString; ensure all checks that reference allowedModels and providerCatalogModels are normalized the same way so mixed-case entries like "OpenAI/GPT-4O" are accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 497-502: The Validate() failures for pc.AllowedModels and
pc.KeyIDs are client input errors but currently get handled as 500 because they
occur after ExecuteTransaction; move the calls to pc.AllowedModels.Validate()
and pc.KeyIDs.Validate() (and the similar checks at lines 582-584) to run before
ExecuteTransaction so invalid allowlists return early, or alternatively wrap
their errors in a sentinel type (e.g., ErrBadRequest) and ensure the outer
handler recognizes that sentinel and maps it to StatusBadRequest; update the
code paths around ExecuteTransaction and the outer handler error mapping to
treat sentinel validation errors as 400 rather than 500.
- Around line 848-853: Validation failures from pc.AllowedModels.Validate() and
pc.KeyIDs.Validate() (and MCP tool allowlist checks) are treated as generic
errors and surface as 500; change the update path to treat these as client input
errors by validating before or inside the transaction and returning a
BadRequest-typed error so the caller maps it to 400. Concretely: move or
duplicate the Validate() calls for AllowedModels and KeyIDs (and MCP tool
allowlist validation) to run prior to committing the transaction (or, if they
must run inside the tx, return a specific sentinel error type such as
ErrBadRequest or use the existing api bad-request helper) and ensure the outer
error handler maps that sentinel/type to a 400 response instead of the generic
500; update the code paths that call pc.AllowedModels.Validate(),
pc.KeyIDs.Validate(), and the MCP allowlist checks so they return the
BadRequest-typed error on validation failure.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 458-463: The Key struct is being created with Models set to an
empty deny-by-default WhiteList (schemas.WhiteList{}), which blocks
header-provided direct keys; change the Models initialization to the
unrestricted whitelist variant (e.g., use the library's unrestricted constructor
such as schemas.Unrestricted() or the equivalent helper) when building
schemas.Key in ctx.go and apply the same change in the Azure direct-key
constructor in the OpenAI integration so that header/provider direct keys remain
unrestricted.
---
Outside diff comments:
In `@framework/modelcatalog/main.go`:
- Around line 547-562: The whitelist/catalog checks are currently
case-sensitive; update the logic in the allowedModels loop to perform
case-insensitive comparisons: use case-insensitive equality for the direct match
(replace the == check with strings.EqualFold or equivalent), when checking
slices.Contains(providerCatalogModels, allowedModel) compare against a
lowercased/normalized providerCatalogModels set (or perform a case-insensitive
contains), and compare modelPart to model with a case-insensitive comparison
(strings.EqualFold) after schemas.ParseModelString; ensure all checks that
reference allowedModels and providerCatalogModels are normalized the same way so
mixed-case entries like "OpenAI/GPT-4O" are accepted.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90c0bed8-9e00-4828-b7cf-d094e993fc47
📒 Files selected for processing (16)
core/bifrost.gocore/mcp/agent.gocore/mcp/utils.gocore/schemas/account.gocore/schemas/mcp.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey.goframework/modelcatalog/main.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/integrations/openai.gotransports/bifrost-http/lib/ctx.go
🚧 Files skipped from review as they are similar to previous changes (6)
- core/mcp/utils.go
- transports/bifrost-http/handlers/providers.go
- core/mcp/agent.go
- core/bifrost.go
- core/schemas/account.go
- transports/bifrost-http/handlers/mcpserver.go
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
cb27490 to
61d5323
Compare
Pratham-Mishra04
force-pushed
the
03-16-feat_standardize_empty_array_conventions_in_bifrost
branch
2 times, most recently
from
80ebe07 to
413b2bb
Compare
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
61d5323 to
791ecbf
Compare
9 tasks
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 (10)
core/providers/anthropic/models.go (1)
21-29:⚠️ Potential issue | 🟡 MinorDon't return a continuation token for a deny-all whitelist.
allowedModels.IsEmpty()means the filtered result is permanently empty, butNextPageTokenhas already been populated here. That makes clients paginate through empty pages and diverges from the sibling implementation incore/providers/replicate/models.go, which returns before setting pagination state.🔧 Suggested fix
- // Map Anthropic's cursor-based pagination to Bifrost's token-based pagination - // If there are more results, set next_page_token to last_id so it can be used in the next request - if response.HasMore && response.LastID != nil { - bifrostResponse.NextPageToken = *response.LastID - } - if !unfiltered && allowedModels.IsEmpty() { return bifrostResponse } + + // Map Anthropic's cursor-based pagination to Bifrost's token-based pagination + // If there are more results, set next_page_token to last_id so it can be used in the next request + if response.HasMore && response.LastID != nil { + bifrostResponse.NextPageToken = *response.LastID + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/anthropic/models.go` around lines 21 - 29, The code sets bifrostResponse.NextPageToken from response.HasMore/response.LastID before checking the whitelist, causing a continuation token to be returned for a permanently-empty (deny-all) filtered result; update the logic so NextPageToken is only set when pagination makes sense—either move the pagination block (the response.HasMore && response.LastID check that sets bifrostResponse.NextPageToken) to after the unfiltered && allowedModels.IsEmpty() check, or add a guard so you set bifrostResponse.NextPageToken only if response.HasMore && response.LastID != nil && !( !unfiltered && allowedModels.IsEmpty() ), keeping checks for unfiltered and allowedModels.IsEmpty() and leaving the rest of the function unchanged.transports/bifrost-http/lib/ctx.go (1)
146-160:⚠️ Potential issue | 🟠 MajorDenylist
authorizationtoo.Blocking
x-api-keyandx-goog-api-keyhere still leaves a credential tunnel: callers can smuggleauthorizationthroughx-bf-eh-*or the raw allowlist path. That can leak or override the outbound provider auth header.🔒 Proposed fix
securityDenylist := map[string]bool{ + "authorization": true, "proxy-authorization": true, "cookie": true, "host": true, "content-length": true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 146 - 160, The security denylist in ctx.go (the securityDenylist map) is missing the "authorization" header key; add "authorization": true to the securityDenylist map to ensure Authorization headers cannot be tunneled via x-bf-eh-* or other paths and update any related tests/uses of securityDenylist to account for the new denied header.framework/modelcatalog/main.go (1)
533-565:⚠️ Potential issue | 🟠 MajorDon't reimplement whitelist matching with raw string equality.
This branch bypasses
WhiteList's case-insensitive semantics by usingallowedModel == modelandslices.Contains(...). An entry likeGPT-4Ois denied here even thoughWhiteList.Contains("gpt-4o")should accept it.♻️ Suggested fix
func (mc *ModelCatalog) IsModelAllowedForProvider(provider schemas.ModelProvider, model string, allowedModels schemas.WhiteList) bool { // Case 1: ["*"] = allow all models; use catalog to determine support // Empty allowedModels = deny all (fail-safe deny-by-default) if allowedModels.IsUnrestricted() { supportedProviders := mc.GetProvidersForModel(model) return slices.Contains(supportedProviders, provider) } if allowedModels.IsEmpty() { return false } + if allowedModels.Contains(model) { + return true + } // Case 2: Explicit allowedModels = check if model matches any entry // Get provider's catalog models for validation of prefixed entries providerCatalogModels := mc.GetModelsForProvider(provider) for _, allowedModel := range allowedModels { - // Direct match: "gpt-4o" == "gpt-4o" - if allowedModel == model { - return true - } - // Provider-prefixed match: verify it exists in provider's catalog first // This ensures we only allow provider-specific model combinations that are actually supported if strings.Contains(allowedModel, "/") { // Check if this exact prefixed model exists in the provider's catalog // e.g., for openrouter, check if "openai/gpt-4o" is in its catalog - if slices.Contains(providerCatalogModels, allowedModel) { + if slices.ContainsFunc(providerCatalogModels, func(candidate string) bool { + return strings.EqualFold(candidate, allowedModel) + }) { // Extract the model part and compare with request _, modelPart := schemas.ParseModelString(allowedModel, "") - if modelPart == model { + if strings.EqualFold(modelPart, model) { return true } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 533 - 565, The loop in IsModelAllowedForProvider currently uses raw string equality and slices.Contains, which bypasses the WhiteList's case-insensitive semantics; update it to use WhiteList.Contains for comparisons: replace the direct check "if allowedModel == model" with a call to allowedModels.Contains(model) (or otherwise use allowedModels.Contains(allowedModel) appropriately), and for provider-prefixed entries keep the provider catalog existence check but after ParseModelString compare the parsed model part using allowedModels.Contains(modelPart) (instead of slices.Contains or ==); this ensures case-insensitive whitelist matching while preserving the provider-catalog validation in GetModelsForProvider and ParseModelString logic.core/providers/openai/models.go (1)
21-42:⚠️ Potential issue | 🟠 MajorNormalize the dedupe key before backfilling allowed models.
Line 23 now uses case-insensitive whitelist matching, but
includedModelsis still keyed by the rawmodel.ID. If the config containsGPT-4oand the provider returnsgpt-4o, the real model is included and Lines 36-42 still backfillGPT-4oas a second entry. That yields duplicate/phantom model IDs under the new WhiteList semantics.Possible fix
import ( + "strings" + "github.com/maximhq/bifrost/core/schemas" ) @@ includedModels := make(map[string]bool) for _, model := range response.Data { if !unfiltered && !allowedModels.IsUnrestricted() && !allowedModels.Contains(model.ID) { continue } + normalizedModelID := strings.ToLower(model.ID) bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ ID: string(providerKey) + "/" + model.ID, Created: model.Created, OwnedBy: schemas.Ptr(model.OwnedBy), ContextLength: model.ContextWindow, }) - includedModels[model.ID] = true + includedModels[normalizedModelID] = true } @@ if !unfiltered && !allowedModels.IsUnrestricted() { for _, allowedModel := range allowedModels { - if !includedModels[allowedModel] { + if !includedModels[strings.ToLower(allowedModel)] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ ID: string(providerKey) + "/" + allowedModel, Name: schemas.Ptr(allowedModel), }) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/models.go` around lines 21 - 42, The dedupe map includedModels is keyed by raw model.ID which breaks the new case-insensitive whitelist logic and causes duplicate entries when casing differs; update the logic in the response.Data loop and the backfill loop to use the same normalized key (e.g., strings.ToLower or the same normalization used by allowedModels) when setting and checking includedModels so that model IDs are compared case-insensitively (apply normalization to model.ID when adding to includedModels and to allowedModel when checking if includedModels contains it) while still writing the original provider ID or the allowedModel string into bifrostResponse.Data as before.core/providers/vertex/utils.go (1)
163-205:⚠️ Potential issue | 🟠 MajorBackfill can create a second Vertex model entry without
Deployment.Line 169 can match
aliascase-insensitively, butaddedModelIDsis keyed by the original casing and Lines 192-205 backfill the raw allowlist value. If the cases differ, you return both the deployment-backed model and a second backfilled model with noDeployment, so selecting the latter cannot be routed correctly.Possible fix
addedModelIDs := make(map[string]bool) @@ for alias, deploymentValue := range deployments { if restrictAllowed && !allowedModels.Contains(alias) { continue } + normalizedAlias := strings.ToLower(alias) modelID := string(schemas.Vertex) + "/" + alias - if addedModelIDs[modelID] { + if addedModelIDs[normalizedAlias] { continue } @@ response.Data = append(response.Data, modelEntry) - addedModelIDs[modelID] = true + addedModelIDs[normalizedAlias] = true } @@ for _, allowedModel := range allowedModels { - modelID := string(schemas.Vertex) + "/" + allowedModel - if addedModelIDs[modelID] { + normalizedAllowedModel := strings.ToLower(allowedModel) + if addedModelIDs[normalizedAllowedModel] { continue } + modelID := string(schemas.Vertex) + "/" + allowedModel @@ response.Data = append(response.Data, modelEntry) - addedModelIDs[modelID] = true + addedModelIDs[normalizedAllowedModel] = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/utils.go` around lines 163 - 205, The duplicate-entry bug is caused by case differences between deployment aliases and allowlist entries: addedModelIDs keys use original casing so a backfilled allowedModel with different case produces a second Model without Deployment. Fix by normalizing the model ID when checking/setting the map (e.g., compute modelID as before but use a normalizedKey := strings.ToLower(modelID) for lookups and assignments in addedModelIDs) in both the deployments loop and the allowedModels loop so case-insensitive matches dedupe correctly; ensure you still construct the Model.ID and Name as existing code does and add an import for strings if needed.core/mcp/agent.go (1)
437-476:⚠️ Potential issue | 🔴 CriticalDon’t rebuild code-mode auto-exec permissions from raw config after filtering.
GetToolPerClient(ctx)has already appliedToolsToExecuteand request-level include filters, but Lines 455-470 rebuildallowedTools[clientName]fromToolsToAutoExecutealone and collapse unrestricted configs to"*". That letsexecute_tool_codeauto-run tools the request explicitly filtered out. It also means a client filtered down to zero visible tools disappears fromallClientNames, soisToolCallAllowedForCodeModecan misclassifyclient.tool()as a built-in and allow it.Please derive the code-mode allowlist from
availableToolsPerClient[clientName]and intersect that withToolsToAutoExecute, instead of trusting the raw config a second time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/mcp/agent.go` around lines 437 - 476, The current logic rebuilds code-mode auto-exec allowlists from raw ExecutionConfig.ToolsToAutoExecute and can re-enable tools the request filtered out; instead, derive the allowlist from availableToolsPerClient (from ClientManager.GetToolPerClient) and intersect that with client.ExecutionConfig.ToolsToAutoExecute: for each clientName in availableToolsPerClient, keep the client in allClientNames, take the visible tool names from availableToolsPerClient[clientName], map/normalize those names the same way you do now (replace "-" with "_" and call parseToolName), then if ToolsToAutoExecute.IsUnrestricted() allow only the visible set (or mark "*" only if all visible tools are allowed by config), otherwise compute the intersection between the normalized visible tool names and ToolsToAutoExecute and set allowedTools[clientName] to that intersection; this preserves request-level filtering and prevents isToolCallAllowedForCodeMode/client.tool() misclassification.core/providers/huggingface/models.go (1)
56-62:⚠️ Potential issue | 🟡 MinorBackfill can duplicate models when allowlist casing differs.
Because Line 40 matching is case-insensitive,
model.ModelIDmay already be included even ifallowedModelhas different casing; Line 58’s direct map lookup can still treat it as missing and backfill a duplicate.💡 Suggested fix
- includedModels := make(map[string]bool) + includedModels := make(map[string]bool) @@ - includedModels[model.ModelID] = true + includedModels[strings.ToLower(model.ModelID)] = true @@ if !unfiltered && !allowedModels.IsUnrestricted() { for _, allowedModel := range allowedModels { - if !includedModels[allowedModel] { + if !includedModels[strings.ToLower(allowedModel)] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ ID: fmt.Sprintf("%s/%s/%s", providerKey, inferenceProvider, allowedModel), Name: schemas.Ptr(allowedModel), }) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/huggingface/models.go` around lines 56 - 62, The backfill loop can add duplicates because allowedModel is checked against includedModels with exact casing; normalize comparisons (e.g., use strings.ToLower or strings.EqualFold) so the presence check is case-insensitive. Update the logic around allowedModels and includedModels in the backfill block (the loop that appends to bifrostResponse.Data) to compare allowedModel in a case-insensitive manner—either populate includedModels with lowercased keys when it's built or perform a case-insensitive lookup for each allowedModel before appending to avoid duplicates.core/providers/gemini/models.go (1)
54-60:⚠️ Potential issue | 🟡 MinorCase-insensitive allow checks can produce duplicate backfilled models.
At Line 38, matching is case-insensitive via
allowedModels.Contains(modelName), but Line 56 checksincludedModels[allowedModel]with case-sensitive keys. A casing mismatch can append a duplicate model in backfill.💡 Suggested fix
- includedModels := make(map[string]bool) + includedModels := make(map[string]bool) for _, model := range response.Models { @@ - includedModels[modelName] = true + includedModels[strings.ToLower(modelName)] = true } @@ if !unfiltered && !allowedModels.IsUnrestricted() { for _, allowedModel := range allowedModels { - if !includedModels[allowedModel] { + if !includedModels[strings.ToLower(allowedModel)] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ ID: string(providerKey) + "/" + allowedModel, Name: schemas.Ptr(allowedModel), }) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/models.go` around lines 54 - 60, The backfill loop uses case-sensitive map lookups on includedModels while allowedModels.Contains is case-insensitive, causing duplicates when casing differs; normalize casing before lookup by converting keys consistently (e.g., lowercasing) — ensure the includedModels map is populated with a consistent case or call strings.ToLower on allowedModel before checking includedModels[allowedModel] and before constructing the ID/Name in the append (referencing includedModels, allowedModels.Contains, the allowedModel variable, and the bifrostResponse.Data append using providerKey).core/providers/vertex/vertex.go (1)
329-329:⚠️ Potential issue | 🟠 MajorPass
allowedModelsthrough to the publisher-model converter.This code path is hit exactly when
allowedModelsis unrestricted (["*"]) and there are no deployments. PassingnilmakesToBifrostListModelsResponsesee an empty allowlist and return an empty catalog, so wildcard keys list no publisher models.💡 Proposed fix
- response := aggregatedResponse.ToBifrostListModelsResponse(nil, request.Unfiltered) + response := aggregatedResponse.ToBifrostListModelsResponse(allowedModels, request.Unfiltered)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/vertex.go` at line 329, The call to aggregatedResponse.ToBifrostListModelsResponse currently passes nil for allowedModels which causes the publisher-model conversion to treat unrestricted (["*"]) as an empty allowlist and return no models; change the call to pass the actual allowedModels (the request's allowlist/wildcard set) instead of nil so ToBifrostListModelsResponse receives the real allowlist and returns the correct publisher catalog (update the call site where aggregatedResponse.ToBifrostListModelsResponse(nil, request.Unfiltered) is invoked to supply the allowedModels variable/field).transports/bifrost-http/handlers/providers.go (1)
748-780:⚠️ Potential issue | 🟠 MajorKeep
/api/models?keys=aligned with the newWhiteListsemantics.Once key filtering is requested, selected keys with
Models: []should yield no accessible models, not the full catalog. This helper also builds a case-sensitive map fromkey.Models, so["GPT-4O"]will not matchgpt-4oeven thoughWhiteListmembership is now case-insensitive.💡 Proposed fix
allowedModels := make(map[string]bool) hasRestrictedKey := false hasUnrestrictedKey := false for _, keyID := range keyIDs { for _, key := range config.Keys { if key.ID == keyID { if key.Models.IsUnrestricted() { // Key allows all models (wildcard) hasUnrestrictedKey = true } else if !key.Models.IsEmpty() { // Key has specific model restrictions - add them to allowedModels hasRestrictedKey = true for _, model := range key.Models { - allowedModels[model] = true + allowedModels[strings.ToLower(model)] = true } } // else: empty Models = deny all — key contributes nothing break } } } // If any key is unrestricted, return all models (union of "all" and restricted subsets is "all") if hasUnrestrictedKey { return models } - // If no keys have model restrictions (e.g., unknown key IDs), return all models + // With an explicit keys filter, no positive grants means no accessible models. if !hasRestrictedKey { - return models + return []string{} } // Filter models based on restrictions from restricted keys only filtered := []string{} for _, model := range models { - if allowedModels[model] { + if allowedModels[strings.ToLower(model)] { filtered = append(filtered, model) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/providers.go` around lines 748 - 780, The helper currently treats absence of restricted keys as "allow all" and matches model names case-sensitively; change it so keys with Models: [] deny access (i.e., if no restricted keys return an empty list), and perform case-insensitive matching when building and checking allowedModels. Specifically, keep the hasUnrestrictedKey branch returning models, but replace the "if !hasRestrictedKey { return models }" branch to return an empty slice, populate allowedModels using strings.ToLower(model) when iterating key.Models (while still skipping empty Models), and compare using strings.ToLower(model) when filtering the models slice so WhiteList membership is case-insensitive; references: allowedModels, hasRestrictedKey, hasUnrestrictedKey, key.Models.IsUnrestricted(), key.Models.IsEmpty(), and models.
🧹 Nitpick comments (1)
framework/configstore/tables/mcp.go (1)
71-86: Plan for malformed MCP whitelist rows that already exist.
BeforeSaveonly protects writes after this deploy. Existingconfig_mcp_clientsrows with duplicate entries or mixed*+ specific tools will still load viaAfterFind, so the new invariant is not enforced until those rows are updated. Consider validating after unmarshal or running a one-time backfill during rollout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/mcp.go` around lines 71 - 86, Add enforcement for the new tools whitelist invariant on read and provide a rollout backfill: implement the same validation currently in BeforeSave inside the AfterFind hook (in the MCP model) so when loading rows from config_mcp_clients you validate/normalize ToolsToExecute and ToolsToAutoExecute after json.Unmarshal and error (or fix) when duplicates or mixed "*" + specific tools exist, and also create a one-time migration/backfill that scans existing config_mcp_clients rows, normalizes duplicate entries, removes invalid mixed "*" + specific tool sets, and writes back normalized ToolsToExecuteJSON/ToolsToAutoExecuteJSON so the invariant holds for pre-existing rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/providers/azure/models.go`:
- Around line 11-14: findMatchingAllowedModel currently uses wl.Contains(value)
and returns the input value, which loses the canonical allowlist entry casing;
change the function to return the matched whitelist entry (the allowlist item)
when an exact case-insensitive match is found (e.g., by using the whitelist
lookup that returns the stored entry or by iterating wl to find the matching
item) instead of returning value, and apply the same fix where the helper is
used around the allowlist handling in the Azure model backfill logic (the code
path referenced at lines handling the append backfill rows) so the canonical
allowlist string is used downstream.
In `@core/providers/bedrock/models.go`:
- Around line 124-136: findMatchingAllowedModel currently returns the input
string (value/withoutPrefix) when wl.Contains(...) is true which loses the
configured allowlist spelling; change the branches in findMatchingAllowedModel
to return the actual allowlist entry instead of the request value by looking up
the matching whitelist item (e.g., add a helper to find the whitelist entry by
case-insensitive compare or use an existing wl.EntryFor(value) if available, or
iterate wl.Items and use strings.EqualFold to pick the configured spelling) and
apply the same fix where models are added to includedModels (the other
occurrences called out around lines 333-340) so the code keys models using the
configured allowlist casing rather than the Bedrock response casing.
In `@core/providers/elevenlabs/models.go`:
- Around line 22-23: Normalize provider prefixes before matching: ensure
comparisons between allowedModels and model.ModelID strip a single leading
"elevenlabs/" so entries like "elevenlabs/voice_v2" match and backfill doesn't
double-prefix. Update the check around allowedModels.Contains(model.ModelID)
(and the same block handling unfiltered and allowedModels.IsUnrestricted()) to
normalize both the whitelist entry set and model.ModelID (or normalize the
modelID into a local variable) by removing one leading "elevenlabs/" if present,
then call allowedModels.Contains with the normalized id.
In `@core/providers/openrouter/openrouter.go`:
- Around line 191-199: The inclusion/backfill logic is mixing case-insensitive
model checks (allowedModels.Contains) with case-sensitive map keys and
TrimPrefix, causing duplicate backfilled IDs; update the loop that builds
includedModels (map[string]bool) to normalize keys (e.g., strings.ToLower)
before inserting and before checking/adding, and perform a case-insensitive
provider-prefix strip (compare lowercased rawID to lowercased providerPrefix and
slice off the prefix length) instead of using strings.TrimPrefix directly; apply
the same normalization fixes to the other analogous block referenced around the
206-214 logic so all comparisons and stored model IDs use the same lowercased
form (refer to allowedModels.Contains, includedModels, providerPrefix, and the
openrouterResponse.Data handling).
In `@core/providers/replicate/models.go`:
- Around line 33-34: When checking allowlist membership for Replicate
deployments, normalize provider-prefixed entries before comparing: update the
logic around allowedModels.Contains(deploymentID) (and the similar block
handling lines 66-72) to also consider the provider-prefixed form (e.g.,
"replicate/"+deploymentID) or normalize entries in allowedModels by stripping a
leading "replicate/" prefix before matching; ensure you apply this normalization
consistently when testing both unfiltered/allowedModels.IsUnrestricted()
branches so values like "replicate/acme/foo" correctly match the live deployment
ID instead of being double-prefixed.
In `@core/providers/vertex/models.go`:
- Around line 166-168: The restrict-only branch uses
allowedModels.Contains(customModelID) which only records a boolean and loses the
allowlist's canonical spelling; change the check to perform a case-insensitive
match that returns the matched allowlist entry (or iterate allowedModels entries
and compare strings case-insensitively) and, when matched, set modelID (or the
variable used later) to that matched canonical spelling instead of keeping
customModelID; do the same fix for the other restricted-only codepath referenced
around lines 225-241 so the stored model entry preserves the allowlist spelling
rather than relying on later backfilling.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 1159-1163: The duplicate-error detection in the update handler is
brittle because it checks for the exact substring "already exists'" (with a
trailing quote); update the condition in the error-mapping block that uses
badReqErr and strings.Contains to check for robust variants by replacing
strings.Contains(err.Error(), "already exists'") with a case-insensitive check
such as strings.Contains(strings.ToLower(err.Error()), "already exists") (and
keep the existing "duplicate key" check), or alternatively normalize the error
text and match both "already exists" and "duplicate key", so SendError(ctx, 400,
...) triggers correctly when duplicates occur (refer to the same error-mapping
block and SendError usage).
---
Outside diff comments:
In `@core/mcp/agent.go`:
- Around line 437-476: The current logic rebuilds code-mode auto-exec allowlists
from raw ExecutionConfig.ToolsToAutoExecute and can re-enable tools the request
filtered out; instead, derive the allowlist from availableToolsPerClient (from
ClientManager.GetToolPerClient) and intersect that with
client.ExecutionConfig.ToolsToAutoExecute: for each clientName in
availableToolsPerClient, keep the client in allClientNames, take the visible
tool names from availableToolsPerClient[clientName], map/normalize those names
the same way you do now (replace "-" with "_" and call parseToolName), then if
ToolsToAutoExecute.IsUnrestricted() allow only the visible set (or mark "*" only
if all visible tools are allowed by config), otherwise compute the intersection
between the normalized visible tool names and ToolsToAutoExecute and set
allowedTools[clientName] to that intersection; this preserves request-level
filtering and prevents isToolCallAllowedForCodeMode/client.tool()
misclassification.
In `@core/providers/anthropic/models.go`:
- Around line 21-29: The code sets bifrostResponse.NextPageToken from
response.HasMore/response.LastID before checking the whitelist, causing a
continuation token to be returned for a permanently-empty (deny-all) filtered
result; update the logic so NextPageToken is only set when pagination makes
sense—either move the pagination block (the response.HasMore && response.LastID
check that sets bifrostResponse.NextPageToken) to after the unfiltered &&
allowedModels.IsEmpty() check, or add a guard so you set
bifrostResponse.NextPageToken only if response.HasMore && response.LastID != nil
&& !( !unfiltered && allowedModels.IsEmpty() ), keeping checks for unfiltered
and allowedModels.IsEmpty() and leaving the rest of the function unchanged.
In `@core/providers/gemini/models.go`:
- Around line 54-60: The backfill loop uses case-sensitive map lookups on
includedModels while allowedModels.Contains is case-insensitive, causing
duplicates when casing differs; normalize casing before lookup by converting
keys consistently (e.g., lowercasing) — ensure the includedModels map is
populated with a consistent case or call strings.ToLower on allowedModel before
checking includedModels[allowedModel] and before constructing the ID/Name in the
append (referencing includedModels, allowedModels.Contains, the allowedModel
variable, and the bifrostResponse.Data append using providerKey).
In `@core/providers/huggingface/models.go`:
- Around line 56-62: The backfill loop can add duplicates because allowedModel
is checked against includedModels with exact casing; normalize comparisons
(e.g., use strings.ToLower or strings.EqualFold) so the presence check is
case-insensitive. Update the logic around allowedModels and includedModels in
the backfill block (the loop that appends to bifrostResponse.Data) to compare
allowedModel in a case-insensitive manner—either populate includedModels with
lowercased keys when it's built or perform a case-insensitive lookup for each
allowedModel before appending to avoid duplicates.
In `@core/providers/openai/models.go`:
- Around line 21-42: The dedupe map includedModels is keyed by raw model.ID
which breaks the new case-insensitive whitelist logic and causes duplicate
entries when casing differs; update the logic in the response.Data loop and the
backfill loop to use the same normalized key (e.g., strings.ToLower or the same
normalization used by allowedModels) when setting and checking includedModels so
that model IDs are compared case-insensitively (apply normalization to model.ID
when adding to includedModels and to allowedModel when checking if
includedModels contains it) while still writing the original provider ID or the
allowedModel string into bifrostResponse.Data as before.
In `@core/providers/vertex/utils.go`:
- Around line 163-205: The duplicate-entry bug is caused by case differences
between deployment aliases and allowlist entries: addedModelIDs keys use
original casing so a backfilled allowedModel with different case produces a
second Model without Deployment. Fix by normalizing the model ID when
checking/setting the map (e.g., compute modelID as before but use a
normalizedKey := strings.ToLower(modelID) for lookups and assignments in
addedModelIDs) in both the deployments loop and the allowedModels loop so
case-insensitive matches dedupe correctly; ensure you still construct the
Model.ID and Name as existing code does and add an import for strings if needed.
In `@core/providers/vertex/vertex.go`:
- Line 329: The call to aggregatedResponse.ToBifrostListModelsResponse currently
passes nil for allowedModels which causes the publisher-model conversion to
treat unrestricted (["*"]) as an empty allowlist and return no models; change
the call to pass the actual allowedModels (the request's allowlist/wildcard set)
instead of nil so ToBifrostListModelsResponse receives the real allowlist and
returns the correct publisher catalog (update the call site where
aggregatedResponse.ToBifrostListModelsResponse(nil, request.Unfiltered) is
invoked to supply the allowedModels variable/field).
In `@framework/modelcatalog/main.go`:
- Around line 533-565: The loop in IsModelAllowedForProvider currently uses raw
string equality and slices.Contains, which bypasses the WhiteList's
case-insensitive semantics; update it to use WhiteList.Contains for comparisons:
replace the direct check "if allowedModel == model" with a call to
allowedModels.Contains(model) (or otherwise use
allowedModels.Contains(allowedModel) appropriately), and for provider-prefixed
entries keep the provider catalog existence check but after ParseModelString
compare the parsed model part using allowedModels.Contains(modelPart) (instead
of slices.Contains or ==); this ensures case-insensitive whitelist matching
while preserving the provider-catalog validation in GetModelsForProvider and
ParseModelString logic.
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 748-780: The helper currently treats absence of restricted keys as
"allow all" and matches model names case-sensitively; change it so keys with
Models: [] deny access (i.e., if no restricted keys return an empty list), and
perform case-insensitive matching when building and checking allowedModels.
Specifically, keep the hasUnrestrictedKey branch returning models, but replace
the "if !hasRestrictedKey { return models }" branch to return an empty slice,
populate allowedModels using strings.ToLower(model) when iterating key.Models
(while still skipping empty Models), and compare using strings.ToLower(model)
when filtering the models slice so WhiteList membership is case-insensitive;
references: allowedModels, hasRestrictedKey, hasUnrestrictedKey,
key.Models.IsUnrestricted(), key.Models.IsEmpty(), and models.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 146-160: The security denylist in ctx.go (the securityDenylist
map) is missing the "authorization" header key; add "authorization": true to the
securityDenylist map to ensure Authorization headers cannot be tunneled via
x-bf-eh-* or other paths and update any related tests/uses of securityDenylist
to account for the new denied header.
---
Nitpick comments:
In `@framework/configstore/tables/mcp.go`:
- Around line 71-86: Add enforcement for the new tools whitelist invariant on
read and provide a rollout backfill: implement the same validation currently in
BeforeSave inside the AfterFind hook (in the MCP model) so when loading rows
from config_mcp_clients you validate/normalize ToolsToExecute and
ToolsToAutoExecute after json.Unmarshal and error (or fix) when duplicates or
mixed "*" + specific tools exist, and also create a one-time migration/backfill
that scans existing config_mcp_clients rows, normalizes duplicate entries,
removes invalid mixed "*" + specific tool sets, and writes back normalized
ToolsToExecuteJSON/ToolsToAutoExecuteJSON so the invariant holds for
pre-existing rows.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b8200b7-d37a-41a5-9a1f-05132a7cbd2b
📒 Files selected for processing (31)
core/bifrost.gocore/mcp/agent.gocore/mcp/utils.gocore/providers/anthropic/models.gocore/providers/azure/models.gocore/providers/bedrock/models.gocore/providers/cohere/models.gocore/providers/elevenlabs/models.gocore/providers/gemini/models.gocore/providers/huggingface/models.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.gocore/schemas/mcp.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey.goframework/modelcatalog/main.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/handlers/utils.gotransports/bifrost-http/integrations/openai.gotransports/bifrost-http/lib/ctx.go
🚧 Files skipped from review as they are similar to previous changes (4)
- transports/bifrost-http/integrations/openai.go
- plugins/governance/resolver.go
- core/schemas/account.go
- transports/bifrost-http/handlers/mcpserver.go
Pratham-Mishra04
force-pushed
the
03-16-feat_standardize_empty_array_conventions_in_bifrost
branch
from
413b2bb to
24b4bdb
Compare
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
791ecbf to
bede6b1
Compare
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
bede6b1 to
06b9ba6
Compare
Collaborator
Author
|
@coderabbiteai full-review |
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
transports/bifrost-http/lib/ctx.go (1)
146-160:⚠️ Potential issue | 🔴 CriticalBlock forwarded
authorizationheaders too.
x-bf-eh-authorizationand direct-allowlistedauthorizationstill bypass this denylist, so callers can inject an upstream auth header even after the new key-header guards are in place.🔒 Suggested fix
securityDenylist := map[string]bool{ + "authorization": true, "proxy-authorization": true, "cookie": true, "host": true, "content-length": true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/lib/ctx.go` around lines 146 - 160, The security denylist in ctx.go (securityDenylist) fails to block forwarded authorization headers—add "authorization" and "x-bf-eh-authorization" (lowercased) to the map and ensure any allowlist/forwarding logic that inspects headers (e.g., where securityDenylist is consulted) treats header keys normalized to lowercase so these keys cannot be bypassed by x-bf-eh-authorization or a direct "Authorization" header; update securityDenylist and the header-checking code paths that reference it to enforce the deny rule.core/providers/vertex/utils.go (1)
163-205:⚠️ Potential issue | 🟡 MinorNormalize
addedModelIDsbefore the backfill pass.
allowedModels.Contains(alias)is case-insensitive, but the dedupe map is not. If the deployment alias and allowlist entry differ only by case, the same logical model is appended once fromdeploymentsand again fromallowedModels.🔧 Suggested fix
addedModelIDs := make(map[string]bool) @@ - modelID := string(schemas.Vertex) + "/" + alias - if addedModelIDs[modelID] { + modelID := string(schemas.Vertex) + "/" + alias + modelKey := strings.ToLower(modelID) + if addedModelIDs[modelKey] { continue } @@ - addedModelIDs[modelID] = true + addedModelIDs[modelKey] = true } @@ - modelID := string(schemas.Vertex) + "/" + allowedModel - if addedModelIDs[modelID] { + modelID := string(schemas.Vertex) + "/" + allowedModel + modelKey := strings.ToLower(modelID) + if addedModelIDs[modelKey] { continue } @@ - addedModelIDs[modelID] = true + addedModelIDs[modelKey] = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/utils.go` around lines 163 - 205, The dedupe map addedModelIDs is using case-sensitive keys while allowedModels.Contains is case-insensitive, causing duplicates when alias casing differs; normalize the keys stored and checked in addedModelIDs (e.g., use strings.ToLower on modelID) both where you set addedModelIDs[modelID] = true in the deployments loop and where you check addedModelIDs[modelID] in the allowedModels backfill loop so the same logical model (constructed in modelID in both loops) is treated identically regardless of case.framework/modelcatalog/main.go (1)
546-563:⚠️ Potential issue | 🟠 MajorKeep whitelist checks case-insensitive here.
WhiteList.Contains()is case-insensitive, but this path still falls back to raw==andslices.Contains()checks. An entry likeOpenAI/GPT-4ocan survive validation and other list-model paths in the stack, then get rejected here.🔧 Suggested fix
// Case 2: Explicit allowedModels = check if model matches any entry // Get provider's catalog models for validation of prefixed entries providerCatalogModels := mc.GetModelsForProvider(provider) + if allowedModels.Contains(model) { + return true + } + providerCatalogSet := make(map[string]struct{}, len(providerCatalogModels)) + for _, providerCatalogModel := range providerCatalogModels { + providerCatalogSet[strings.ToLower(providerCatalogModel)] = struct{}{} + } for _, allowedModel := range allowedModels { - // Direct match: "gpt-4o" == "gpt-4o" - if allowedModel == model { - return true - } - // Provider-prefixed match: verify it exists in provider's catalog first // This ensures we only allow provider-specific model combinations that are actually supported if strings.Contains(allowedModel, "/") { // Check if this exact prefixed model exists in the provider's catalog // e.g., for openrouter, check if "openai/gpt-4o" is in its catalog - if slices.Contains(providerCatalogModels, allowedModel) { + if _, ok := providerCatalogSet[strings.ToLower(allowedModel)]; ok { // Extract the model part and compare with request _, modelPart := schemas.ParseModelString(allowedModel, "") - if modelPart == model { + if strings.EqualFold(modelPart, model) { return true } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 546 - 563, The whitelist checks in the loop (variables providerCatalogModels, allowedModel, model and call sites using slices.Contains and raw ==) are case-sensitive and must be made case-insensitive to match WhiteList.Contains() behavior; update the logic to compare using a normalized form (e.g., strings.ToLower) for the direct match (compare lowercased allowedModel == lowercased model), for provider-prefixed checks run slices.Contains against a lowercased copy of providerCatalogModels (or check by lowercasing each element), and when extracting modelPart from schemas.ParseModelString compare strings.ToLower(modelPart) == strings.ToLower(model) so provider-prefixed matches are detected regardless of case.core/providers/mistral/models.go (1)
24-46:⚠️ Potential issue | 🟠 MajorAccept
mistral/-scoped allowlist entries here too.Line 24 only matches raw model IDs. A scoped entry like
mistral/codestral-latestwill be treated as missing, and Line 43 then backfillsmistral/mistral/codestral-latest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/mistral/models.go` around lines 24 - 46, The allowlist backfill is duplicating the "mistral/" scope because allowedModels entries may already be scoped; update the backfill loop that iterates allowedModels so it normalizes each allowedModel by removing a "mistral/" prefix (e.g. via strings.TrimPrefix) before checking against includedModels and before constructing the backfilled schemas.Model ID/Name; continue to use strings.ToLower for keys in includedModels and ensure when building the ID you prefix exactly one "mistral/" so you don't produce "mistral/mistral/..." and you still handle raw unscoped entries.core/providers/huggingface/models.go (1)
40-62:⚠️ Potential issue | 🟠 MajorStrip the
provider/inferenceProviderprefix before matching.Line 40 only compares
allowedModelsagainst raw HF IDs. If the stack passeshuggingface/fireworks-ai/meta-llama/..., the live row is skipped and Lines 58-62 backfillhuggingface/fireworks-ai/huggingface/fireworks-ai/....🛠️ Suggested normalization
bifrostResponse := &schemas.BifrostListModelsResponse{ Data: make([]schemas.Model, 0, len(response.Models)), } + modelPrefix := fmt.Sprintf("%s/%s/", providerKey, inferenceProvider) + modelPrefixLower := strings.ToLower(modelPrefix) + normalizeAllowedModel := func(model string) string { + if strings.HasPrefix(strings.ToLower(model), modelPrefixLower) { + return model[len(modelPrefix):] + } + return model + } @@ - if !unfiltered && allowedModels.IsRestricted() && !allowedModels.Contains(model.ModelID) { + if !unfiltered && allowedModels.IsRestricted() && + !(allowedModels.Contains(model.ModelID) || allowedModels.Contains(modelPrefix+model.ModelID)) { continue } @@ - if !includedModels[strings.ToLower(allowedModel)] { + normalizedAllowedModel := normalizeAllowedModel(allowedModel) + if !includedModels[strings.ToLower(normalizedAllowedModel)] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ - ID: fmt.Sprintf("%s/%s/%s", providerKey, inferenceProvider, allowedModel), - Name: schemas.Ptr(allowedModel), + ID: modelPrefix + normalizedAllowedModel, + Name: schemas.Ptr(normalizedAllowedModel), }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/huggingface/models.go` around lines 40 - 62, The code compares allowedModels to raw HF IDs without removing the prefix (providerKey/inferenceProvider), causing mismatches; update the matching to normalize both sides by stripping the provider/inferenceProvider prefix before comparing. Specifically, when adding to includedModels use a normalized key (e.g., strings.ToLower(strings.TrimPrefix(model.ModelID, fmt.Sprintf("%s/%s/", providerKey, inferenceProvider)))) and when iterating allowedModels for the backfill check normalize allowedModel the same way (use strings.TrimPrefix + strings.ToLower) before testing includedModels or appending to bifrostResponse.Data so the presence check and backfill use the same canonical model identifier.core/providers/openai/models.go (1)
25-43:⚠️ Potential issue | 🟠 MajorAccept
openai/-scoped allowlist entries here.Line 25 still only checks raw IDs like
gpt-4o, and Lines 40-43 backfill with the unnormalized allowlist value. If the stack suppliesopenai/gpt-4o, the live row is filtered out and the fallback row becomesopenai/openai/gpt-4o.🛠️ Suggested normalization
bifrostResponse := &schemas.BifrostListModelsResponse{ Data: make([]schemas.Model, 0, len(response.Data)), } + providerPrefix := string(providerKey) + "/" + providerPrefixLower := strings.ToLower(providerPrefix) + normalizeAllowedModel := func(model string) string { + if strings.HasPrefix(strings.ToLower(model), providerPrefixLower) { + return model[len(providerPrefix):] + } + return model + } @@ - if !unfiltered && allowedModels.IsRestricted() && !allowedModels.Contains(model.ID) { + if !unfiltered && allowedModels.IsRestricted() && + !(allowedModels.Contains(model.ID) || allowedModels.Contains(providerPrefix+model.ID)) { continue } @@ - if !includedModels[strings.ToLower(allowedModel)] { + normalizedAllowedModel := normalizeAllowedModel(allowedModel) + if !includedModels[strings.ToLower(normalizedAllowedModel)] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ - ID: string(providerKey) + "/" + allowedModel, - Name: schemas.Ptr(allowedModel), + ID: providerPrefix + normalizedAllowedModel, + Name: schemas.Ptr(normalizedAllowedModel), }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/models.go` around lines 25 - 43, The code currently compares model.ID to allowedModels entries and backfills using allowedModel without normalizing vendor prefixes, causing mismatches and duplicate prefixes (e.g., openai/openai/gpt-4o); update the logic around allowedModels (used in the loop that checks allowedModels.IsRestricted(), the filter that uses model.ID, and the backfill that appends allowedModel) to normalize entries by stripping any leading "openai/" prefix before containment checks and before composing the appended Model.ID (combine providerKey with the normalized model name) and when populating includedModels use strings.ToLower(normalizedName) so comparisons and backfilled IDs match the live model entries.
♻️ Duplicate comments (4)
core/providers/replicate/models.go (1)
33-34:⚠️ Potential issue | 🟠 MajorNormalize
replicate/-prefixed allowlist entries before filtering.Line 33 only matches raw deployment IDs like
owner/name, and Lines 66-72 backfill with the unnormalized allowlist entry. If the stack passesreplicate/owner/name, the live deployment is skipped and the fallback row becomesreplicate/replicate/owner/name.🛠️ Suggested normalization
bifrostResponse := &schemas.BifrostListModelsResponse{ Data: make([]schemas.Model, 0), } + providerPrefix := string(providerKey) + "/" + providerPrefixLower := strings.ToLower(providerPrefix) + normalizeAllowedModel := func(model string) string { + if strings.HasPrefix(strings.ToLower(model), providerPrefixLower) { + return model[len(providerPrefix):] + } + return model + } @@ - if !unfiltered && allowedModels.IsRestricted() && !allowedModels.Contains(deploymentID) { + if !unfiltered && allowedModels.IsRestricted() && + !(allowedModels.Contains(deploymentID) || allowedModels.Contains(providerPrefix+deploymentID)) { continue } @@ - if !includedModels[strings.ToLower(allowedModel)] { + deploymentID := normalizeAllowedModel(allowedModel) + if !includedModels[strings.ToLower(deploymentID)] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ - ID: string(providerKey) + "/" + allowedModel, - Name: schemas.Ptr(allowedModel), + ID: providerPrefix + deploymentID, + Name: schemas.Ptr(deploymentID), }) }Also applies to: 66-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/replicate/models.go` around lines 33 - 34, The allowlist check misses entries prefixed with "replicate/", so update the filtering logic around allowedModels.Contains(deploymentID) to normalize allowedModels entries and the deploymentID by stripping a leading "replicate/" before comparison (and do the same normalization for the backfill logic that currently produces the duplicate "replicate/replicate/..." rows in the block around where rows are added/filtered (the code near the current lines referencing allowedModels and the fallback/backfill handling)); ensure normalization is applied consistently when building allowedModels and when comparing in both the initial skip (the if that checks !unfiltered && allowedModels.IsRestricted() && !allowedModels.Contains(deploymentID)) and the backfill code that adds the fallback row so entries like "replicate/owner/name" match "owner/name" and vice versa.core/providers/azure/models.go (1)
13-24:⚠️ Potential issue | 🟠 MajorNormalize
azure/-scoped allowlist entries before matching.This helper and the later backfill logic still treat
allowedModelsas raw model or alias strings. A configuredazure/<model>orazure/<alias>either misses the live row or gets emitted asazure/azure/...later, and Line 16 also drops the canonical whitelist entry when casing differs.Also applies to: 87-98, 147-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/azure/models.go` around lines 13 - 24, The findMatchingAllowedModel helper (and the backfill logic referenced at lines 87-98 and 147-166) fails to normalize azure-scoped entries and uses case-sensitive checks, causing misses and duplicated "azure/azure/..." names; fix by normalizing comparisons: when checking wl.Contains(value) and when iterating the whitelist and calling schemas.SameBaseModel(item, value), first strip a leading "azure/" prefix (if present) and compare in a case-insensitive form, but keep and return the original whitelist item (not the stripped string) so the canonical allowedModels name is preserved; apply the same normalize-before-compare approach to the backfill code paths mentioned so they won’t re-emit double-prefixed names.core/providers/vertex/models.go (1)
166-168:⚠️ Potential issue | 🟡 MinorPreserve canonical allowlist entry in restricted-only flow to avoid case-variant backfill duplicates.
Line 168 only records a boolean
Containshit, so the first pass can keep API casing while Line 225-241 backfills allowlist casing as a separate model ID.💡 Minimal fix sketch
-shouldFilter = !allowedModels.Contains(customModelID) +matchedAllowedModel := findMatchingAllowedModel(allowedModels, customModelID) +shouldFilter = matchedAllowedModel == "" - addedModelIDs := make(map[string]bool) + addedModelIDs := make(map[string]bool) - addedModelIDs[modelEntry.ID] = true + addedModelIDs[strings.ToLower(modelEntry.ID)] = true - if addedModelIDs[modelID] { + if addedModelIDs[strings.ToLower(modelID)] { continue }Also applies to: 225-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/models.go` around lines 166 - 168, When handling the "restrictAllowed" branch you currently only record a boolean from allowedModels.Contains(customModelID), which lets the original API-cased customModelID be kept and later causes the backfill code (the logic around the model ID backfill currently in the 225–241 region) to insert a canonical allowlist entry as a separate ID; instead, when allowedModels.Contains(customModelID) is true, capture and use the canonical allowlist entry (lookup the canonical ID from the allowlist) rather than just the boolean so the stored model ID uses the canonical casing; update the code paths that add model IDs (the branch using allowedModels.Contains and the backfill logic in the 225–241 block) to insert the canonical allowlist ID (not the API-provided customModelID) to avoid case-variant duplicates.core/providers/bedrock/models.go (1)
124-137:⚠️ Potential issue | 🟡 Minor
findMatchingAllowedModelstill returns request spelling onContainshits.Line 126 and Line 135 return
value/withoutPrefixinstead of the matched whitelist entry, which conflicts with the function contract and can lose configured canonical spelling.💡 Suggested fix
func findMatchingAllowedModel(wl schemas.WhiteList, value string) string { - if wl.Contains(value) { - return value - } + for _, item := range wl { + if strings.EqualFold(item, value) { + return item + } + } valuePrefix := extractPrefix(value) if valuePrefix != "" { withoutPrefix := removePrefix(value) - if wl.Contains(withoutPrefix) { - return withoutPrefix - } + for _, item := range wl { + if strings.EqualFold(item, withoutPrefix) { + return item + } + } }
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/mcp.go (1)
306-307: Optional: extract the auto-clear invariant into a helper.Both create and update paths repeat the same
ToolsToExecute.IsEmpty() => ToolsToAutoExecute = schemas.WhiteList{}rule; a shared helper would reduce drift risk.Also applies to: 508-509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcp.go` around lines 306 - 307, Extract the repeated invariant into a small helper (e.g., ensureAutoClearTools(req *schemas.MCPRequest) or normalizeToolsAutoExecute(req *schemas.MCPRequest)) that checks req.ToolsToExecute.IsEmpty() and, when true, sets req.ToolsToAutoExecute = schemas.WhiteList{}; call that helper from both the create and update code paths where the current lines repeat the logic (the branches that reference ToolsToExecute.IsEmpty() and ToolsToAutoExecute) to keep behavior identical and avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/schemas/account.go`:
- Around line 61-74: The Validate method for WhiteList currently allows empty or
whitespace-padded entries which later break Contains()/IsAllowed(); update
WhiteList.Validate() to trim each entry (use strings.TrimSpace) and reject any
entry that is empty after trimming by returning an error, use the
trimmed-and-lowercased value for duplicate detection (seen map key) and for
error messages, and ensure the wildcard check (wl.Contains("*")) is performed
against trimmed values so entries like " * " are treated correctly.
---
Outside diff comments:
In `@core/providers/huggingface/models.go`:
- Around line 40-62: The code compares allowedModels to raw HF IDs without
removing the prefix (providerKey/inferenceProvider), causing mismatches; update
the matching to normalize both sides by stripping the provider/inferenceProvider
prefix before comparing. Specifically, when adding to includedModels use a
normalized key (e.g., strings.ToLower(strings.TrimPrefix(model.ModelID,
fmt.Sprintf("%s/%s/", providerKey, inferenceProvider)))) and when iterating
allowedModels for the backfill check normalize allowedModel the same way (use
strings.TrimPrefix + strings.ToLower) before testing includedModels or appending
to bifrostResponse.Data so the presence check and backfill use the same
canonical model identifier.
In `@core/providers/mistral/models.go`:
- Around line 24-46: The allowlist backfill is duplicating the "mistral/" scope
because allowedModels entries may already be scoped; update the backfill loop
that iterates allowedModels so it normalizes each allowedModel by removing a
"mistral/" prefix (e.g. via strings.TrimPrefix) before checking against
includedModels and before constructing the backfilled schemas.Model ID/Name;
continue to use strings.ToLower for keys in includedModels and ensure when
building the ID you prefix exactly one "mistral/" so you don't produce
"mistral/mistral/..." and you still handle raw unscoped entries.
In `@core/providers/openai/models.go`:
- Around line 25-43: The code currently compares model.ID to allowedModels
entries and backfills using allowedModel without normalizing vendor prefixes,
causing mismatches and duplicate prefixes (e.g., openai/openai/gpt-4o); update
the logic around allowedModels (used in the loop that checks
allowedModels.IsRestricted(), the filter that uses model.ID, and the backfill
that appends allowedModel) to normalize entries by stripping any leading
"openai/" prefix before containment checks and before composing the appended
Model.ID (combine providerKey with the normalized model name) and when
populating includedModels use strings.ToLower(normalizedName) so comparisons and
backfilled IDs match the live model entries.
In `@core/providers/vertex/utils.go`:
- Around line 163-205: The dedupe map addedModelIDs is using case-sensitive keys
while allowedModels.Contains is case-insensitive, causing duplicates when alias
casing differs; normalize the keys stored and checked in addedModelIDs (e.g.,
use strings.ToLower on modelID) both where you set addedModelIDs[modelID] = true
in the deployments loop and where you check addedModelIDs[modelID] in the
allowedModels backfill loop so the same logical model (constructed in modelID in
both loops) is treated identically regardless of case.
In `@framework/modelcatalog/main.go`:
- Around line 546-563: The whitelist checks in the loop (variables
providerCatalogModels, allowedModel, model and call sites using slices.Contains
and raw ==) are case-sensitive and must be made case-insensitive to match
WhiteList.Contains() behavior; update the logic to compare using a normalized
form (e.g., strings.ToLower) for the direct match (compare lowercased
allowedModel == lowercased model), for provider-prefixed checks run
slices.Contains against a lowercased copy of providerCatalogModels (or check by
lowercasing each element), and when extracting modelPart from
schemas.ParseModelString compare strings.ToLower(modelPart) ==
strings.ToLower(model) so provider-prefixed matches are detected regardless of
case.
In `@transports/bifrost-http/lib/ctx.go`:
- Around line 146-160: The security denylist in ctx.go (securityDenylist) fails
to block forwarded authorization headers—add "authorization" and
"x-bf-eh-authorization" (lowercased) to the map and ensure any
allowlist/forwarding logic that inspects headers (e.g., where securityDenylist
is consulted) treats header keys normalized to lowercase so these keys cannot be
bypassed by x-bf-eh-authorization or a direct "Authorization" header; update
securityDenylist and the header-checking code paths that reference it to enforce
the deny rule.
---
Duplicate comments:
In `@core/providers/azure/models.go`:
- Around line 13-24: The findMatchingAllowedModel helper (and the backfill logic
referenced at lines 87-98 and 147-166) fails to normalize azure-scoped entries
and uses case-sensitive checks, causing misses and duplicated "azure/azure/..."
names; fix by normalizing comparisons: when checking wl.Contains(value) and when
iterating the whitelist and calling schemas.SameBaseModel(item, value), first
strip a leading "azure/" prefix (if present) and compare in a case-insensitive
form, but keep and return the original whitelist item (not the stripped string)
so the canonical allowedModels name is preserved; apply the same
normalize-before-compare approach to the backfill code paths mentioned so they
won’t re-emit double-prefixed names.
In `@core/providers/replicate/models.go`:
- Around line 33-34: The allowlist check misses entries prefixed with
"replicate/", so update the filtering logic around
allowedModels.Contains(deploymentID) to normalize allowedModels entries and the
deploymentID by stripping a leading "replicate/" before comparison (and do the
same normalization for the backfill logic that currently produces the duplicate
"replicate/replicate/..." rows in the block around where rows are added/filtered
(the code near the current lines referencing allowedModels and the
fallback/backfill handling)); ensure normalization is applied consistently when
building allowedModels and when comparing in both the initial skip (the if that
checks !unfiltered && allowedModels.IsRestricted() &&
!allowedModels.Contains(deploymentID)) and the backfill code that adds the
fallback row so entries like "replicate/owner/name" match "owner/name" and vice
versa.
In `@core/providers/vertex/models.go`:
- Around line 166-168: When handling the "restrictAllowed" branch you currently
only record a boolean from allowedModels.Contains(customModelID), which lets the
original API-cased customModelID be kept and later causes the backfill code (the
logic around the model ID backfill currently in the 225–241 region) to insert a
canonical allowlist entry as a separate ID; instead, when
allowedModels.Contains(customModelID) is true, capture and use the canonical
allowlist entry (lookup the canonical ID from the allowlist) rather than just
the boolean so the stored model ID uses the canonical casing; update the code
paths that add model IDs (the branch using allowedModels.Contains and the
backfill logic in the 225–241 block) to insert the canonical allowlist ID (not
the API-provided customModelID) to avoid case-variant duplicates.
---
Nitpick comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 306-307: Extract the repeated invariant into a small helper (e.g.,
ensureAutoClearTools(req *schemas.MCPRequest) or normalizeToolsAutoExecute(req
*schemas.MCPRequest)) that checks req.ToolsToExecute.IsEmpty() and, when true,
sets req.ToolsToAutoExecute = schemas.WhiteList{}; call that helper from both
the create and update code paths where the current lines repeat the logic (the
branches that reference ToolsToExecute.IsEmpty() and ToolsToAutoExecute) to keep
behavior identical and avoid duplication.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61869a93-5383-4c02-a4a7-6c3a84cc6a9b
📒 Files selected for processing (31)
core/bifrost.gocore/mcp/agent.gocore/mcp/utils.gocore/providers/anthropic/models.gocore/providers/azure/models.gocore/providers/bedrock/models.gocore/providers/cohere/models.gocore/providers/elevenlabs/models.gocore/providers/gemini/models.gocore/providers/huggingface/models.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.gocore/schemas/mcp.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey.goframework/modelcatalog/main.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/handlers/utils.gotransports/bifrost-http/integrations/openai.gotransports/bifrost-http/lib/ctx.go
🚧 Files skipped from review as they are similar to previous changes (10)
- core/bifrost.go
- core/providers/vertex/vertex.go
- plugins/governance/main.go
- transports/bifrost-http/handlers/providers.go
- transports/bifrost-http/integrations/openai.go
- plugins/governance/resolver.go
- core/providers/elevenlabs/models.go
- core/schemas/mcp.go
- transports/bifrost-http/handlers/utils.go
- core/mcp/agent.go
Contributor
|
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
framework/modelcatalog/main.go (1)
533-569:⚠️ Potential issue | 🟠 MajorPreserve case-insensitive whitelist matching in
IsModelAllowedForProvider.Line 550, Line 559, and Line 562 use exact/case-sensitive comparisons, which can block valid allowlisted models after the WhiteList migration.
🔧 Proposed fix
@@ for _, allowedModel := range allowedModels { // Direct match: "gpt-4o" == "gpt-4o" - if allowedModel == model { + if strings.EqualFold(allowedModel, model) { return true } @@ if strings.Contains(allowedModel, "/") { // Check if this exact prefixed model exists in the provider's catalog // e.g., for openrouter, check if "openai/gpt-4o" is in its catalog - if slices.Contains(providerCatalogModels, allowedModel) { + existsInCatalog := false + for _, providerModel := range providerCatalogModels { + if strings.EqualFold(providerModel, allowedModel) { + existsInCatalog = true + break + } + } + if existsInCatalog { // Extract the model part and compare with request _, modelPart := schemas.ParseModelString(allowedModel, "") - if modelPart == model { + if strings.EqualFold(modelPart, model) { return true } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 533 - 569, The IsModelAllowedForProvider logic currently uses case-sensitive comparisons which breaks whitelist matching after the WhiteList migration; update all comparisons in IsModelAllowedForProvider (including the unrestricted branch that checks slices.Contains(supportedProviders, provider), the direct match check for allowedModel == model, the provider catalog membership check for slices.Contains(providerCatalogModels, allowedModel), and the final modelPart == model comparison) to be case-insensitive — e.g., use strings.EqualFold or normalize both sides to lower-case before comparing and implement a case-insensitive contains helper when checking slices (for provider lists and providerCatalogModels) so existing behavior is preserved but comparisons ignore case; reference functions/values: IsModelAllowedForProvider, GetProvidersForModel, GetModelsForProvider, schemas.ParseModelString, WhiteList.IsUnrestricted, and WhiteList.IsEmpty.transports/bifrost-http/handlers/providers.go (1)
755-763:⚠️ Potential issue | 🟠 MajorCase-insensitive whitelist behavior is still broken in
filterModelsByKeys.Line 761 stores raw whitelist values and Line 787 compares raw model names, so this path remains case-sensitive despite
WhiteListmoving to case-insensitive semantics.🔧 Proposed fix
@@ - allowedModels := make(map[string]bool) + allowedModels := make(map[string]bool) @@ - for _, model := range key.Models { - allowedModels[model] = true + for _, model := range key.Models { + allowedModels[strings.ToLower(model)] = true } @@ - for _, model := range models { - if allowedModels[model] { + for _, model := range models { + if allowedModels[strings.ToLower(model)] { filtered = append(filtered, model) } }Also applies to: 785-788
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/providers.go` around lines 755 - 763, filterModelsByKeys still treats whitelist entries case-sensitively: when iterating key.Models in the block that sets hasRestrictedKey and populates allowedModels, store normalized keys (e.g., strings.ToLower(model)) and when later comparing models (the checks around allowedModels access) compare against the same normalized form (e.g., strings.ToLower(modelName)). Update both the population of allowedModels and the model membership checks in filterModelsByKeys to use a consistent lowercase normalization so the WhiteList semantics become case-insensitive; keep hasUnrestrictedKey/hasRestrictedKey logic unchanged.core/providers/vertex/utils.go (1)
158-190:⚠️ Potential issue | 🟠 MajorEmpty whitelist still exposes every configured Vertex deployment.
IsRestricted()is false for both["*"]and[]. In the empty case, Lines 168-186 still append all deployments before Line 189 returns, so this helper remains allow-all instead of deny-by-default. Add an explicitIsEmpty()guard before populatingresponse.🛠️ Suggested fix
func buildResponseFromConfig(deployments map[string]string, allowedModels schemas.WhiteList) *schemas.BifrostListModelsResponse { response := &schemas.BifrostListModelsResponse{ Data: make([]schemas.Model, 0), } + + if allowedModels.IsEmpty() { + return response + } addedModelIDs := make(map[string]bool) restrictAllowed := allowedModels.IsRestricted()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/utils.go` around lines 158 - 190, The function buildResponseFromConfig currently treats an empty allowedModels as permissive because IsRestricted() is false for both ["*"] and [], so add an explicit empty-whitelist guard: call allowedModels.IsEmpty() near the start of buildResponseFromConfig (before the loop that iterates deployments and before using addedModelIDs/response) and if it returns true, return the empty response immediately; this ensures an empty whitelist denies access instead of exposing all deployments while keeping the existing logic for restricted and wildcard cases intact.core/providers/openai/models.go (1)
23-43:⚠️ Potential issue | 🟠 MajorNormalize
openai/-prefixed IDs in the whitelist path.This converter emits
openai/<id>on Line 29, but filtering/backfill still only speaks raw<id>. If a caller round-trips one of those IDs intoallowedModels, the live model is skipped and then backfilled asopenai/openai/<id>.Suggested normalization
+ providerPrefix := string(providerKey) + "/" + providerPrefixLower := strings.ToLower(providerPrefix) + normalizeAllowedModel := func(model string) string { + if strings.HasPrefix(strings.ToLower(model), providerPrefixLower) { + return model[len(providerPrefix):] + } + return model + } + includedModels := make(map[string]bool) for _, model := range response.Data { - if !unfiltered && allowedModels.IsRestricted() && !allowedModels.Contains(model.ID) { + if !unfiltered && allowedModels.IsRestricted() && + !(allowedModels.Contains(model.ID) || allowedModels.Contains(providerPrefix+model.ID)) { continue } bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ ID: string(providerKey) + "/" + model.ID, @@ // Backfill allowed models that were not in the response if !unfiltered && allowedModels.IsRestricted() { for _, allowedModel := range allowedModels { - if !includedModels[strings.ToLower(allowedModel)] { + normalized := normalizeAllowedModel(allowedModel) + if !includedModels[strings.ToLower(normalized)] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ - ID: string(providerKey) + "/" + allowedModel, - Name: schemas.Ptr(allowedModel), + ID: providerPrefix + normalized, + Name: schemas.Ptr(normalized), }) + includedModels[strings.ToLower(normalized)] = true } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/openai/models.go` around lines 23 - 43, The whitelist logic needs normalization of provider-prefixed IDs: when checking allowedModels in the loop (the allowedModels.Contains(...) call) check both the raw model.ID and the provider-prefixed form (string(providerKey)+"/"+model.ID), and when marking includedModels add both strings.ToLower(model.ID) and strings.ToLower(string(providerKey)+"/"+model.ID); likewise, when backfilling from allowedModels, strip a leading provider prefix (e.g. "openai/") before constructing the backfilled schemas.Model ID so you don't produce double-prefixed entries—adjust the code around includedModels, allowedModels.Contains, and the Model ID construction to implement this normalization.core/providers/cohere/models.go (1)
61-79:⚠️ Potential issue | 🟠 MajorNormalize
cohere/-prefixed IDs before matching.This function emits
cohere/<model>IDs on Line 65, but the whitelist path still only compares raw model names. Round-tripping one of those IDs intoallowedModelswill skip the live model and then backfillcohere/cohere/<model>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/cohere/models.go` around lines 61 - 79, The whitelist backfill loop is comparing raw allowedModel strings against includedModels which stores lowercased raw model names, so IDs like "cohere/<model>" in allowedModels won't match and will be backfilled as "cohere/cohere/<model>". Fix: in the loop that iterates allowedModels (and before checking includedModels[strings.ToLower(allowedModel)]), normalize allowedModel by removing the provider prefix if present (e.g., if strings.HasPrefix(allowedModel, string(providerKey)+"/") then allowedModel = strings.TrimPrefix(allowedModel, string(providerKey)+"/")), then lowercase for the lookup; keep references to allowedModels, includedModels, providerKey, bifrostResponse.Data and model.Name while making this change.core/providers/huggingface/models.go (1)
40-61:⚠️ Potential issue | 🟠 MajorMatch Hugging Face allowlists on the full provider-scoped ID.
This function emits
huggingface/<inferenceProvider>/<modelID>, but the whitelist path only compares baremodel.ModelID. That breaks round-tripping and also makes it impossible to allow the same HF model on one inference provider without implicitly allowing/backfilling it on the others. Based on learnings, "ensure that model catalogs and allowed models are handled per inference provider ... so each provider's model IDs (providerKey/inferenceProvider/modelID) are represented distinctly."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/huggingface/models.go` around lines 40 - 61, The code currently compares bare model.ModelID against allowedModels and stores includedModels keyed by lowercased model.ModelID, but newModel.ID is provider-scoped (fmt.Sprintf("%s/%s/%s", providerKey, inferenceProvider, model.ModelID)), causing mismatches; update the checks and keys to use the full provider-scoped ID: when testing allowlist membership use the fullID (fmt.Sprintf with providerKey and inferenceProvider) instead of model.ModelID, store includedModels[strings.ToLower(fullID)] = true, and when backfilling iterate allowedModels as provider-scoped IDs (or construct fullID for comparison) so the bifrostResponse.Data append and allowedModels membership checks consistently use the same provider-scoped ID; adjust references in the block around includedModels, allowedModels, newModel.ID, and the backfill loop accordingly.core/providers/gemini/models.go (1)
38-59:⚠️ Potential issue | 🟠 MajorNormalize the canonical Gemini ID before filtering and backfilling.
This function emits
gemini/<model>IDs on Line 42, but the whitelist path still only compares the raw trimmed model name. Round-tripping one of those returned IDs intoallowedModelswill skip the live model and then backfillgemini/gemini/<model>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/gemini/models.go` around lines 38 - 59, Normalize and compare using the canonical provider-prefixed model ID when filtering and backfilling: compute canonicalID := string(providerKey) + "/" + modelName (and use strings.ToLower for keys) and use allowedModels.Contains(canonicalID) instead of only modelName, store includedModels[strings.ToLower(canonicalID)] = true, and when iterating allowedModels derive a canonicalAllowed := allowedModel (prefix with providerKey if it doesn’t already have it) then check includedModels[strings.ToLower(canonicalAllowed)] before appending to bifrostResponse.Data and append the canonicalAllowed as the ID so you never backfill a duplicate like gemini/gemini/<model>; update references to modelName checks in the filtering block and the backfill loop accordingly.
♻️ Duplicate comments (2)
core/providers/replicate/models.go (1)
33-34:⚠️ Potential issue | 🟠 MajorStill normalize
replicate/-prefixed IDs before matching and backfilling.This function emits
replicate/<owner>/<name>IDs, but the whitelist path still only compares raw<owner>/<name>. Round-tripping one of those IDs intoallowedModelswill skip the live deployment and then backfillreplicate/replicate/<owner>/<name>.Also applies to: 66-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/replicate/models.go` around lines 33 - 34, Normalize deployment IDs that may begin with "replicate/" before whitelist checks and before scheduling backfills: where the code checks allowedModels.IsRestricted() and calls allowedModels.Contains(deploymentID), and in the later block that emits/schedules backfills, detect and strip a leading "replicate/" prefix from the deploymentID (e.g., via strings.HasPrefix/strings.TrimPrefix) so the Contains check and backfill insertion use the raw "<owner>/<name>" form and avoid double-prefixing ("replicate/replicate/...").core/providers/vertex/models.go (1)
180-194:⚠️ Potential issue | 🟡 MinorNormalize the restricted-backfill dedupe key.
WhiteList.Containsis case-insensitive, butaddedModelIDsis still tracked with original casing. If the allowlist or deployment alias differs only by case from the provider response, the second/third passes can append the configured spelling as a duplicate row.💡 Minimal fix
- addedModelIDs[modelEntry.ID] = true + addedModelIDs[strings.ToLower(modelEntry.ID)] = true } } restrictAllowed := !unfiltered && allowedModels.IsRestricted() // Second pass: Backfill deployments that were not matched from the API response if !unfiltered && len(deployments) > 0 { for alias, deploymentValue := range deployments { // Check if model already exists in the list modelID := string(schemas.Vertex) + "/" + alias - if addedModelIDs[modelID] { + if addedModelIDs[strings.ToLower(modelID)] { continue } @@ bifrostResponse.Data = append(bifrostResponse.Data, modelEntry) - addedModelIDs[modelID] = true + addedModelIDs[strings.ToLower(modelID)] = true } } @@ modelEntry := schemas.Model{ ID: modelID, Name: schemas.Ptr(modelName), } bifrostResponse.Data = append(bifrostResponse.Data, modelEntry) - addedModelIDs[modelID] = true + addedModelIDs[strings.ToLower(modelID)] = true } }Also applies to: 200-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/models.go` around lines 180 - 194, The dedupe map addedModelIDs is using original casing while WhiteList.Contains is case-insensitive, causing duplicates when casing differs; normalize the dedupe key by using a consistent canonical form (e.g., strings.ToLower) for keys when setting and checking addedModelIDs while leaving modelEntry.ID (the displayed ID) unchanged; update all occurrences around modelEntry.ID assignment and checks (including the blocks that set Deployment and the later 200-241 section) so both insertion and membership tests use the same lowercased key.
🧹 Nitpick comments (1)
framework/configstore/tables/mcp.go (1)
70-86: Cover legacy MCP whitelist rows during rollout.
BeforeSavenow rejects new mixed/duplicate entries, but pre-existingtools_to_execute/tools_to_auto_executerows can still be loaded unchanged until they are rewritten. Since the rest of this stack now branches on validatedWhiteListsemantics, make sure the rollout includes either a backfill or read-time validation so old rows cannot bypass the new invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 96-104: UpdateVirtualKeyRequest currently uses non-pointer
WhiteList fields causing omitted fields in partial updates to decode as empty
slices (deny-all) and be written back; change AllowedModels and KeyIDs inside
the ProviderConfigs anonymous struct and ToolsToExecute inside the MCPConfigs
anonymous struct to pointer types (*schemas.WhiteList) and update the codepaths
that assign these fields to check for nil (nil = no-change, non-nil empty slice
= explicit deny) before writing to the DB or merging updates so partial updates
no longer silently revoke access.
---
Outside diff comments:
In `@core/providers/cohere/models.go`:
- Around line 61-79: The whitelist backfill loop is comparing raw allowedModel
strings against includedModels which stores lowercased raw model names, so IDs
like "cohere/<model>" in allowedModels won't match and will be backfilled as
"cohere/cohere/<model>". Fix: in the loop that iterates allowedModels (and
before checking includedModels[strings.ToLower(allowedModel)]), normalize
allowedModel by removing the provider prefix if present (e.g., if
strings.HasPrefix(allowedModel, string(providerKey)+"/") then allowedModel =
strings.TrimPrefix(allowedModel, string(providerKey)+"/")), then lowercase for
the lookup; keep references to allowedModels, includedModels, providerKey,
bifrostResponse.Data and model.Name while making this change.
In `@core/providers/gemini/models.go`:
- Around line 38-59: Normalize and compare using the canonical provider-prefixed
model ID when filtering and backfilling: compute canonicalID :=
string(providerKey) + "/" + modelName (and use strings.ToLower for keys) and use
allowedModels.Contains(canonicalID) instead of only modelName, store
includedModels[strings.ToLower(canonicalID)] = true, and when iterating
allowedModels derive a canonicalAllowed := allowedModel (prefix with providerKey
if it doesn’t already have it) then check
includedModels[strings.ToLower(canonicalAllowed)] before appending to
bifrostResponse.Data and append the canonicalAllowed as the ID so you never
backfill a duplicate like gemini/gemini/<model>; update references to modelName
checks in the filtering block and the backfill loop accordingly.
In `@core/providers/huggingface/models.go`:
- Around line 40-61: The code currently compares bare model.ModelID against
allowedModels and stores includedModels keyed by lowercased model.ModelID, but
newModel.ID is provider-scoped (fmt.Sprintf("%s/%s/%s", providerKey,
inferenceProvider, model.ModelID)), causing mismatches; update the checks and
keys to use the full provider-scoped ID: when testing allowlist membership use
the fullID (fmt.Sprintf with providerKey and inferenceProvider) instead of
model.ModelID, store includedModels[strings.ToLower(fullID)] = true, and when
backfilling iterate allowedModels as provider-scoped IDs (or construct fullID
for comparison) so the bifrostResponse.Data append and allowedModels membership
checks consistently use the same provider-scoped ID; adjust references in the
block around includedModels, allowedModels, newModel.ID, and the backfill loop
accordingly.
In `@core/providers/openai/models.go`:
- Around line 23-43: The whitelist logic needs normalization of
provider-prefixed IDs: when checking allowedModels in the loop (the
allowedModels.Contains(...) call) check both the raw model.ID and the
provider-prefixed form (string(providerKey)+"/"+model.ID), and when marking
includedModels add both strings.ToLower(model.ID) and
strings.ToLower(string(providerKey)+"/"+model.ID); likewise, when backfilling
from allowedModels, strip a leading provider prefix (e.g. "openai/") before
constructing the backfilled schemas.Model ID so you don't produce
double-prefixed entries—adjust the code around includedModels,
allowedModels.Contains, and the Model ID construction to implement this
normalization.
In `@core/providers/vertex/utils.go`:
- Around line 158-190: The function buildResponseFromConfig currently treats an
empty allowedModels as permissive because IsRestricted() is false for both ["*"]
and [], so add an explicit empty-whitelist guard: call allowedModels.IsEmpty()
near the start of buildResponseFromConfig (before the loop that iterates
deployments and before using addedModelIDs/response) and if it returns true,
return the empty response immediately; this ensures an empty whitelist denies
access instead of exposing all deployments while keeping the existing logic for
restricted and wildcard cases intact.
In `@framework/modelcatalog/main.go`:
- Around line 533-569: The IsModelAllowedForProvider logic currently uses
case-sensitive comparisons which breaks whitelist matching after the WhiteList
migration; update all comparisons in IsModelAllowedForProvider (including the
unrestricted branch that checks slices.Contains(supportedProviders, provider),
the direct match check for allowedModel == model, the provider catalog
membership check for slices.Contains(providerCatalogModels, allowedModel), and
the final modelPart == model comparison) to be case-insensitive — e.g., use
strings.EqualFold or normalize both sides to lower-case before comparing and
implement a case-insensitive contains helper when checking slices (for provider
lists and providerCatalogModels) so existing behavior is preserved but
comparisons ignore case; reference functions/values: IsModelAllowedForProvider,
GetProvidersForModel, GetModelsForProvider, schemas.ParseModelString,
WhiteList.IsUnrestricted, and WhiteList.IsEmpty.
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 755-763: filterModelsByKeys still treats whitelist entries
case-sensitively: when iterating key.Models in the block that sets
hasRestrictedKey and populates allowedModels, store normalized keys (e.g.,
strings.ToLower(model)) and when later comparing models (the checks around
allowedModels access) compare against the same normalized form (e.g.,
strings.ToLower(modelName)). Update both the population of allowedModels and the
model membership checks in filterModelsByKeys to use a consistent lowercase
normalization so the WhiteList semantics become case-insensitive; keep
hasUnrestrictedKey/hasRestrictedKey logic unchanged.
---
Duplicate comments:
In `@core/providers/replicate/models.go`:
- Around line 33-34: Normalize deployment IDs that may begin with "replicate/"
before whitelist checks and before scheduling backfills: where the code checks
allowedModels.IsRestricted() and calls allowedModels.Contains(deploymentID), and
in the later block that emits/schedules backfills, detect and strip a leading
"replicate/" prefix from the deploymentID (e.g., via
strings.HasPrefix/strings.TrimPrefix) so the Contains check and backfill
insertion use the raw "<owner>/<name>" form and avoid double-prefixing
("replicate/replicate/...").
In `@core/providers/vertex/models.go`:
- Around line 180-194: The dedupe map addedModelIDs is using original casing
while WhiteList.Contains is case-insensitive, causing duplicates when casing
differs; normalize the dedupe key by using a consistent canonical form (e.g.,
strings.ToLower) for keys when setting and checking addedModelIDs while leaving
modelEntry.ID (the displayed ID) unchanged; update all occurrences around
modelEntry.ID assignment and checks (including the blocks that set Deployment
and the later 200-241 section) so both insertion and membership tests use the
same lowercased key.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a90a0a87-3e6e-4cd4-8f7c-eaf420f24f9f
📒 Files selected for processing (31)
core/bifrost.gocore/mcp/agent.gocore/mcp/utils.gocore/providers/anthropic/models.gocore/providers/azure/models.gocore/providers/bedrock/models.gocore/providers/cohere/models.gocore/providers/elevenlabs/models.gocore/providers/gemini/models.gocore/providers/huggingface/models.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.gocore/schemas/mcp.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey.goframework/modelcatalog/main.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/handlers/utils.gotransports/bifrost-http/integrations/openai.gotransports/bifrost-http/lib/ctx.go
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
06b9ba6 to
eaf1f57
Compare
Pratham-Mishra04
force-pushed
the
03-16-feat_standardize_empty_array_conventions_in_bifrost
branch
from
24b4bdb to
dd9d995
Compare
Collaborator
Author
|
@coderabbiteai full-review |
Contributor
|
✅ Actions performedFull review triggered. |
Pratham-Mishra04
force-pushed
the
03-16-feat_standardize_empty_array_conventions_in_bifrost
branch
from
dd9d995 to
eab2c1b
Compare
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
eaf1f57 to
75d59f2
Compare
18 tasks
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
framework/modelcatalog/main.go (1)
548-563:⚠️ Potential issue | 🟠 MajorCase-sensitive checks here can incorrectly deny allowed models.
This path bypasses
WhiteList.Containssemantics and uses exact matching (==/slices.Contains), so case variants likeGPT-4Ovsgpt-4ocan fail unexpectedly.🔧 Suggested fix
for _, allowedModel := range allowedModels { // Direct match: "gpt-4o" == "gpt-4o" - if allowedModel == model { + if strings.EqualFold(allowedModel, model) { return true } // Provider-prefixed match: verify it exists in provider's catalog first // This ensures we only allow provider-specific model combinations that are actually supported if strings.Contains(allowedModel, "/") { // Check if this exact prefixed model exists in the provider's catalog // e.g., for openrouter, check if "openai/gpt-4o" is in its catalog - if slices.Contains(providerCatalogModels, allowedModel) { + if slices.ContainsFunc(providerCatalogModels, func(catalogModel string) bool { + return strings.EqualFold(catalogModel, allowedModel) + }) { // Extract the model part and compare with request _, modelPart := schemas.ParseModelString(allowedModel, "") - if modelPart == model { + if strings.EqualFold(modelPart, model) { return true } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/modelcatalog/main.go` around lines 548 - 563, The direct equality and slices.Contains checks in the model validation loop (comparing allowedModel to model and checking providerCatalogModels for allowedModel) are case-sensitive and can reject case-variant names; update the logic to normalize comparisons (e.g., use strings.EqualFold or lowercasing) when comparing allowedModel, model, and the providerCatalogModels entries and ensure the provider-prefixed branch that calls schemas.ParseModelString also compares modelPart to model using a case-insensitive comparison so behavior matches WhiteList.Contains semantics.core/providers/vertex/utils.go (2)
169-205:⚠️ Potential issue | 🟡 MinorUse case-insensitive keys for
addedModelIDs.
Contains()is case-insensitive now, butaddedModelIDsstill keys by the original alias casing. A configured alias likeGemini-2.0-Flashplus an allowlist entrygemini-2.0-flashwill include the live deployment and then backfill a second syntheticvertex/gemini-2.0-flashrow.Suggested fix
addedModelIDs := make(map[string]bool) @@ modelID := string(schemas.Vertex) + "/" + alias - if addedModelIDs[modelID] { + normalizedModelID := strings.ToLower(modelID) + if addedModelIDs[normalizedModelID] { continue } @@ response.Data = append(response.Data, modelEntry) - addedModelIDs[modelID] = true + addedModelIDs[normalizedModelID] = true } @@ for _, allowedModel := range allowedModels { modelID := string(schemas.Vertex) + "/" + allowedModel - if addedModelIDs[modelID] { + normalizedModelID := strings.ToLower(modelID) + if addedModelIDs[normalizedModelID] { continue } @@ response.Data = append(response.Data, modelEntry) - addedModelIDs[modelID] = true + addedModelIDs[normalizedModelID] = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/utils.go` around lines 169 - 205, The map addedModelIDs must use a case-insensitive key to match the case-insensitive allowedModels.Contains checks; when you build keys for the map in the deployment loop and the allowedModels backfill loop, normalize the alias/allowedModel (e.g., strings.ToLower(alias) / strings.ToLower(allowedModel)) before creating the map key and checking/setting addedModelIDs. Keep using the original alias for modelEntry.ID/Name if desired (so only the map key is normalized), and apply the same normalization logic in both places where modelID is computed/checked (the loops that reference alias, allowedModel, addedModelIDs and formatDeploymentName).
165-190:⚠️ Potential issue | 🟠 MajorHonor
WhiteList.IsEmpty()before treating this config as unrestricted.
IsRestricted()is false for both[]and["*"], so the current!restrictAllowedbranch returns every configured deployment for an empty whitelist. The other list-model builders in this stack guardIsEmpty()first, so this path breaks the new deny-by-default behavior.Suggested fix
func buildResponseFromConfig(deployments map[string]string, allowedModels schemas.WhiteList) *schemas.BifrostListModelsResponse { response := &schemas.BifrostListModelsResponse{ Data: make([]schemas.Model, 0), } + if allowedModels.IsEmpty() { + return response + } + addedModelIDs := make(map[string]bool) restrictAllowed := allowedModels.IsRestricted()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/utils.go` around lines 165 - 190, The code treats an empty whitelist as unrestricted because it only checks allowedModels.IsRestricted(); add an early guard that returns response when allowedModels.IsEmpty() is true so empty whitelist is treated as deny-all; specifically, after computing restrictAllowed (allowedModels.IsRestricted()), check allowedModels.IsEmpty() and return response before the deployments loop so the subsequent logic that uses restrictAllowed and deployments/addedModelIDs behaves correctly.framework/configstore/tables/mcp.go (1)
70-91:⚠️ Potential issue | 🟠 MajorLegacy MCP rows can still bypass the new
WhiteListvalidation.These checks only guard future writes. Rows created before this PR can still contain mixed
["*", "..."]or duplicate tool lists, andAfterFindcurrently unmarshals them without re-validating. That leaves malformed ACLs active after upgrade even thoughValidate()is now supposed to reject them. Please add read-time validation or a migration/backfill for the existing JSON columns.Suggested hardening
if c.ToolsToExecuteJSON != "" { if err := sonic.Unmarshal([]byte(c.ToolsToExecuteJSON), &c.ToolsToExecute); err != nil { return err } + if err := c.ToolsToExecute.Validate(); err != nil { + return fmt.Errorf("invalid tools_to_execute: %w", err) + } } if c.ToolsToAutoExecuteJSON != "" { if err := sonic.Unmarshal([]byte(c.ToolsToAutoExecuteJSON), &c.ToolsToAutoExecute); err != nil { return err } + if err := c.ToolsToAutoExecute.Validate(); err != nil { + return fmt.Errorf("invalid tools_to_auto_execute: %w", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/tables/mcp.go` around lines 70 - 91, AfterFind currently unmarshals ToolsToExecuteJSON / ToolsToAutoExecuteJSON but doesn't re-run the new WhiteList validation, so legacy rows can bypass Validate(); update the AfterFind method to, after decoding into ToolsToExecute and ToolsToAutoExecute, call their Validate() methods (the same validation used on writes) and handle failures by either returning an error or normalizing/fixing the ACLs and reserializing into ToolsToExecuteJSON / ToolsToAutoExecuteJSON; ensure you reference and update ToolsToExecute, ToolsToAutoExecute, ToolsToExecuteJSON, ToolsToAutoExecuteJSON and use the existing Validate() logic so read-time entries are canonicalized or rejected/backfilled on read.transports/bifrost-http/handlers/providers.go (1)
758-763:⚠️ Potential issue | 🟡 MinorNormalize restricted model keys to keep WhiteList matching semantics consistent.
This path still filters via exact map lookup, so casing differences can deny models that should pass under case-insensitive allowlist behavior.
💡 Proposed fix
allowedModels := make(map[string]bool) @@ } else if !key.Models.IsEmpty() { // Key has specific model restrictions - add them to allowedModels hasRestrictedKey = true for _, model := range key.Models { - allowedModels[model] = true + allowedModels[strings.ToLower(model)] = true } } else { @@ filtered := []string{} for _, model := range models { - if allowedModels[model] { + if allowedModels[strings.ToLower(model)] { filtered = append(filtered, model) } }Also applies to: 786-789
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/providers.go` around lines 758 - 763, The code that builds allowedModels from key.Models (inside the branch where key.Models.IsEmpty() is false) is using raw model strings for map lookups which breaks case-insensitive whitelist semantics; update the loop that iterates over key.Models (and the similar loop at the other location around the 786-789 block) to normalize each model string (e.g., strings.ToLower or a shared normalizeModel function) before assigning allowedModels[normalizedModel] = true and ensure any later lookups against allowedModels also normalize the queried model name (so hasRestrictedKey, allowedModels, key.Models and any checks against allowedModels use the same normalization).
♻️ Duplicate comments (3)
core/providers/replicate/models.go (1)
33-34:⚠️ Potential issue | 🟠 MajorNormalize provider-prefixed Replicate allowlist entries before matching.
This path still only compares against bare
owner/name. IfallowedModelscontainsreplicate/acme/foo, the live deployment is filtered out and the backfill loop emitsreplicate/replicate/acme/foo. The OpenRouter path in this stack already handles both forms; Replicate needs the same normalization.Suggested fix
func ToBifrostListModelsResponse( deploymentsResponse *ReplicateDeploymentListResponse, providerKey schemas.ModelProvider, allowedModels schemas.WhiteList, unfiltered bool, ) *schemas.BifrostListModelsResponse { bifrostResponse := &schemas.BifrostListModelsResponse{ Data: make([]schemas.Model, 0), } + providerPrefix := string(providerKey) + "/" + providerPrefixLower := strings.ToLower(providerPrefix) + normalizeAllowedModel := func(model string) string { + if strings.HasPrefix(strings.ToLower(model), providerPrefixLower) { + return model[len(providerPrefix):] + } + return model + } if !unfiltered && allowedModels.IsEmpty() { return bifrostResponse } @@ - if !unfiltered && allowedModels.IsRestricted() && !allowedModels.Contains(deploymentID) { + if !unfiltered && allowedModels.IsRestricted() && + !(allowedModels.Contains(deploymentID) || allowedModels.Contains(providerPrefix+deploymentID)) { continue } @@ if !unfiltered && allowedModels.IsRestricted() { for _, allowedModel := range allowedModels { - if !includedModels[strings.ToLower(allowedModel)] { + deploymentID := normalizeAllowedModel(allowedModel) + if !includedModels[strings.ToLower(deploymentID)] { bifrostResponse.Data = append(bifrostResponse.Data, schemas.Model{ - ID: string(providerKey) + "/" + allowedModel, - Name: schemas.Ptr(allowedModel), + ID: providerPrefix + deploymentID, + Name: schemas.Ptr(deploymentID), }) + includedModels[strings.ToLower(deploymentID)] = true } } }Also applies to: 66-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/replicate/models.go` around lines 33 - 34, The allowlist check for Replicate currently only compares bare deploymentID (owner/name) against allowedModels, so entries like "replicate/acme/foo" won't match; update the checks in the Replicate filtering logic (the block using variables unfiltered, allowedModels.IsRestricted(), allowedModels.Contains(deploymentID)) to normalize/match provider-prefixed entries by also checking allowedModels.Contains("replicate/"+deploymentID) (or strip a leading "replicate/" from entries before matching), and apply the same two-way normalization in the other Replicate block around the lines referenced (the second block at 66-72) to prevent emitting duplicated "replicate/replicate/..." backfill IDs. Ensure both containment checks use the same normalization approach so deploymentID matching and backfill emission remain consistent.core/providers/vertex/models.go (1)
150-168:⚠️ Potential issue | 🟡 MinorNormalize
addedModelIDsin the restricted path.
WhiteList.Contains()is case-insensitive, butaddedModelIDsis still keyed with raw IDs and aliases. If the allowlist spelling differs only by case, the first pass adds one entry and the third pass backfills the configured spelling as a second model. Lower-case the tracking keys (or carry the matched allowlist entry forward) before the backfill checks.Suggested fix
- addedModelIDs := make(map[string]bool) + addedModelIDs := make(map[string]bool) @@ - addedModelIDs[modelEntry.ID] = true + addedModelIDs[strings.ToLower(modelEntry.ID)] = true @@ - if addedModelIDs[modelID] { + if addedModelIDs[strings.ToLower(modelID)] { continue } @@ - addedModelIDs[modelID] = true + addedModelIDs[strings.ToLower(modelID)] = true @@ - if addedModelIDs[modelID] { + if addedModelIDs[strings.ToLower(modelID)] { continue } @@ - addedModelIDs[modelID] = true + addedModelIDs[strings.ToLower(modelID)] = trueAlso applies to: 198-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/vertex/models.go` around lines 150 - 168, The restricted-path logic currently uses raw IDs/aliases when updating addedModelIDs which causes duplicates when allowlist matches differ only by case; update the code in the branch that computes restrictAllowed/shouldFilter (references: allowedModels.IsRestricted, findDeploymentMatch, allowedModels.Contains, deployments, customModelID) so that when a deploymentAlias or customModelID matches allowedModels you normalize the tracking key (e.g., lowercase the key) or carry the exact allowlist entry string forward before adding to addedModelIDs; apply the same normalization/carry-forward change to the similar block around lines 198-241 to ensure backfill checks use the canonical allowlist spelling and avoid case-variant duplicates.core/providers/azure/models.go (1)
13-17:⚠️ Potential issue | 🟡 MinorReturn the canonical allowlist entry for exact matches.
Line 16 returns the input
valueinstead of the matchedwlentry. If API casing differs from configured allowlist casing, downstream inclusion/backfill logic can treat it as missing and append duplicates.💡 Proposed fix
func findMatchingAllowedModel(wl schemas.WhiteList, value string) string { // First check exact match (case-insensitive) - if wl.Contains(value) { - return value - } + for _, item := range wl { + if strings.EqualFold(item, value) { + return item + } + } // Additional layer: check base model matches (ignoring version suffixes) // This handles cases where model versions differ but base model is the same // Return the item from whitelist (not value) to use the actual name from allowedModels🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/providers/azure/models.go` around lines 13 - 17, The function findMatchingAllowedModel currently returns the input value on exact matches which can preserve incorrect casing; instead return the canonical allowlist entry from wl so downstream logic sees the configured casing. Replace the current wl.Contains(value) branch to fetch and return the stored whitelist entry (e.g., use a wl.Get / wl.Lookup method if available, or iterate wl's entries comparing case-insensitively and return the matching entry) rather than returning the raw input; keep the rest of findMatchingAllowedModel logic unchanged.
🧹 Nitpick comments (3)
core/schemas/account.go (1)
53-57: ClarifyIsRestricted()doc to match current behavior.
IsRestricted()returnstruefor empty lists ([]) as well, not only for lists containing entries other than"*". Please align the comment with the actual logic to avoid misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/account.go` around lines 53 - 57, The comment for WhiteList.IsRestricted is inaccurate: IsRestricted() returns true for empty lists as well as for lists without "*" because it simply negates IsUnrestricted(); update the docstring for the IsRestricted method to state that it returns true when the whitelist does not allow all values (i.e., when the list is empty or does not contain "*"), and reference the related IsUnrestricted() behavior in the description so readers understand the relationship between the two methods.core/bifrost.go (1)
6176-6202: Update the nearby inline comment to match the new allowlist behavior.The code now enforces
IsAllowed(model)(empty list denies all), but the earlier comment still says empty model lists are allowed for all models. Please align the comment to avoid future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/bifrost.go` around lines 6176 - 6202, Update the inline comment that currently says an empty model list equals “supported for all models” to reflect the new allowlist semantics: empty model lists now deny all models and IsAllowed(model) is enforced; modify the comment near the supportedKeys/skipModelCheck logic (referencing skipModelCheck, supportedKeys, keys, k.Enabled, key.Models.IsAllowed, CanProviderKeyValueBeEmpty and model) to state that ["*"] allows all, an empty list denies all, and specific entries allow only those models so future readers understand the current behavior.transports/bifrost-http/handlers/governance.go (1)
497-502: Consider extracting repeated allowlist validation into helpers.The same validation patterns are repeated across create/update provider and MCP paths. A small helper would reduce drift risk.
♻️ Suggested refactor
+func validateProviderAllowLists(provider string, allowedModels, keyIDs schemas.WhiteList) error { + if err := allowedModels.Validate(); err != nil { + return &badRequestError{err: fmt.Errorf("invalid allowed_models for provider %s: %w", provider, err)} + } + if err := keyIDs.Validate(); err != nil { + return &badRequestError{err: fmt.Errorf("invalid key_ids for provider %s: %w", provider, err)} + } + return nil +} + +func validateMCPToolsAllowList(mcpClientName string, tools schemas.WhiteList) error { + if err := tools.Validate(); err != nil { + return &badRequestError{err: fmt.Errorf("invalid tools_to_execute for mcp client %s: %w", mcpClientName, err)} + } + return nil +}Also applies to: 582-584, 848-853, 926-931, 1097-1099
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 497 - 502, Extract the repeated allowlist validation (the pattern calling pc.AllowedModels.Validate() and pc.KeyIDs.Validate() and returning &badRequestError{err: fmt.Errorf(...)} ) into a small helper function, e.g., validateProviderAllowlists(pc *ProviderConfig) error, and replace the duplicate blocks in the create/update provider and MCP handlers (where pc.AllowedModels.Validate() and pc.KeyIDs.Validate() are currently used) with a single call to that helper; ensure the helper returns the same formatted badRequestError with the provider name to preserve behavior and reuse it in all locations (those around the pc.* Validate calls and the other occurrences noted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/providers/vertex/utils.go`:
- Around line 169-205: The map addedModelIDs must use a case-insensitive key to
match the case-insensitive allowedModels.Contains checks; when you build keys
for the map in the deployment loop and the allowedModels backfill loop,
normalize the alias/allowedModel (e.g., strings.ToLower(alias) /
strings.ToLower(allowedModel)) before creating the map key and checking/setting
addedModelIDs. Keep using the original alias for modelEntry.ID/Name if desired
(so only the map key is normalized), and apply the same normalization logic in
both places where modelID is computed/checked (the loops that reference alias,
allowedModel, addedModelIDs and formatDeploymentName).
- Around line 165-190: The code treats an empty whitelist as unrestricted
because it only checks allowedModels.IsRestricted(); add an early guard that
returns response when allowedModels.IsEmpty() is true so empty whitelist is
treated as deny-all; specifically, after computing restrictAllowed
(allowedModels.IsRestricted()), check allowedModels.IsEmpty() and return
response before the deployments loop so the subsequent logic that uses
restrictAllowed and deployments/addedModelIDs behaves correctly.
In `@framework/configstore/tables/mcp.go`:
- Around line 70-91: AfterFind currently unmarshals ToolsToExecuteJSON /
ToolsToAutoExecuteJSON but doesn't re-run the new WhiteList validation, so
legacy rows can bypass Validate(); update the AfterFind method to, after
decoding into ToolsToExecute and ToolsToAutoExecute, call their Validate()
methods (the same validation used on writes) and handle failures by either
returning an error or normalizing/fixing the ACLs and reserializing into
ToolsToExecuteJSON / ToolsToAutoExecuteJSON; ensure you reference and update
ToolsToExecute, ToolsToAutoExecute, ToolsToExecuteJSON, ToolsToAutoExecuteJSON
and use the existing Validate() logic so read-time entries are canonicalized or
rejected/backfilled on read.
In `@framework/modelcatalog/main.go`:
- Around line 548-563: The direct equality and slices.Contains checks in the
model validation loop (comparing allowedModel to model and checking
providerCatalogModels for allowedModel) are case-sensitive and can reject
case-variant names; update the logic to normalize comparisons (e.g., use
strings.EqualFold or lowercasing) when comparing allowedModel, model, and the
providerCatalogModels entries and ensure the provider-prefixed branch that calls
schemas.ParseModelString also compares modelPart to model using a
case-insensitive comparison so behavior matches WhiteList.Contains semantics.
In `@transports/bifrost-http/handlers/providers.go`:
- Around line 758-763: The code that builds allowedModels from key.Models
(inside the branch where key.Models.IsEmpty() is false) is using raw model
strings for map lookups which breaks case-insensitive whitelist semantics;
update the loop that iterates over key.Models (and the similar loop at the other
location around the 786-789 block) to normalize each model string (e.g.,
strings.ToLower or a shared normalizeModel function) before assigning
allowedModels[normalizedModel] = true and ensure any later lookups against
allowedModels also normalize the queried model name (so hasRestrictedKey,
allowedModels, key.Models and any checks against allowedModels use the same
normalization).
---
Duplicate comments:
In `@core/providers/azure/models.go`:
- Around line 13-17: The function findMatchingAllowedModel currently returns the
input value on exact matches which can preserve incorrect casing; instead return
the canonical allowlist entry from wl so downstream logic sees the configured
casing. Replace the current wl.Contains(value) branch to fetch and return the
stored whitelist entry (e.g., use a wl.Get / wl.Lookup method if available, or
iterate wl's entries comparing case-insensitively and return the matching entry)
rather than returning the raw input; keep the rest of findMatchingAllowedModel
logic unchanged.
In `@core/providers/replicate/models.go`:
- Around line 33-34: The allowlist check for Replicate currently only compares
bare deploymentID (owner/name) against allowedModels, so entries like
"replicate/acme/foo" won't match; update the checks in the Replicate filtering
logic (the block using variables unfiltered, allowedModels.IsRestricted(),
allowedModels.Contains(deploymentID)) to normalize/match provider-prefixed
entries by also checking allowedModels.Contains("replicate/"+deploymentID) (or
strip a leading "replicate/" from entries before matching), and apply the same
two-way normalization in the other Replicate block around the lines referenced
(the second block at 66-72) to prevent emitting duplicated
"replicate/replicate/..." backfill IDs. Ensure both containment checks use the
same normalization approach so deploymentID matching and backfill emission
remain consistent.
In `@core/providers/vertex/models.go`:
- Around line 150-168: The restricted-path logic currently uses raw IDs/aliases
when updating addedModelIDs which causes duplicates when allowlist matches
differ only by case; update the code in the branch that computes
restrictAllowed/shouldFilter (references: allowedModels.IsRestricted,
findDeploymentMatch, allowedModels.Contains, deployments, customModelID) so that
when a deploymentAlias or customModelID matches allowedModels you normalize the
tracking key (e.g., lowercase the key) or carry the exact allowlist entry string
forward before adding to addedModelIDs; apply the same
normalization/carry-forward change to the similar block around lines 198-241 to
ensure backfill checks use the canonical allowlist spelling and avoid
case-variant duplicates.
---
Nitpick comments:
In `@core/bifrost.go`:
- Around line 6176-6202: Update the inline comment that currently says an empty
model list equals “supported for all models” to reflect the new allowlist
semantics: empty model lists now deny all models and IsAllowed(model) is
enforced; modify the comment near the supportedKeys/skipModelCheck logic
(referencing skipModelCheck, supportedKeys, keys, k.Enabled,
key.Models.IsAllowed, CanProviderKeyValueBeEmpty and model) to state that ["*"]
allows all, an empty list denies all, and specific entries allow only those
models so future readers understand the current behavior.
In `@core/schemas/account.go`:
- Around line 53-57: The comment for WhiteList.IsRestricted is inaccurate:
IsRestricted() returns true for empty lists as well as for lists without "*"
because it simply negates IsUnrestricted(); update the docstring for the
IsRestricted method to state that it returns true when the whitelist does not
allow all values (i.e., when the list is empty or does not contain "*"), and
reference the related IsUnrestricted() behavior in the description so readers
understand the relationship between the two methods.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 497-502: Extract the repeated allowlist validation (the pattern
calling pc.AllowedModels.Validate() and pc.KeyIDs.Validate() and returning
&badRequestError{err: fmt.Errorf(...)} ) into a small helper function, e.g.,
validateProviderAllowlists(pc *ProviderConfig) error, and replace the duplicate
blocks in the create/update provider and MCP handlers (where
pc.AllowedModels.Validate() and pc.KeyIDs.Validate() are currently used) with a
single call to that helper; ensure the helper returns the same formatted
badRequestError with the provider name to preserve behavior and reuse it in all
locations (those around the pc.* Validate calls and the other occurrences
noted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a09af450-da64-414e-bde3-64c33cdd4fe6
📒 Files selected for processing (31)
core/bifrost.gocore/mcp/agent.gocore/mcp/utils.gocore/providers/anthropic/models.gocore/providers/azure/models.gocore/providers/bedrock/models.gocore/providers/cohere/models.gocore/providers/elevenlabs/models.gocore/providers/gemini/models.gocore/providers/huggingface/models.gocore/providers/mistral/models.gocore/providers/openai/models.gocore/providers/openrouter/openrouter.gocore/providers/replicate/models.gocore/providers/vertex/models.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.gocore/schemas/mcp.goframework/configstore/tables/mcp.goframework/configstore/tables/virtualkey.goframework/modelcatalog/main.goplugins/governance/main.goplugins/governance/resolver.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/mcpserver.gotransports/bifrost-http/handlers/providers.gotransports/bifrost-http/handlers/utils.gotransports/bifrost-http/integrations/openai.gotransports/bifrost-http/lib/ctx.go
🚧 Files skipped from review as they are similar to previous changes (5)
- transports/bifrost-http/lib/ctx.go
- transports/bifrost-http/handlers/mcpserver.go
- plugins/governance/resolver.go
- transports/bifrost-http/handlers/utils.go
- core/schemas/mcp.go
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
75d59f2 to
1a620fd
Compare
Pratham-Mishra04
force-pushed
the
03-16-feat_standardize_empty_array_conventions_in_bifrost
branch
from
eab2c1b to
112aee8
Compare
Collaborator
Author
Merge activity
|
Pratham-Mishra04
changed the base branch from
03-16-feat_standardize_empty_array_conventions_in_bifrost
to
graphite-base/2125
Pratham-Mishra04
force-pushed
the
03-17-refactor_added_whitelist_type
branch
from
1a620fd to
27bf9ba
Compare
This was referenced Mar 23, 2026
Merged
akshaydeo
added a commit
that referenced
this pull request
Apr 18, 2026
* refactor: standardize empty array conventions for VK Provider & MCP Configs, and makes Provider Config weight optional for routing (#1932)
## Summary
Changes Virtual Key provider and MCP configurations from "allow-all by default" to "deny-by-default" security model. Virtual Keys now require explicit provider and MCP client configurations to allow access, improving security posture.
## Changes
- **Provider Configs**: Empty `provider_configs` now blocks all providers instead of allowing all
- **MCP Configs**: Empty `mcp_configs` now blocks all MCP tools instead of allowing all
- **Weight Field**: Changed provider `weight` from required `float64` to optional `*float64` - null weight excludes provider from weighted routing
- **Migration**: Added automatic backfill migration to preserve existing Virtual Key behavior by adding all available providers/MCP clients to VKs with empty configs
- **Documentation**: Updated all references to reflect new deny-by-default behavior
- **UI Updates**: Modified Virtual Key creation/editing interface to reflect new behavior and weight handling
## Type of change
- [x] Feature
- [x] Refactor
- [x] Documentation
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Test Virtual Key creation and provider/MCP access:
```sh
# Core/Transports
go version
go test ./...
# Test Virtual Key with no provider configs blocks requests
curl -X POST http://localhost:8080/v1/chat/completions \
-H "Authorization: Bearer sk-bf-empty-vk" \
-H "Content-Type: application/json" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Should return error about no providers configured
# Test Virtual Key with provider configs allows requests
curl -X POST http://localhost:8080/v1/chat/completions \
-H "Authorization: Bearer sk-bf-configured-vk" \
-H "Content-Type: application/json" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Should work normally
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
## Breaking changes
- [x] Yes
**Impact**: Existing Virtual Keys with empty `provider_configs` or `mcp_configs` would be blocked after this change.
**Migration**: Automatic migration `migrationBackfillEmptyVirtualKeyConfigs` runs on startup to backfill existing Virtual Keys with all available providers/MCP clients, preserving current behavior. New Virtual Keys created after this change will use deny-by-default.
## Security considerations
This change significantly improves security posture by requiring explicit configuration of allowed providers and MCP tools for Virtual Keys. The automatic migration ensures no disruption to existing deployments while new Virtual Keys benefit from the more secure default behavior.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add MCP auto tool injection toggle (#1933)
## Summary
Adds a new configuration option `DisableAutoToolInject` to the MCP (Model Context Protocol) system that allows disabling automatic tool injection into requests. When enabled, MCP tools are only included when explicitly requested via context headers or filters, providing more granular control over tool availability.
## Changes
- Added `DisableAutoToolInject` field to `MCPToolManagerConfig` schema with runtime update support
- Implemented atomic boolean storage in `ToolsManager` to safely handle concurrent access
- Added logic in `ParseAndAddToolsToRequest` to respect the disable flag and only inject tools when explicit context filters are present
- Extended configuration management with database migration, UI controls, and API endpoints
- Added hot-reload capability through `UpdateMCPDisableAutoToolInject` methods across the stack
- Updated UI with a toggle switch and clear documentation about the feature's behavior
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Validate the new MCP auto tool injection toggle:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Test the feature:
1. Configure MCP clients and tools
2. Enable "Disable Auto Tool Injection" in the MCP configuration UI
3. Make requests without explicit tool headers - tools should not be injected
4. Make requests with `x-bf-mcp-include-tools` header - tools should be injected
5. Verify hot-reload works by toggling the setting without server restart
## Screenshots/Recordings
UI changes include a new toggle switch in the MCP configuration view with descriptive text explaining when tools are injected based on explicit headers.
## Breaking changes
- [ ] Yes
- [x] No
This is a backward-compatible addition with a default value of `false` (auto injection enabled).
## Related issues
This addresses the need for more granular control over MCP tool injection behavior in request processing.
## Security considerations
The feature provides better control over tool exposure by allowing administrators to require explicit opt-in for tool injection, potentially reducing unintended tool access.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: VK MCP config now works as an AllowList (#1940)
## Summary
This PR implements MCP tool governance by enforcing virtual key MCP configurations as an execution-time allow-list. When virtual keys have empty MCPConfigs, all MCP tools are denied. When non-empty, each tool is validated against the configured allow-list at both inference time and MCP tool execution.
## Changes
- **Context parameter updates**: Changed MCP-related functions to use `*schemas.BifrostContext` instead of `context.Context` to enable tool tracking
- **Tool tracking**: Added `BifrostContextKeyMCPAddedTools` context key to track which MCP tools are added to requests
- **Governance enforcement**: Virtual key MCP configurations now act as execution-time allow-lists with validation in both `PreMCPHook` and `evaluateGovernanceRequest`
- **Auto-injection control**: Added `DisableAutoToolInject` configuration option that respects the toggle and skips auto-injection when headers are already set by callers
- **Decision type**: Added `DecisionMCPToolBlocked` for MCP tool governance violations
- **UI improvements**: Updated MCP view description and sidebar item naming for better clarity
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Test MCP tool governance with virtual keys:
```sh
# Core/Transports
go version
go test ./...
# Test with virtual key having empty MCPConfigs (should deny all MCP tools)
curl -X POST /v1/chat/completions \
-H "x-bf-virtual-key: test-vk-empty-mcp" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Test with virtual key having specific MCP tool allowlist
curl -X POST /v1/chat/completions \
-H "x-bf-virtual-key: test-vk-with-mcp" \
-d '{"model": "gpt-4", "messages": [{"role": "user", "content": "test"}]}'
# Test disable auto tool inject configuration
curl -X PUT /v1/config/mcp/disable-auto-tool-inject \
-d '{"disable": true}'
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
New configuration options:
- `disable_auto_tool_inject`: Boolean flag to disable automatic MCP tool injection
- Virtual key `MCPConfigs`: Array of MCP client configurations that act as allow-lists
## Screenshots/Recordings
UI changes include updated MCP configuration view with clearer descriptions for the disable auto tool injection toggle and improved sidebar navigation labels.
## Breaking changes
- [x] Yes
- [ ] No
**Impact**: MCP-related function signatures now require `*schemas.BifrostContext` instead of `context.Context`. Virtual keys with empty MCPConfigs will now deny all MCP tools by default.
**Migration**: Update any custom MCP integrations to use the new context parameter type. Configure MCPConfigs on virtual keys that need MCP tool access.
## Related issues
Implements MCP tool governance and execution-time validation for virtual key configurations.
## Security considerations
- **Access control**: Virtual key MCP configurations now enforce strict allow-lists for tool execution
- **Context isolation**: Tool tracking is isolated per request context to prevent cross-request leakage
- **Validation**: Both pre-execution and execution-time validation prevent unauthorized tool access
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: standardize empty array conventions for VK Provider Config Allowed Keys (#2006)
## Summary
Migrates VK provider config allowed keys from implicit allow-all semantics to explicit deny-by-default behavior. Adds `AllowAllKeys` boolean field to enable granular key access control while maintaining backward compatibility.
## Changes
- Added `AllowAllKeys` boolean field to `TableVirtualKeyProviderConfig` with database migration
- Backfilled existing configs with `allow_all_keys=true` to preserve current behavior
- Updated key resolution logic: empty keys now denies all access, `["*"]` wildcard allows all keys
- Modified governance resolver to set empty `includeOnlyKeys` slice when no keys are configured
- Enhanced HTTP handlers to recognize `["*"]` wildcard and set `AllowAllKeys` flag appropriately
- Updated UI to display "Allow All Keys" option and show deny-by-default messaging
- Added JSON unmarshaling support for `["*"]` wildcard in config files
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Validate the migration and new key access control behavior:
```sh
# Core/Transports
go version
go test ./...
# Test migration runs successfully
go run main.go migrate
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Test scenarios:
1. Create VK with empty `key_ids` - should deny all keys
2. Create VK with `key_ids: ["*"]` - should allow all keys
3. Create VK with specific key IDs - should allow only those keys
4. Verify existing VKs maintain their current behavior after migration
## Screenshots/Recordings
UI now shows:
- "Allow All Keys" option in key selection dropdown
- "No keys allowed" vs "All keys allowed" status indicators
- "No providers configured (deny-by-default)" messaging
## Breaking changes
- [ ] Yes
- [x] No
The migration preserves existing behavior by setting `allow_all_keys=true` for configs that previously had no keys specified.
## Related issues
Part of VK access control enhancement initiative.
## Security considerations
Improves security posture by implementing deny-by-default semantics for key access. Existing deployments maintain current access patterns through automatic backfill migration.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: standardize empty array conventions for allowed models (#2113)
## Summary
Standardizes empty array conventions across Bifrost to implement deny-by-default security semantics. Previously, empty arrays for `allowed_models` and `Models` fields meant "allow all", creating potential security gaps. Now `["*"]` explicitly means "allow all" while empty arrays mean "deny all".
## Changes
- **Core Logic**: Updated model filtering in `bifrost.go` and `selectKeyFromProviderForModel` to treat empty `Models` arrays as deny-all and `["*"]` as allow-all
- **Database Migration**: Added `migrationBackfillAllowedModelsWildcard` to convert existing empty arrays to `["*"]` preserving current behavior for existing records
- **Model Catalog**: Updated `IsModelAllowedForProvider` to use wildcard semantics with deny-by-default fallback
- **Schema Defaults**: Changed default `Models` value from `[]` to `["*"]` in table definitions and form schemas
- **UI Components**: Enhanced `ModelMultiselect` with `allowAllOption` prop and updated virtual key forms to handle wildcard selection
- **Documentation**: Updated JSON schemas, comments, and tooltips to reflect new conventions
- **Governance**: Updated provider config filtering logic to use new wildcard semantics
- **Server Bootstrap**: Added wildcard filtering when loading models to prevent literal "*" from appearing as a model name
## Type of change
- [x] Refactor
- [ ] Bug fix
- [ ] Feature
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Validate the migration and new semantics:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Test scenarios:
1. Create new virtual keys - should default to `["*"]` for allowed models
2. Create new provider keys - should default to `["*"]` for models
3. Verify existing keys with empty arrays are migrated to `["*"]`
4. Test that empty arrays now deny all models/keys as expected
5. Verify UI shows "All models allowed" for wildcard and "No models (deny all)" for empty arrays
## Screenshots/Recordings
UI changes include:
- Model multiselect now shows "Allow All Models" option
- Virtual key details display "All Models" badge for wildcard vs "No models (deny all)" for empty
- Form placeholders updated to reflect new semantics
## Breaking changes
- [x] Yes
- [ ] No
**Migration Impact**: The database migration automatically converts existing empty `allowed_models` and `models_json` arrays to `["*"]`, preserving current behavior. However, any new configurations with empty arrays will now deny access instead of allowing all. Applications relying on "empty = allow all" semantics must be updated to use `["*"]` explicitly.
## Related issues
Part of security hardening initiative to implement explicit allow-lists and deny-by-default semantics across Bifrost configuration.
## Security considerations
This change significantly improves security posture by:
- Eliminating ambiguous "empty means allow all" semantics
- Implementing explicit deny-by-default for new configurations
- Requiring intentional wildcard usage via `["*"]` for broad access
- Maintaining backward compatibility through automatic migration
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: replace string slices with WhiteList for allowlist fields (#2125)
## Summary
Introduces a new `WhiteList` type to standardize whitelist behavior across the codebase, replacing manual slice operations and string comparisons with semantic methods for handling allow/deny lists.
## Changes
- Added `WhiteList` type with methods `IsAllowed()`, `IsUnrestricted()`, `IsEmpty()`, `Contains()`, and `Validate()`
- Replaced `[]string` fields with `WhiteList` for model restrictions, tool filtering, and key access controls
- Updated all whitelist logic to use semantic methods instead of manual `slices.Contains()` checks
- Added validation to ensure wildcards ("*") aren't mixed with specific values and prevent duplicates
- Improved case-insensitive matching for whitelist comparisons
## Type of change
- [x] Refactor
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Plugins
## How to test
Verify that whitelist behavior remains consistent across all affected components:
```sh
# Core/Transports
go version
go test ./...
# Test specific whitelist scenarios:
# - Empty lists deny all access
# - ["*"] allows all access
# - Specific lists only allow listed items
# - Mixed wildcards and specific items are rejected
# - Duplicate entries are rejected
```
Test key model filtering, MCP tool execution, and virtual key configurations to ensure whitelist logic works correctly.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
The `WhiteList` type maintains the same JSON serialization format as `[]string`, so existing configurations remain compatible.
## Related issues
N/A
## Security considerations
Improves security by standardizing deny-by-default behavior and adding validation to prevent misconfigured whitelists that could inadvertently grant excessive permissions.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add request-level extra headers support for MCP tool execution (#2126)
## Summary
This PR adds support for request-level extra headers in MCP tool execution, allowing callers to forward specific headers to MCP servers at runtime based on a per-client allowlist configuration.
## Changes
- Added `AllowedExtraHeaders` field to MCP client configuration with allowlist semantics (empty array = deny all, `["*"]` = allow all)
- Introduced `BifrostContextKeyMCPExtraHeaders` context key to track headers forwarded to MCP tools
- Created `core/mcp/utils` package with `GetHeadersForToolExecution` function to merge static and dynamic headers
- Updated MCP tool execution in both regular tool manager and Starlark code mode to use the new header forwarding system
- Added database migration for `allowed_extra_headers_json` column in MCP client table
- Updated UI to include allowed extra headers configuration in MCP client management
- Enhanced auth demo server example to demonstrate tool-execution level authentication patterns
## Type of change
- [x] Feature
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] UI (Next.js)
## How to test
1. Configure an MCP client with allowed extra headers:
```json
{
"name": "test-client",
"connection_string": "http://localhost:3002/",
"auth_type": "headers",
"headers": {
"X-API-Key": "connection-secret"
},
"allowed_extra_headers": ["X-Tool-Token"],
"tools_to_execute": ["*"]
}
```
2. Make requests with extra headers that should be forwarded:
```bash
curl -X POST http://localhost:8080/v1/chat/completions \
-H "Authorization: Bearer your-key" \
-H "X-Tool-Token: tool-execution-secret" \
-d '{
"model": "gpt-4",
"messages": [{"role": "user", "content": "Use the secret_data tool"}],
"tools": [{"type": "function", "function": {"name": "secret_data"}}]
}'
```
3. Test the auth demo server:
```bash
cd examples/mcps/auth-demo-server
go run main.go
# Server demonstrates two-tier auth: connection-level (X-API-Key) and tool-level (X-Tool-Token)
```
4. Run tests:
```sh
go test ./core/mcp/...
go test ./transports/bifrost-http/...
cd ui
pnpm test
pnpm build
```
## Breaking changes
- [ ] Yes
- [x] No
This is a backward-compatible addition. Existing MCP clients will have empty `allowed_extra_headers` (deny all extra headers) which maintains current behavior.
## Security considerations
- Extra headers are filtered through a strict allowlist per MCP client
- Security denylist prevents auth header overrides via extra headers
- Two-tier authentication pattern demonstrated: connection-level + tool-execution level
- Headers are only forwarded to MCP servers that explicitly allow them
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: apply MCP tool filtering headers to tools/list response when using bifrost as MCP gateway (#2127)
## Summary
Adds support for `x-bf-mcp-include-clients` and `x-bf-mcp-include-tools` request headers to filter MCP tools/list response when using Bifrost as an MCP gateway. This ensures that tool filtering is respected at the MCP protocol level, not just during inference.
## Changes
- Implemented dynamic tool filtering in MCP server handlers that respects per-request include headers
- Added `makeIncludeClientsFilter()` function that filters tools based on request context values
- Registered the tool filter on both global and virtual key MCP servers during initialization
- Updated documentation to clarify that `mcp-include-tools` requires `clientName-toolName` format
- Enhanced examples in documentation to show proper tool naming format
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [x] Docs
## How to test
Test MCP gateway functionality with tool filtering:
```sh
# Test tools/list filtering with include-tools header
curl --location 'http://localhost:8080/mcp/tools/list' \
--header 'x-bf-mcp-include-tools: gmail-send_email,filesystem-read_file' \
--header 'Authorization: Bearer your-vk-here'
# Test tools/list filtering with include-clients header
curl --location 'http://localhost:8080/mcp/tools/list' \
--header 'x-bf-mcp-include-clients: gmail,filesystem' \
--header 'Authorization: Bearer your-vk-here'
# Verify chat completions still respect the same headers
curl --location 'http://localhost:8080/v1/chat/completions' \
--header 'x-bf-mcp-include-tools: gmail-send_email' \
--header 'Content-Type: application/json' \
--data '{
"model": "openai/gpt-4o-mini",
"messages": [{"role": "user", "content": "What tools are available?"}]
}'
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
The tool filtering mechanism ensures that virtual key restrictions are properly enforced at the MCP protocol level, preventing unauthorized access to tools that should be filtered out based on request headers.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* refactor: parallelize model listing for providers to speed up startup time (#2151)
## Summary
Parallelizes model listing operations for providers during server startup and provider reloading to significantly reduce initialization time. Previously, model listing was performed sequentially for each provider, causing slower startup times especially when multiple providers were configured.
## Changes
- Added concurrent execution using goroutines and sync.WaitGroup for model listing operations in three key functions: `ReloadProvider`, `ForceReloadPricing`, and `Bootstrap`
- In `ReloadProvider`, both filtered and unfiltered model listing requests now run concurrently for the same provider
- In `ForceReloadPricing` and `Bootstrap`, model listing for different providers now runs in parallel instead of sequentially
- Moved provider key retrieval earlier in `ReloadProvider` to ensure it happens before concurrent model listing
- Added proper context cancellation with defer statements for bifrost contexts
## Type of change
- [x] Refactor
## Affected areas
- [x] Transports (HTTP)
## How to test
Test server startup time with multiple providers configured to verify the performance improvement:
```sh
# Core/Transports
go version
go test ./...
# Test with multiple providers configured
# Measure startup time before and after the change
time go run main.go
```
Configure multiple providers in your bifrost configuration and observe faster startup times, especially noticeable when providers have high latency or many models.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications. The change maintains the same authentication and authorization patterns while improving performance through parallelization.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: reorder migrations and set AllowAllKeys to true for virtual key provider configs (#2158)
## Summary
Fixes database migration ordering issue and ensures virtual key configurations are properly initialized with the AllowAllKeys field set to true.
## Changes
- Reordered database migrations to execute `migrationAddAllowAllKeysToProviderConfig` before `migrationBackfillEmptyVirtualKeyConfigs` to ensure the AllowAllKeys column exists before backfilling
- Added `AllowAllKeys: true` to provider configurations created during virtual key backfill migration to enable unrestricted key access by default
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that database migrations run successfully and virtual key configurations are created with proper defaults:
```sh
# Core/Transports
go version
go test ./...
```
Test migration ordering by running against a fresh database to ensure no column reference errors occur.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
This change enables unrestricted key access by default for virtual key configurations, which may have security implications depending on the intended access control model.
## 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
* feat: implement scoped pricing override
* refactor: custom pricing refactor
* fix: resolve merge conflicts in config loading and governance functions (#2230)
## Summary
Resolves Git merge conflicts in the bifrost-http configuration loading code by cleaning up duplicate function definitions and consolidating the configuration initialization flow.
## Changes
- Removed Git merge conflict markers and duplicate code blocks from `LoadConfig` function
- Consolidated governance configuration loading by keeping both `loadGovernanceConfigFromFile` and `loadGovernanceConfig` functions with distinct purposes
- Removed duplicate `convertSchemasMCPClientConfigToTable` function definition
- Moved pricing overrides initialization logic to `initFrameworkConfig` function for better organization
- Cleaned up extensive duplicate default configuration loading code that was causing merge conflicts
- Changed error handling for pricing overrides from returning error to logging warning
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that configuration loading works correctly without merge conflicts:
```sh
# Core/Transports
go version
go test ./...
go build ./transports/bifrost-http/...
```
Test configuration loading with various scenarios:
- Config file present
- Config file absent (default loading)
- Store-based configuration
- Governance and MCP configuration loading
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this is a merge conflict resolution that maintains existing functionality.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add Stability AI model support for Bedrock image generation (#2180)
## Summary
Adds support for Stability AI image generation models (stability.stable-image-*) to the Bedrock provider, enabling text-to-image generation with models like stability.stable-image-core-v1:1 and stability.stable-image-ultra-v1:1.
## Changes
- Added `isStabilityAIModel()` function to detect Stability AI models by "stability." prefix
- Created `ToStabilityAIImageGenerationRequest()` to convert Bifrost requests to Stability AI's flat request format
- Implemented `StabilityAIImageGenerationRequest` type with support for prompt, mode, aspect_ratio, output_format, seed, and negative_prompt parameters
- Added conditional routing in `ImageGeneration()` to use Stability AI request format when appropriate
- Extended known fields for image generation parameters to include "aspect_ratio" and "input_images"
- Updated documentation comment to reflect Stability AI model support
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test Stability AI image generation through the Bedrock provider:
```sh
# Core/Transports
go version
go test ./...
# Test with a Stability AI model
curl -X POST http://localhost:8080/v1/images/generations \
-H "Content-Type: application/json" \
-H "Authorization: Bearer your-key" \
-d '{
"model": "stability.stable-image-core-v1:1",
"prompt": "A beautiful sunset over mountains",
"aspect_ratio": "16:9",
"output_format": "PNG"
}'
```
Ensure AWS credentials are configured for Bedrock access and the Stability AI models are available in your region.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No additional security implications beyond existing Bedrock provider authentication and AWS credential handling.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add Stability AI image edit models support to Bedrock provider (#2225)
## Summary
Adds support for Stability AI image editing models in the Bedrock provider, expanding image editing capabilities beyond the existing Titan and Nova Canvas models.
## Changes
- Added `getStabilityAIEditTaskType()` function to infer edit task types from Stability AI model names (inpaint, outpaint, recolor, search-replace, erase-object, remove-bg, control-sketch, control-structure, style-guide, style-transfer, upscale-creative, upscale-conservative, upscale-fast)
- Created `ToStabilityAIImageEditRequest()` function to convert Bifrost requests to Stability AI's flat JSON format, with task-specific field validation
- Added `StabilityAIImageEditRequest` struct with comprehensive field support for all Stability AI edit operations
- Enhanced `BedrockImageGenerationResponse` with Seeds and FinishReasons fields for Stability AI compatibility
- Modified `ImageEdit()` method to route requests to appropriate conversion function based on model type
- Updated documentation to reflect expanded model support
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test with various Stability AI edit models through the Bedrock provider:
```sh
# Core/Transports
go version
go test ./...
# Test image editing with Stability AI models
# Example: stable-image-inpaint, stable-outpaint, stable-creative-upscale, etc.
```
Verify that task-specific parameters are correctly mapped and invalid fields are filtered out based on the detected task type.
## Screenshots/Recordings
N/A - Backend functionality only
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Image data is handled as base64-encoded strings. Mask and image parameters are properly validated before processing.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* fix: send back accumulated usage in MCP agent mode (#2246)
## Summary
This PR fixes token usage tracking in MCP agent mode by accumulating usage across all LLM calls in the agent loop and returning the total usage in the final response.
## Changes
- Added usage accumulation logic in the MCP agent execution loop to track token consumption across multiple LLM calls
- Implemented `mergeUsage` function to combine token counts and costs from multiple `BifrostLLMUsage` values, handling all detail sub-fields including prompt tokens, completion tokens, and cost breakdowns
- Extended agent API adapters with `extractUsage` and `applyUsage` methods to handle usage extraction and application for both Chat API and Responses API
- Applied accumulated usage to the final response before returning it to the client
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test MCP agent mode with multiple tool calls to verify usage accumulation:
```sh
# Core/Transports
go version
go test ./...
# Test MCP agent mode with multiple LLM calls
# Verify that the returned usage reflects the sum of all calls in the agent loop
# Check that both token counts and cost details are properly accumulated
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this change only affects usage tracking and reporting.
## 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
* [codemode]: fixing string escape corruption, enable top-level control flow in starlark, refining the prompt of executecode tool (#2206)
## Changes
- **Enhanced Starlark dialect configuration**: Enabled top-level control flow statements (if/for/while), while loops, set() builtin, global variable reassignment, and recursive functions for a more Python-like experience
- **Improved string escape handling**: Removed automatic `\n` to newline conversion, allowing Starlark's native string escape processing to handle `\n`, `\t`, and other escape sequences correctly
- **Updated tool description**: Streamlined the executeToolCode tool description with clearer syntax notes, explicit documentation of Starlark differences from Python (no try/except, no classes, no imports, no f-strings), and emphasis on fresh isolated scope per execution
- **Enhanced error hints**: Added specific error messages for unsupported Python features like try/except/finally/raise, with guidance on alternative approaches and scope persistence warnings
- **Comprehensive test coverage**: Added tests for dialect options, string escape preservation, unsupported feature detection, and end-to-end JSON deserialization scenarios
## Type of change
- [ ] Feature
- [ ] Bug fix
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go) - Starlark CodeMode improvements
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Test the enhanced Starlark features with MCP CodeMode:
```sh
# Test dialect options (top-level control flow, while loops, etc.)
make test-mcp TESTCASE=TestStarlarkDialectOptions
# Test string escape handling
make test-mcp PATTERN=TestStarlarkStringEscape
# Test unsupported feature detection
make test-mcp PATTERN=TestStarlarkUnsupportedFeatures
```
## Breaking changes
- [ ] Yes
- [x] No
The Starlark changes are additive and maintain backward compatibility while enabling more Python-like syntax.
## Security considerations
Starlark CodeMode maintains its existing sandboxing with no additional network or filesystem access. The dialect enhancements only affect language features within the existing security boundary.
* logging in plugins (#2215)
## Summary
Reorders middleware initialization in the Bifrost HTTP server to ensure tracing middleware is added before transport interceptor middleware in the inference pipeline.
## Changes
- Moved tracing middleware initialization and setup earlier in the bootstrap process
- Reordered middleware registration so tracing middleware is prepended before transport interceptor middleware
- Updated comments to clarify the middleware ordering logic and rationale
The change ensures that tracing context and trace IDs are properly established before other middleware components process requests.
## Type of change
- [x] Bug fix
- [ ] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that tracing middleware executes before transport interceptor middleware by checking trace logs and middleware execution order.
```sh
# Core/Transports
go version
go test ./...
```
Test with tracing enabled to ensure trace IDs are properly set in context before subsequent middleware processing.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
No security implications - this is a middleware ordering change that affects observability components.
## 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
* fix: handling text, vtt, srt response format for transcriptions (#2102)
* feat: add virtual key access management for MCP clients (#2255)
## Summary
Adds virtual key access management to MCP client configuration, allowing administrators to control which virtual keys can access specific MCP servers and which tools they can execute on a per-VK basis.
## Changes
- Added `vk_configs` field to MCP client update API that accepts an array of virtual key configurations
- Each VK config specifies a virtual key ID and the tools it's allowed to execute on that MCP server
- When `vk_configs` is provided, it atomically replaces all existing VK assignments for the MCP client
- Added database method `GetVirtualKeyMCPConfigsByMCPClientID` to retrieve VK configs by MCP client
- Updated OpenAPI documentation to describe the new VK configuration functionality
- Enhanced UI with virtual key access management section in the MCP client sheet
- Added Go SDK context keys for MCP tool filtering: `MCPContextKeyIncludeClients`, `MCPContextKeyIncludeTools`, and `BifrostContextKeyMCPExtraHeaders`
- Updated context keys documentation with comprehensive MCP configuration examples
## Type of change
- [x] Feature
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] UI (Next.js)
- [x] Docs
## How to test
1. Create an MCP client with tools available
2. Create virtual keys in the system
3. Update the MCP client with VK configurations:
```sh
curl -X PUT /api/mcp/client/{id} \
-H "Content-Type: application/json" \
-d '{
"name": "test-client",
"vk_configs": [
{
"virtual_key_id": "vk-123",
"tools_to_execute": ["*"]
},
{
"virtual_key_id": "vk-456",
"tools_to_execute": ["read_file", "write_file"]
}
]
}'
```
4. Verify VK assignments are created/updated in the database
5. Test the UI by opening an MCP client sheet and managing virtual key access
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
## Screenshots/Recordings
The UI now includes a "Virtual Key Access" section in the MCP client configuration sheet where administrators can:
- Add virtual keys to grant access to the MCP server
- Configure which specific tools each virtual key can execute
- Remove virtual key access entirely
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
This enables fine-grained access control for MCP servers at the virtual key level, complementing the existing governance and budgeting features.
## Security considerations
- VK access controls are enforced through the governance plugin during MCP tool execution
- The atomic replacement of VK assignments prevents partial updates that could leave the system in an inconsistent state
- Tool-level restrictions allow principle of least privilege by limiting which MCP tools each virtual key can access
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: adds option to allow MCP clients to run on all virtual keys (#2258)
## Summary
Adds a new `AllowOnAllVirtualKeys` configuration option for MCP clients that enables them to be accessible to all virtual keys without requiring explicit per-key assignment. When enabled, all tools from the MCP client are available to every virtual key.
## Changes
- Added `AllowOnAllVirtualKeys` boolean field to `MCPClientConfig` schema and database table
- Updated MCP client manager to handle the new field during client updates
- Modified governance plugin to check for clients with `AllowOnAllVirtualKeys` enabled and automatically include their tools for all virtual keys
- Added database migration to add the new column to `TableMCPClient`
- Updated UI to include a toggle for the new setting with tooltip explanation
- Added OpenAPI documentation for the new field
- Updated configuration store methods to persist and retrieve the new field
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
1. Create or update an MCP client with `allow_on_all_virtual_keys: true`
2. Verify that the client's tools are available to all virtual keys without explicit assignment
3. Test that the governance plugin correctly allows tools from such clients
4. Verify the UI toggle works correctly in the MCP client edit sheet
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
The new configuration field `allow_on_all_virtual_keys` defaults to `false` to maintain backward compatibility.
## Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
## Breaking changes
- [ ] Yes
- [x] No
This is a backward-compatible addition with the new field defaulting to `false`.
## Related issues
Link related issues and discussions. Example: Closes #123
## Security considerations
This feature reduces access control granularity by allowing MCP clients to bypass virtual key restrictions when enabled. Administrators should carefully consider which MCP clients should have this permission as it grants broad access across all virtual keys.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* feat: add provider keys CRUD to configstore and in-memory store (#2159)
## Summary
Adds dedicated CRUD operations for individual provider keys at the data layer
(configstore interface + RDB implementation) and in-memory store. This enables
key-level operations without replacing the entire provider key set, which is
required for the new `/api/providers/{provider}/keys/*` endpoints.
## Changes
- Added `GetProviderKeys`, `GetProviderKey`, `CreateProviderKey`,
`UpdateProviderKey`, `DeleteProviderKey` to `ConfigStore` interface
- Implemented all five methods in `RDBConfigStore` with proper GORM queries,
error handling, and `ErrNotFound` propagation
- Extracted `schemaKeyFromTableKey` and `tableKeyFromSchemaKey` helpers to
deduplicate key conversion logic (previously inlined in `GetProvidersConfig`
and `GetProviderConfig`)
- Added `AddProviderKey`, `UpdateProviderKey`, `RemoveProviderKey` to in-memory
`Config` with mutex locking, DB persistence, and rollback on client update
failure
- Added `GetProviderKeysRaw`, `GetProviderKeysRedacted`, `GetProviderKeyRaw`,
`GetProviderKeyRedacted` read methods
- Added `TestProviderKeyCRUD` and `TestProviderKeyCRUD_ProviderMustExist`
integration tests
- Updated `MockConfigStore` with all five new interface methods
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Run configstore tests
go test ./framework/configstore/... -v -run TestProviderKeyCRUD
# Run config tests (mock store)
go test ./transports/bifrost-http/lib/... -v
```
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
Key values are handled through existing redaction infrastructure. No new secret
exposure paths introduced.
## 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)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: add provider keys HTTP handlers and refactor optional keys (#2160)
## Summary
Adds HTTP handlers for the dedicated provider keys CRUD endpoints and removes
`keys` from provider API responses and payloads. Keys are now exclusively
managed via `/api/providers/{provider}/keys/*`. Also fixes a context timeout bug
in `ReloadProvider` where model discovery could exhaust the shared context
budget, causing subsequent DB calls to fail.
## Changes
### Provider keys handlers (`provider_keys.go`)
- New file with five handlers: `listProviderKeys`, `getProviderKey`,
`createProviderKey`, `updateProviderKey`, `deleteProviderKey`
- Includes `mergeUpdatedKey` (redacted value preservation logic used by
`updateProviderKey`)
- Key handlers enforce keyless provider validation and trigger model discovery
after mutations
### Provider handlers cleanup (`providers.go`)
- Registered new key routes: `GET/POST /api/providers/{provider}/keys`,
`GET/PUT/DELETE /api/providers/{provider}/keys/{key_id}`
- Extracted inline anonymous structs into named `providerCreatePayload` and
`providerUpdatePayload` types (without `Keys` field)
- Removed `Keys` field from `ProviderResponse`
- Switched `addProvider` from `json.Unmarshal` to `sonic.Unmarshal`
- Removed `oldConfigRedacted` fetch and the entire key merge block
(`mergeKeys`, `hasKeys`, `slices` usage) from `updateProvider`
- Removed `Keys` from `getProviderResponseFromConfig` response builder
- Removed unused `encoding/json` import
### Context timeout fix (`server.go`)
- Split shared `bfCtx` in `ReloadProvider` into separate contexts:
`filteredBfCtx` (15s) for filtered `ListModelsRequest` and `unfilteredBfCtx`
(fresh 15s) for unfiltered `ListModelsRequest`, each cancelled after use
- Changed `GetKeysByProvider` to use `context.Background()` since it's a local
DB call that shouldn't be gated by model discovery timeouts
- Added `hasNoKeys` check to emit warn-level logs instead of errors when model
discovery fails because no keys are configured
- Read in-memory key count via `GetProviderKeysRaw` for the `hasNoKeys` check
### Tests (`providers_test.go`)
- Cleared file (contained only tests for removed inline struct decoding)
## Type of change
- [x] Feature
- [x] Bug fix
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
```sh
# Build
go build ./transports/bifrost-http/...
# Manual: start Bifrost, then test key CRUD
curl -X POST localhost:8080/api/providers/openai/keys -d '{"name":"test-key","value":"sk-test"}'
curl localhost:8080/api/providers/openai/keys
curl -X PUT localhost:8080/api/providers/openai/keys/{key_id} -d '{"name":"updated","value":"sk-new"}'
curl -X DELETE localhost:8080/api/providers/openai/keys/{key_id}
# Verify provider endpoints no longer return keys
curl localhost:8080/api/providers/openai | jq 'has("keys")' # should be false
```
## Screenshots/Recordings
N/A
## Breaking changes
- [x] Yes
- [ ] No
Provider API responses no longer include `keys` field. Provider create/update
payloads no longer accept `keys`. Clients must use the dedicated
`/api/providers/{provider}/keys/*` endpoints for key management.
## Related issues
N/A
## Security considerations
- Key handlers use existing redaction infrastructure (`GetProviderKeyRedacted`)
before returning responses
- Keyless provider validation prevents key creation on providers that don't
support keys
## 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)
- [ ] I verified the CI pipeline passes locally if applicable
* feat: migrate frontend to dedicated provider keys API (#2161)
## Summary
Migrates the frontend from reading provider keys via `provider.keys` (removed
from provider API response in PR #2160) to the dedicated `getProviderKeys`
query and `/api/keys` endpoint. Removes `keys` from all provider TypeScript
types. Key mutations patch caches from authoritative server responses; provider
updates invalidate the `ProviderKeys` tag to refresh key statuses after model
discovery. Also adds a read-only routing rule info sheet.
## Changes
### Types (`config.ts`, `schemas.ts`)
- Removed `keys` field from `ModelProviderConfig`, `AddProviderRequest`, and
`UpdateProviderRequest`
- Added `CreateProviderKeyRequest`, `UpdateProviderKeyRequest`,
`ListProviderKeysResponse` types
### Store (`providersApi.ts`, `baseApi.ts`)
- Added `ProviderKeys` tag type to `baseApi`
- Changed `getProviderKeys`/`getProviderKey` from `Providers` tag to
`ProviderKeys` tag (avoids invalidating provider cache on key changes)
- Added `invalidatesTags: [ProviderKeys, DBKeys]` on `updateProvider` mutation
(refreshes key statuses after model discovery)
- Removed `getProvider`/`getProviders` cache patches from `createProviderKey`,
`updateProviderKey`, `deleteProviderKey` (providers no longer carry keys)
- Added duplicate-check guards on `createProviderKey` cache patches to prevent
ghost keys
- Each key mutation patches `getProviderKeys` and `getAllKeys` caches from
authoritative server response
### Components
- **`modelProviderKeysTableView.tsx`**: Already uses `useGetProviderKeysQuery`;
formatting/indentation fixes
- **`page.tsx`**: Removed `keys: []` from fallback provider object and
`createProvider` call; simplified `KeyDiscoveryFailedBadge` to only check
provider-level status (removed per-key status check since keys are no longer
on provider)
- **`routingRuleSheet.tsx`**: `TargetRow` now receives `allKeys` prop (from
`useGetAllKeysQuery`) instead of `providersData` with `.keys`; filters keys
by target provider
- **`routingRuleInfoSheet.tsx`**: New read-only sheet component that displays
routing rule details (conditions, targets with provider icons and weight bars,
fallback chain, scope, priority, timestamps)
- **`settingsPanel.tsx`**: Uses `useGetAllKeysQuery` to determine configured
providers (replaces `p.keys.length > 0` check) and derive
`providerKeyConfigs` per provider
### Other frontend changes (from prior commit, unchanged)
- Added `getProviderKeys`, `getProviderKey` RTK Query endpoints
- Added `createProviderKey`, `updateProviderKey`, `deleteProviderKey` mutations
- Added `buildProviderUpdatePayload` utility for key-free provider updates
- Migrated `providerKeyForm.tsx` to separate create/update mutations
- Updated `addNewKeySheet.tsx` props from `keyIndex` to `keyId`
- Updated all 6 provider form fragments to use `buildProviderUpdatePayload`
- Removed dead `selectedProvider.keys` sync matchers from `providerSlice.ts`
## Type of change
- [x] Feature
- [x] Refactor
- [ ] Bug fix
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
```sh
cd ui
npm run build
npm run lint
```
Manual testing:
1. Navigate to Providers page, select a provider with keys
2. Verify keys table loads correctly from dedicated API
3. Create a new key — verify it appears immediately (no ghost/duplicate)
4. Toggle enable/disable — verify switch updates immediately
5. Edit a key — verify form pre-populates, save works
6. Delete a key — verify it disappears immediately
7. Update provider settings — verify key statuses refresh after save
8. Check sidebar badge shows provider-level discovery failures
9. Open Playground settings — verify provider/key dropdowns work
10. Open Routing Rules — verify target key selector works
11. Click a routing rule row — verify info sheet opens with correct details
(conditions, targets, fallbacks, scope, priority)
## Screenshots/Recordings
N/A — no visual changes to existing features; routing rule info sheet is new.
## Breaking changes
- [ ] Yes
- [x] No
Frontend-only changes consuming the new API shape from PR #2160.
## Related issues
N/A
## Security considerations
No new security considerations. Key values continue to be handled through
existing redaction on the backend.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [ ] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
* refactor: replace string slice with WhiteList type for model restrictions (#2282)
## Summary
Refactored model access control logic by replacing string slice with a dedicated `WhiteList` type for the `Models` field in `TableKey`. This change introduces a more structured approach to handling wildcard permissions and improves code readability.
## Changes
- Changed `Models` field type from `[]string` to `schemas.WhiteList` in `TableKey` struct
- Replaced manual wildcard checking (`model == "*"`) with `IsUnrestricted()` method calls across multiple functions
- Added missing mock method `GetVirtualKeyMCPConfigsByMCPClientIDs` to test configuration store
- Applied the refactoring consistently in `ReloadProvider`, `ForceReloadPricing`, and `Bootstrap` methods
## Type of change
- [x] Refactor
- [ ] Bug fix
- [ ] Feature
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that model access control continues to work correctly with both wildcard and specific model permissions:
```sh
# Core/Transports
go version
go test ./...
# Test specific areas affected by the changes
go test ./framework/configstore/tables/...
go test ./transports/bifrost-http/...
```
Test scenarios should include:
- Keys with wildcard permissions (`["*"]`)
- Keys with specific model restrictions
- Keys with empty model lists (deny-by-default behavior)
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
This refactoring maintains the existing security model for API key permissions. The deny-by-default behavior and wildcard functionality remain unchanged, just implemented through a more structured type system.
## 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
* feat: add Plus icon and responsive text to pricing override create button (#2285)
## Summary
Improves the visual design and mobile responsiveness of the pricing overrides section by adding a Plus icon to the create button and optimizing the button text for different screen sizes.
## Changes
- Added Plus icon import from lucide-react
- Enhanced the "Create Override" button with a Plus icon and responsive text that shows "New Override" on larger screens and hides text on mobile
- Adjusted container spacing by removing top margin and changing flex alignment from `items-start` to `items-center` for better visual balance
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [ ] Docs
## How to test
Navigate to the custom pricing overrides page and verify:
1. The "New Override" button displays with a Plus icon
2. On mobile screens, only the Plus icon is visible
3. On larger screens (sm and above), both icon and "New Override" text are visible
4. The button functionality remains unchanged when clicked
```sh
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
## Screenshots/Recordings
Before/after screenshots showing the button design changes and responsive behavior would be helpful.
## Breaking changes
- [x] Yes
- [ ] No
## Related issues
## Security considerations
No security implications - this is a purely visual enhancement.
## 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
* refactor: blacklist models on new convention (#2305)
## Summary
Implements comprehensive blacklist support for model filtering across all providers. This adds the ability to explicitly deny access to specific models at the key level, with blacklist rules taking precedence over allowlist rules.
## Changes
- Added `BlackList` type with semantic validation (supports wildcard "*" for block-all)
- Updated key selection logic to check both allowlist and blacklist constraints
- Modified all provider model listing functions to filter out blacklisted models
- Enhanced UI to support blacklist configuration with improved UX for wildcard selection
- Added blacklist filtering to model catalog and provider handlers
- Updated test cases to verify blacklist functionality
Key design decisions:
- Blacklist always wins over allowlist when conflicts occur
- Wildcard "*" in blacklist blocks all models for that key
- Empty blacklist blocks nothing (permissive default)
- Consistent filtering logic across all providers (Anthropic, Azure, Bedrock, Cohere, etc.)
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [x] Providers/Integrations
- [ ] Plugins
- [x] UI (Next.js)
- [x] Docs
## How to test
Test blacklist functionality with provider keys:
```sh
# Core/Transports
go version
go test ./...
# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build
```
Example configuration to test:
```json
{
"keys": [{
"id": "test-key",
"models": ["*"],
"blacklisted_models": ["gpt-4", "claude-3"]
}]
}
```
Verify that blacklisted models are excluded from model listings and key selection.
## Screenshots/Recordings
UI now shows "Blocked Models" field with improved tooltips and wildcard handling for denying access to specific models.
## Breaking changes
- [ ] Yes
- [x] No
The `blacklisted_models` field was already present in the schema but not fully implemented. This change makes it functional without breaking existing configurations.
## Related issues
Enhances model access control capabilities for fine-grained permission management.
## Security considerations
Improves security by allowing explicit denial of access to sensitive or expensive models at the key level. Blacklist rules cannot be bypassed by allowlist configurations.
## Checklist
- [x] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [x] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [x] I verified the CI pipeline passes locally if applicable
* minor fix add blacklisted model field in tableKeyFromSchemaKey (#2324)
## Summary
This PR adds support for the `BlacklistedModels` field when converting schema keys to table keys in the configuration store's RDB implementation.
## Changes
- Added `BlacklistedModels: key.BlacklistedModels` field mapping in the `tableKeyFromSchemaKey` function
- Ensures that blacklisted model information is properly preserved when converting between schema and table representations
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (Next.js)
- [ ] Docs
## How to test
Verify that configuration keys with blacklisted models are properly stored and retrieved from the RDB configstore.
```sh
# Core/Transports
go version
go test ./...
```
Test creating configuration entries with `BlacklistedModels` specified and ensure they persist correctly through the RDB layer.
## Screenshots/Recordings
N/A
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
N/A
## Security considerations
None - this change only adds field mapping for existing blacklisted models functionality.
## 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
* feat: add image edit input view on logs (#2321)
## Summary
Adds support for logging image edit and image variation requests by introducing new database columns and UI components to track and display these image manipulation operations alongside existing image generation functionality.
## Changes
- Added `image_edit_input` and `image_variation_input` columns to the logs table with corresponding database migrations
- Extended the Log struct with new fields for storing and parsing image edit/variation input data
- Updated logging plugin to capture image edit and variation request data with large payload threshold handling
- Enhanced UI to display input images and prompts for image edit operations and input images for variation operations
- Added image MIME type detection for proper display of base64-encoded images in the UI
## Type of change
- [x] Feature
- [ ] Bug fix
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (Next.js)
- [ ] Do…
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
Introduces a new
WhiteListtype to standardize whitelist behavior across the codebase, replacing manual slice operations and string comparisons with semantic methods for handling allow/deny lists.Changes
WhiteListtype with methodsIsAllowed(),IsUnrestricted(),IsEmpty(),Contains(), andValidate()[]stringfields withWhiteListfor model restrictions, tool filtering, and key access controlsslices.Contains()checksType of change
Affected areas
How to test
Verify that whitelist behavior remains consistent across all affected components:
Test key model filtering, MCP tool execution, and virtual key configurations to ensure whitelist logic works correctly.
Screenshots/Recordings
N/A
Breaking changes
The
WhiteListtype maintains the same JSON serialization format as[]string, so existing configurations remain compatible.Related issues
N/A
Security considerations
Improves security by standardizing deny-by-default behavior and adding validation to prevent misconfigured whitelists that could inadvertently grant excessive permissions.
Checklist
docs/contributing/README.mdand followed the guidelines