fix Client settings UI to accept * as accepted headers#2400
Conversation
|
|
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Merge activity
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughHeader-filter validation was changed to only consider exact security header names in allowlists/denylists (wildcards are not expanded during validation). Tests were updated to reflect runtime-stripped wildcard behavior. A UI toast message was corrected from "core" to "client" config on save failure. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
akshaydeo
deleted the
03-30-fix_client_settings_ui_to_accept_as_accepted_headers
branch
Confidence Score: 2/5Not safe to merge — the runtime security claim underpinning the change is incorrect for A P1 security defect: the PR removes a validation guard based on the incorrect premise that
Important Files Changed
Reviews (1): Last reviewed commit: "fix Client settings UI to accept * as ac..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Check allowlist for exact security header names. | ||
| // Wildcard patterns are allowed — security headers are always stripped at runtime | ||
| // unconditionally in ctx.go, regardless of allowlist/denylist configuration. | ||
| for _, header := range config.Allowlist { | ||
| headerLower := strings.ToLower(strings.TrimSpace(header)) | ||
| if strings.Contains(headerLower, "*") { | ||
| for _, secHeader := range securityHeaders { | ||
| if lib.HeaderMatchesPattern(headerLower, secHeader) && !slices.Contains(foundSecurityHeaders, secHeader) { | ||
| foundSecurityHeaders = append(foundSecurityHeaders, secHeader) | ||
| } | ||
| } | ||
| continue | ||
| } | ||
| if slices.Contains(securityHeaders, headerLower) { | ||
| foundSecurityHeaders = append(foundSecurityHeaders, headerLower) | ||
| } | ||
| } |
There was a problem hiding this comment.
authorization is NOT in the runtime security denylist
The PR comment says "security headers are always stripped at runtime unconditionally in ctx.go", but that's only true for most of the security headers — authorization is not in the securityDenylist map in ctx.go:
// ctx.go ~line 146
securityDenylist := map[string]bool{
"proxy-authorization": true,
"cookie": true,
"host": true,
"content-length": true,
"connection": true,
"transfer-encoding": true,
"x-api-key": true,
"x-goog-api-key": true,
"x-bf-api-key": true,
"x-bf-api-key-id": true,
"x-bf-vk": true,
// ← "authorization" is absent
}The direct-header-forwarding path in ctx.go (lines ~358–378) only guards against securityDenylist headers. Because authorization is absent from that map, a request-level client that sends Authorization: Bearer sk-ant-… will have that header forwarded directly to the upstream AI provider whenever:
- The allowlist contains a wildcard such as
authorization*or*(now accepted by this PR), and - The
authorizationheader doesn't happen to carry a governance virtual-key prefix (which causesreturn trueearly at line 231–237, before the forwarding section is reached).
In practice this means: if an admin configures authorization* in the header-filter allowlist (newly valid under this change), any API caller can inject an arbitrary Authorization header and Bifrost will forward it to the upstream provider — overriding Bifrost's own credential management.
The original validation that rejected wildcard patterns matching security headers was the correct guard. If the intent is to allow authorization* for non-security custom headers, authorization should first be added to the securityDenylist in ctx.go so the runtime actually enforces the strip unconditionally.
2 participants
Summary
Relaxes header filter validation to allow wildcard patterns that match security headers while maintaining strict validation for exact security header names. This change recognizes that security headers are unconditionally stripped at runtime regardless of configuration.
Changes
validateHeaderFilterConfigto only reject exact security header names in allowlist/denylistType of change
Affected areas
How to test
Validate that wildcard patterns in header filter configuration are now accepted:
Screenshots/Recordings
N/A - Backend validation changes only
Breaking changes
Related issues
N/A
Security considerations
This change maintains security by ensuring security headers are always stripped at runtime in
ctx.go, regardless of allowlist/denylist configuration. The validation change only affects configuration acceptance, not runtime behavior.Checklist
docs/contributing/README.mdand followed the guidelines