fix(credentials): normalize provider slugs at write time and auto-mig… - #5432
fix(credentials): normalize provider slugs at write time and auto-mig…#5432aryash45 wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughProvider identifiers are normalized to lowercase during storage and profile loading. Credential and profile lookups accept provider names without case sensitivity. Regression tests cover token retrieval, UI detection parity, provider filtering, and persisted active-profile migration. ChangesCredential provider normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/security/credentials/core.rs (1)
152-173: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSupport bare provider slugs during profile selection.
A credential stored as
provider:DeepSeekbecomesprovider:deepseek. A lookup fordeepseekcannot match its active key, default profile ID, or stored provider value. The assertion at Line 1419 fails. Bare-slug callers also cannot read this credential.Use one shared provider-equivalence helper that treats
provider:<slug>and<slug>as the same provider. Apply it to active-profile and fallback-profile matching.Proposed fix
+fn provider_matches(left: &str, right: &str) -> bool { + let bare = |value: &str| value.strip_prefix("provider:").unwrap_or(value); + bare(left).eq_ignore_ascii_case(bare(right)) +} + - .find(|(k, _)| k.eq_ignore_ascii_case(provider)) + .find(|(k, _)| provider_matches(k, provider)) .map(|(_, v)| v) @@ - .provider - .eq_ignore_ascii_case(provider) + .provider + .as_str() + .pipe(|stored_provider| provider_matches(stored_provider, provider)) .then(|| id.clone())Use an ordinary local binding instead of
.pipe(...)if that helper is not already available.🤖 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/security/credentials/core.rs` around lines 152 - 173, Update the profile-selection logic around the active-profile lookup and fallback search to use one shared provider-equivalence helper that treats provider:<slug> and the bare <slug> as equivalent, matching case-insensitively. Apply this helper to active profile keys, default profile IDs, and stored profile.provider values so bare-slug callers resolve existing credentials; use a local binding instead of .pipe(...) if needed.
🤖 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/security/credentials/core.rs`:
- Around line 45-46: In src/openhuman/security/credentials/core.rs:45-46, add
grep-friendly diagnostics for provider normalization and the credential storage
result, logging only the normalized provider and sanitized outcome; in
core.rs:152-173, log which selection branch was used—exact active key, fallback
active key, default profile, provider fallback, or no match—without profile
names or metadata; in src/openhuman/security/credentials/ops.rs:964-964, add
entry, normalized-provider, and sanitized failure/completion diagnostics,
ensuring tokens, metadata values, and full PII are never logged.
In `@src/openhuman/security/credentials/ops.rs`:
- Line 964: Update list_provider_credentials to compare provider_filter with
stored provider values using eq_ignore_ascii_case instead of exact equality,
while preserving existing filtering behavior for other fields. Add a regression
test that stores a mixed-case provider and successfully retrieves it using a
differently cased provider filter.
In `@src/openhuman/security/credentials/profiles.rs`:
- Around line 854-862: Update the active-profile migration loop in the
profile-loading method around new_active to detect case-insensitive key
collisions before insertion instead of silently overwriting entries. Prefer an
existing lowercase key, record and report conflicts where legacy keys differ
only by case, and add a regression test covering two case variants that
reference different profile IDs while preserving the selected lowercase profile.
- Around line 854-867: The persisted active-profile key normalization branch
lacks a migration diagnostic. Update the key-migration flow around key_migrated
and new_active to emit a verbose, grep-friendly [auth] log when normalization
occurs, including the count of migrated keys, while excluding provider keys and
profile IDs.
---
Outside diff comments:
In `@src/openhuman/security/credentials/core.rs`:
- Around line 152-173: Update the profile-selection logic around the
active-profile lookup and fallback search to use one shared provider-equivalence
helper that treats provider:<slug> and the bare <slug> as equivalent, matching
case-insensitively. Apply this helper to active profile keys, default profile
IDs, and stored profile.provider values so bare-slug callers resolve existing
credentials; use a local binding instead of .pipe(...) if needed.
🪄 Autofix
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 Plus
Run ID: f68665c0-a4bd-40fb-989b-d0b23ffdee95
📒 Files selected for processing (5)
src/openhuman/security/credentials/core.rssrc/openhuman/security/credentials/ops.rssrc/openhuman/security/credentials/ops_tests.rssrc/openhuman/security/credentials/profiles.rssrc/openhuman/security/credentials/profiles_tests.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e48d512fc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // 3. Short slug lookup | ||
| let token_slug = auth | ||
| .get_provider_bearer_token("deepseek", None) |
There was a problem hiding this comment.
This assertion cannot pass with the implementation in this commit: the store writes only the normalized provider:deepseek:default profile, while get_provider_bearer_token("deepseek", None) searches the active/default/provider entries for the bare deepseek key and never strips the provider: namespace. The newly added test will panic here and fail CI unless short-slug lookup is implemented or this expectation is removed.
Useful? React with 👍 / 👎.
| for (k, v) in &persisted.active_profiles { | ||
| let lower = k.to_ascii_lowercase(); | ||
| if lower != *k { | ||
| key_migrated = true; | ||
| } | ||
| new_active.insert(lower, v.clone()); |
There was a problem hiding this comment.
For a legacy store containing profiles["provider:DeepSeek:default"], this only lowercases the active_profiles map key while leaving the active value, profile id, and profile.provider mixed-case. That makes the credential selectable via get_profile("provider:deepseek"), but later auth_remove_provider_credentials derives provider:deepseek:default from the normalized provider and returns removed=false, so the user cannot actually clear that migrated/stale secret. Either migrate the profile IDs/provider fields as part of this rewrite or make removal resolve legacy ids case-insensitively.
Useful? React with 👍 / 👎.
e48d512 to
1ceee59
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/security/credentials/profiles.rs`:
- Around line 854-880: The active-profile migration count currently uses
persisted entry count minus conflicts rather than the number of keys whose
casing changed. In the migration loop around new_active and migration_conflicts,
add a counter incremented only when lower != *k, then use that counter in the
normalized-key debug message while preserving collision handling and existing
diagnostics.
🪄 Autofix
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 Plus
Run ID: 3f660c77-d2be-4449-8a8b-6cb972b1b765
📒 Files selected for processing (5)
src/openhuman/security/credentials/core.rssrc/openhuman/security/credentials/ops.rssrc/openhuman/security/credentials/ops_tests.rssrc/openhuman/security/credentials/profiles.rssrc/openhuman/security/credentials/profiles_tests.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/openhuman/security/credentials/ops_tests.rs
- src/openhuman/security/credentials/core.rs
- src/openhuman/security/credentials/ops.rs
|
hi @Al629176 please review this pr and tell me if anything needs to be changed |
…rate legacy profiles
1ceee59 to
47a3d1f
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
hi @senamakel @Al629176 please review this pr |
|
hello @sanil-23 @senamakel @senamakel-droid @Al629176 please review this pr |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.1117 · 71,019 in / 5,065 out · 3,584 cached (5%) · minimax/minimax-m3, moonshotai/kimi-k3
critique: $0.0412 · 13,009 in / 1,236 out · 0 cached (0%) · moonshotai/kimi-k3, minimax/minimax-m3
security: $0.0548 · 16,112 in / 430 out · 0 cached (0%) · moonshotai/kimi-k3
tests: $0.0019 · 6,585 in / 157 out · 896 cached (14%) · minimax/minimax-m3
commits: $0.0038 · 10,595 in / 866 out · 1,792 cached (17%) · minimax/minimax-m3
description: $0.0036 · 7,867 in / 1,166 out · 768 cached (10%) · minimax/minimax-m3
| let mut new_active: BTreeMap<String, String> = BTreeMap::new(); | ||
| let mut migration_conflicts: usize = 0; | ||
| let mut casing_changed_count: usize = 0; | ||
| for (k, v) in &persisted.active_profiles { |
There was a problem hiding this comment.
Collision migration can drop the canonical lowercase entry
persisted.active_profiles is a BTreeMap, whose iteration order is lexicographic by bytes. Uppercase ASCII letters sort before lowercase, so for a slug like provider:DeepSeek vs provider:deepseek, the mixed-case key is visited first. The loop inserts new_active["provider:deepseek"] = <mixed-case value>, then when the canonical lowercase key is visited lower == *k, the new_active.contains_key(&lower) guard is skipped (it lives inside the if lower != *k block), and new_active.insert(lower, v.clone()) silently overwrites the mixed-case winner — wait, actually the reverse also matters: since the mixed-case key comes first, it claims the lowercase slot, and the later canonical lowercase entry overwrites it. The overwrite is invisible: no migration_conflicts increment, no warning, and the 'prefer existing' logic never fires.
More importantly, the guard is on the wrong side of the ordering: whether the lowercase entry wins or loses depends entirely on which variant sorts first in BTreeMap, not on which one is canonical. For keys whose mixed-case form sorts after the lowercase form (e.g. provider:deepSeek — lowercase d... no, S < s, but compare provider:deepseek vs provider:deepSeek: at the S/s position, S sorts first), the mixed-case entry is processed first, inserts under the lowercase key, and then the canonical lowercase entry arrives and overwrites it — again silently, because lower == *k skips the conflict check entirely.
So the deterministic outcome is: whichever entry is visited last wins, and conflicts are only counted when a mixed-case key is visited after a lowercase one was already inserted. The companion test migration_collision_prefers_existing_lowercase_key passes only because its chosen keys happen to order the lowercase entry last (provider:DeepSeek < provider:deepseek since D < d). Flip the casing to something like provider:deepseek vs provider:DEEPSEEK (D < d, so provider:DEEPSEEK is visited first, inserts under provider:deepseek, then the canonical provider:deepseek entry overwrites — that still prefers lowercase). But provider:deepseek vs provider:deepSeeK? S < s so mixed-case first again. In fact for any pair differing only in case, the variant with more uppercase in the earliest differing position is visited first — meaning the lowercase entry, when present, is always visited last and always wins by overwrite, never via the conflict path. The contains_key conflict branch is therefore dead code for the two-entry case, and migration_conflicts is only reachable with three or more case variants.
The real hazard: the silent overwrite means the intended 'prefer the existing entry, log a conflict' behavior never executes for the common two-variant case, and if iteration semantics ever change (or a third variant exists), which profile ID survives depends on map ordering rather than on a canonical rule. Make the preference explicit: first collect all lowercase-keyed entries, then merge in case-variant entries only for lowercase keys not already claimed, counting every dropped variant as a conflict regardless of visit order.
rule iteration-order-collision ·
| return result; | ||
| } | ||
|
|
||
| // Short-slug namespace fallback: a bare slug (e.g. "deepseek") also |
There was a problem hiding this comment.
Diff adds a bare-slug → namespaced-slug resolution path the description does not
Beyond the documented write-time normalization and case-insensitive read fallback, select_profile_id now resolves a bare slug (deepseek) against the namespaced form (provider:deepseek) on disk. This is a meaningful behaviour change (different caller inputs can resolve to the same stored credential) that the PR summary does not mention. Please either document it in the description, with rationale, or restrict it to a narrower case.
rule The PR description scopes the change to: lowercase-at-write, eq_ignore_ascii_casefallback on reads, and a migration of legacyactive_profileskeys. It never mentions that a bare slug likedeepseekwill now resolve credentials stored underprovider:deepseek. That is a separate, user-visible behaviour change with security implications — two callers asking for different slugs can hit the same on-disk key. ·
| if lower != *k { | ||
| key_migrated = true; | ||
| casing_changed_count += 1; | ||
| // Collision: a canonical lowercase key already exists. Prefer |
There was a problem hiding this comment.
Migration collision policy silently drops a mixed-case entry; not described
When a lowercased key already exists, the migration drops the mixed-case entry without retaining the profile ID it pointed at. The PR description does not mention this collision-handling policy or the loss it can cause. Please document the behaviour (and consider logging the dropped profile ID at debug level for forensics).
rule The PR description says legacy active_profiles keys are normalised to lowercase on first load. It does not say that when a mixed-case key collides with an already-canonical lowercase key, the mixed-case entry is silently discarded (only its count is logged, the actual provider key / profile ID are not). That is a destructive-on-load policy decision that should be called out. ·
| set_active: bool, | ||
| ) -> Result<AuthProfile> { | ||
| let mut profile = AuthProfile::new_token(provider, profile_name, token.to_string()); | ||
| let provider = normalize_provider(provider)?; |
There was a problem hiding this comment.
normalize_provider is used but not defined or imported in this diff
The core.rs change calls normalize_provider(provider)? but the diff does not add or re-export this function. Please confirm the symbol is already in scope (and, if not, either define it here or import it from its existing home); otherwise the build is broken.
rule The diff introduces a call to normalize_provider(provider)?instore_provider_token, but no definition, use, or re-export of normalize_provideris added in the diff. The symbol must already exist in the module; if it does, this is a real fix; if it does not,cargo test -p openhuman credentials (claimed passing in the PR) cannot have run. Worth confirming before merge. ·
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/security/credentials/core.rs`:
- Around line 188-204: Update the bare-provider fallback in the
profile-selection function to resolve provider:{slug} through the existing
active-profile, default-profile, and provider-scan precedence rather than
directly scanning data.profiles. Preserve the fallback log and return behavior,
and add a regression test with two namespaced profiles where
active_profiles["provider:deepseek"] selects the non-first profile.
🪄 Autofix
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 Plus
Run ID: efeb5d22-a0ce-454a-8a4c-63c9bdf58abd
📒 Files selected for processing (1)
src/openhuman/security/credentials/core.rs
6c1b731 to
47a3d1f
Compare
There was a problem hiding this comment.
Requesting changes: 4 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0972 · 63,597 in / 6,973 out · 5,504 cached (9%) · moonshotai/kimi-k3, minimax/minimax-m3
critique: $0.0236 · 12,123 in / 225 out · 1,152 cached (10%) · moonshotai/kimi-k3, minimax/minimax-m3
security: $0.0566 · 17,023 in / 781 out · 2,304 cached (14%) · moonshotai/kimi-k3
tests: $0.0057 · 11,422 in / 2,084 out · 768 cached (7%) · minimax/minimax-m3
commits: $0.0048 · 10,276 in / 1,509 out · 512 cached (5%) · minimax/minimax-m3
description: $0.0065 · 12,753 in / 2,374 out · 768 cached (6%) · minimax/minimax-m3
|
@senamakel please guide me through this |
…ug lookup & migration resolution
1 participant
Summary
provider:DeepSeek) while read paths used case-sensitive exact matching, causing a silent save/read mismatch.store_provider_tokenandstore_provider_credentialsnow normalize provider slugs to lowercase at write time vianormalize_provider()/.to_ascii_lowercase().select_profile_idand bearer-token resolution gain a case-insensitive fallback (eq_ignore_ascii_case) for profiles written before this fix.AuthProfilesStore::load()auto-migrates legacy on-diskactive_profileskeys to lowercase on first load and rewrites the file; subsequent reads are no-ops.ControllerSchemanamespace strings are untouched.Problem
After saving a DeepSeek API key via Settings, the UI confirmed success but the dialog immediately showed the key as missing and all API calls failed. The problem persisted across restarts.
Root cause: the write path stored the profile under
provider:DeepSeek(mixed case from the frontend slug), while read paths looked upprovider:deepseek(lowercase). The keys never matched, soget_provider_bearer_tokenand thehas_tokenUI check both returnedfalsefor a key that was physically on disk.Solution
Normalize at write time (primary fix):
core.rs—store_provider_token:let provider = normalize_provider(provider)?before buildingAuthProfile.ops.rs—store_provider_credentials:let provider = provider.trim().to_ascii_lowercase()before any storage call.Case-insensitive read fallback (defence-in-depth):
select_profile_idand active-profile resolution useeq_ignore_ascii_caseso stale mixed-case profiles still resolve.On-load migration (existing data):
profiles.rs—AuthProfilesStore::load()detects anyactive_profileskey wherekey != key.to_lowercase(), rebuilds the map, and rewrites the file. Runs once; free on all subsequent loads.Submission Checklist
store_and_retrieve_provider_token_case_insensitive— mixed-case write resolves correctly on readlegacy_mixed_case_active_profile_key_migrated_on_load— stale on-disk fixture normalised and rewritten on firstload()ui_has_api_key_check_and_backend_get_profile_agree_for_same_credential—has_token(UI) andget_provider_bearer_token(backend) can never silently disagreesecurity/credentials/are covered by the three tests aboveCloses #NNN— see RelatedImpact
~/.openhuman/; no mobile/web/CLI impact.active_profiles(typically ≤ 5 entries).Related
ProviderSlugnewtype that enforces lowercase at construction time to make future regressions a compile error.AI Authored PR Metadata
Linear Issue
Commit & Branch
fix/deepseek-provider-slug-normalizatione48d512fcValidation Run
pnpm --filter openhuman-app format:check— Blocked (no frontend files changed; CI will verify)pnpm typecheck— Blocked (node_modules not installed locally; no frontend files changed)cargo test -p openhuman credentials— passedcargo fmt --all --check— clean (exit 0)Validation Blocked
command:pnpm --filter openhuman-app format:checkerror:node_modules missingimpact:None — no frontend files modified in this PRBehavior Changes
Parity Contract
eq_ignore_ascii_casefallback on read paths ensures profiles written before this fix continue to resolve without data loss.ui_has_api_key_check_and_backend_get_profile_agree_for_same_credentialenforces strict equality between UI surface and backend resolution path.Duplicate / Superseded PR Handling
4dc3e9b) — pre-rebase, wrong file pathsgit push --force-with-leaseSummary by CodeRabbit
Bug Fixes
Tests