Skip to content

PM-40278: bug: Freeze public key and fingerprint during auth request and validate against updates#7166

Merged
david-livefront merged 1 commit into
mainfrom
PM-40278-freeze-public-key
Jul 21, 2026
Merged

PM-40278: bug: Freeze public key and fingerprint during auth request and validate against updates#7166
david-livefront merged 1 commit into
mainfrom
PM-40278-freeze-public-key

Conversation

@david-livefront

@david-livefront david-livefront commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

PM-40278

📔 Objective

This PR updates the Auth Requests flow. By locking the public key (and fingerprint) in with the first request but validating against subsequent requests we can ensure that the server has not changed during the process.

@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug labels Jul 14, 2026
@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.67%. Comparing base (c89dd5a) to head (0449afd).

Files with missing lines Patch % Lines
...warden/data/auth/manager/AuthRequestManagerImpl.kt 96.29% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7166      +/-   ##
==========================================
- Coverage   86.07%   85.67%   -0.40%     
==========================================
  Files         885      928      +43     
  Lines       64652    66433    +1781     
  Branches     9628     9671      +43     
==========================================
+ Hits        55646    56916    +1270     
- Misses       5553     6054     +501     
- Partials     3453     3463      +10     
Flag Coverage Δ
app-data 17.53% <96.29%> (-0.19%) ⬇️
app-ui-auth-tools 18.75% <0.00%> (-0.01%) ⬇️
app-ui-platform 16.81% <0.00%> (+0.31%) ⬆️
app-ui-vault 27.98% <0.00%> (+0.55%) ⬆️
authenticator 6.11% <0.00%> (-0.01%) ⬇️
lib-core-network-bridge 4.03% <0.00%> (+0.01%) ⬆️
lib-data-ui 1.14% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@david-livefront
david-livefront marked this pull request as ready for review July 16, 2026 21:09
@david-livefront
david-livefront requested a review from a team as a code owner July 16, 2026 21:09
@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR hardens the auth request approval flow in AuthRequestManagerImpl by freezing the publicKey and fingerprint from the initial request and validating each polled response's recomputed fingerprint against it. On mismatch the request is hard-declined via a non-null responseDate, and the frozen publicKey is what ultimately flows into the key-wrapping step during approval. I traced every branch of the polling loop (approved, declined, mismatch-decline, expired, update, and fingerprint-fetch failure) and found the trust-boundary logic sound with no bypass. Test coverage is strong, including new cases for fingerprint mismatch, publicKey freezing, and fingerprint failures during polling.

Code Review Details

No blocking or actionable findings.

@david-livefront
david-livefront force-pushed the PM-40278-freeze-public-key branch from ea1838a to 0449afd Compare July 17, 2026 18:45
while (currentCoroutineContext().isActive && !isComplete) {
delay(PASSWORDLESS_APPROVER_INTERVAL_MILLIS)
val updateResult = result as AuthRequestUpdatesResult.Update
delay(PASSWORDLESS_APPROVER_INTERVAL_MILLIS.milliseconds)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

kudos for dealing with the deprecated overload! :feelsgood:


@Suppress("MaxLineLength")
@Test
fun `getAuthRequestByIdFlow should emit update then declined and cancel when polled fingerprint does not match initial fingerprint`() =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

n🧊

@vvolkgang vvolkgang left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:shipit:

@david-livefront

Copy link
Copy Markdown
Collaborator Author

Thanks @vvolkgang

@david-livefront
david-livefront added this pull request to the merge queue Jul 21, 2026
Merged via the queue into main with commit c6f7a47 Jul 21, 2026
35 checks passed
@david-livefront
david-livefront deleted the PM-40278-freeze-public-key branch July 21, 2026 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants