fix: enforce session check before creating custom LLM providers#2014
Merged
senamakel merged 3 commits intoMay 18, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds primary-cloud-based routing for provider selection and gates custom provider strings behind a session verification check that requires a non-empty ChangesProvider Routing with Session Gate
Sequence DiagramsequenceDiagram
participant Caller
participant CreateChatProvider
participant VerifySession
participant Scheduler
participant AuthService
Caller->>CreateChatProvider: create_chat_provider_from_string(config, provider_str)
CreateChatProvider->>VerifySession: verify_session_active(config)?
VerifySession->>Scheduler: is_signed_out()
Scheduler-->>VerifySession: signed-out?
alt signed out
VerifySession-->>CreateChatProvider: SESSION_EXPIRED error
CreateChatProvider-->>Caller: error
else not signed out
VerifySession->>AuthService: read app-session token from state dir
AuthService-->>VerifySession: token (present/empty/missing)
alt Token Missing or Empty
VerifySession-->>CreateChatProvider: SESSION_EXPIRED error
CreateChatProvider-->>Caller: error
else Token Valid
VerifySession-->>CreateChatProvider: ok
CreateChatProvider->>CreateChatProvider: parse and create provider (ollama:<model> / <slug>:<model>)
CreateChatProvider-->>Caller: ChatProvider
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/openhuman/inference/provider/factory_test.rs (1)
372-372: 💤 Low valueRemove unused
mutqualifiers.
configis never mutated in these tests; Clippy will warn.♻️ Proposed fix
fn verify_session_active_rejects_when_token_is_empty() { let tmp = TempDir::new().expect("tempdir"); - let mut config = config_in_tempdir(&tmp); + let config = config_in_tempdir(&tmp);fn verify_session_active_passes_when_session_token_present() { let tmp = TempDir::new().expect("tempdir"); - let mut config = config_in_tempdir(&tmp); + let config = config_in_tempdir(&tmp);Also applies to: 386-386
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/inference/provider/factory_test.rs` at line 372, The variable declaration "let mut config = config_in_tempdir(&tmp);" uses an unnecessary mutable qualifier; change it to an immutable binding ("let config = config_in_tempdir(&tmp);") wherever applicable (e.g., the occurrences around the tests using config at lines similar to 372 and 386) because the variable is never mutated. Update any other instances of "let mut config" in those tests to "let config" and run cargo clippy to ensure no remaining unused mut warnings.src/openhuman/inference/provider/factory.rs (1)
238-264: ⚡ Quick winAdd debug logging for session verification outcomes.
This security-critical function lacks diagnostic logging. Adding trace/debug logs on rejection and success paths aids audit trails and debugging auth issues.
As per coding guidelines: "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions."♻️ Proposed logging additions
fn verify_session_active(config: &Config) -> anyhow::Result<()> { // Fast path: the scheduler gate already knows the session is dead. if crate::openhuman::scheduler_gate::is_signed_out() { + log::debug!("[providers][session-gate] rejected: scheduler reports signed-out"); anyhow::bail!( "SESSION_EXPIRED: backend session not active — sign in to use custom providers" ); } // Verify the app-session JWT actually exists in auth-profiles. let state_dir = config .config_path .parent() .map(std::path::PathBuf::from) .unwrap_or_else(|| { directories::UserDirs::new() .map(|d| d.home_dir().join(".openhuman")) .unwrap_or_else(|| std::path::PathBuf::from(".openhuman")) }); let auth = AuthService::new(&state_dir, config.secrets.encrypt); let has_session = auth .get_provider_bearer_token(crate::openhuman::credentials::APP_SESSION_PROVIDER, None)? .filter(|s| !s.trim().is_empty()) .is_some(); if !has_session { + log::debug!("[providers][session-gate] rejected: no app-session token in state_dir={:?}", state_dir); anyhow::bail!("SESSION_EXPIRED: no backend session — sign in to use OpenHuman") } + log::trace!("[providers][session-gate] session verified"); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/inference/provider/factory.rs` around lines 238 - 264, Add trace/debug logging to verify_session_active: log entry (with config identifier but not secrets) at trace/debug, log a debug/trace message when crate::openhuman::scheduler_gate::is_signed_out() short‑circuits indicating the session is known dead, log a debug/trace message when AuthService::get_provider_bearer_token (using crate::openhuman::credentials::APP_SESSION_PROVIDER) returns no/empty token before bailing, and log a success debug/trace just before returning Ok() indicating the session is active; use the project's tracing/log crate and avoid including token or secret values in the messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/inference/provider/factory_test.rs`:
- Around line 402-419: The test
verify_session_active_called_for_custom_provider_not_for_openhuman declares an
unused variable `err` when calling create_chat_provider_from_string("reasoning",
"ollama:llama3", &config); rename `err` to `_` (or prefix with underscore) to
silence the unused variable warning, then run cargo fmt to fix formatting issues
so the test file (including the verify_session_active and
create_chat_provider_from_string usages) is properly formatted.
In `@src/openhuman/inference/provider/factory.rs`:
- Around line 458-460: The file contains a formatting issue around the test
module declaration (the lines with #[cfg(test)] #[path = "factory_test.rs"] mod
factory_test;) causing cargo fmt to report a diff; run `cargo fmt` (or format
that area) to remove trailing whitespace/newline inconsistencies and ensure the
module attributes and declaration follow rustfmt style so the pipeline passes.
---
Nitpick comments:
In `@src/openhuman/inference/provider/factory_test.rs`:
- Line 372: The variable declaration "let mut config = config_in_tempdir(&tmp);"
uses an unnecessary mutable qualifier; change it to an immutable binding ("let
config = config_in_tempdir(&tmp);") wherever applicable (e.g., the occurrences
around the tests using config at lines similar to 372 and 386) because the
variable is never mutated. Update any other instances of "let mut config" in
those tests to "let config" and run cargo clippy to ensure no remaining unused
mut warnings.
In `@src/openhuman/inference/provider/factory.rs`:
- Around line 238-264: Add trace/debug logging to verify_session_active: log
entry (with config identifier but not secrets) at trace/debug, log a debug/trace
message when crate::openhuman::scheduler_gate::is_signed_out() short‑circuits
indicating the session is known dead, log a debug/trace message when
AuthService::get_provider_bearer_token (using
crate::openhuman::credentials::APP_SESSION_PROVIDER) returns no/empty token
before bailing, and log a success debug/trace just before returning Ok()
indicating the session is active; use the project's tracing/log crate and avoid
including token or secret values in the messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2af3c1d3-bb7c-4aa3-90fb-ff509659da61
📒 Files selected for processing (2)
src/openhuman/inference/provider/factory.rssrc/openhuman/inference/provider/factory_test.rs
xinzhuwang-wxz
force-pushed
the
fix/session-gate-for-custom-providers
branch
3 times, most recently
from
f0977cd to
126d44d
Compare
xinzhuwang-wxz
force-pushed
the
fix/session-gate-for-custom-providers
branch
from
126d44d to
7ddfdf3
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/inference/provider/factory.rs (1)
238-264: ⚡ Quick winAdd diagnostic logging for session verification.
Per coding guidelines, use
log/tracingat debug level on error paths and state transitions. This function should log when session verification fails to aid debugging authentication issues.Also, the error message at line 261 says "sign in to use OpenHuman" but the user is actually trying to use a custom provider. Consider aligning with the message at line 242.
♻️ Proposed fix
fn verify_session_active(config: &Config) -> anyhow::Result<()> { // Fast path: the scheduler gate already knows the session is dead. if crate::openhuman::scheduler_gate::is_signed_out() { + log::debug!("[providers][chat-factory] session gate: scheduler reports signed out"); anyhow::bail!( "SESSION_EXPIRED: backend session not active — sign in to use custom providers" ); } // Verify the app-session JWT actually exists in auth-profiles. let state_dir = config .config_path .parent() .map(std::path::PathBuf::from) .unwrap_or_else(|| { directories::UserDirs::new() .map(|d| d.home_dir().join(".openhuman")) .unwrap_or_else(|| std::path::PathBuf::from(".openhuman")) }); let auth = AuthService::new(&state_dir, config.secrets.encrypt); let has_session = auth .get_provider_bearer_token(crate::openhuman::credentials::APP_SESSION_PROVIDER, None)? .filter(|s| !s.trim().is_empty()) .is_some(); if !has_session { - anyhow::bail!("SESSION_EXPIRED: no backend session — sign in to use OpenHuman") + log::debug!("[providers][chat-factory] session gate: no app-session token found"); + anyhow::bail!("SESSION_EXPIRED: no backend session — sign in to use custom providers") } + log::trace!("[providers][chat-factory] session gate: verified active session"); Ok(()) }As per coding guidelines: "Use
log/tracingatdebugortracelevel on... error paths, state transitions".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/inference/provider/factory.rs` around lines 238 - 264, The verify_session_active function needs debug-level tracing on the two failure paths and an aligned error message: add a debug/tracing log call before the scheduler gate bail (referencing crate::openhuman::scheduler_gate::is_signed_out) that records the signed-out state, and add a debug/tracing log before the auth-session bail (referencing AuthService::new and get_provider_bearer_token) that records that no app-session token was found; change the second bail message to match the first context (e.g., "SESSION_EXPIRED: backend session not active — sign in to use custom providers") so both logs and the returned error consistently indicate the custom-provider use case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/inference/provider/factory.rs`:
- Around line 238-264: The verify_session_active function needs debug-level
tracing on the two failure paths and an aligned error message: add a
debug/tracing log call before the scheduler gate bail (referencing
crate::openhuman::scheduler_gate::is_signed_out) that records the signed-out
state, and add a debug/tracing log before the auth-session bail (referencing
AuthService::new and get_provider_bearer_token) that records that no app-session
token was found; change the second bail message to match the first context
(e.g., "SESSION_EXPIRED: backend session not active — sign in to use custom
providers") so both logs and the returned error consistently indicate the
custom-provider use case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 078d384c-7814-49a1-a632-0531472520f7
📒 Files selected for processing (2)
src/openhuman/inference/provider/factory.rssrc/openhuman/inference/provider/factory_test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/inference/provider/factory_test.rs
CodeRabbit flagged let err = ... as unused. The result is intentionally discarded in the test path — renamed to let _ = to suppress the warning.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An unregistered user can configure every workload (reasoning, coding, agentic, memory, heartbeat, etc.) to use a custom cloud provider (e.g. DeepSeek, OpenAI, Anthropic) and bypass the OpenHuman session requirement entirely. The core never creates an
OpenHumanBackendProvider, so the JWT check inresolve_bearer()is never reached.Root Cause
provider_for_role()returns"openhuman"when no explicit workload override is configured — but if every workload has an explicit override pointing to a custom provider, no role ever resolves to"openhuman", and the session gate is bypassed.Fix
Add
verify_session_active()check increate_chat_provider_from_string()that gates custom provider creation behind an active session, mirroring the same guard used for the primary cloud path.Changes
src/openhuman/inference/provider/factory.rs: +69 lines — session gatesrc/openhuman/inference/provider/factory_test.rs: +73 lines — testsSummary by CodeRabbit
New Features
Bug Fixes
Tests