Skip to content

fix: login refresh when user key is missing - #9

Merged
Hureru merged 2 commits into
mainfrom
fix/login-refresh-user-key
Jun 5, 2026
Merged

fix: login refresh when user key is missing#9
Hureru merged 2 commits into
mainfrom
fix/login-refresh-user-key

Conversation

@Hureru

@Hureru Hureru commented Jun 5, 2026

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • Bug Fixes
    • Improved vault unlock messaging to distinguish between unlocking a locked vault and refreshing an active session.
    • Added session validation check that prompts users to re-authenticate when the current session cannot access vault data.

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Hureru, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 3 minutes and 2 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3ef50572-9b77-4a60-b35a-1c4dbe27e174

📥 Commits

Reviewing files that changed from the base of the PR and between a598630 and 697d90b.

📒 Files selected for processing (1)
  • src/main.rs
📝 Walkthrough

Walkthrough

The PR improves vault authentication resilience by distinguishing password-based unlock scenarios from session-refresh flows, and adds an early validation check during sync to detect and report broken Bitwarden sessions before attempting data decryption.

Changes

Vault Authentication & Session Management

Layer / File(s) Summary
Password unlock state tracking and response messaging
src/main.rs
ControlAction::UnlockPassword records whether the vault was locked before attempting login, then uses that state to report either "Vault unlocked…" (first unlock) or "Bitwarden session refreshed…" (already-unlocked refresh) after successful authentication and sync.
Sync-time session decryption check
src/main.rs
do_sync now checks client.has_user_key() early and returns an error instructing re-authentication if the current Bitwarden session cannot decrypt vault data.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 The vault now knows when it first awoke,
From locked to refreshed, a subtle stroke!
Sessions checked before the sync flows through—
Re-auth when keys go missing too.
A rabbit's touch on auth so true!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: login refresh when user key is missing' directly describes the main change: adding a user key capability check in do_sync to handle missing user key scenarios and improving the login refresh flow.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/main.rs (1)

4209-4313: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reuse the normal post-sync reconciliation here.

Because Line 4209 now lets UnlockPassword run while the vault is already unlocked, this branch becomes the session-refresh path. Unlike do_sync on Lines 4781-4823, it never clears authorization_memory, never updates key_material_fingerprints, and never refreshes the envelope local cache. That means a rotated key can keep its remembered approval, and the next lock/unlock can resurrect stale cached keys from disk. Please route this path through the same reconciliation logic as do_sync, or extract that block into a shared helper.

🤖 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/main.rs` around lines 4209 - 4313, This branch must perform the same
post-sync reconciliation as do_sync: either call or extract do_sync's
reconciliation block and invoke it here instead of only calling
resolve_pending_sync; specifically ensure you clear authorization_memory, update
key_material_fingerprints, and refresh the envelope local cache/state
(local_key_cache_data and local_cache_key_state) so rotated keys lose remembered
approvals and stale disk keys are not resurrected. Replace or augment the
current sequence after loading keys (where resolve_pending_sync(...) is called
and before returning) to invoke the shared reconciliation helper (or do_sync)
that mutates authorization_memory, key_material_fingerprints,
local_key_cache_data, and local_cache_key_state accordingly. Use the existing
symbols do_sync (or the new helper name), authorization_memory,
key_material_fingerprints, local_key_cache_data, local_cache_key_state, and
resolve_pending_sync to locate and integrate the logic.
🤖 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.

Outside diff comments:
In `@src/main.rs`:
- Around line 4209-4313: This branch must perform the same post-sync
reconciliation as do_sync: either call or extract do_sync's reconciliation block
and invoke it here instead of only calling resolve_pending_sync; specifically
ensure you clear authorization_memory, update key_material_fingerprints, and
refresh the envelope local cache/state (local_key_cache_data and
local_cache_key_state) so rotated keys lose remembered approvals and stale disk
keys are not resurrected. Replace or augment the current sequence after loading
keys (where resolve_pending_sync(...) is called and before returning) to invoke
the shared reconciliation helper (or do_sync) that mutates authorization_memory,
key_material_fingerprints, local_key_cache_data, and local_cache_key_state
accordingly. Use the existing symbols do_sync (or the new helper name),
authorization_memory, key_material_fingerprints, local_key_cache_data,
local_cache_key_state, and resolve_pending_sync to locate and integrate the
logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f5e6fd86-affa-41a7-a84a-cfcd39cfa5e4

📥 Commits

Reviewing files that changed from the base of the PR and between 820aa88 and a598630.

📒 Files selected for processing (1)
  • src/main.rs

@Hureru
Hureru merged commit c539340 into main Jun 5, 2026
6 checks passed
@Hureru
Hureru deleted the fix/login-refresh-user-key branch June 5, 2026 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant