Skip to content

[PM-22781] Implement autoconnect to desktop app on app start #15266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

quexten
Copy link
Contributor

@quexten quexten commented Jun 20, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-22871

📔 Objective

Autostart is not working after a process reload, because the desktop app is not yet connected, and so the unlock options initially show biometrics as unavailable (it then becomes available just a moment later), so the lock components defaults to MP/PIN. There a numerous ways to fix this, in this case this PR adds autoconnect code. As long as biometrics is enabled in the settings, the browser will attempt to auto-connect to the desktop app in the background.

This means that IPC to the desktop app is available directly, without any wait, also fixing auto-prompt as a side-effect.

Tested on MacOS.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

@quexten quexten marked this pull request as ready for review June 20, 2025 11:41
@quexten quexten requested a review from a team as a code owner June 20, 2025 11:41
@quexten quexten requested a review from Thomas-Avery June 20, 2025 11:41
Copy link
Contributor

github-actions bot commented Jun 20, 2025

Logo
Checkmarx One – Scan Summary & Details366b8a5d-930a-48fe-94e4-73e4e3943e06

New Issues (4)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
HIGH CVE-2025-5063 Npm-electron-36.3.1
detailsRecommended version: 37.2.0
Description: Use After Free in Compositing in Google Chrome prior to 137.0.7151.55 allowed a remote attacker to potentially exploit heap corruption via a crafte...
Attack Vector: NETWORK
Attack Complexity: LOW

ID: oVisGuMjh19WLMOCW2GsV28jOhBr6Jol8xaPXMczzH8%3D
Vulnerable Package
MEDIUM CVE-2025-5067 Npm-electron-36.3.1
detailsRecommended version: 37.2.0
Description: Inappropriate implementation in Tab Strip in Google Chrome prior to 137.0.7151.55 allowed a remote attacker to perform UI spoofing via a crafted HT...
Attack Vector: NETWORK
Attack Complexity: LOW

ID: urqxqtmehvHeOHKORCOOVn%2FqvjuOluk%2BwTXhcxzQDFQ%3D
Vulnerable Package
LOW CVE-2025-5889 Npm-brace-expansion-1.1.11
detailsRecommended version: 1.1.12
Description: A vulnerability was found in juliangruber brace-expansion. It has been rated as problematic. Affected by this issue is the function "expand" of the...
Attack Vector: NETWORK
Attack Complexity: HIGH

ID: lmtoqi3b2V0ggWryAXnZCxHvQzF9YmQmr%2FWM%2BfM48fE%3D
Vulnerable Package
LOW CVE-2025-5889 Npm-brace-expansion-2.0.1
detailsRecommended version: 2.0.2
Description: A vulnerability was found in juliangruber brace-expansion. It has been rated as problematic. Affected by this issue is the function "expand" of the...
Attack Vector: NETWORK
Attack Complexity: HIGH

ID: rWKrxw4iYRSibCaknW364gfVqhgZlLDG6nEc01V7K%2B0%3D
Vulnerable Package

Copy link

codecov bot commented Jun 20, 2025

Codecov Report

Attention: Patch coverage is 31.25000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 37.16%. Comparing base (662a973) to head (b70fe01).
Report is 116 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...iometrics/background-browser-biometrics.service.ts 45.45% 6 Missing ⚠️
...owser/src/background/nativeMessaging.background.ts 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15266      +/-   ##
==========================================
+ Coverage   36.90%   37.16%   +0.26%     
==========================================
  Files        3225     3277      +52     
  Lines       93192    94429    +1237     
  Branches    14016    14228     +212     
==========================================
+ Hits        34388    35098     +710     
- Misses      57380    57880     +500     
- Partials     1424     1451      +27     

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

One minor thing from me. Might be a good idea to have @mzieniukbw look at this one as well, as I'm less familiar with this area.

Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com>
@quexten quexten requested a review from mzieniukbw June 20, 2025 17:08
@quexten
Copy link
Contributor Author

quexten commented Jun 20, 2025

Good point, I have requested maciej's review.

try {
await this.nativeMessagingBackground().connect();
} catch (e) {
this.logService.warning(
Copy link
Contributor

Choose a reason for hiding this comment

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

When desktop app is not running (linux, windows only), this will spam errors on console in here and in

this.logService.error("NativeMessaging port disconnected because of error: " + error);
at the same time.
Isn't it a bit too much ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is fair. I'm supressing the error here now.

filter(([_]) => !this.nativeMessagingBackground().connected),
switchMap(async () => {
try {
await this.nativeMessagingBackground().connect();
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ ideally, this should only be working when extension is actually in background, not open. We already try to connect via sending a message over IPC every 1 second on unlock page.
🤔 I think this is fine, but got me wondered, since unlock page is on extension main thread and this is background worker thread, is it possible for connect() to be in race condition between the two and potentially create some unexpected behaviour, like connected twice ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to:

  • Run only every 30 seconds in background
  • Disable running on a timer while the popup is open

I wonder whether in the long run it makes sense to decouple calling from connecting, and make it so that:

  • If is not connected, make the interface return sensible states (throw for unlock / return "unavailable" for getStatus)
  • Always only connect via background autoconnect, but have the interval change between something longer (like 30-60 seconds), and something shorter (5 seconds or less?) depending on whether the popup is open.

Copy link
Contributor

@mzieniukbw mzieniukbw Jun 24, 2025

Choose a reason for hiding this comment

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

If someone just opened Bitwarden desktop app, unlocked and opens browser extension, then the connect request should happen on request, if not connected, like it is right now. (no change).
Otherwise background check would have to be much more frequent for good UX, like maybe even every second. In fact, if we would additionally check for connecting flag, then i don't see much risk in moving the connect entirely here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the connecting check to the connect function, so that even calling it multiple times is expected to be safe.

// If it is open, then sending regular IPC messages will already attempt to connect.
const backgroundTimer = timer(0, this.BACKGROUND_POLLING_INTERVAL).pipe(
concatMap(async () => {
return await BrowserApi.isPopupOpen();
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this returns false when the extension window is open, but not in popup mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this and polling is now always happening.

Copy link

sonarqubecloud bot commented Jul 4, 2025

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.

3 participants