fix: handle nil configStore in handlers and server initialization #784
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughPlugins and session handlers were made resilient to a nil config store: plugin endpoints now report status-driven data when no config store is present and creation/updates/deletes are disallowed; session handler constructors and auth checks guard against nil config; server now always constructs the PluginsHandler; version bumped to 1.3.21. Several GitHub workflow configs, scripts, and CI/release automation files were added or modified. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant PluginsHandler
participant PluginsLoader
participant ConfigStore
rect rgb(223,242,225)
Server->>PluginsHandler: NewPluginsHandler(configStore, pluginsLoader) (now always constructed)
end
Server->>PluginsHandler: HTTP GET /plugins
alt configStore == nil
PluginsHandler->>PluginsLoader: GetPluginStatus()
PluginsHandler-->>Server: 200 { plugins: [name, enabled:true, config:{}, isCustom:true, status] , count }
else configStore != nil
PluginsHandler->>ConfigStore: ListPlugins()
PluginsHandler->>PluginsLoader: GetPluginStatus()
PluginsHandler-->>Server: 200 { plugins: merged(name,enabled,config,isCustom,path,status), count }
end
sequenceDiagram
participant Client
participant SessionHandler
participant ConfigStore
Client->>SessionHandler: POST /login
alt configStore == nil
SessionHandler-->>Client: 403 "Authentication is not enabled"
else
SessionHandler->>ConfigStore: GetAuthConfig(ctx)
SessionHandler-->>Client: 200/401 (normal auth flow)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (20)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Pratham-Mishra04
force-pushed
the
11-07-fix_handle_case_when_config_store_is_nil_in_session_and_plugins_handlers
branch
from
48984cf to
9078a03
Compare
akshaydeo
force-pushed
the
11-07-fix_handle_case_when_config_store_is_nil_in_session_and_plugins_handlers
branch
from
9078a03 to
3e478a3
Compare
akshaydeo
force-pushed
the
11-07-fix_handle_case_when_config_store_is_nil_in_session_and_plugins_handlers
branch
from
3e478a3 to
ecf240a
Compare
Contributor
Merge activity
|
akshaydeo
deleted the
11-07-fix_handle_case_when_config_store_is_nil_in_session_and_plugins_handlers
branch
akshaydeo
added a commit
that referenced
this pull request
Nov 17, 2025
## Summary Added nil checks for configStore in handler constructors to prevent potential nil pointer dereferences and improved route registration logic. ## Changes - Added nil checks in `NewPluginsHandler` and `NewSessionHandler` to return nil when configStore is nil - Removed redundant nil check in `isAuthEnabled` method since it's now handled at construction time - Updated route registration in `BifrostHTTPServer.RegisterRoutes` to only register routes for non-nil handlers ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (Next.js) - [ ] Docs ## How to test 1. Run the server with and without a configStore to verify handlers are properly initialized: ```sh # Core/Transports go test ./transports/bifrost-http/handlers/... go test ./transports/bifrost-http/server/... ``` 2. Verify that API endpoints respond correctly when configStore is nil. ## Breaking changes - [ ] Yes - [x] No ## Related issues Fixes potential nil pointer dereference issues when configStore is not provided. ## Security considerations Prevents potential server crashes from nil pointer dereferences, which could be exploited for denial of service. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Added nil checks for configStore in handler constructors to prevent potential nil pointer dereferences and improved route registration logic.
Changes
NewPluginsHandlerandNewSessionHandlerto return nil when configStore is nilisAuthEnabledmethod since it's now handled at construction timeBifrostHTTPServer.RegisterRoutesto only register routes for non-nil handlersType of change
Affected areas
How to test
Breaking changes
Related issues
Fixes potential nil pointer dereference issues when configStore is not provided.
Security considerations
Prevents potential server crashes from nil pointer dereferences, which could be exploited for denial of service.
Checklist