[PM-17121/17204] Fix fingerprint dialogs and disabled active biometric lock component #12928
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12928 +/- ##
==========================================
+ Coverage 34.41% 35.02% +0.61%
==========================================
Files 2983 2976 -7
Lines 90759 90641 -118
Branches 16993 16986 -7
==========================================
+ Hits 31233 31747 +514
+ Misses 57060 56432 -628
+ Partials 2466 2462 -4 ☔ View full report in Codecov by Sentry. |
|
Great job, no security vulnerabilities found in this Pull Request |
quexten
changed the title
| @@ -320,6 +309,18 @@ export class BiometricMessageHandlerService { | |||
| appId: string, | |||
| ) { | |||
| const messageUserId = message.userId as UserId; | |||
| if (!(await this.validateFingerprint(appId))) { | |||
There was a problem hiding this comment.
Do we need to add this validateFingerprint check to the case BiometricsCommands.Unlock? Since it's no longer being checked during setupEncryption.
If I'm understanding this correctly, it would be the case when running an updated desktop and an outdated extension?
There was a problem hiding this comment.
It seems not possible to make this happen in a way that is backwards compatible with the old client, because it only shows the old fingerprint validation dialog, if the shared secret is null. So instead, the desktop client now aborts the request and shows a message that the user needs to update their browser extension or disable browser integration fingerprint validation.
| if (this.trustedAppIds.has(appId)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
it make more sense that this condition is within next condition if (await firstValueFrom(this.desktopSettingService.browserIntegrationFingerprintEnabled$)) {.
There was a problem hiding this comment.
I moved it as you requested for now, but in the long term we might move it back, because there will be other ways to trust besides fingerprints. The idea was that if you don't have fingerprint verification enabled, a connection is always "trusted".
(The other mechanisms can be done by inspecting the unix socket / named pipe using getpeercred, and then validating that the connecting process is a signed, correctly installed e.g. google chrome).
| private publicKeysForAppId: Map<string, Uint8Array> = new Map(); | ||
| private trustedAppIds: Set<string> = new Set(); |
There was a problem hiding this comment.
shouldn't we clear those when this.desktopSettingService.browserIntegrationFingerprintEnabled$ changes ?
Or when biometrics are disabled in browser settings for specific app / user logs out, we should probably reset ?
There was a problem hiding this comment.
For fingerprintEnabled probably. Biometrics is a per-user setting, but the browser-desktop connection is inherently not per-user, but per-app, so I don't think it should be affected by any user logging out.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-17121
https://bitwarden.atlassian.net/browse/PM-17204
📔 Objective
Fixes a bug where when biometric unlock was the active method, and biometrics became unavailable, the browser window was just empty, due to how the lock component decided on when to render the biometrics component.
Further, this fixes fingerprint dialogs. Note: With an outdated desktop app, this will now show a "please update or disable fingerprint" dialog in the extension.
The fix is to now establish encryption always by default. Sensitive actions are only allowed though (if the fingerprint requirement is enabled), if the fingerprint was accepted. So status gets communicated fine, but once a biometrics prompt happens, the fingerprint phrase is prompted (once).
📸 Screenshots
Screen.Recording.2025-01-17.at.11.32.02.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes