feat: remove restart required for auth config #1251
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactors authentication into an in-memory atomic AuthMiddleware with runtime updates and session flushing, adds FlushSessions to the config store, updates server and handlers to use the middleware (no restart), adds a GitHub Action to bundle OpenAPI docs, updates docs/UI, and bumps several versions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server
participant AuthMiddleware as AuthMiddleware (in-memory)
participant ConfigStore
participant RDB as RDBConfigStore
Note over Client,Server: Auth-protected request flow
Client->>Server: HTTP request
Server->>AuthMiddleware: invoke middleware
AuthMiddleware->>ConfigStore: validate session / token
ConfigStore->>RDB: query sessions table
RDB-->>ConfigStore: session lookup result
ConfigStore-->>AuthMiddleware: validation outcome
AuthMiddleware-->>Server: allow / deny
Server-->>Client: response
Note over Server,ConfigStore: Admin updates auth config (no restart)
Client->>Server: POST /api/config/auth
Server->>ConfigStore: save new auth config
ConfigStore->>RDB: persist config
Server->>RDB: FlushSessions()
RDB-->>ConfigStore: sessions cleared
Server->>AuthMiddleware: UpdateAuthConfig(newConfig)
AuthMiddleware-->>AuthMiddleware: atomic swap
Server-->>Client: 200 OK
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Pratham-Mishra04
force-pushed
the
01-05-feat_auth_config_fixes_and_enhancements
branch
from
5dd303c to
bb4d584
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @docs/quickstart/gateway/cli-agents.mdx:
- Around line 146-152: The Model Selection Guide table incorrectly lists "Opus
4.5"; update the Opus entry to "Opus 4.1" (or confirm and set to 4.5 if publicly
released) so the table rows read **Sonnet 4.5**, **Opus 4.1**, **Haiku 4.5**;
edit the markdown table under the "Model Selection Guide" header and change the
"Opus 4.5" cell to "Opus 4.1" (or replace with the verified public release
version) to ensure accuracy.
In @transports/bifrost-http/handlers/config.go:
- Around line 165-168: The code calls h.store.ConfigStore.GetAuthConfig(ctx)
twice and should reuse the initially fetched authConfig; remove the second call
at the later block and reference the previously obtained authConfig variable
instead, preserving the original error handling logic from the first fetch
(handle or return if err non-nil) so you don't mask failures—search for the
initial GetAuthConfig(ctx) call and replace the duplicate block that re-fetches
authConfig with a usage of that existing authConfig value.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/configstore/rdb.go (1)
1974-2010: Critical: Backward compatibility break for existing auth configurations.Line 2001 returns
nilwhenauthHashisnil, which will break existing deployments that have authentication configured but no hash stored yet (pre-upgrade state).Impact:
- Existing authenticated users will be unable to access the system after upgrade
- Auth config will appear as non-existent even though credentials are stored
- No automatic migration path for existing configs
Recommendation: Handle missing hash gracefully:
🔎 Proposed fix to handle missing hash
- if username == nil || password == nil || authHash == nil { + if username == nil || password == nil { return nil, nil } + + // Handle missing hash for backward compatibility (pre-upgrade configs) + var hash string + if authHash != nil { + hash = *authHash + } else { + // Generate hash for existing config without one + h := sha256.New() + h.Write([]byte(*username)) + h.Write([]byte(*password)) + hash = hex.EncodeToString(h.Sum(nil)) + + // Persist the generated hash + if err := s.db.WithContext(ctx).Save(&tables.TableGovernanceConfig{ + Key: tables.ConfigAuthHashKey, + Value: hash, + }).Error; err != nil { + // Log but don't fail - hash can be generated on next update + s.logger.Warn(fmt.Sprintf("failed to persist generated auth hash: %v", err)) + } + } + return &AuthConfig{ AdminUserName: *username, AdminPassword: *password, IsEnabled: isEnabled, DisableAuthOnInference: disableAuthOnInference, - Hash: *authHash, + Hash: hash, }, nil
♻️ Duplicate comments (1)
framework/configstore/rdb.go (1)
2040-2049: Critical: Insecure cryptographic hashing of sensitive data.Lines 2042-2043 use SHA-256 to hash admin credentials, which is inappropriate for password hashing due to:
- Speed: SHA-256 is computationally inexpensive, enabling brute-force attacks
- No salt: Vulnerable to rainbow table attacks
- Deterministic: Same input always produces same output, leaking information
While this hash is used for "config change detection" rather than authentication, hashing sensitive password data with SHA-256 is still a security anti-pattern.
Recommended solutions:
Option 1 (Preferred): Use a version counter instead of hashing
+ // Generate a new version/timestamp for change detection + configVersion := fmt.Sprintf("%d", time.Now().UnixNano()) if err := tx.Save(&tables.TableGovernanceConfig{ Key: tables.ConfigAuthHashKey, - Value: hex.EncodeToString(hash.Sum(nil)), + Value: configVersion, }).Error; err != nil { return err }Option 2: Hash only non-sensitive config elements
- // Generating hash of the config - hash := sha256.New() - hash.Write([]byte(config.AdminUserName)) - hash.Write([]byte(config.AdminPassword)) + // Generate hash of non-sensitive config elements for change detection + // Hash format: username + enabled_state to detect config changes without hashing password + hashInput := fmt.Sprintf("%s:%t:%t", config.AdminUserName, config.IsEnabled, config.DisableAuthOnInference) + hash := sha256.Sum256([]byte(hashInput)) if err := tx.Save(&tables.TableGovernanceConfig{ Key: tables.ConfigAuthHashKey, - Value: hex.EncodeToString(hash.Sum(nil)), + Value: hex.EncodeToString(hash[:]), }).Error; err != nil { return err }Option 3: Use HMAC-SHA256 with server secret
+ import "crypto/hmac" + + // Generate HMAC of config using server secret (from encryption key or separate constant) + // This provides better security than plain SHA-256 + mac := hmac.New(sha256.New, []byte("your-server-secret-key-here")) + mac.Write([]byte(config.AdminUserName)) + mac.Write([]byte(config.AdminPassword)) if err := tx.Save(&tables.TableGovernanceConfig{ Key: tables.ConfigAuthHashKey, - Value: hex.EncodeToString(hash.Sum(nil)), + Value: hex.EncodeToString(mac.Sum(nil)), }).Error; err != nil { return err }Note: The codebase already has
framework/encrypt/encrypt.gowith a properHash()function using bcrypt, but bcrypt generates different hashes each time (due to salt), making it unsuitable for change detection.
🧹 Nitpick comments (2)
.github/workflows/openapi-bundle.yml (2)
54-55: The--skip-pipelinecommit message suffix may not prevent recursive workflow runs on GitHub.GitHub Actions uses
[skip ci],[ci skip], or[skip actions]in the commit message to skip workflow runs. The--skip-pipelinesuffix is a GitLab CI convention and won't be recognized by GitHub Actions.However, since the workflow excludes
docs/openapi/openapi.jsonfrom its trigger paths (line 8, 12), the auto-committed changes won't re-trigger this workflow anyway. The--skip-pipelinesuffix is harmless but ineffective—consider removing it or replacing with[skip ci]for clarity.🔎 Suggested fix
- git commit -m "chore: regenerate openapi.json --skip-pipeline" + git commit -m "chore: regenerate openapi.json [skip ci]"
39-49: Consider failing PR checks whenopenapi.jsonis out of sync.Currently, the workflow runs
bundle.pyon PRs but doesn't fail if the bundled output differs from the committedopenapi.json. This means PRs could be merged with an outdated spec, relying on the post-merge commit to fix it.To catch this earlier, you could add a step that fails when changes are detected on PRs:
🔎 Suggested enhancement
- name: Check for changes id: changes run: | if git diff --quiet docs/openapi/openapi.json; then echo "changed=false" >> $GITHUB_OUTPUT else echo "changed=true" >> $GITHUB_OUTPUT fi + - name: Fail if openapi.json is out of sync (PR only) + if: steps.changes.outputs.changed == 'true' && github.event_name == 'pull_request' + run: | + echo "::error::docs/openapi/openapi.json is out of sync. Please run 'python docs/openapi/bundle.py' locally and commit the changes." + exit 1 - name: Commit and push changes if: steps.changes.outputs.changed == 'true' && github.event_name == 'push'
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/openapi-bundle.ymldocs/features/mcp/agent-mode.mdxdocs/openapi/openapi.jsondocs/quickstart/gateway/cli-agents.mdxframework/changelog.mdframework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/changelog.mdui/lib/store/apis/baseApi.tsui/lib/store/apis/configApi.tsui/lib/types/config.tsui/lib/utils/authConfigManager.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/changelog.mdui/lib/store/apis/configApi.tsframework/configstore/clientconfig.goui/lib/store/apis/baseApi.tsframework/changelog.mddocs/features/mcp/agent-mode.mdxui/lib/types/config.tsdocs/quickstart/gateway/cli-agents.mdxui/lib/utils/authConfigManager.tsframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.goframework/configstore/rdb.go
🧠 Learnings (5)
📚 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.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.goframework/configstore/rdb.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
framework/configstore/clientconfig.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.goframework/configstore/rdb.go
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/features/mcp/agent-mode.mdxdocs/quickstart/gateway/cli-agents.mdx
📚 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/config.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/handlers/config.go
🧬 Code graph analysis (5)
framework/configstore/clientconfig.go (2)
framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(45-48)ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(310-313)
ui/lib/types/config.ts (2)
framework/configstore/tables/config.go (2)
GlobalProxyConfig(24-37)RestartRequiredConfig(18-21)core/network/http.go (1)
GlobalProxyConfig(55-68)
ui/lib/utils/authConfigManager.ts (1)
ui/lib/store/apis/baseApi.ts (1)
clearAuthStorage(44-62)
transports/bifrost-http/handlers/config.go (4)
framework/configstore/store.go (1)
ConfigStore(17-157)framework/encrypt/encrypt.go (1)
Hash(63-69)ui/lib/types/config.ts (1)
AuthConfig(267-272)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)
framework/configstore/rdb.go (3)
framework/configstore/tables/config.go (3)
TableGovernanceConfig(51-54)TableGovernanceConfig(57-57)ConfigAuthHashKey(10-10)framework/configstore/clientconfig.go (1)
AuthConfig(724-730)framework/encrypt/encrypt.go (1)
Hash(63-69)
🪛 GitHub Actions: PR #1251
transports/bifrost-http/handlers/config.go
[error] 170-170: Field or method 'Hash' undefined for type *configstore.AuthConfig. This causes failure during the go extractor step.
🪛 GitHub Check: CodeQL
framework/configstore/rdb.go
[failure] 2043-2043: Use of a broken or weak cryptographic hashing algorithm on sensitive data
Sensitive data (password) is used in a hashing algorithm (SHA256) that is insecure for password hashing, since it is not a computationally expensive hash function.
🪛 LanguageTool
transports/changelog.md
[grammar] ~2-~2: Ensure spelling is correct
Context: ...config hash validation to automatically logout users when auth configuration changes -...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (23)
transports/changelog.md (1)
1-4: LGTM!The changelog entries accurately document the changes introduced in this PR. The static analysis hint about "logout" is a false positive—it's correct terminology in this software context.
framework/configstore/tables/config.go (1)
10-10: LGTM!The new constant
ConfigAuthHashKeyfollows the established naming pattern and is well-placed among related auth configuration constants.framework/changelog.md (1)
1-1: LGTM!The changelog entry accurately describes the auth hash tracking feature added to the framework.
framework/configstore/clientconfig.go (2)
36-51: LGTM!The
ClientConfigstruct changes appear to be formatting/alignment adjustments. TheConfigHashfield correctly usesjson:"-"to exclude it from serialization, as it's meant for internal reconciliation only.
724-730: No action needed—field name transformation is already implemented correctly.The HTTP handler at
transports/bifrost-http/handlers/config.goline 170 explicitly mapsauthConfig.Hash(Go field) to"auth_hash"(JSON response key) when constructing the response map. The frontend receives the correct field name as expected inBifrostConfig.auth_hash.Likely an incorrect or invalid review comment.
ui/lib/store/apis/configApi.ts (2)
83-96: Good fix to the mutation return type.The signature change from
<null, BifrostConfig>to<BifrostConfig, BifrostConfig>correctly reflects that the endpoint returns aBifrostConfigresponse. ThetransformResponseproperly saves the newauth_hashafter successful updates.
16-26: First-time user handling is already correct in the code.The first concern is addressed:
validateAuthHashexplicitly saves the hash on first encounter (if (!storedHash) { saveAuthHash(currentHash); return true; }), so new sessions won't be treated as auth mismatches.The second observation about the response being returned remains valid but is acceptable.
handleAuthHashMismatch()does use async operations (dynamic import +.then()for the redirect), but the practical impact is minimal since token clearance and redirect happen nearly instantaneously. The stale data is processed briefly before the logout completes, but the redirect occurs fast enough to prevent meaningful damage.The implementation is solid.
ui/lib/types/config.ts (1)
338-338: Field naming is correct - verified that backend explicitly maps toauth_hashin API response.The
auth_hashfield addition toBifrostConfigis properly aligned. The backend handler intransports/bifrost-http/handlers/config.goexplicitly mapsauthConfig.HashtomapConfig["auth_hash"]when building the config response, confirming the frontend type name matches the actual API response field.docs/features/mcp/agent-mode.mdx (1)
16-18: LGTM: Clear documentation of streaming limitation.The warning appropriately explains that Agent Mode requires complete responses for its autonomous execution loop and directs users to non-streaming endpoints. The technical reasoning is sound and the guidance is actionable.
ui/lib/store/apis/baseApi.ts (2)
52-53: LGTM: Auth hash clearing integrated properly.The addition of auth hash removal in
clearAuthStoragecorrectly complements the new centralized auth config hash validation mechanism introduced in this PR.
178-178: LGTM: Formatting improvement.Minor spacing adjustment improves code consistency.
framework/configstore/rdb.go (1)
330-330: LGTM: KeyID preservation maintains data integrity.Correctly preserves the original KeyID when updating keys found by name lookup, ensuring consistent key identification across reloads.
transports/bifrost-http/handlers/config.go (2)
469-490: Auth config update logic is sound.The refactored authentication update flow correctly:
- Uses
configManager.UpdateAuthConfigfor centralized persistence- Preserves existing credentials when disabling authentication (lines 476-483)
- Handles redacted password fields appropriately
This logic assumes the
Hashfield exists onAuthConfig(address the compilation error first).
165-171: Remove redundant GetAuthConfig call—reuse the result from line 133.The
authConfigis already fetched at line 133. The second call at line 165 is unnecessary and queries the database again for the same data. Either add theauth_hashassignment within the first auth config block (after line 153), or reuse theauthConfigvariable from line 133 by moving the auth_hash logic to that scope to avoid the duplicate database query.Note: The
Hashfield is properly defined in theAuthConfigstruct and does not cause compilation failure.ui/lib/utils/authConfigManager.ts (3)
1-50: LGTM: Robust storage utilities with proper guards.The auth hash storage functions demonstrate good defensive programming:
- Consistent SSR guards (
typeof window === "undefined") prevent server-side errors- Try/catch blocks with error logging provide resilience
- Clean separation of concerns with single-responsibility functions
57-73: LGTM: Validation logic handles all cases correctly.The
validateAuthHashfunction properly handles:
- Auth disabled (line 58-61): Returns
truewhen no hash provided- First-time setup (lines 65-69): Saves hash and returns
trueto preserve session- Hash comparison (lines 71-72): Validates subsequent requests
This prevents false logouts on initial setup while ensuring detection of actual config changes.
79-92: LGTM: Clean mismatch handling with circular dependency solution.The
handleAuthHashMismatchfunction implements a clean logout flow:
- Console warning for observability
- Dynamic import (line 84) elegantly avoids circular dependency with
baseApi.ts- Conditional redirect (line 88) prevents redirect loops
- Sequential cleanup ensures complete session invalidation
docs/quickstart/gateway/cli-agents.mdx (6)
57-57: LGTM!Clear and concise description that accurately conveys Claude Code's purpose.
19-54: AI summary inconsistency: LibreChat section unchanged.The AI-generated summary claims that "Mirrored these Claude Code changes in the LibreChat subsection, keeping content parallel to the main Claude Code changes." However, the LibreChat section (lines 19-54) shows no changes in the annotated code.
154-174: The example is correct and well-structured. GPT-5 is available as of January 2026, making theopenai/gpt-5reference for the Opus tier override valid. No issues remain.
106-116: Update model names to match currently supported versions.The
--modelflag exists and works as documented, but the model name examples are incorrect. Useclaude-opus-4-1-20250805instead ofclaude-opus-4-5-20251101andclaude-3-5-haiku-20241022instead ofclaude-haiku-4-5-20251001:Corrected code block
# Start with Opus claude --model claude-opus-4-1-20250805 # Start with Haiku for lightweight tasks claude --model claude-3-5-haiku-20241022Likely an incorrect or invalid review comment.
83-104: No issues found. These are official Claude Code environment variables documented in Claude Code's official documentation. The documentation correctly describes how to configure Claude Code's model tier defaults (Sonnet, Opus, Haiku) when routing through Bifrost, allowing users to replace these tiers with any Bifrost-configured model while maintaining the required tool use capabilities.
118-144: Verify whether Claude Code supports the/modelcommand and all described functionality.The documentation describes an interactive
/modelcommand for Claude Code with features including shorthand syntax (/model opus), full model names, cross-provider routing via Bifrost (/model vertex/claude-haiku-4-5), and instantaneous context-aware switching. Since Claude Code is an external tool and these claims cannot be verified from the Bifrost codebase, you should confirm these capabilities exist in Claude Code's actual implementation before publishing this documentation.
Pratham-Mishra04
force-pushed
the
01-05-feat_auth_config_fixes_and_enhancements
branch
from
bb4d584 to
688a6ab
Compare
Pratham-Mishra04
force-pushed
the
01-05-feat_auth_config_fixes_and_enhancements
branch
from
688a6ab to
6bb4cdf
Compare
Pratham-Mishra04
changed the title
Pratham-Mishra04
changed the title
Pratham-Mishra04
force-pushed
the
01-05-feat_auth_config_fixes_and_enhancements
branch
2 times, most recently
from
adadfe1 to
7364a37
Compare
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 (1)
transports/bifrost-http/handlers/middlewares.go (1)
196-203: Auth config is captured at middleware creation time, not per-request — updates won't take effect.The
authConfigis loaded once whenMiddleware()is called (at server bootstrap), not on each incoming request. This means:
- If auth is initially disabled, the pass-through middleware is returned forever—even if auth is later enabled via
UpdateAuthConfig().- If the admin password is changed at runtime, the middleware continues using the old credentials captured at startup (used in the comparison at lines 261-273).
This defeats the PR's core objective of automatic logout when auth configuration changes.
🔎 Proposed fix: Load authConfig inside the request handler
func (m *AuthMiddleware) Middleware() lib.BifrostHTTPMiddleware { - authConfig := m.authConfig.Load() - if authConfig == nil || !authConfig.IsEnabled { - logger.Debug("auth middleware is disabled because auth config is not present or not enabled") - return func(next fasthttp.RequestHandler) fasthttp.RequestHandler { - return next - } - } whitelistedRoutes := []string{ "/api/session/is-auth-enabled", "/api/session/login", "/api/session/logout", "/health", } return func(next fasthttp.RequestHandler) fasthttp.RequestHandler { return func(ctx *fasthttp.RequestCtx) { + // Load auth config on each request to pick up runtime updates + authConfig := m.authConfig.Load() + if authConfig == nil || !authConfig.IsEnabled { + next(ctx) + return + } // We skip authorization for the login route if slices.Contains(whitelistedRoutes, string(ctx.Request.URI().RequestURI())) {Also update the credential comparison block (lines 261-273) to use the freshly loaded
authConfigvariable.
🤖 Fix all issues with AI Agents
In @transports/bifrost-http/handlers/config.go:
- Around line 482-501: The current flow flushes sessions first
(h.store.ConfigStore.FlushSessions) then reads the new auth config
(h.store.ConfigStore.GetAuthConfig) and updates middleware
(GetAuthMiddleware().UpdateAuthConfig), which can leave sessions cleared if
GetAuthConfig fails; change the order so you first persist the new auth config,
then call GetAuthConfig, then call authMiddleware.UpdateAuthConfig, and only
after those succeed call h.store.ConfigStore.FlushSessions; ensure error
handling returns immediately on any failure before flushing, and reuse the same
authConfig object for UpdateAuthConfig to avoid race/inconsistency.
🧹 Nitpick comments (2)
docs/quickstart/gateway/setting-up-auth.mdx (1)
46-48: Document automatic logout behavior when credentials change.While the "no restart required" statement is accurate, this section should also inform users that changing authentication credentials (username or password) will automatically log out all currently logged-in users. This is a security feature introduced in this PR but isn't documented, which could surprise users.
Based on learnings: The PR test instructions mention "change admin password in UI, verify automatic logout," confirming this behavior is intentional.
Suggested documentation enhancement
### Step 4: Save Changes 1. Click **Save Changes** to apply your authentication settings 2. Changes take effect immediately - no restart required + +<Warning> +If you modify the username or password, all currently logged-in users (including yourself) will be automatically logged out for security reasons. You will need to log in again with the new credentials. +</Warning>.github/workflows/openapi-bundle.yml (1)
32-33: Consider caching Python dependencies for faster workflow runs.While
pyyamlinstalls quickly, adding dependency caching would improve workflow performance and reduce registry load.🔎 Proposed enhancement with pip caching
- name: Set up Python uses: actions/setup-python@v5 with: python-version: "3.11" + cache: 'pip' + cache-dependency-path: '**/requirements*.txt' - name: Install dependencies run: pip install pyyamlAlternatively, if you create a
docs/openapi/requirements.txtfile:- name: Set up Python uses: actions/setup-python@v5 with: python-version: "3.11" + cache: 'pip' - name: Install dependencies - run: pip install pyyaml + run: pip install -r docs/openapi/requirements.txt
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/openapi-bundle.ymldocs/features/mcp/agent-mode.mdxdocs/openapi/openapi.jsondocs/quickstart/gateway/cli-agents.mdxdocs/quickstart/gateway/provider-configuration.mdxdocs/quickstart/gateway/setting-up-auth.mdxframework/changelog.mdframework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/views/securityView.tsxui/lib/store/apis/baseApi.tsui/lib/types/config.ts
💤 Files with no reviewable changes (1)
- framework/configstore/tables/config.go
✅ Files skipped from review due to trivial changes (3)
- docs/quickstart/gateway/provider-configuration.mdx
- ui/lib/types/config.ts
- framework/configstore/clientconfig.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/features/mcp/agent-mode.mdx
- transports/changelog.md
- framework/configstore/rdb.go
- ui/lib/store/apis/baseApi.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/store.godocs/quickstart/gateway/cli-agents.mdxui/app/workspace/config/views/securityView.tsxtransports/bifrost-http/server/server.godocs/quickstart/gateway/setting-up-auth.mdxtransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/middlewares.goframework/changelog.md
🧠 Learnings (5)
📚 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/store.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
framework/configstore/store.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/quickstart/gateway/cli-agents.mdxdocs/quickstart/gateway/setting-up-auth.mdx
📚 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/lib/config.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/middlewares.go
🪛 GitHub Actions: PR #1251
transports/bifrost-http/handlers/config.go
[error] 484-484: Method 'FlushSessions' is undefined for type 'configstore.ConfigStore'. This indicates a missing implementation or incorrect usage.
🔇 Additional comments (22)
docs/quickstart/gateway/setting-up-auth.mdx (2)
91-97: LGTM!The "No Restart Required" note accurately reflects the immediate effect of authentication changes implemented in this PR. The documentation is clear and informative.
98-106: LGTM!The addition of "immediately" correctly reflects the instant effect when disabling authentication, consistent with the broader changes in this PR that eliminate restart requirements.
.github/workflows/openapi-bundle.yml (1)
3-12: LGTM! Well-structured trigger configuration.The path filters correctly trigger the workflow on changes to OpenAPI source files while excluding the generated bundle. This prevents infinite workflow loops.
docs/quickstart/gateway/cli-agents.mdx (5)
57-57: LGTM!The updated description is clear and accurately describes Claude Code's terminal-based coding capabilities.
106-144: Excellent documentation for model switching workflows.The section clearly explains both initial model selection and mid-session switching with practical examples. The warnings about tool capabilities and provider-specific features are particularly valuable for preventing user confusion.
83-174: Comprehensive model configuration documentation.The new Model Configuration section significantly enhances the documentation by providing clear, actionable guidance on model selection, switching, and customization. The structured approach with warnings, tips, and practical examples will greatly help users understand and leverage Bifrost's model flexibility with Claude Code.
146-152: Update Opus model version in the table.The table still lists Opus 4.5, but according to the previous review comment (which included web verification), the latest confirmed public release for Opus is version 4.1 (August 2025), not 4.5. Please update the table row to reflect Opus 4.1 for accuracy.
This same issue also affects:
- Line 96:
"anthropic/claude-opus-4-5-20251101"- Line 112:
claude --model claude-opus-4-5-20251101- Line 129:
/model claude-opus-4-5-20251101All references to Opus 4.5 throughout the document should be updated to Opus 4.1 with the appropriate date identifier.
Likely an incorrect or invalid review comment.
83-104: No issues found. The environment variable names (ANTHROPIC_DEFAULT_SONNET_MODEL,ANTHROPIC_DEFAULT_OPUS_MODEL,ANTHROPIC_DEFAULT_HAIKU_MODEL) are correct and match Claude Code's configuration interface. The provider-prefixed model format (openai/,anthropic/,azure/) is the standard Bifrost notation for cross-provider model routing. Claude Opus 4.5 was released on November 24, 2025, confirming the model version is current. All referenced models natively support tool use capabilities required by Claude Code.ui/app/workspace/config/views/securityView.tsx (2)
102-121: LGTM: Auth handlers simplified correctly.The removal of restart tracking from authentication handlers aligns with the PR's objective of eliminating restart requirements for auth config changes. The empty dependency arrays are correct since these handlers only use stable setState functions.
123-146: LGTM: Save handler correctly preserves restart tracking for non-auth changes.The
setNeedsRestart(false)at line 142 still applies to origin changes and other restart-requiring modifications. Only auth-specific restart tracking was removed, which is correct.framework/changelog.md (1)
1-1: LGTM: Changelog entry accurately documents the new feature.framework/configstore/store.go (1)
135-135: LGTM: New FlushSessions method added to interface.The addition of
FlushSessions(ctx context.Context) errorto the ConfigStore interface is correct and aligns with the PR objective of automatically logging out users when auth configuration changes.transports/bifrost-http/lib/config.go (1)
1415-1417: LGTM: Auth config now requires config store.The change to require a config store for loading auth configuration (rather than falling back to file/environment) aligns with the centralized authentication middleware approach introduced in this PR.
transports/bifrost-http/handlers/config.go (3)
466-480: LGTM: Credential preservation when disabling auth is good UX.The logic to preserve
AdminUserNameandAdminPasswordwhen disabling authentication ensures that users don't lose their credentials when temporarily disabling auth. This is a thoughtful UX improvement.
482-489: The FlushSessions method is properly defined and implemented. The interface exists inframework/configstore/store.go:135, and the implementation is present inframework/configstore/rdb.go:2153-2155forRDBConfigStore. The code at line 484 is correct and will compile successfully.Likely an incorrect or invalid review comment.
491-501: Remove concern about nil authMiddleware—this cannot occur in practice.The auth middleware is initialized during server bootstrap in
server.go:1174with error handling that prevents server startup if initialization fails. The config update handler is only reachable after successful server initialization, soGetAuthMiddleware()is guaranteed to return a non-nil value at line 492. The existing nil check is defensive but unnecessary given the initialization guarantees.No changes are required; the code is safe as written.
transports/bifrost-http/server/server.go (2)
1174-1180: LGTM on middleware initialization.The auth middleware is properly initialized with the config store and error handling is correct. The singleton pattern allows the middleware to be shared across route groups.
1187-1189: Consistent middleware application for inference routes.The same auth middleware instance is correctly applied to inference routes for non-enterprise contexts, maintaining parity with API routes.
transports/bifrost-http/handlers/middlewares.go (4)
160-165: LGTM on singleton structure.The
AuthMiddlewarestruct withatomic.Pointer[configstore.AuthConfig]is correctly set up for thread-safe config updates.
167-190: Initialization and update methods are correctly implemented.
InitAuthMiddlewareproperly validates the store, fetches the initial config, and stores it atomically.UpdateAuthConfigcorrectly uses atomic store for thread-safe updates.
229-232: LGTM on WebSocket session validation.Correctly uses the receiver's store (
m.store) for session validation.
279-287: LGTM on Bearer token session validation.Correctly uses
m.storefor session validation.
Pratham-Mishra04
force-pushed
the
01-05-feat_auth_config_fixes_and_enhancements
branch
from
7364a37 to
b7be645
Compare
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 (1)
transports/bifrost-http/handlers/middlewares.go (1)
160-291: Auth middleware ignoresDisableAuthOnInferenceand the comment is staleThe new
AuthMiddlewaresingleton +atomic.Pointerdesign is solid and enables live config updates, but there are two behavior/polish issues:
DisableAuthOnInferenceis no longer honored
AuthConfigstill exposesDisableAuthOnInference, butMiddleware():
- Always applies the same checks to every route (except the small whitelist), and
- Never inspects
authConfig.DisableAuthOnInference.Given
Bootstrapnow attaches this middleware to both API and inference routes, any deployment that previously setDisableAuthOnInference = truewill see inference endpoints start requiring auth again, despite the flag and UI suggesting otherwise.To preserve the existing contract while still centralizing auth, you likely want to short‑circuit on inference routes when that flag is set, e.g.:
func (m *AuthMiddleware) Middleware() lib.BifrostHTTPMiddleware { whitelistedRoutes := []string{ "/api/session/is-auth-enabled", "/api/session/login", "/api/session/logout", "/health", } return func(next fasthttp.RequestHandler) fasthttp.RequestHandler { return func(ctx *fasthttp.RequestCtx) { authConfig := m.authConfig.Load() if authConfig == nil || !authConfig.IsEnabled { logger.Debug("auth middleware is disabled because auth config is not present or not enabled") next(ctx) return } + // Optionally skip auth for inference routes when configured + if authConfig.DisableAuthOnInference && isInferenceRoute(ctx) { + next(ctx) + return + } // existing whitelist + Basic/Bearer/WebSocket logic... } } } +// isInferenceRoute determines whether the current request targets an inference endpoint. +// Adjust prefixes to match your actual inference routing (OpenAI/Anthropic/Gemini, etc.). +func isInferenceRoute(ctx *fasthttp.RequestCtx) bool { + path := string(ctx.Path()) + return strings.HasPrefix(path, "/v1/") || + strings.HasPrefix(path, "/openai/") || + strings.HasPrefix(path, "/anthropic/") || + strings.HasPrefix(path, "/genai/") +}You can refine
isInferenceRouteto match your actual router layout, but the key is: respectDisableAuthOnInferencesomewhere in this middleware so the flag remains meaningful and dynamically updatable.
- Comment above
Middlewareno longer matches behaviorThe doc block still says:
No session tracking is used, so this is not suitable for production environments
The implementation now clearly uses session tokens (
validateSession+SessionsTable) for Bearer and websocket paths, and is intended for production dashboard/API usage. Updating this comment to reflect the current, session‑based behavior would avoid confusion.
🤖 Fix all issues with AI Agents
In @transports/changelog.md:
- Around line 1-3: Update the changelog line "fix: resolved issue where new auth
config were not being created" to correct the grammar by using either plural or
singular consistently, e.g. "fix: resolved issue where new auth configs were not
being created" or "fix: resolved issue where new auth config was not being
created", replacing the existing sentence in transports/changelog.md.
🧹 Nitpick comments (2)
framework/configstore/store.go (1)
131-135: Interface addition forFlushSessionslooks good; consider documenting semanticsAdding
FlushSessions(ctx context.Context) erroralongside the other session CRUD methods is consistent with the rest of the interface and fits the “force logout all sessions” use case.It may be worth adding a brief comment in this interface block clarifying that
FlushSessionsis expected to delete all sessions (and whether it should be best-effort vs. fatal on partial failures), to guide any alternative implementations.transports/bifrost-http/lib/config.go (1)
1403-1417: Auth config now correctly depends on ConfigStore, but surfaceGetAuthConfigerrorsThe new behavior—only seeding auth config into the store when
ConfigStoreis present and has no existing auth row, and warning whenConfigStoreis nil—matches the new “auth must live in the store” invariant.One small improvement: if
ConfigStore.GetAuthConfigreturns an error, it’s currently ignored. Logging that error (and skipping the seed) would make it much easier to debug misconfigured or unavailable stores during bootstrap.For example:
if config.ConfigStore != nil { configStoreAuthConfig, err := config.ConfigStore.GetAuthConfig(ctx) - if err == nil && configStoreAuthConfig == nil { + if err == nil && configStoreAuthConfig == nil { if err := config.ConfigStore.UpdateAuthConfig(ctx, configData.AuthConfig); err != nil { logger.Warn("failed to update auth config: %v", err) } + } else if err != nil { + logger.Warn("failed to get auth config from store: %v", err) } } else {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/openapi-bundle.ymldocs/features/mcp/agent-mode.mdxdocs/openapi/openapi.jsondocs/quickstart/gateway/cli-agents.mdxdocs/quickstart/gateway/provider-configuration.mdxdocs/quickstart/gateway/setting-up-auth.mdxframework/changelog.mdframework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/views/securityView.tsxui/lib/store/apis/baseApi.tsui/lib/types/config.ts
💤 Files with no reviewable changes (1)
- framework/configstore/tables/config.go
✅ Files skipped from review due to trivial changes (2)
- framework/changelog.md
- ui/lib/types/config.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/quickstart/gateway/setting-up-auth.mdx
- ui/lib/store/apis/baseApi.ts
- framework/configstore/rdb.go
- framework/configstore/clientconfig.go
- ui/app/workspace/config/views/securityView.tsx
- docs/quickstart/gateway/provider-configuration.mdx
- transports/bifrost-http/handlers/config.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/server/server.goframework/configstore/store.godocs/quickstart/gateway/cli-agents.mdxdocs/features/mcp/agent-mode.mdxtransports/changelog.mdtransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.go
🧠 Learnings (5)
📚 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.goframework/configstore/store.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
transports/bifrost-http/server/server.goframework/configstore/store.gotransports/bifrost-http/handlers/middlewares.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/server/server.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/quickstart/gateway/cli-agents.mdxdocs/features/mcp/agent-mode.mdx
🧬 Code graph analysis (2)
transports/bifrost-http/server/server.go (2)
transports/bifrost-http/handlers/middlewares.go (1)
InitAuthMiddleware(167-182)framework/configstore/store.go (1)
ConfigStore(17-158)
transports/bifrost-http/handlers/middlewares.go (3)
framework/configstore/clientconfig.go (1)
AuthConfig(724-729)ui/lib/types/config.ts (1)
AuthConfig(267-272)transports/bifrost-http/lib/middleware.go (1)
BifrostHTTPMiddleware(7-7)
🔇 Additional comments (3)
docs/features/mcp/agent-mode.mdx (1)
16-18: Excellent documentation addition.The warning is clear, well-placed, and includes both the limitation and the technical rationale. The explanation about why streaming cannot work with autonomous tool execution (cannot speculatively buffer all chunks) helps users understand the architectural constraint rather than just stating a restriction.
docs/quickstart/gateway/cli-agents.mdx (1)
83-170: Claude Code model config docs look accurate and aligned with current model IDsThe new Claude Code sections (env overrides,
/modelusage, and the model selection guide) are consistent with the current Claude 4.5 family model IDs and how Claude Code selects models. Nothing blocking from a correctness perspective.transports/bifrost-http/server/server.go (1)
1170-1193: Bootstrap hard-fails on nil ConfigStore, breaking deployments whereconfig_store.enabled: falseThe code now unconditionally calls:
authMiddleware, err := handlers.InitAuthMiddleware(s.Config.ConfigStore) if err != nil { return fmt.Errorf("failed to initialize auth middleware: %v", err) }This breaks deployments in two scenarios:
Intentionally disabled ConfigStore: The
.github/workflows/configs/noconfigstorenologstore/config.jsonshows a supported configuration whereconfig_store.enabled: false. Your code already handles nil ConfigStore elsewhere (e.g.,config.go:1295), but Bootstrap now fails unconditionally.Enterprise mode: The middleware is only appended when
ctx.Value("isEnterprise") == nil(lines 1178, 1187), yet Bootstrap initializes it regardless. In enterprise mode, initialization is wasted effort and fails unnecessarily.The
AuthMiddleware.Middleware()itself already gracefully handles nil authConfig, andGetAuthConfig()returnsnil, nilwhen no auth is configured—so the plumbing is there. Either:
- Skip
InitAuthMiddlewarewhenConfigStoreis nil, or- Move the initialization inside the
if ctx.Value("isEnterprise") == nilguard to avoid initializing unused middleware.
Pratham-Mishra04
force-pushed
the
01-05-feat_auth_config_fixes_and_enhancements
branch
2 times, most recently
from
c60b02d to
73e64f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @transports/bifrost-http/handlers/config.go:
- Around line 490-500: GetAuthMiddleware() may return nil and the code currently
skips updating middleware silently; change the block that calls
GetAuthMiddleware() so when authMiddleware == nil you log a warning (using
logger.Warn or similar) or return an error to surface the unexpected state, and
otherwise call authMiddleware.UpdateAuthConfig(authConfig) as before — update
the handler around GetAuthMiddleware, authMiddleware, and UpdateAuthConfig to
ensure a clear warning or error is emitted when the middleware is missing.
🧹 Nitpick comments (2)
.github/workflows/openapi-bundle.yml (2)
32-33: Consider pinning the pyyaml version.Installing
pyyamlwithout a version constraint may lead to reproducibility issues if the package releases breaking changes. Consider pinning to a specific version:- - name: Install dependencies - run: pip install pyyaml + - name: Install dependencies + run: pip install pyyaml==6.0.1Alternatively, use a
requirements.txtfile for better dependency management.
48-55: Consider adding error handling for the git push operation.The commit and push logic is correct and appropriately conditioned to only run on push events (not pull requests). However, there's no verification that the push succeeds. While Git operations typically fail fast with non-zero exit codes (causing the workflow to fail), you might consider adding explicit error handling for better observability:
🔎 Optional: Add explicit push error handling
- name: Commit and push changes if: steps.changes.outputs.changed == 'true' && github.event_name == 'push' run: | git config --local user.email "github-actions[bot]@users.noreply.github.com" git config --local user.name "github-actions[bot]" git add docs/openapi/openapi.json git commit -m "chore: regenerate openapi.json --skip-pipeline" - git push + if ! git push; then + echo "::error::Failed to push changes to repository" + exit 1 + fi
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.github/workflows/openapi-bundle.ymldocs/features/mcp/agent-mode.mdxdocs/openapi/openapi.jsondocs/quickstart/gateway/cli-agents.mdxdocs/quickstart/gateway/provider-configuration.mdxdocs/quickstart/gateway/setting-up-auth.mdxframework/changelog.mdframework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdui/app/workspace/config/views/securityView.tsxui/lib/store/apis/baseApi.tsui/lib/types/config.ts
💤 Files with no reviewable changes (1)
- framework/configstore/tables/config.go
✅ Files skipped from review due to trivial changes (2)
- ui/lib/types/config.ts
- framework/changelog.md
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/app/workspace/config/views/securityView.tsx
- transports/changelog.md
- framework/configstore/store.go
- transports/bifrost-http/lib/config.go
- framework/configstore/rdb.go
- docs/quickstart/gateway/provider-configuration.mdx
- docs/features/mcp/agent-mode.mdx
- ui/lib/store/apis/baseApi.ts
- transports/bifrost-http/server/server.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/handlers/config.goframework/configstore/clientconfig.godocs/quickstart/gateway/cli-agents.mdxdocs/quickstart/gateway/setting-up-auth.mdxtransports/bifrost-http/handlers/middlewares.go
🧠 Learnings (5)
📚 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/handlers/config.goframework/configstore/clientconfig.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
transports/bifrost-http/handlers/config.goframework/configstore/clientconfig.gotransports/bifrost-http/handlers/middlewares.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/config.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.go
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/quickstart/gateway/cli-agents.mdxdocs/quickstart/gateway/setting-up-auth.mdx
🧬 Code graph analysis (2)
transports/bifrost-http/handlers/config.go (5)
framework/configstore/clientconfig.go (1)
AuthConfig(724-729)ui/lib/types/config.ts (1)
AuthConfig(267-272)core/providers/gemini/types.go (1)
AuthConfig(360-373)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)transports/bifrost-http/handlers/middlewares.go (1)
GetAuthMiddleware(184-186)
framework/configstore/clientconfig.go (2)
ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(310-313)framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(44-47)
🪛 GitHub Actions: PR #1251
transports/bifrost-http/handlers/config.go
[error] 483-483: method 'FlushSessions' not found in 'configstore.ConfigStore' type
🔇 Additional comments (12)
.github/workflows/openapi-bundle.yml (5)
1-12: LGTM! Trigger configuration is correct.The workflow correctly triggers on changes to OpenAPI source files while excluding the generated bundle (
openapi.json) to prevent infinite loops. The path filters are properly configured for both push and pull request events.
14-15: LGTM! Appropriate permissions.The
contents: writepermission is necessary for the workflow to commit and push the bundled OpenAPI spec back to the repository. The permission scope follows the principle of least privilege.
27-30: LGTM! Python setup is appropriate.Pinning Python to version 3.11 ensures reproducible builds across workflow runs.
35-37: LGTM! Bundle execution is straightforward.The script execution is correctly configured with the appropriate working directory.
39-46: LGTM! Change detection logic is correct.The use of
git diff --quietwithGITHUB_OUTPUTfollows current GitHub Actions best practices for detecting and communicating changes.docs/quickstart/gateway/cli-agents.mdx (2)
57-57: Clear and accurate description.The updated description effectively conveys Claude Code's purpose and fits well within the documentation flow.
83-144: Excellent documentation structure and content.This Model Configuration section is comprehensive and well-organized:
- Clear explanation of the three-tier model system
- Practical examples for environment variable overrides
- Helpful warnings about tool-use requirements and feature compatibility
- Demonstrates Bifrost's cross-provider flexibility effectively
The examples and tips will significantly improve the user experience for Claude Code integration.
transports/bifrost-http/handlers/middlewares.go (2)
196-291: LGTM! Centralized auth middleware with atomic config updates.The refactored middleware correctly:
- Uses atomic config storage for thread-safe runtime updates
- Whitelists
/api/session/logoutand/healthappropriately- Handles WebSocket authentication via query token
- Consistently validates sessions using the instance store
This enables dynamic auth config changes without restart, aligning with the PR objective.
184-186: No changes needed. The nil case is already handled appropriately.
GetAuthMiddleware()returns a package-level singleton initialized during server startup. While theoretically it could returnnilifInitAuthMiddleware()is never called, server startup fails if initialization doesn't succeed, so in practicenilis only a concern during the initialization window. The only caller at config.go:491 already includes a proper nil check (if authMiddleware != nil { ... }), making the code safe and correct as-is.Likely an incorrect or invalid review comment.
transports/bifrost-http/handlers/config.go (2)
416-430: LGTM! Auth change detection logic is correct.The logic properly identifies authentication changes by comparing:
- Enabled state transitions
- Username modifications
- Password changes (excluding redacted/empty values)
This ensures sessions are only flushed when credentials actually change.
481-500: FlushSessions method exists and is properly implemented—this is not a critical issue.The
FlushSessionsmethod is defined in the ConfigStore interface atframework/configstore/store.go:135and implemented inframework/configstore/rdb.go:2153. The code at line 483 is valid and will compile without errors.Likely an incorrect or invalid review comment.
docs/quickstart/gateway/setting-up-auth.mdx (1)
48-48: LGTM! Documentation accurately reflects no-restart authentication updates.The documentation correctly states that authentication changes take effect immediately, which aligns with the code implementation:
- Atomic config updates in the middleware
- Session flushing on credential changes
- Runtime middleware configuration refresh
The updates are consistent across all relevant sections (Step 4, Important Notes, and Disabling Authentication).
Also applies to: 93-93, 106-106
Pratham-Mishra04
force-pushed
the
01-05-feat_auth_config_fixes_and_enhancements
branch
from
73e64f0 to
edd1470
Compare
Pratham-Mishra04
force-pushed
the
01-05-feat_auth_config_fixes_and_enhancements
branch
from
edd1470 to
eb99204
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @transports/bifrost-http/handlers/middlewares.go:
- Around line 185-188: The comment above AuthMiddleware is outdated: it claims
"No session tracking is used" but the code calls validateSession() for WebSocket
and Bearer flows (see validateSession, WebSocket handling and Bearer token
handling). Update the comment to accurately state that session validation is
performed (via validateSession) for WS and Bearer token flows, remove the "not
suitable for production" blanket statement or qualify it (e.g., basic auth with
session validation may be acceptable for limited use), and mention which routes
use basic auth vs session-validated tokens to avoid confusion.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
.github/workflows/openapi-bundle.ymldocs/features/mcp/agent-mode.mdxdocs/openapi/openapi.jsondocs/quickstart/gateway/cli-agents.mdxdocs/quickstart/gateway/provider-configuration.mdxdocs/quickstart/gateway/setting-up-auth.mdxframework/changelog.mdframework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.goframework/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/otel/changelog.mdplugins/otel/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/middlewares_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdtransports/versionui/app/workspace/config/views/securityView.tsxui/lib/store/apis/baseApi.tsui/lib/types/config.ts
💤 Files with no reviewable changes (1)
- framework/configstore/tables/config.go
✅ Files skipped from review due to trivial changes (16)
- plugins/jsonparser/changelog.md
- plugins/logging/version
- plugins/semanticcache/changelog.md
- plugins/governance/changelog.md
- plugins/otel/version
- plugins/governance/version
- plugins/semanticcache/version
- plugins/logging/changelog.md
- ui/lib/store/apis/baseApi.ts
- framework/version
- plugins/maxim/changelog.md
- plugins/jsonparser/version
- plugins/telemetry/version
- plugins/maxim/version
- transports/version
- ui/lib/types/config.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/quickstart/gateway/provider-configuration.mdx
- framework/changelog.md
- framework/configstore/store.go
- ui/app/workspace/config/views/securityView.tsx
- transports/bifrost-http/lib/config.go
- docs/quickstart/gateway/setting-up-auth.mdx
- .github/workflows/openapi-bundle.yml
- docs/quickstart/gateway/cli-agents.mdx
- framework/configstore/rdb.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/changelog.mdplugins/otel/changelog.mdplugins/telemetry/changelog.mdtransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares_test.gotransports/bifrost-http/handlers/middlewares.godocs/features/mcp/agent-mode.mdxtransports/bifrost-http/handlers/config.goframework/configstore/clientconfig.go
🧠 Learnings (5)
📚 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/middlewares_test.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/config.goframework/configstore/clientconfig.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares_test.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/config.goframework/configstore/clientconfig.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/middlewares_test.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/config.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/middlewares_test.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/config.go
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/features/mcp/agent-mode.mdx
🧬 Code graph analysis (5)
transports/bifrost-http/server/server.go (4)
transports/bifrost-http/handlers/middlewares.go (2)
AuthMiddleware(160-163)InitAuthMiddleware(165-179)framework/configstore/clientconfig.go (1)
AuthConfig(724-729)ui/lib/types/config.ts (1)
AuthConfig(267-272)framework/configstore/store.go (1)
ConfigStore(17-158)
transports/bifrost-http/handlers/middlewares_test.go (5)
transports/bifrost-http/server/server.go (1)
SetLogger(117-119)transports/bifrost-http/handlers/middlewares.go (1)
AuthMiddleware(160-163)framework/configstore/clientconfig.go (1)
AuthConfig(724-729)ui/lib/types/config.ts (1)
AuthConfig(267-272)plugins/mocker/main.go (1)
Response(97-103)
transports/bifrost-http/handlers/middlewares.go (4)
framework/configstore/store.go (1)
ConfigStore(17-158)framework/configstore/clientconfig.go (1)
AuthConfig(724-729)ui/lib/types/config.ts (1)
AuthConfig(267-272)transports/bifrost-http/lib/middleware.go (1)
BifrostHTTPMiddleware(7-7)
transports/bifrost-http/handlers/config.go (4)
framework/configstore/clientconfig.go (1)
AuthConfig(724-729)ui/lib/types/config.ts (1)
AuthConfig(267-272)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)framework/configstore/store.go (1)
ConfigStore(17-158)
framework/configstore/clientconfig.go (2)
ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(310-313)framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(44-47)
🪛 GitHub Actions: PR #1251
transports/bifrost-http/handlers/config.go
[error] 483-483: The method 'FlushSessions' is undefined for type 'configstore.ConfigStore' in the file 'config.go'.
🔇 Additional comments (18)
plugins/telemetry/changelog.md (1)
1-1: ✓ LGTMChangelog entry accurately documents the framework version bump to 1.1.59 using conventional changelog format.
plugins/otel/changelog.md (1)
1-1: ✓ LGTMChangelog entry consistently documents the framework version bump to 1.1.59 across plugins, maintaining uniform convention across the codebase.
framework/configstore/clientconfig.go (1)
37-50: LGTM - Formatting improvements for readability.The struct tag alignment changes improve code readability without affecting functionality or serialization behavior.
transports/changelog.md (1)
1-3: LGTM - Clear changelog entries.The changelog entries accurately document the PR changes and align with the stated objectives. The grammar issue from the previous review has been resolved.
docs/features/mcp/agent-mode.mdx (1)
16-18: LGTM - Clear and helpful warning.The warning clearly communicates the streaming limitation with Agent Mode, explains the architectural rationale, and provides actionable guidance for users.
transports/bifrost-http/handlers/config.go (4)
416-430: LGTM - Auth change detection logic is sound.The change detection correctly identifies when authentication configuration has meaningfully changed, handling both initial setup and updates while properly treating
"<redacted>"as indicating no password change.
458-479: LGTM - Consistent auth config persistence with credential preservation.The logic correctly handles both enabling and disabling auth while preserving existing credentials when appropriate. The use of
configManager.UpdateAuthConfigprovides a clean abstraction for persistence.
489-489: LGTM - Helpful architectural comment.The comment clearly documents that AuthMiddleware updates are handled via the ServerCallbacks pattern, providing useful context for maintainers.
481-488: No issues found with the session flush logic.The
FlushSessions(ctx context.Context) errormethod is properly defined in theConfigStoreinterface (framework/configstore/store.go, line 36) and implemented inRDBConfigStore(framework/configstore/rdb.go). The code at lines 481-488 will compile and execute correctly.Likely an incorrect or invalid review comment.
transports/bifrost-http/handlers/middlewares_test.go (1)
529-746: LGTM - Comprehensive AuthMiddleware test coverage.The test suite thoroughly validates AuthMiddleware behavior across all configuration states (nil, disabled, enabled) and dynamic transitions. The tests correctly verify request blocking, whitelisted route handling, and runtime configuration updates.
transports/bifrost-http/handlers/middlewares.go (5)
160-163: LGTM! Thread-safe struct design.Using
atomic.PointerforauthConfigensures lock-free concurrent reads/writes, which is appropriate for a frequently accessed middleware configuration.
165-179: LGTM! Clean initialization with proper validation.The function validates the store, fetches the initial config, and atomically stores it. Error handling is appropriate.
198-203: LGTM! Correct short-circuit for disabled auth.When auth config is nil or disabled, the middleware correctly passes through to the next handler with an appropriate debug log.
211-228: LGTM! WebSocket upgrade handling with token validation.The WebSocket upgrade path correctly extracts the token from query params and validates the session before allowing the upgrade.
239-269: LGTM! Basic auth flow is correctly implemented.The Basic auth flow properly decodes base64, splits credentials, validates username, and uses secure hash comparison for the password.
transports/bifrost-http/server/server.go (3)
111-111: LGTM! Field addition for centralized auth middleware.Adding the
AuthMiddlewarefield to the server struct enables runtime updates without restart, which aligns with the PR objective.
682-711: LGTM! UpdateAuthConfig with proper validation and in-memory sync.The implementation:
- Validates auth config is not nil
- Requires credentials when enabling auth (but allows disabling without them)
- Persists to config store
- Syncs the in-memory AuthMiddleware config
The fallback at lines 700-708 is reasonable - if fetching the updated config fails, it still applies the original update to avoid leaving the middleware in an inconsistent state.
1192-1207: LGTM! Auth middleware initialization and conditional application.The middleware is correctly initialized once and applied to both API and inference routes when not in enterprise mode. The
isEnterprisecheck is intentionally duplicated becauseapiMiddlewaresandinferenceMiddlewaresare built separately (inference also getsTransportInterceptorMiddlewareprepended at line 1209).
Pratham-Mishra04
force-pushed
the
01-05-feat_auth_config_fixes_and_enhancements
branch
from
eb99204 to
3f4cd79
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
transports/bifrost-http/server/server.go (1)
85-112: Auth middleware wiring is coherent; add nil‑middleware visibility and confirm empty‑store behaviorThe new flow—central
AuthMiddlewarefield onBifrostHTTPServer,UpdateAuthConfigwriting viaConfigStoreand then updating the in‑memory middleware, and Bootstrap wiring.Middleware()into both API and inference stacks (non‑enterprise)—matches the intended design of live‑updatable, in‑memory auth without restarts.A couple of edge points to tighten:
- In
UpdateAuthConfig, whens.AuthMiddlewareis unexpectedlynil, the update is silently skipped. Logging a warning in that branch would make mis‑wiring much easier to diagnose.InitAuthMiddlewarefails the bootstrap on anyGetAuthConfigerror. IfGetAuthConfigreturns a sentinel likeErrNotFoundfor “no auth config yet,” you probably want to treat that as(nil, nil)and still create a middleware instance withauthConfig=nilso a fresh install without auth configured can still start.[summary]
The design is solid; a small log for nil middleware and confirming “no auth config” behavior would improve robustness.Also applies to: 682-711, 1192-1207
transports/bifrost-http/handlers/middlewares.go (1)
160-183: AuthMiddleware structure and atomic config updates look solid; verify inference‑auth toggle is enforcedThe new
AuthMiddlewarestruct with anatomic.Pointer[configstore.AuthConfig]and instance‑scopedMiddleware()gives you a clean, lock‑free way to read auth config on the hot path, andUpdateAuthConfig’s atomic store is exactly what you want here.Two things worth a quick check:
DisableAuthOnInference: the UI exposes this flag andAuthConfigcarries it, but it isn’t referenced in this middleware. Given the middleware is applied to both API and inference routes, you likely want to skip or relax auth checks for inference endpoints whenauthConfig.DisableAuthOnInferenceis true (e.g., by path prefix or handler grouping). Please verify where this flag is actually enforced and that toggling it in the UI has the intended effect.- Init behavior:
InitAuthMiddlewarecurrently treats anyGetAuthConfigerror as fatal. EnsureGetAuthConfigreturns(nil, nil)rather than an error for “no auth configured yet”; otherwise, a fresh install with an empty auth table could fail to bootstrap.Overall, the new middleware design is a nice improvement in clarity and concurrency safety.
Also applies to: 185-287
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
.github/workflows/openapi-bundle.ymldocs/features/mcp/agent-mode.mdxdocs/openapi/openapi.jsondocs/quickstart/gateway/cli-agents.mdxdocs/quickstart/gateway/provider-configuration.mdxdocs/quickstart/gateway/setting-up-auth.mdxframework/changelog.mdframework/configstore/clientconfig.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/config.goframework/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/otel/changelog.mdplugins/otel/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/middlewares_test.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.gotransports/changelog.mdtransports/versionui/app/workspace/config/views/securityView.tsxui/lib/store/apis/baseApi.tsui/lib/types/config.ts
💤 Files with no reviewable changes (1)
- framework/configstore/tables/config.go
✅ Files skipped from review due to trivial changes (5)
- framework/changelog.md
- framework/version
- ui/lib/store/apis/baseApi.ts
- plugins/semanticcache/version
- plugins/maxim/changelog.md
🚧 Files skipped from review as they are similar to previous changes (19)
- plugins/semanticcache/changelog.md
- plugins/governance/changelog.md
- plugins/governance/version
- .github/workflows/openapi-bundle.yml
- plugins/jsonparser/changelog.md
- framework/configstore/store.go
- docs/quickstart/gateway/setting-up-auth.mdx
- plugins/otel/changelog.md
- transports/bifrost-http/handlers/middlewares_test.go
- ui/lib/types/config.ts
- plugins/logging/changelog.md
- transports/changelog.md
- plugins/jsonparser/version
- docs/features/mcp/agent-mode.mdx
- plugins/maxim/version
- plugins/logging/version
- transports/version
- framework/configstore/rdb.go
- plugins/telemetry/version
🧰 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/server/server.gotransports/bifrost-http/handlers/config.goplugins/telemetry/changelog.mdtransports/bifrost-http/handlers/middlewares.goplugins/otel/versiontransports/bifrost-http/lib/config.godocs/quickstart/gateway/provider-configuration.mdxui/app/workspace/config/views/securityView.tsxdocs/quickstart/gateway/cli-agents.mdx
🧠 Learnings (5)
📚 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/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
framework/configstore/clientconfig.gotransports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.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/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-29T09:14:16.633Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 888
File: transports/bifrost-http/handlers/middlewares.go:246-256
Timestamp: 2025-12-29T09:14:16.633Z
Learning: In the bifrost HTTP transport, fasthttp.RequestCtx is the primary context carrier and should be passed directly to functions that expect a context.Context. Do not convert to context.Context unless explicitly required. Ensure tracer implementations and related components are designed to accept fasthttp.RequestCtx directly, and document this architectural decision for maintainers.
Applied to files:
transports/bifrost-http/server/server.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/lib/config.go
📚 Learning: 2025-12-30T05:37:48.365Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1180
File: docs/features/mcp/connecting-to-servers.mdx:452-458
Timestamp: 2025-12-30T05:37:48.365Z
Learning: When reviewing documentation PRs in a Graphite-managed stack, first check related or previous PRs in the stack for feature implementations before flagging documentation as incorrect or unsupported. Documentation MDX files often reference features implemented in earlier stack PRs; verify that the documented behavior exists in earlier changes and that the docs accurately reflect the implemented state before requesting edits.
Applied to files:
docs/quickstart/gateway/provider-configuration.mdxdocs/quickstart/gateway/cli-agents.mdx
🧬 Code graph analysis (4)
framework/configstore/clientconfig.go (2)
ui/lib/types/config.ts (1)
GlobalHeaderFilterConfig(310-313)framework/configstore/tables/config.go (1)
GlobalHeaderFilterConfig(44-47)
transports/bifrost-http/server/server.go (3)
transports/bifrost-http/handlers/middlewares.go (2)
AuthMiddleware(160-163)InitAuthMiddleware(165-179)framework/configstore/clientconfig.go (1)
AuthConfig(724-729)ui/lib/types/config.ts (1)
AuthConfig(267-272)
transports/bifrost-http/handlers/config.go (5)
framework/configstore/clientconfig.go (1)
AuthConfig(724-729)ui/lib/types/config.ts (1)
AuthConfig(267-272)core/providers/gemini/types.go (1)
AuthConfig(360-373)transports/bifrost-http/handlers/utils.go (1)
SendError(35-44)framework/configstore/store.go (1)
ConfigStore(17-158)
transports/bifrost-http/handlers/middlewares.go (3)
framework/configstore/clientconfig.go (1)
AuthConfig(724-729)ui/lib/types/config.ts (1)
AuthConfig(267-272)transports/bifrost-http/lib/middleware.go (1)
BifrostHTTPMiddleware(7-7)
🪛 GitHub Actions: PR #1251
transports/bifrost-http/handlers/config.go
[error] 483-483: Undefined method or field 'FlushSessions' for type 'configstore.ConfigStore'.
🔇 Additional comments (9)
plugins/otel/version (1)
1-1: LGTM! Version bump aligns with PR scope.The patch version increment is appropriate for the bug fixes and enhancements included in this PR.
plugins/telemetry/changelog.md (1)
1-1: LGTM! Changelog entry follows conventional commit format.The changelog appropriately documents the framework version dependency update using standard conventional commit style.
framework/configstore/clientconfig.go (1)
37-50: LGTM! Formatting improvements for consistency.The struct field tag alignment improves readability and maintains a consistent code style. The
ConfigHashfield correctly remains non-serialized (json:"-"), which is appropriate for its use in reconciliation logic.docs/quickstart/gateway/cli-agents.mdx (2)
83-144: LGTM: Comprehensive model configuration documentation.The Model Configuration section is well-structured and provides clear guidance for:
- Overriding default model tiers via environment variables
- Starting sessions with specific models using CLI flags
- Switching models mid-session with the
/modelcommand- Cross-provider model selection through Bifrost
The warnings about tool-use capabilities and feature compatibility are appropriate and helpful.
154-174: LGTM: Practical complete setup example.The full Bifrost setup example effectively demonstrates:
- Core configuration with ANTHROPIC_API_KEY and ANTHROPIC_BASE_URL
- Custom model tier overrides (optional)
- Complete workflow from environment setup to running Claude Code
The note about persisting exports to shell configuration files is a helpful touch for user experience.
docs/quickstart/gateway/provider-configuration.mdx (1)
338-338: Image reference at line 338 is correct.The change to
ui-concurrency-timeout.pngis appropriate for the "Custom Network Settings" section, which discusses timeout and base URL configuration. The image file exists and the reference is contextually aligned with the section content.ui/app/workspace/config/views/securityView.tsx (1)
102-121: Auth callbacks and save payload correctly decouple auth from restart semanticsUsing functional
setAuthConfiginhandleAuthToggle,handleDisableAuthOnInferenceToggle, andhandleAuthFieldChangewith empty deps is safe and avoids stale closures, andhandleSavenow cleanly sendsauth_configwithout drivingneedsRestart, which matches the “no restart required for auth” behavior.Also applies to: 133-140
transports/bifrost-http/lib/config.go (1)
1403-1417: Auth config load now hard-requires a ConfigStore – confirm this is intentional
loadAuthConfigFromFileonly seedsAuthConfigwhen aConfigStoreexists and is empty, and otherwise just logs a warning ifConfigStoreis nil. This meansauth_configentries inconfig.jsonare ignored in non‑DB setups, which is consistent with the new store‑backed auth/session design but makes a ConfigStore effectively mandatory for dashboard auth. Please confirm that “no‑ConfigStore + file‑only auth” is no longer a supported mode.transports/bifrost-http/handlers/config.go (1)
416-487: Auth change detection and session flush flow is properly implementedThe updated auth logic cleanly handles:
- First‑time enablement vs updates (
authChangedflag correctly distinguishes cases).- Password redaction via
<redacted>sentinel and re‑hashing only on actual changes.- Disabling auth while preserving stored credentials.
- Flushing all sessions only when auth enable/username/password actually change.
The
FlushSessionsmethod is fully wired: it is declared in theConfigStoreinterface and implemented inRDBConfigStore, so the call at line 483 will compile and execute without issue. No changes needed.
Merge activity
|
Summary
This PR adds an OpenAPI bundle workflow, improves auth configuration security, and adds XAI as a supported provider in the OpenAPI schema.
Fixes
Closes #1239
Changes
Type of change
Affected areas
How to test
Breaking changes
Related issues
Improves security by ensuring users are logged out when auth configuration changes.
Security considerations
This PR enhances security by tracking auth configuration changes with a hash. When admin credentials are modified, all currently logged-in users will be automatically logged out, preventing unauthorized access with outdated credentials.
Checklist
docs/contributing/README.mdand followed the guidelines