adds allowlist and denylist for headers #1178
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a global header filtering feature: new schema and types (allowlist/denylist), DB persistence and store APIs, runtime loading and hot-reload, request-context filtering integration (ConvertToBifrostContext), UI/schema/types for editing, and test/mock updates. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (ClientSettings)
participant API as Bifrost HTTP API
participant Server as BifrostHTTPServer
participant Store as ConfigStore (DB)
Note over UI,API: Admin updates header_filter_config via UI
UI->>API: PUT /api/config (header_filter_config)
API->>Server: updateConfig(...) with new header_filter_config
alt config changed
Server->>Server: call ReloadHeaderFilterConfig(ctx, config)
Server->>Store: UpdateHeaderFilterConfig(ctx, config)
Store-->>Server: OK
Server-->>API: 200 OK
else no change
Server-->>API: 200 OK (no reload)
end
Note over RequestFlow: Incoming request uses current config
participant Req as Client Request
Req->>API: HTTP request
API->>Server: ConvertToBifrostContext(ctx, allowDirectKeys, headerFilterConfig)
Server->>Server: shouldAllowHeader() checks (allowlist/denylist + securityDenylist)
Server-->>Req: process request with filtered forwarded headers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
transports/bifrost-http/lib/config.go (1)
39-41: Expose header filter via HandlerStore; consider future-proofing concurrent accessWiring
GetHeaderFilterConfigthroughHandlerStoreand implementing it onConfigby returningc.ClientConfig.HeaderFilterConfiggives the router/context code clean read-only access, and mirrors the existingShouldAllowDirectKeyspattern.If you expect frequent runtime reloads of the header filter config, it may be worth eventually backing
HeaderFilterConfigwith anatomic.Pointeror guarding writes with the existing mutex to keep the race detector quiet while still allowing cheap reads from hot paths. Not a blocker given the existing pattern, just something to consider.Also applies to: 2065-2071
transports/config.schema.json (1)
84-103: Clarify header_filter_config header name format (case and prefix).Runtime filtering (in
ConvertToBifrostContext) lowercases header names and strips thex-bf-eh-prefix before checking allowlist/denylist, but this schema’s descriptions don’t say that. If users configure"User-Agent"or include the prefix, those entries will never match the normalized"user-agent"label, and filtering will appear ineffective.Consider either:
- documenting explicitly that entries must be lowercase header names without the
x-bf-eh-prefix (e.g."user-agent"), or- normalizing the configured allowlist/denylist to lowercase at load time so configuration is case-insensitive.
This will avoid subtle misconfigurations and align user expectations with the actual filter behavior.
ui/lib/types/schemas.ts (1)
680-691: Consider normalizing and deduplicating header names in the Zod schemas.The schemas correctly mirror the backend shape, but you may want to harden UX here:
- Normalize to lowercase so config is case-insensitive and matches the runtime behavior that already lowercases header names before filtering.
- Optionally deduplicate entries to avoid redundant values.
For example:
export const globalHeaderFilterConfigSchema = z.object({ allowlist: z .array(z.string().min(1)) .transform(list => Array.from(new Set(list.map(h => h.toLowerCase())))) .optional(), denylist: z .array(z.string().min(1)) .transform(list => Array.from(new Set(list.map(h => h.toLowerCase())))) .optional(), });This keeps the API the same but makes misconfiguration much harder.
Also applies to: 709-710
transports/bifrost-http/handlers/inference.go (1)
449-451: Header filter wiring into ConvertToBifrostContext is correct but quite repetitive.All the updated call sites correctly pass
h.handlerStore.ShouldAllowDirectKeys()andh.config.GetHeaderFilterConfig()intoConvertToBifrostContext, so behavior-wise this looks good.Given how often this pattern appears, consider adding a small helper on
CompletionHandler, e.g.:func (h *CompletionHandler) bifrostContext(ctx *fasthttp.RequestCtx) (*context.Context, context.CancelFunc) { return lib.ConvertToBifrostContext(ctx, h.handlerStore.ShouldAllowDirectKeys(), h.config.GetHeaderFilterConfig()) }and then reusing it across handlers. That will make future changes to context creation (additional flags, config, etc.) much easier and less error-prone.
Also applies to: 573-574, 649-650, 731-732, 803-804, 879-880, 1010-1011, 1096-1097, 1395-1396, 1447-1449, 1487-1488, 1526-1527, 1565-1566, 1647-1648, 1706-1707, 1745-1746, 1784-1785, 1823-1824
transports/bifrost-http/handlers/config.go (1)
648-701: Consider deduplicating and filtering empty entries server-side.While the frontend filters empty strings before submission, the backend should also validate to ensure data integrity. Additionally, consider deduplicating entries in the allowlist/denylist to prevent user confusion.
🔎 Proposed enhancement for input sanitization
// Normalize header names to lowercase for case-insensitive matching + seen := make(map[string]bool) + var cleanedAllowlist []string for i, header := range payload.Allowlist { - payload.Allowlist[i] = strings.ToLower(strings.TrimSpace(header)) + normalized := strings.ToLower(strings.TrimSpace(header)) + if normalized != "" && !seen[normalized] { + cleanedAllowlist = append(cleanedAllowlist, normalized) + seen[normalized] = true + } } + payload.Allowlist = cleanedAllowlist + + seen = make(map[string]bool) + var cleanedDenylist []string for i, header := range payload.Denylist { - payload.Denylist[i] = strings.ToLower(strings.TrimSpace(header)) + normalized := strings.ToLower(strings.TrimSpace(header)) + if normalized != "" && !seen[normalized] { + cleanedDenylist = append(cleanedDenylist, normalized) + seen[normalized] = true + } } + payload.Denylist = cleanedDenylistui/app/workspace/config/views/headerFilterView.tsx (1)
30-47: Consider improving type safety for useFieldArray.The
as nevertype casts suggest a type mismatch between the form schema and useFieldArray expectations. This is a common pattern when using useFieldArray with primitive arrays (string[]) rather than object arrays.For better type safety, you could wrap the header strings in objects:
// In schema: allowlist: z.array(z.object({ value: z.string() })).optional() // In useFieldArray: name: "allowlist" // No cast neededHowever, since this works correctly at runtime and the current approach is common with react-hook-form, this is a minor improvement that can be deferred.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/header-filter/page.tsxui/app/workspace/config/views/headerFilterView.tsxui/components/sidebar.tsxui/lib/store/apis/baseApi.tsui/lib/store/apis/configApi.tsui/lib/types/config.tsui/lib/types/schemas.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/app/workspace/config/header-filter/page.tsxtransports/config.schema.jsonui/lib/types/config.tsui/lib/types/schemas.tstransports/bifrost-http/server/server.goui/components/sidebar.tsxtransports/bifrost-http/handlers/config.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/lib/ctx.goframework/configstore/clientconfig.goframework/configstore/store.goui/app/workspace/config/views/headerFilterView.tsxtransports/bifrost-http/handlers/inference.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/integrations/router.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/mcp.goframework/configstore/rdb.goui/lib/store/apis/configApi.tsui/lib/store/apis/baseApi.ts
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/lib/ctx.goframework/configstore/clientconfig.goframework/configstore/store.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/integrations/router.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/mcp.goframework/configstore/rdb.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/handlers/mcp.go
🧬 Code graph analysis (13)
ui/app/workspace/config/header-filter/page.tsx (1)
ui/app/workspace/config/views/headerFilterView.tsx (1)
HeaderFilterView(18-257)
ui/lib/types/config.ts (1)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)
transports/bifrost-http/server/server.go (3)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
transports/bifrost-http/handlers/config.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/integrations/bedrock_test.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
framework/configstore/clientconfig.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
framework/configstore/store.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
transports/bifrost-http/integrations/router.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
framework/configstore/tables/config.go (4)
core/providers/gemini/types.go (1)
Type(783-783)ui/lib/types/config.ts (1)
GlobalProxyType(269-269)core/network/http.go (1)
GlobalProxyType(46-46)core/schemas/provider.go (1)
NoProxy(147-147)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
ui/lib/store/apis/configApi.ts (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (27)
ui/lib/store/apis/baseApi.ts (1)
138-150: HeaderFilterConfig RTK Query tag wiring looks correctAdding
"HeaderFilterConfig"totagTypesis consistent with the new config endpoints and should allow proper cache invalidation with no behavioral regressions.ui/components/sidebar.tsx (1)
3-34: New “Header Filter” config nav entry is consistent and well-scopedImporting
Filterand adding the “Header Filter” sub-item under Config, gated byhasSettingsAccessand routed to/workspace/config/header-filter, aligns with existing sidebar patterns and RBAC usage.Also applies to: 532-538
transports/bifrost-http/lib/config.go (1)
385-388: HeaderFilterConfig load/merge and persistence pipeline is coherentThe way you:
- merge
HeaderFilterConfigwith DB-precedence inmergeClientConfig,- sync any file-/client-derived
HeaderFilterConfiginto the store inloadClientConfigFromFile, and- then hydrate
ClientConfig.HeaderFilterConfigfrom the store vialoadDefaultHeaderFilterConfig(for both file-backed and default paths)keeps a single source of truth in the config store while still supporting initial config.json defaults and the “no store” mode. This matches how other config sections are handled.
Also applies to: 501-507, 548-559, 1578-1580, 1763-1773
transports/bifrost-http/integrations/bedrock_test.go (1)
9-10: mockHandlerStore correctly updated for new HandlerStore contractExtending
mockHandlerStorewith aheaderFilterConfigfield andGetHeaderFilterConfigimplementation, plus the added tables import, cleanly aligns the tests with the newlib.HandlerStoreinterface without changing existing test behavior (nil config → default filtering).Also applies to: 17-19, 25-27
transports/bifrost-http/integrations/router.go (1)
392-399: Router now passes headerFilterConfig into ConvertToBifrostContext as intendedUsing
g.handlerStore.GetHeaderFilterConfig()alongsideShouldAllowDirectKeys()when callingConvertToBifrostContextcleanly threads the global header filtering rules into every integration route without touching downstream handler logic.ui/app/workspace/config/header-filter/page.tsx (1)
1-11: HeaderFilterPage wiring is minimal and consistent with other config pagesClient-side
HeaderFilterPagesimply wrapsHeaderFilterViewin the usualmax-w-7xllayout and lives at the path referenced by the new sidebar item, which is exactly what’s needed here.framework/configstore/clientconfig.go (1)
37-51: ClientConfig field and hash integration for HeaderFilterConfig look consistent.The new
HeaderFilterConfigfield and its inclusion inGenerateClientConfigHashare wired correctly: allowlist/denylist are sorted before hashing for determinism, and nil vs empty lists intentionally have no hash impact, matching their identical runtime behavior.No changes needed here.
Also applies to: 144-170
transports/bifrost-http/handlers/mcp.go (1)
53-73: MCP tool handler correctly adopts header filter–aware context.Using
ConvertToBifrostContext(ctx, false, h.store.GetHeaderFilterConfig())keeps MCP requests opt-out of direct keys while still applying the global header filter configuration. This is consistent with the rest of the stack and doesn’t introduce new behavior quirks.Looks good as-is.
framework/configstore/store.go (1)
122-129: ConfigStore interface extension for header filter config is consistent; ensure all impls are updated.Adding
GetHeaderFilterConfig/UpdateHeaderFilterConfighere fits the existing pattern for global config (proxy, restart, auth, etc.) and matches thetables.GlobalHeaderFilterConfigtype.Just make sure every concrete
ConfigStoreimplementation and any mocks (e.g., sqlite/postgres stores, in-memory test doubles) implement these methods so the stack compiles and tests continue to pass.ui/lib/types/config.ts (1)
302-313: TS config types for global header filter match backend and defaults correctly.
GlobalHeaderFilterConfigandDefaultGlobalHeaderFilterConfigalign withtables.GlobalHeaderFilterConfig, and wiring it intoBifrostConfigviaheader_filter_config?: GlobalHeaderFilterConfigmaintains backward compatibility (field is optional) while exposing the new capability to the UI.No changes needed here.
Also applies to: 321-329
framework/configstore/rdb.go (1)
1952-1981: LGTM! Methods follow established patterns.The
GetHeaderFilterConfigandUpdateHeaderFilterConfigmethods are consistent with existing governance config methods likeGetProxyConfigandGetRestartRequiredConfig. The error handling, nil checks, and JSON marshalling patterns align well with the codebase conventions.framework/configstore/tables/config.go (2)
40-49: LGTM! Well-documented struct with clear filter logic.The
GlobalHeaderFilterConfigstruct is well-designed with clear documentation explaining the filter precedence logic. The JSON tags withomitemptyallow for flexible partial configuration.
12-12: LGTM!The constant follows the established naming convention for governance config keys.
transports/bifrost-http/server/server.go (2)
841-856: LGTM! Clean hot-reload implementation.The method correctly stores the header filter config in memory and provides useful debug logging. Unlike proxy configuration, header filter changes take effect immediately without requiring a restart, which is appropriate since the filter is applied per-request.
66-66: LGTM!The interface extension is consistent with the existing callback pattern.
ui/lib/store/apis/configApi.ts (2)
90-106: LGTM! RTK Query endpoints follow established patterns.The new endpoints correctly mirror the existing proxy config pattern. The cache invalidation strategy invalidating both
HeaderFilterConfigandConfigtags ensures the UI stays synchronized when header filter settings change.
124-125: The hook nameuseUpdateHeaderFilterConfigMutationis correct and consistent throughout the codebase. No typo exists.Likely an incorrect or invalid review comment.
transports/bifrost-http/handlers/config.go (2)
626-645: LGTM! Handler follows established patterns.The
getHeaderFilterConfighandler correctly returns empty arrays when config is nil, providing a consistent response structure for the frontend.
163-169: LGTM!Header filter config is correctly included in the aggregated config response, consistent with how proxy config is handled.
ui/app/workspace/config/views/headerFilterView.tsx (4)
49-59: LGTM! Proper form synchronization with fetched data.The useEffect correctly resets the form when server data is fetched, ensuring arrays are properly initialized even if the server returns undefined values.
61-73: LGTM! Clean data sanitization before submission.The onSubmit handler correctly filters out empty strings before sending to the API, preventing invalid entries from being persisted.
114-126: LGTM! Clear and informative filter precedence documentation.The alert clearly explains the filter logic to users, which matches the implementation in the backend. This is excellent UX design.
239-251: LGTM! Security transparency.Displaying the list of always-blocked headers provides good transparency to users about the security measures in place.
transports/bifrost-http/lib/ctx.go (4)
100-116: LGTM! Security denylist is comprehensive.The security denylist correctly blocks sensitive headers that could be exploited:
- Authentication headers (authorization, proxy-authorization, cookie)
- Connection headers (host, content-length, connection, transfer-encoding)
- API key headers that could override governance controls
118-145: LGTM! Filter logic implementation is correct.The
shouldAllowHeaderfunction correctly implements the documented precedence:
- Nil config → allow all
- Allowlist check (if present) → must be in list
- Denylist check (if present) → must not be in list
- Both empty → allow all
The implementation matches the documentation in
GlobalHeaderFilterConfigand the UI explanation.
287-294: LGTM! Filter application is in the correct order.The security denylist is checked first (always enforced), followed by the configurable filter. This ensures security-sensitive headers cannot be forwarded even if mistakenly added to an allowlist.
80-80: All callers have been properly updated. All 21 call sites across router.go, mcp.go, and inference.go pass the required three arguments: context, allowDirectKeys flag, and headerFilterConfig.
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
ba75398 to
a65444e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ui/app/workspace/config/views/clientSettingsView.tsx (3)
30-45: Consider order-insensitive comparison for header filter config.The current comparison is order-sensitive, meaning
["a", "b"]and["b", "a"]are considered different. While this works for tracking local edits, it could cause false "unsaved changes" detection if the server returns headers in a different order than stored locally.🔎 Proposed order-insensitive comparison
function headerFilterConfigEqual (a?: GlobalHeaderFilterConfig, b?: GlobalHeaderFilterConfig): boolean { const aAllowlist = a?.allowlist || [] const bAllowlist = b?.allowlist || [] const aDenylist = a?.denylist || [] const bDenylist = b?.denylist || [] if (aAllowlist.length !== bAllowlist.length || aDenylist.length !== bDenylist.length) { return false } + const sortedAAllow = [...aAllowlist].sort() + const sortedBAllow = [...bAllowlist].sort() + const sortedADeny = [...aDenylist].sort() + const sortedBDeny = [...bDenylist].sort() + return ( - aAllowlist.every((v, i) => v === bAllowlist[i]) && - aDenylist.every((v, i) => v === bDenylist[i]) + sortedAAllow.every((v, i) => v === sortedBAllow[i]) && + sortedADeny.every((v, i) => v === sortedBDeny[i]) ) }
84-99: Consider adding a defensive check before accessingbifrostConfig.The non-null assertion
bifrostConfig!on line 94 is technically safe due to the button's disabled logic, but an explicit guard makes the code more robust against future refactoring.🔎 Proposed defensive check
const handleSave = useCallback(async () => { + if (!bifrostConfig) return try { // Clean up empty strings from header filter config const cleanedConfig = { ...localConfig, header_filter_config: { allowlist: (localConfig.header_filter_config?.allowlist || []).filter((h) => h && h.trim().length > 0), denylist: (localConfig.header_filter_config?.denylist || []).filter((h) => h && h.trim().length > 0), }, } - await updateCoreConfig({ ...bifrostConfig!, client_config: cleanedConfig }).unwrap() + await updateCoreConfig({ ...bifrostConfig, client_config: cleanedConfig }).unwrap() toast.success("Client settings updated successfully.") } catch (error) { toast.error(getErrorMessage(error)) } }, [bifrostConfig, localConfig, updateCoreConfig])
258-278: Usingindexaskeyfor dynamic lists may cause subtle issues.When items are removed from the middle of the list, React may preserve DOM state incorrectly because keys shift. For controlled inputs this usually works, but could cause issues with focus or animations. Consider using a stable identifier if problems arise.
One approach would be to generate unique IDs when adding headers:
// Instead of storing string[], store { id: string, value: string }[] // Then use key={item.id} in the mapFor now, since these are simple controlled inputs and the list is typically small, this is acceptable.
Also applies to: 302-322
transports/bifrost-http/handlers/config.go (1)
25-35: Header filter config wiring is correct; consider order-insensitive equalityThe new pieces here hang together well:
ConfigManagergainsReloadHeaderFilterConfig, andupdateConfiginvokes it only when the client’sHeaderFilterConfigactually differs.getConfigexposesheader_filter_configfrom the store alongside existing config sections, which is what the UI and API need.One behavioral nuance:
headerFilterConfigEqualusesslices.Equaldirectly, so allowlist/denylist comparisons are order‑sensitive and treatnilvs non‑nilGlobalHeaderFilterConfigas different even when both lists are empty. Since the filter semantics are set‑like (order doesn’t matter, and “no headers configured” vs “empty lists” behave the same inConvertToBifrostContext), you might want to normalize before comparison (e.g., sort and/or treat empty configs as equivalent) to avoid unnecessary reloads when only list ordering changes.If you’re fine with “any structural difference triggers a reload,” the current implementation is otherwise sound.
Also applies to: 161-167, 176-176, 188-192, 223-225, 276-285, 633-642
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- framework/configstore/tables/config.go
- framework/configstore/rdb.go
- transports/bifrost-http/lib/config.go
- ui/lib/types/config.ts
- transports/bifrost-http/integrations/bedrock_test.go
- transports/config.schema.json
- ui/lib/types/schemas.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
framework/configstore/clientconfig.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.goui/app/workspace/config/views/clientSettingsView.tsxframework/configstore/store.gotransports/bifrost-http/server/server.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
framework/configstore/clientconfig.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.goframework/configstore/store.gotransports/bifrost-http/server/server.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/mcp.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/server/server.go
🧬 Code graph analysis (7)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
transports/bifrost-http/handlers/config.go (3)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/handlers/inference.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
transports/bifrost-http/integrations/router.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
ui/app/workspace/config/views/clientSettingsView.tsx (4)
ui/lib/types/config.ts (3)
CoreConfig(336-350)DefaultGlobalHeaderFilterConfig(310-313)GlobalHeaderFilterConfig(304-307)ui/app/_fallbacks/enterprise/lib/contexts/rbacContext.tsx (1)
useRbac(63-65)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(174-199)ui/components/ui/input.tsx (1)
Input(15-69)
framework/configstore/store.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (14)
ui/app/workspace/config/views/clientSettingsView.tsx (3)
101-164: LGTM!The header filter list handlers are well-structured with proper memoization via
useCallback. The lowercase normalization on input change aligns with the security requirements mentioned in the PR objectives.
166-221: LGTM!The component layout and toggle sections are well-structured. RBAC checks are consistently applied to disable controls when the user lacks update permissions. The conditional display of dropped request count is a nice UX touch.
223-349: LGTM!The header forwarding section provides excellent documentation through the Alert components, clearly explaining the allowlist/denylist behavior and the always-blocked security headers. The UI flows for managing headers are intuitive.
framework/configstore/clientconfig.go (2)
37-50: ClientConfig: header filter field wiring looks consistentAdding
HeaderFilterConfig *tables.GlobalHeaderFilterConfigwithomitemptyand a clear comment fits the existing config surface and keeps it optional without breaking JSON shape.
144-170: HeaderFilterConfig hashing is deterministic and aligned with semanticsIncluding sorted allowlist/denylist in
GenerateClientConfigHash(with explicit prefixes) makes hash changes stable and order‑insensitive, matching how other slice fields are handled and what the filter logic expects.transports/bifrost-http/handlers/inference.go (1)
450-451: Consistent propagation of header filter config into all inference/file handlersPassing
h.config.GetHeaderFilterConfig()intoConvertToBifrostContextfor every handler keeps header forwarding rules centralized and guarantees that the allowlist/denylist is respected across all Bifrost HTTP endpoints without altering existing cancellation/error behavior.Also applies to: 573-574, 649-650, 731-732, 803-805, 879-880, 1010-1011, 1096-1097, 1395-1397, 1448-1449, 1487-1488, 1526-1527, 1565-1566, 1647-1648, 1706-1707, 1745-1746, 1784-1785, 1823-1824
transports/bifrost-http/integrations/router.go (1)
393-400: Integration router now correctly applies global header filter configUsing
g.handlerStore.GetHeaderFilterConfig()when buildingbifrostCtxaligns integrations with the core handlers so that x‑bf‑eh‑* forwarding is governed by the same allowlist/denylist rules on all routes.transports/bifrost-http/handlers/mcp.go (1)
67-68: MCP execution correctly opts into header filteringPassing
h.store.GetHeaderFilterConfig()intoConvertToBifrostContextfor MCP tool execution keeps header behavior consistent with the rest of the transport while still disallowing direct keys for this endpoint.framework/configstore/store.go (1)
126-129: ConfigStore extension for header filter config is well-scopedAdding dedicated
GetHeaderFilterConfig/UpdateHeaderFilterConfigmethods mirrors existing global config CRUD patterns (e.g., proxy, restart_required) and gives a clear persistence API forGlobalHeaderFilterConfig.transports/bifrost-http/server/server.go (1)
53-67: Server-side reload hook for header filter config is straightforward and sufficientExtending
ServerCallbacksand implementingReloadHeaderFilterConfigby updatings.Config.ClientConfig.HeaderFilterConfigand logging allowlist/denylist sizes gives a clear, low‑friction way to hot‑apply header filter changes without restart and fits the existing Reload*Config patterns in this server.Also applies to: 841-856
transports/bifrost-http/lib/ctx.go (4)
11-11: LGTM!Imports are appropriate for the new header filtering functionality.
Also applies to: 18-18
80-80: LGTM!The function signature extension is clean. Using a pointer allows
nilto represent "no filtering config" with graceful handling inside the function.
100-116: LGTM!Good rename to
securityDenylistto clearly distinguish the always-enforced security list from the user-configurable denylist. The coverage of security-sensitive headers (auth, proxy-auth, cookies, connection headers, and key-override headers) is comprehensive.
287-294: LGTM!Correct enforcement order: security denylist is checked first (always enforced), then the configurable filter is applied. This ensures security-sensitive headers are never forwarded regardless of user configuration.
| shouldAllowHeader := func(headerName string) bool { | ||
| if headerFilterConfig == nil { | ||
| return true | ||
| } | ||
| hasAllowlist := len(headerFilterConfig.Allowlist) > 0 | ||
| hasDenylist := len(headerFilterConfig.Denylist) > 0 | ||
|
|
||
| // If allowlist is non-empty, header must be in allowlist | ||
| if hasAllowlist { | ||
| if !slices.Contains(headerFilterConfig.Allowlist, headerName) { | ||
| return false | ||
| } | ||
| } | ||
| // If denylist is non-empty, header must not be in denylist | ||
| if hasDenylist { | ||
| if slices.Contains(headerFilterConfig.Denylist, headerName) { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file structure and find the ctx.go file
find . -name "ctx.go" -path "*/bifrost-http/*"Repository: maximhq/bifrost
Length of output: 95
🏁 Script executed:
# Read the ctx.go file around the flagged lines and line 286
cat -n transports/bifrost-http/lib/ctx.go | sed -n '115,160p'Repository: maximhq/bifrost
Length of output: 2124
🏁 Script executed:
# Also check around line 286
cat -n transports/bifrost-http/lib/ctx.go | sed -n '275,300p'Repository: maximhq/bifrost
Length of output: 1108
🏁 Script executed:
# Search for where headerFilterConfig is initialized/populated
rg -n "headerFilterConfig\s*=" transports/bifrost-http/ -A 2 -B 2Repository: maximhq/bifrost
Length of output: 402
🏁 Script executed:
# Search for normalization of allowlist/denylist (ToLower, toLower, etc.)
rg -n "ToLower|toLower" transports/bifrost-http/lib/ctx.goRepository: maximhq/bifrost
Length of output: 980
🏁 Script executed:
# Search for GlobalHeaderFilterConfig or config structure definition
rg -n "GlobalHeaderFilterConfig|Allowlist|Denylist" transports/bifrost-http/ --type=go -B 2 -A 2Repository: maximhq/bifrost
Length of output: 8019
🏁 Script executed:
# Check git status to understand if this is part of a PR stack
git log --oneline -10 2>/dev/null || echo "Git not available"Repository: maximhq/bifrost
Length of output: 106
Case-sensitivity mismatch in header filter config comparison.
The incoming headerName is lowercased at line 286 before being passed to shouldAllowHeader(), but slices.Contains() at lines 134 and 140 performs case-sensitive comparison against headerFilterConfig.Allowlist and headerFilterConfig.Denylist. The config values are not normalized to lowercase anywhere (config.go lines 553-557, server.go line 842). This means headers configured as "Content-Type" in the allowlist will not match the lowercased incoming header "content-type", causing the filter to reject valid headers.
Use case-insensitive comparison when checking allowlist/denylist:
if hasAllowlist {
- if !slices.Contains(headerFilterConfig.Allowlist, headerName) {
+ if !slices.ContainsFunc(headerFilterConfig.Allowlist, func(s string) bool {
+ return strings.EqualFold(s, headerName)
+ }) {
return false
}
}
if hasDenylist {
- if slices.Contains(headerFilterConfig.Denylist, headerName) {
+ if slices.ContainsFunc(headerFilterConfig.Denylist, func(s string) bool {
+ return strings.EqualFold(s, headerName)
+ }) {
return false
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| shouldAllowHeader := func(headerName string) bool { | |
| if headerFilterConfig == nil { | |
| return true | |
| } | |
| hasAllowlist := len(headerFilterConfig.Allowlist) > 0 | |
| hasDenylist := len(headerFilterConfig.Denylist) > 0 | |
| // If allowlist is non-empty, header must be in allowlist | |
| if hasAllowlist { | |
| if !slices.Contains(headerFilterConfig.Allowlist, headerName) { | |
| return false | |
| } | |
| } | |
| // If denylist is non-empty, header must not be in denylist | |
| if hasDenylist { | |
| if slices.Contains(headerFilterConfig.Denylist, headerName) { | |
| return false | |
| } | |
| } | |
| return true | |
| } | |
| shouldAllowHeader := func(headerName string) bool { | |
| if headerFilterConfig == nil { | |
| return true | |
| } | |
| hasAllowlist := len(headerFilterConfig.Allowlist) > 0 | |
| hasDenylist := len(headerFilterConfig.Denylist) > 0 | |
| // If allowlist is non-empty, header must be in allowlist | |
| if hasAllowlist { | |
| if !slices.ContainsFunc(headerFilterConfig.Allowlist, func(s string) bool { | |
| return strings.EqualFold(s, headerName) | |
| }) { | |
| return false | |
| } | |
| } | |
| // If denylist is non-empty, header must not be in denylist | |
| if hasDenylist { | |
| if slices.ContainsFunc(headerFilterConfig.Denylist, func(s string) bool { | |
| return strings.EqualFold(s, headerName) | |
| }) { | |
| return false | |
| } | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
In transports/bifrost-http/lib/ctx.go around lines 125 to 145, the header
allow/deny checks are case-sensitive while incoming headerName is lowercased
elsewhere; update the comparison to be case-insensitive by normalizing values or
using case-insensitive comparison. Fix by either (A) normalizing
headerFilterConfig.Allowlist and headerFilterConfig.Denylist to lowercase when
the config is loaded so slices.Contains can be used against the
already-lowercased headerName, or (B) change the checks here to compare using
strings.EqualFold (or compare against a precomputed lowercase set) when testing
membership in allowlist/denylist; ensure both allowlist and denylist comparisons
use the same case-insensitive approach so headers match regardless of case.
a65444e to
312c22a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
transports/bifrost-http/lib/ctx.go (1)
118-145: Header filter allow/deny checks are case-sensitive despite lowercasing header names.
labelNameis normalized to lowercase before callingshouldAllowHeader, but the allow/deny checks useslices.Containsagainst the rawAllowlist/Denylist, which are not normalized. A config entry like"Content-Type"will not match the incoming"content-type", so filters behave unexpectedly and contradict the stated case‑insensitive semantics.Recommend making membership checks case‑insensitive (or normalizing config to lowercase on load). One local fix is:
Proposed fix: use case-insensitive membership via
strings.EqualFold- hasAllowlist := len(headerFilterConfig.Allowlist) > 0 - hasDenylist := len(headerFilterConfig.Denylist) > 0 + allowlist := headerFilterConfig.Allowlist + denylist := headerFilterConfig.Denylist + hasAllowlist := len(allowlist) > 0 + hasDenylist := len(denylist) > 0 @@ - if hasAllowlist { - if !slices.Contains(headerFilterConfig.Allowlist, headerName) { + if hasAllowlist { + if !slices.ContainsFunc(allowlist, func(s string) bool { + return strings.EqualFold(s, headerName) + }) { return false } } // If denylist is non-empty, header must not be in denylist - if hasDenylist { - if slices.Contains(headerFilterConfig.Denylist, headerName) { + if hasDenylist { + if slices.ContainsFunc(denylist, func(s string) bool { + return strings.EqualFold(s, headerName) + }) { return false } }This keeps header matching truly case-insensitive while leaving config storage format unchanged.
🧹 Nitpick comments (5)
ui/lib/types/schemas.ts (1)
680-691: Global header filter schemas/types line up with backend; optional normalization.The Zod schemas and inferred types correctly mirror the backend
GlobalHeaderFilterConfig(allowlist/denylist) and theheader_filter_configwrapper shape, so they should interop cleanly with/api/config. If header names are meant to be case-insensitive, you may optionally add a.transform()(e.g.,s.trim().toLowerCase()) on the string items here so UI input is canonicalized before it hits the backend filters.Also applies to: 709-710
transports/bifrost-http/handlers/config.go (1)
34-35: Header filter config wiring is sound; verify persistence and equality semantics.
- Extending
ConfigManagerwithReloadHeaderFilterConfigand calling it fromupdateConfigwhenHeaderFilterConfigchanges is a good way to hot‑reload filter rules without a restart.getConfigexposingheader_filter_configviaConfigStore.GetHeaderFilterConfiggives the UI a clean, focused payload in addition to whatever is onclient_config.headerFilterConfigEqual’s pointer/len-based comparison (treatingnilvs empty slices as equal and being order-sensitive) is reasonable; just be aware that reordering entries alone will trigger a reload.The only thing that’s not obvious from this file is whether
HeaderFilterConfigchanges are persisted to the same storage that backsGetHeaderFilterConfig(e.g., viaUpdateHeaderFilterConfigor insideUpdateClientConfig). If that linkage isn’t already in place inconfigstore, consider adding it so that:
GET /api/configalways returns the latest header filter config, and- the configuration survives process restarts.
Also applies to: 161-167, 276-285, 633-642
transports/bifrost-http/lib/config.go (1)
36-41: Header filter config bootstrapping and HandlerStore exposure look correct.
- Extending
HandlerStorewithGetHeaderFilterConfigand implementing it onConfigas a lock-free accessor overClientConfig.HeaderFilterConfigis consistent with the existingShouldAllowDirectKeyspattern.- Both config load paths (file-backed and default/SQLite) now call
loadDefaultHeaderFilterConfig, so runtime always picks up whateverConfigStore.GetHeaderFilterConfigreturns.loadClientConfigFromFilesyncingHeaderFilterConfiginto the store andmergeClientConfig’s “DB wins, fill missing allow/deny lists from file” behavior give you a sane single source of truth while still honoring file defaults.If you later decide to canonicalize header names (e.g., trimming and lowercasing allowlist/denylist entries once up front),
loadDefaultHeaderFilterConfigand the sync block inloadClientConfigFromFileare good central places to do that so all consumers see normalized data.Also applies to: 355-373, 501-506, 548-559, 1578-1580, 1763-1773, 2065-2071
ui/app/workspace/config/views/clientSettingsView.tsx (2)
280-300: Using array index askeycan cause input focus and value issues when items are removed.When items are added or removed from the middle of the list, React's reconciliation may not correctly associate inputs with their values, potentially causing the cursor to jump or values to appear in wrong inputs.
Consider generating a stable unique ID when adding items, or using a combination approach.
🔎 Proposed approach using stable IDs
One approach is to store objects with IDs instead of plain strings:
// In state, use objects with stable IDs: interface HeaderEntry { id: string; value: string; } // When adding: const handleAddAllowlistHeader = useCallback(() => { setLocalConfig((prev) => ({ ...prev, header_filter_config: { ...prev.header_filter_config, allowlist: [...(prev.header_filter_config?.allowlist || []), { id: crypto.randomUUID(), value: "" }], }, })); }, []);Alternatively, if keeping plain strings, you could use a composite key, though this is less robust:
key={`${header}-${index}`}
316-336: Samekey={index}concern applies to denylist.As noted for the allowlist above, using index as key here has the same potential for input confusion when items are removed. The same mitigation approach would apply.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- transports/bifrost-http/integrations/bedrock_test.go
- transports/config.schema.json
- framework/configstore/store.go
- framework/configstore/clientconfig.go
- framework/configstore/tables/config.go
- ui/lib/types/config.ts
- transports/bifrost-http/handlers/inference.go
- framework/configstore/rdb.go
- transports/bifrost-http/integrations/router.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.goui/app/workspace/config/views/clientSettingsView.tsxtransports/bifrost-http/handlers/config.goui/lib/types/schemas.tstransports/bifrost-http/lib/config.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/lib/ctx.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.go
🧬 Code graph analysis (4)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-369)
ui/app/workspace/config/views/clientSettingsView.tsx (3)
ui/lib/types/config.ts (3)
DefaultGlobalHeaderFilterConfig(310-313)GlobalHeaderFilterConfig(304-307)CoreConfig(336-350)ui/lib/store/apis/baseApi.ts (1)
getErrorMessage(174-199)ui/components/ui/input.tsx (1)
Input(15-69)
transports/bifrost-http/handlers/config.go (4)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (6)
transports/bifrost-http/handlers/mcp.go (1)
67-68: MCP execute path correctly participates in header filtering.Passing
h.store.GetHeaderFilterConfig()intoConvertToBifrostContextaligns MCP tool execution with the inference/router paths and ensures extra headers here respect the same filter rules.transports/bifrost-http/server/server.go (1)
66-67: Server-side header filter reload is correct; confirm persistence path.The new
ReloadHeaderFilterConfigcallback cleanly updates the in-memoryClientConfig.HeaderFilterConfigand logs a concise summary, which is enough for live header filtering to see changes.Since this method is intentionally in-memory only, please confirm that the handler/configstore path (e.g.,
UpdateClientConfigand/orUpdateHeaderFilterConfig) also persists the same config so that:
GetHeaderFilterConfigused byGET /api/configreturns the updated version, and- the filter configuration survives process restarts.
Also applies to: 841-856
ui/app/workspace/config/views/clientSettingsView.tsx (4)
30-42: Order-sensitive comparison may not detect reordered headers as "unchanged".The
headerFilterConfigEqualfunction compares arrays element-by-element by index. If users manually reorder headers externally or via future drag-and-drop functionality, identical sets in different orders would be flagged as changed.For the current UI (which doesn't support reordering), this works correctly. If reordering is added later, consider using a Set-based comparison.
81-96: Good sanitization of empty entries before save.The cleanup logic correctly filters out empty and whitespace-only strings from both allowlist and denylist before persisting. This prevents invalid entries from being saved.
223-270: Clear and helpful documentation for users.The accordion sections provide good explanations of how the allowlist/denylist precedence works and which headers are always blocked for security. This helps users understand the filtering behavior without needing external documentation.
119-127: Lowercase normalization applied correctly on input change.Both
handleAllowlistChangeandhandleDenylistChangecorrectly normalize header names to lowercase, ensuring case-insensitive matching as specified in the PR objectives.Also applies to: 149-157
akshaydeo
force-pushed
the
12-26-adds_allowlist_and_denylist_for_headers
branch
from
312c22a to
dcb3c25
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
transports/bifrost-http/lib/ctx.go (1)
80-85: Header filter checks are still case‑sensitive against lowercased header names.Incoming headers are normalized to lowercase (
keyStr,labelName), but bothshouldAllowHeaderand the direct forwarding block compare againstheaderFilterConfig.Allowlist/Denylistusingslices.Contains, which is case‑sensitive. The UI now lowercases entries, but configs set via JSON or direct API can still use mixed‑case names (e.g.,Content-Type), which won’t match the lowercased header keys and will silently bypass the filter logic.To make matching truly case‑insensitive and robust across all writers, consider switching these membership tests to something like
slices.ContainsFunc(list, func(s string) bool { return strings.EqualFold(s, headerName) })(and the analogous check in the direct forwarding branch), or factoring out a helper that doesEqualFoldfor both allowlist and denylist.Also applies to: 100-115, 124-144, 279-297, 298-321
🧹 Nitpick comments (3)
transports/bifrost-http/handlers/config.go (1)
632-642: Consider:slices.Equalis order-sensitive.The comparison uses
slices.Equalwhich is order-sensitive. If two configs have the same headers in different order (e.g.,["a", "b"]vs["b", "a"]), they will be considered different, triggering unnecessary reloads.If header order is semantically irrelevant, consider sorting before comparison or using a set-based comparison.
🔎 Optional: Order-insensitive comparison
+import "sort" + +// slicesEqualUnordered compares two string slices ignoring order +func slicesEqualUnordered(a, b []string) bool { + if len(a) != len(b) { + return false + } + aCopy := make([]string, len(a)) + bCopy := make([]string, len(b)) + copy(aCopy, a) + copy(bCopy, b) + sort.Strings(aCopy) + sort.Strings(bCopy) + return slices.Equal(aCopy, bCopy) +} func headerFilterConfigEqual(a, b *configstoreTables.GlobalHeaderFilterConfig) bool { if a == nil && b == nil { return true } if a == nil || b == nil { return false } - return slices.Equal(a.Allowlist, b.Allowlist) && slices.Equal(a.Denylist, b.Denylist) + return slicesEqualUnordered(a.Allowlist, b.Allowlist) && slicesEqualUnordered(a.Denylist, b.Denylist) }transports/bifrost-http/lib/config.go (1)
37-41: Header filter plumbing looks coherent; consider normalizing config values once at load.The HandlerStore extension and the end‑to‑end wiring (client config merge, store sync via
UpdateHeaderFilterConfig, default‐path load, andGetHeaderFilterConfigaccessor) are consistent and match the intended “store as source of truth, file as seed” behavior; semantics for nil vs empty allow/deny lists also align with the runtime logic inctx.go.As a small robustness improvement, you could normalize header filter entries (e.g.,
strings.TrimSpace+strings.ToLower) once when merging/loading intoClientConfig.HeaderFilterConfig, so non‑UI writers (config file / direct API) can safely rely on case‑insensitive matching without having to follow the UI’s lowercase convention; this would keepctx.gosimple while hardening behavior against stray whitespace/case in persisted configs.Also applies to: 385-388, 501-506, 548-559, 1578-1580, 1763-1773, 2065-2071
ui/app/workspace/config/views/clientSettingsView.tsx (1)
27-42: UI wiring for header forwarding looks good; trim+lowercase values to avoid subtle mismatches.The defaulting of
header_filter_config, equality helper, and the allowlist/denylist UI all line up well with the backend, and forcingvalue.toLowerCase()ensures the lists stay consistent with lowercased header keys inctx.go. One small robustness tweak: consider normalizing withvalue.trim().toLowerCase()inhandleAllowlistChange/handleDenylistChange(and optionally serializing the trimmed version inhandleSave) so that stray leading/trailing spaces don’t result in entries like" custom-id"that will never match any real header name.Also applies to: 59-65, 81-91, 98-157, 295-371
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
framework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/inference.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/integrations/router.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.tsui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/config.schema.json
- framework/configstore/store.go
- transports/bifrost-http/handlers/inference.go
- framework/configstore/clientconfig.go
- framework/configstore/tables/config.go
- ui/lib/types/schemas.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/handlers/config.goui/lib/types/config.tstransports/bifrost-http/lib/ctx.goframework/configstore/rdb.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.goui/app/workspace/config/views/clientSettingsView.tsxtransports/bifrost-http/integrations/router.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.goframework/configstore/rdb.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/router.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/integrations/bedrock_test.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/ctx.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/integrations/router.go
🧬 Code graph analysis (10)
transports/bifrost-http/integrations/bedrock_test.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/handlers/config.go (4)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)
ui/lib/types/config.ts (1)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)
transports/bifrost-http/lib/ctx.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
framework/configstore/rdb.go (2)
framework/configstore/tables/config.go (4)
GlobalHeaderFilterConfig(46-49)TableGovernanceConfig(52-55)TableGovernanceConfig(58-58)ConfigHeaderFilterKey(12-12)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)
transports/bifrost-http/lib/config.go (3)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
transports/bifrost-http/server/server.go (4)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(304-307)transports/bifrost-http/lib/config.go (1)
Config(186-217)framework/configstore/clientconfig.go (1)
ClientConfig(36-51)
transports/bifrost-http/handlers/mcp.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
ui/app/workspace/config/views/clientSettingsView.tsx (2)
ui/lib/types/config.ts (3)
DefaultGlobalHeaderFilterConfig(310-313)GlobalHeaderFilterConfig(304-307)CoreConfig(336-350)framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(46-49)
transports/bifrost-http/integrations/router.go (1)
transports/bifrost-http/lib/ctx.go (1)
ConvertToBifrostContext(80-392)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (12)
transports/bifrost-http/handlers/mcp.go (1)
67-67: LGTM!The header filter configuration is correctly passed to
ConvertToBifrostContext, aligning with the updated signature and ensuring MCP tool execution respects header filtering rules.transports/bifrost-http/integrations/bedrock_test.go (1)
15-30: LGTM!The mock correctly implements
GetHeaderFilterConfig()to satisfy the updatedlib.HandlerStoreinterface. The compile-time interface assertion at line 30 ensures the mock stays synchronized with interface changes.transports/bifrost-http/handlers/config.go (2)
161-167: LGTM!Header filter config is correctly fetched from the store and included in the response when available. The nil check ensures no empty entry is added to the response map.
276-284: LGTM!The change detection and reload flow is well-structured. The early return on reload failure ensures partial updates don't persist, maintaining configuration consistency.
ui/lib/types/config.ts (3)
302-313: LGTM!The
GlobalHeaderFilterConfiginterface and default constant are correctly defined and align with the Go backend struct atframework/configstore/tables/config.go. The optional array types match theomitemptyJSON tags in Go.
327-327: LGTM!The
header_filter_configfield is correctly added toBifrostConfig, enabling the UI to receive and display the header filter configuration from the API response.
349-349: LGTM!The
header_filter_configfield inCoreConfigmatches the GoClientConfigstruct, ensuring type consistency for configuration updates.framework/configstore/rdb.go (2)
1952-1969: LGTM!The
GetHeaderFilterConfigimplementation follows the established pattern used byGetProxyConfigand other governance config retrieval methods. The nil handling for both missing records and empty values is appropriate.
1971-1981: LGTM!The
UpdateHeaderFilterConfigimplementation is straightforward and consistent withUpdateProxyConfig. UsingSavewith the governance config key ensures proper upsert behavior.transports/bifrost-http/server/server.go (2)
66-66: LGTM!The
ReloadHeaderFilterConfigmethod is correctly added to theServerCallbacksinterface, enabling the config handler to trigger runtime updates.
841-856: LGTM!The implementation correctly stores the header filter config in
ClientConfig.HeaderFilterConfigand includes defensive nil checks before accessing slice lengths. The logging provides useful visibility into the configuration state.The pattern matches
ReloadProxyConfigabove, maintaining consistency across runtime configuration updates.transports/bifrost-http/integrations/router.go (1)
383-399: Passing header filter config into ConvertToBifrostContext is consistent and minimal.The updated call cleanly threads
GetHeaderFilterConfig()into context creation without affecting the existing request/streaming pipeline, and lines up with the new HandlerStore contract inlib/config.go.
Summary
Add configurable header filtering for extra headers forwarded to LLM providers via the
x-bf-eh-*prefix. This allows administrators to control which headers are allowed to pass through to providers using allowlist and denylist mechanisms.Changes
GlobalHeaderFilterConfigstruct with allowlist and denylist fieldsType of change
Affected areas
How to test
x-bf-eh-*headers to verify filtering works:Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
Checklist
docs/contributing/README.mdand followed the guidelines