fix(provider): fallback to config.api_key when auth-profiles.json has no key#2724
Conversation
… no key When a custom provider's API key is set in config.toml but not saved through the UI into auth-profiles.json, the HTTP request would be sent without an Authorization header, causing authentication failures. This change adds a fallback in lookup_key_for_slug() that reads from the top-level config.api_key field (which is auto-decrypted by the config loader from the enc2:-prefixed value in config.toml). Fixes: API key bug where config.toml has api_key but HTTP requests lack Authorization header for custom providers.
📝 WalkthroughWalkthroughThe PR adds a fallback authentication-key resolution mechanism in ChangesAuth Key Resolution Fallback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.rs`:
- Around line 641-651: The fallback returning config.api_key is currently
unconditional and can leak a global key to any provider; restrict this to only
the legacy direct-inference provider by checking that the provider endpoint for
the given slug matches config.inference_url before returning the key. In
practical terms, in the code around factory.rs where you read config.api_key and
have access to slug, resolve or compute the provider endpoint for slug (the same
logic used elsewhere in this function) and only use
config.api_key.trim().to_string() when endpoint == config.inference_url (and the
trimmed key is non-empty); otherwise skip this fallback and continue with normal
auth-profile lookup.
🪄 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: a98ef126-9f54-4de8-a118-15e9685bb58a
📒 Files selected for processing (1)
src/openhuman/inference/provider/factory.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Summary
PR adds a fallback in lookup_key_for_slug() to read from config.api_key when a custom provider's API key isn't found in auth-profiles.json. Tests pass, CI is green.
Findings
CodeRabbit caught a major security issue — credential leakage. The fallback applies config.api_key to any provider slug when profile lookup fails. If you have multiple cloud_providers configured, they all get the same credential, even if config.api_key was only intended for the legacy inference_url provider.
Scoping required: Only apply the fallback when the provider's endpoint matches config.inference_url. CodeRabbit's suggested fix at line 651 is the right approach — gate the fallback to check if slug points to the provider entry whose endpoint matches the configured inference_url.
This is a blocker — the credential leakage needs to be closed before merge.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
I see a REQUEST_CHANGES is already in place for the credential leakage issue flagged by CodeRabbit. Before I can approve this, that concern needs to be addressed — the fallback should be scoped to only the legacy inference provider (where the endpoint matches config.inference_url) to avoid sending the global config.api_key to unintended providers.
Also, there's a failing CI check (test / Rust Core Tests (Windows — secrets ACL)). Once the credential leakage issue is fixed and CI passes, I'll do a full review.
|
@ccpty unresolved review feedback — please address before we review. |
|
Unresolved review feedback from coderabbitai[bot] — please address before we review. |
|
@ccpty — heads up. I've re-reviewed this and the credential leakage issue from my prior REQUEST_CHANGES hasn't been addressed, and there's a CI failure on Windows (test / Rust Core Tests) that needs to be sorted out first. The problem: the new fallback at line 651 returns config.api_key for any provider slug without checking if it's the right provider. If you have multiple cloud providers configured (openai, azure, custom), all would incorrectly receive the same credential. The fix: scope the fallback to only apply when the slug corresponds to the provider using config.inference_url. Something like:
Once you fix the credential scoping AND resolve the Windows CI failure, I'll review the updated code and we can move forward. Let me know if you need clarification on the fix. |
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Confirming CodeRabbit + @graycyrus's CHANGES_REQUESTED — the credential-leakage concern is still valid against the current single-commit diff. Adding my flag (3rd maintainer) to reinforce.
The leak
Fallback runs after standard auth resolution exhausts, gated on nothing about the slug. Consequence:
- User has
cloud_providers = [openai, anthropic, deepseek]andconfig.api_key = <openai-key>. - Anthropic / DeepSeek factory call: auth-profiles lookup empty for those slugs →
config.api_keyfalls through → OpenAI key gets sent as Bearer to Anthropic's endpoint. - Cross-provider credential leak; the wrong key sits in the wrong provider's audit log.
The OpenAI OAuth fallback at line 624-642 sets the precedent for slug-gated fallbacks (if slug == "openai"). The config.api_key arm should follow the same shape — gate to the legacy direct-inference slug (the only one config.api_key was historically scoped to).
Suggested fix (inline below as a one-click suggestion block)
Determine the legacy slug by matching the provider whose endpoint equals config.inference_url, then gate the fallback on Some(slug) == legacy_inference_slug. Add a regression test that asserts the negative — config.api_key is NOT returned for a non-matching slug.
Suggested regression test
#[test]
fn fallback_to_config_api_key_only_for_legacy_inference_url_slug() {
let config = Config {
api_key: Some("global-key".into()),
inference_url: Some("https://inference.example.com".into()),
cloud_providers: vec![
CloudProviderCreds { slug: "anthropic".into(), endpoint: "https://api.anthropic.com".into(), ..Default::default() },
CloudProviderCreds { slug: "custom".into(), endpoint: "https://inference.example.com".into(), ..Default::default() },
],
..Default::default()
};
// Legacy slug — gets the global key
assert_eq!(lookup_key_for_slug("custom", &config).unwrap(), "global-key");
// Non-matching slug — must NOT inherit the global key
assert_eq!(lookup_key_for_slug("anthropic", &config).unwrap(), "");
}The negative assertion is the load-bearing one — locks the no-leak guarantee against future refactors.
CI
1 fail: test / Rust Core Tests (Windows — secrets ACL) — same infra flake (~20min timeout) hitting multiple PRs this week. Not PR-caused. Re-run will likely clear once the scoping fix lands.
Question for the author
Was the unscoped fallback intentional (you want config.api_key to apply project-wide regardless of provider), or did the cross-provider implication just not surface in your testing? If intentional, please add a UI warning + docs note about the implication. If accidental, applying the scoping above closes the gap.
| // Fallback: read from top-level config.api_key (direct config.toml api_key). | ||
| // This handles the case where a key was set in config.toml but not saved | ||
| // through the UI into auth-profiles.json. | ||
| if let Some(config_key) = config.api_key.as_ref() { | ||
| if !config_key.trim().is_empty() { | ||
| log::debug!( | ||
| "[providers][chat-factory] auth lookup slug={} key_present=true (config.toml fallback)", | ||
| slug | ||
| ); | ||
| return Ok(config_key.trim().to_string()); | ||
| } |
There was a problem hiding this comment.
Blocker (3rd maintainer flag — confirming CodeRabbit + @graycyrus) — unscoped fallback leaks config.api_key to any provider whose auth-profile lookup returned empty. One-click suggestion below gates to the legacy direct-inference slug (the only one this key was historically scoped to).
| // Fallback: read from top-level config.api_key (direct config.toml api_key). | |
| // This handles the case where a key was set in config.toml but not saved | |
| // through the UI into auth-profiles.json. | |
| if let Some(config_key) = config.api_key.as_ref() { | |
| if !config_key.trim().is_empty() { | |
| log::debug!( | |
| "[providers][chat-factory] auth lookup slug={} key_present=true (config.toml fallback)", | |
| slug | |
| ); | |
| return Ok(config_key.trim().to_string()); | |
| } | |
| // Fallback: read from top-level config.api_key (direct config.toml api_key). | |
| // Only applies to the legacy direct-inference provider — the slug whose | |
| // endpoint matches `config.inference_url`. Unscoped fallback would leak | |
| // the global key to any provider whose auth-profile lookup returned empty | |
| // (CodeRabbit + @graycyrus flagged this on initial review). | |
| let legacy_inference_slug = config.inference_url.as_deref().and_then(|url| { | |
| let norm = url.trim_end_matches('/'); | |
| config | |
| .cloud_providers | |
| .iter() | |
| .find(|p| p.endpoint.trim_end_matches('/') == norm) | |
| .map(|p| p.slug.as_str()) | |
| }); | |
| if Some(slug) == legacy_inference_slug { | |
| if let Some(config_key) = config.api_key.as_ref() { | |
| if !config_key.trim().is_empty() { | |
| log::debug!( | |
| "[providers][chat-factory] auth lookup slug={} key_present=true (config.toml fallback for legacy inference_url)", | |
| slug | |
| ); | |
| return Ok(config_key.trim().to_string()); | |
| } | |
| } | |
| } |
Also add the negative-assertion regression test in factory_test.rs (see PR body for the test code). Without the negative assertion any future refactor could silently re-open the leak.
There was a problem hiding this comment.
Fixed in 83e0b2e. The cross-provider implication was an oversight, not intentional — applied the scoping rather than a project-wide warning. The config.api_key fallback now only fires for the legacy direct-inference slug (the cloud_providers entry whose endpoint matches config.inference_url) via a new legacy_inference_slug() helper that reuses the existing normalize_endpoint_for_compare. Any other slug with an empty auth-profile lookup still resolves to "", so no global key leaks across providers. Added your suggested negative-assertion test (config_api_key_fallback_does_not_leak_to_other_slugs) plus positive and inert-without-inference_url cases.
The config.api_key fallback in lookup_key_for_slug() applied to any provider slug whose auth-profiles.json lookup returned empty, leaking the global key to unintended provider endpoints (e.g. an OpenAI key sent as Bearer to Anthropic's endpoint). Scope the fallback to the legacy direct-inference provider only — the cloud_providers entry whose endpoint matches config.inference_url, the single provider config.api_key was historically paired with. Add a legacy_inference_slug() helper reusing the existing endpoint-normalize comparison, and regression tests covering the positive case, the load-bearing negative (no cross-provider leak), and the inert case when inference_url is unset. Addresses CodeRabbit, @graycyrus, and @oxoxDev CHANGES_REQUESTED on src/openhuman/inference/provider/factory.rs:651 (PR tinyhumansai#2724).
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
All prior CHANGES_REQUESTED have been addressed in 83e0b2e.
Review Summary:
- Credential leakage vulnerability fixed:
config.api_keyfallback now scoped to legacy direct-inference provider (vialegacy_inference_slug()matching againstconfig.inference_url) legacy_inference_slug()properly reusesnormalize_endpoint_for_compare()for consistency and filters openhuman backend- Regression tests cover all three cases: positive (legacy slug gets the key), negative (cross-provider leak prevention), inert (no inference_url)
- CI all green (all checks pass)
The fix addresses @oxoxDev's and CodeRabbit's security concerns. Ready to merge once you resolve your reviews.
oxoxDev
left a comment
oxoxDev
left a comment
There was a problem hiding this comment.
Walkthrough
Converting my prior CHANGES_REQUESTED (2026-05-28T18:38) to APPROVED. senamakel's 83e0b2e4 lands the exact fix shape I + CR + graycyrus asked for: a new legacy_inference_slug(config) helper that returns Some(slug) only when a cloud_providers entry's normalized endpoint matches config.inference_url AND is not the OpenHuman backend. lookup_key_for_slug then gates the config.api_key fallback on legacy_inference_slug(config) == Some(slug). No cross-provider leak. All CI green.
Verified
- Gating semantics correct:
legacy_inference_slugshort-circuits toNoneon empty/missinginference_url, on OpenHuman-backend endpoint vialooks_like_openhuman_backend, and on cloud entries viais_openhuman_cloud_entry. The fallback can fire for at most one slug per Config, and never for the OpenHuman backend slug ✓ - Endpoint comparison reuses
normalize_endpoint_for_compare— consistent with sibling matchers, no drift risk ✓ - Doc comment in
lookup_key_for_slugcalls out the historicalconfig.api_key↔inference_urlpairing and explicitly references the cross-provider leak concern (flagged by CodeRabbit + maintainers on #2724). Future reviewers won't re-discover the same hazard ✓ - Tests cover all three paths:
- Positive:
config_api_key_fallback_applies_to_legacy_inference_slug— matchingcustomslug inherits"global-key"✓ - Negative (the load-bearing assertion I asked for):
config_api_key_fallback_does_not_leak_to_other_slugs—anthropicslug returns"", not"global-key"✓ - Inert:
config_api_key_fallback_inert_without_inference_url— noinference_url→ no fallback ✓
- Positive:
- CI fully green across all matrices (Rust Core / Quality / Tauri / Frontend / E2E / Coverage / Smoke).
Nits
- Optional: a 4th test covering the OpenHuman-backend short-circuit (
inference_urlset to backend URL →legacy_inference_slugreturnsNone→ no slug inherits the key) would pin the explicit defense-in-depth path. Today this is covered transitively by thelooks_like_openhuman_backendfilter being unit-tested elsewhere, so don't block — but worth a 1-liner in the next cleanup.
Questions
- @graycyrus stale
CHANGES_REQUESTED(2026-05-27T11:04) plus my priorCHANGES_REQUESTED(2026-05-28T18:38) — both predate83e0b2e4. This APPROVE supersedes mine; re-requested you for a fresh pass to formally approve and clear yours.
4 participants
When a custom provider's API key is set in config.toml but not saved through the UI into auth-profiles.json, the HTTP request would be sent without an Authorization header, causing authentication failures.
This change adds a fallback in
lookup_key_for_slug()that reads from the top-levelconfig.api_keyfield (which is auto-decrypted by the config loader from the enc2:-prefixed value in config.toml).Before: api_key bytes=0 (key not found, requests fail)
After: api_key bytes=51 (key found via fallback, requests succeed)
Summary by CodeRabbit