PM-26250 Explore options to enable direct importer for mac app store build

[harr1424]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-26250?atlOrigin=eyJpIjoiN2Y1Y2NhNmQ4OTBhNDU4YzkwZWNmYmI1MTdiNzc1NDgiLCJwIjoiaiJ9

📔 Objective

For the direct importer, browser profiles aren't populating when the desktop app was installed via the mac app store. This is because macOS apps can be sandboxed and won't have access to system directories to look for browser profiles.

The goal of this effort is to enable the direct importer for the mac app store build.

🎥 Screencast

⏰ 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

[github-actions]

Checkmarx One – Scan Summary & Details – 55a1b140-eee4-496e-b266-62bbf29cdf1a

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 2.83688% with 274 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.93%. Comparing base (f5d9340) to head (17ac926).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...chromium_importer/src/chromium/platform/sandbox.rs	0.00%	136 Missing ⚠️
...sktop_native/chromium_importer/src/chromium/mod.rs	0.00%	53 Missing ⚠️
...sktop/desktop_native/napi/src/chromium_importer.rs	0.00%	38 Missing ⚠️
...p/src/app/tools/import/import-desktop.component.ts	5.00%	19 Missing ⚠️
...src/main/tools/import/chromium-importer.service.ts	0.00%	14 Missing ⚠️
...r/src/components/chrome/import-chrome.component.ts	0.00%	10 Missing ⚠️
...e/chromium_importer/src/chromium/platform/macos.rs	0.00%	1 Missing ⚠️
apps/desktop/src/app/tools/preload.ts	0.00%	1 Missing ⚠️
apps/desktop/src/main.ts	0.00%	1 Missing ⚠️
libs/importer/src/components/import.component.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17479      +/-   ##
==========================================
- Coverage   47.10%   46.93%   -0.17%     
==========================================
  Files        3951     3952       +1     
  Lines      119755   119872     +117     
  Branches    18349    18328      -21     
==========================================
- Hits        56413    56267     -146     
- Misses      59107    59363     +256     
- Partials     4235     4242       +7     

☔ 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.

[claude]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR enables the Chromium direct importer for Mac App Store (sandboxed) builds by introducing an Objective-C BrowserAccessManager that drives NSOpenPanel and persists NSURL security-scoped bookmarks, plus a Rust ScopedBrowserAccess wrapper that flows the bookmark-resolved path into get_available_profiles and import_logins. The latest commit (17ac9268d9) addresses the previously-flagged i18n gap by resolving NSOpenPanel strings through the renderer's i18n service and threading them via IPC into the Objective-C requestAccessCommand. Earlier review concerns — deprecated [defaults synchronize], the getMetadata trust-boundary divergence, generic CommandResult<T> typing, and Local State JSON validation — have all been addressed.

Code Review Details

No new findings.

[coroiu]

It seems odd put swift into a crate called a objc, but also, do we have to add another language? The build file for objc was so simple, and the interface contained an easy to use async/future-compatible API with error handling and allowed complex data structures by using JSON serialization. Aren't all Apple APIs compatible with objc? Can't we just use that?

[harr1424]

Aren't all Apple APIs compatible with objc? Can't we just use that?

I can check, Apple has introduced APIs available in Swift that are not backwards compatible with objc. Swift has something like FFI (even cleaner) for working with any objc code, but the reverse is not guaranteed to be true.

This Swift code is used to display an NSOpenPanel(), I have a feeling that existed in the objc days, but I am not sure what the equivalent of DispatchQueue.main.sync is, and a few of the other Apple APIs used here, I will need to check on.

[harr1424]

@coroiu I ran the Swift file by Claude and all of the APIs have equivalents in objc, I'll get started on making the change.

[coroiu]

@coltonhurst You're currently the only users of the objc crate and its runCommand framework. In hindsight I admit that the JSON approach might not have been the best, but there's still some callback-to-async and string interop for automatically freeing memory (without depending on libc) that I feel is worth using/saving. Does Autofill have any current plans for improving the objc crate? Would you also prefer swift?

[coltonhurst]

@coroiu thanks for the mention. I am not intimately familiar with this area of the code base, but I agree with your points.

I think keeping only Objective-C code in the objc directory is a good move. It looks like that might have been updated in this PR since your message and my time in looking at this? (Thanks for your patience!)

Regarding the JSON pattern... I don't think we should deviate from an existing pattern, creating two different patterns, unless there is a plan to transition the old stuff, or if there is a specific reason we need to do it a different way. This is just my preference; I would rather code patterns be consistent (even if not the "perfect" solution), rather than "right" stuff that is not consistent and less easily understood to new readers. So... the tldr for this point is it might be worth looking into a way to do this in a way that follows the existing pattern, rather than creating new patterns.

[harr1424]

@coroiu please let me know if you believe anything currently in this PR deviates from the preferences mentioned above; I believe I have addressed all concerns and have a working MAS build running locally.

[coroiu]

@harr1424 Have you looked at how the existing objc code looks, how it is executed, how parameters are sent, how data is returned and how memory is cleared? I see significant differences:

• Functions referenced directly using extern "C"
• Parameters are constructed manually in every consuming function using unsafe rust
• Return data is converted manually in every consuming function using unsafe rust
• Memory is manually cleared
• Libc is added as a dependency to clear memory

The reason I asked @coltonhurst is because we might very well want to move over to referencing functions directly, I think that would make writing cross-platform passkey autofill code easier, but in that case I think we should be more restrictive with where we put unsafe rust, isolate it more, and make a plan for how to support async functions.

Seeing as we do not want to deviate from existing patterns then this code, @harr1424, needs to be convert to using the JSON command structure and be called using run_command instead of referenced directly using extern "c" blocks. We should also remove libc as a dependency since run_command will manage the memory for you. run_command will also add some exception handling for you which I think might be missing in the current version of your code (not sure if it's actually able to throw anything, but doesn't hurt to be safe).

[coltonhurst]

@harr1424 do you mind letting me know when the files relevant to this convo are ready for review? (That may be now, I'm not sure.)

I also think it would be nice to also add some architecture documentation to our contributing docs around the pattern so it's at least documented more. Any ideas @coroiu?

[harr1424]

@coltonhurst my apologies I've moved this PR back to draft status pending Oscar's review. I'll reach out once it is ready for your re-review.

[coltonhurst]

@harr1424 no problem! I don't think I'm a required code owner anyways, just was getting involved!

[theMickster]

Hello @harr1424 et. al,

I'm field testing a new performing-multi-agent-code-review skill and, after seeing your review hit my Github Notifications, I gave it a local run against the PR. The findings are below for your analysis. Please incorporate the findings that are true signals and let me know if there are egregious errors that need to be corrected with planned tuning to a CLAUDE.md file. Many Thanks!

Sonnet Code Review: PM-26250 Explore options to enable direct importer for mac app store build (#17479)

Date: 2026-05-05 | Reviewed by: Claude Code | Model: Sonnet

Summary

Severity	Count
🛑 Blocker	0
⚠️ Important	4
♻️ Refactor	2

This PR enables the Chromium-based direct importer for Mac App Store sandboxed builds by adding a security-scoped bookmark flow: an NSOpenPanel is shown to the user, the chosen folder is validated, and a bookmark is persisted in NSUserDefaults. The Rust layer resumes the bookmark via a new ObjC command bridge before each profile/login read, and releases it on Drop via a fire-and-forget tokio task. The architecture is sound, but four ⚠️ Important issues affect security-scope lifetime correctness, error sanitization, and IPC trust boundaries — all four are recommended fixes before merge. Two ♻️ Refactor issues (a duplicated browser-bundle-ID list and duplicated error-string parsing) can be addressed in follow-ups.

Findings

⚠️ Important

Raw native error strings rendered in user-facing toast without sanitization

libs/importer/src/components/chrome/import-chrome.component.ts:193-195
Caught by: Security & logic agent

Details

The translateValidationError fallback return message; passes unfiltered Rust/ObjC error strings directly to handleChromeImportError, which places them in a toast message.

These strings originate deep in the native layer — for example anyhow!("Failed to call ObjC command: {}", e) and anyhow!("Failed to parse ObjC response: {}", e) in sandbox.rs. They may contain:

• Full filesystem paths (e.g., the resolved bookmark path surfaced on a bookmark-resolution failure)
• Internal Rust/ObjC type names or stack context
• Content derived from a browser's Local State JSON file, which is attacker-influenced data — a crafted browser installation could place arbitrary strings in that file that bubble up through JSON parse errors

This violates P05 (Controlled Access to Vault Data) — vault-adjacent operation context leaks to the UI — and constitutes Data Leaking per Bitwarden's security vocabulary. It also breaks the Trusted Channel model for output authenticity: the user cannot distinguish a genuine Bitwarden error from a string injected via a crafted browser data directory.

CWE-209 (Generation of Error Message Containing Sensitive Information).

Fix. Remove the raw fallback. All unrecognized messages should map to the generic "errorOccurred" i18n key. The existing string-equality check for "browserAccessDenied" is also fragile — prefer matching all known sentinel keys up-front and catching everything else with the generic key.

stopAccessingBrowser resolves a new NSURL instance, breaking Apple's required start/stop symmetry

apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:156-175
Caught by: Security & logic agent

Details

startAccessingBrowser: calls [url startAccessingSecurityScopedResource] on a resolved NSURL instance and returns its .path string. The NSURL itself is not retained anywhere.

stopAccessingBrowser: (called later from Drop) resolves the bookmark data a second time via URLByResolvingBookmarkData: to get a new NSURL instance, and calls stopAccessingSecurityScopedResource on that new instance.

Apple's documentation explicitly states: "You must call this method on the same URL object on which you called startAccessingSecurityScopedResource." Calling stop on a different NSURL instance — even one resolved from the same bookmark data — does not decrement the correct scope counter. The result is that the security scope opened in startAccessingBrowser: is never balanced: the sandboxed process retains read access to the browser directory indefinitely (until process exit) rather than only for the duration of the import operation.

This violates P05 (Controlled Access to Vault Data) — the app retains filesystem access beyond the user's intended grant window — and undermines P06 (Minimized Impact of Security Breaches) because the extended open scope increases the window during which a compromised renderer could initiate additional reads.

CWE-772 (Missing Release of Resource after Effective Lifetime).

Fix. Retain the NSURL from startAccessingBrowser: in an NSMutableDictionary keyed by browserName (similar to how bookmarks are stored), and call stopAccessingSecurityScopedResource on that retained instance in stopAccessingBrowser:.

Fire-and-forget Drop silently skips stop_access if the Tokio runtime is unavailable

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:146-164
Caught by: Security & logic agent

Details

ScopedBrowserAccess::Drop spawns an async task via tokio::task::spawn without storing the JoinHandle. If:

• The Tokio runtime has already begun shutdown when Drop fires (e.g., the import completes and the process tears down),
• The task is dropped before it executes, or
• tokio::task::spawn panics because no runtime is active on the current thread (e.g., a synchronous test path),

then the stop_access ObjC command never executes. Combined with the NSURL symmetry bug above, this creates two compounding failure paths where the security scope is never released.

Even in the non-shutdown case, fire-and-forget spawning means there is no observable signal if the stop silently fails, making the access leak undetectable at the application layer.

This violates P05 (Controlled Access to Vault Data): access to the browser directory (which contains plaintext saved passwords and browsing history — Vault-adjacent data) remains open beyond the user's authorized operation.

CWE-772 (Missing Release of Resource after Effective Lifetime).

Fix. Use a synchronous IPC path for the stop operation in Drop, or redesign ScopedBrowserAccess as an async-only type with an explicit async fn close(self) that is .awaited at the call site, removing the Drop impl entirely. At minimum, gate the spawn on tokio::runtime::Handle::try_current() so a missing runtime is a no-op rather than a panic.

getMetadata trusts renderer-supplied isMas value instead of authoritative isMacAppStore() check

apps/desktop/src/main/tools/import/chromium-importer.service.ts:7
Caught by: Security & logic agent

Details

The IPC handler for chromium_importer.getMetadata accepts isMas: boolean from the renderer process and forwards it directly to native code:

ipcMain.handle("chromium_importer.getMetadata", async (event, isMas: boolean) => {
  return await chromium_importer.getMetadata(isMas);
});

All other new handlers (requestBrowserAccess, getAvailableProfiles, importLogins) correctly use the authoritative isMacAppStore() check in the main process and ignore any renderer-supplied value. getMetadata is the only handler that defers to the renderer.

When mas_build=true is passed, DefaultInstalledBrowserRetriever::get_installed_browsers returns all supported browsers without probing the filesystem. A compromised renderer can call getMetadata(true) on a non-MAS build to enumerate all supported browser names without any filesystem-presence check — bypassing the gate that normally prevents disclosure of which browser configurations Bitwarden supports on the user's machine.

This is a trust boundary violation at the IPC layer: the renderer is an untrusted context (runs in a sandboxed renderer process) and should not control security-relevant flags. This implicates P05 (Controlled Access to Vault Data) and TC (Trusted Channel) — the IPC channel is not authenticating the legitimacy of the renderer-supplied flag.

CWE-20 (Improper Input Validation) / CWE-610 (Externally Controlled Reference to a Resource).

Fix. Replace the renderer-supplied parameter with the authoritative main-process value, identical to the pattern used for the other three handlers:

ipcMain.handle("chromium_importer.getMetadata", async (_event) => {
  return await chromium_importer.getMetadata(isMacAppStore());
});

Drop isMas from apps/desktop/src/app/tools/preload.ts and desktop-import-metadata.service.ts accordingly.

♻️ Refactor

Browser name-to-bundle-ID mapping duplicated alongside SUPPORTED_BROWSERS in macos.rs

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:7-16
Caught by: Architecture agent

Details

BROWSER_BUNDLE_IDS in sandbox.rs lists the same 7 browsers — Chrome, Chromium, Microsoft Edge, Brave, Arc, Opera, Vivaldi — that are already enumerated in SUPPORTED_BROWSERS in apps/desktop/desktop_native/chromium_importer/src/chromium/platform/macos.rs. There is no compile-time enforcement keeping them in sync.

The is_browser_installed function silently falls back to return Ok(true) when a browser name has no corresponding bundle ID entry (let Some(bundle_id) = bundle_id else { return Ok(true); }). This means a browser added to SUPPORTED_BROWSERS in macos.rs without a matching entry in BROWSER_BUNDLE_IDS will bypass the installation check entirely and show up in the UI as installed on MAS builds.

Fix. Add an optional bundle_id: Option<&'static str> field to BrowserConfig and derive the bundle-ID lookup from that field rather than maintaining a parallel list. Alternatively, replace the silent Ok(true) fallback with Ok(false) (or an Err) so the parity gap fails loudly.

Error-message parsing logic for browser-access errors duplicated across two layers

apps/desktop/src/app/tools/import/import-desktop.component.ts:60-86
Caught by: Code quality agent

Details

ImportDesktopComponent._onLoadProfilesFromBrowser (desktop component) and ImportChromeComponent.translateValidationError (shared importer library) both contain the same pattern-matching logic for the chromiumImporterBrowserNotInstalled:... error key and the browserAccessDenied string.

Because _onLoadProfilesFromBrowser runs first and re-throws with an already-translated human-readable string, the matching code in translateValidationError is dead for errors originating from the desktop profile-loading path. If the error-key format ever changes, both locations must be updated in sync — with no compile-time guarantee they stay consistent.

Fix. The error encoding/decoding contract should live in one place. Either both layers pass structured error codes (not translated strings) up the chain, or the translation happens exclusively at the outermost catch boundary. Mixing translated strings with error-code patterns makes the flow hard to follow and easy to break silently.

Reviewed and Dismissed

🔍 5 initial findings dismissed after validation

getMetadata accepts isMas from renderer; other handlers derive it in main process

apps/desktop/src/main/tools/import/chromium-importer.service.ts:7
Caught by: Architecture agent
Original severity: ♻️ Refactor
Original confidence: 92/100
Dismissed at: Step 5 severity audit
Dismissed because: Duplicate of sec-4 — same underlying issue (getMetadata trusting renderer-supplied isMas), weaker framing.

chromiumImporterBrowserNotInstalled error-string parsing duplicated across two components

apps/desktop/src/app/tools/import/import-desktop.component.ts:60-86
Caught by: Architecture agent
Original severity: ♻️ Refactor
Original confidence: 85/100
Dismissed at: Step 5 severity audit
Dismissed because: Duplicate of quality-3 — same underlying issue (error-string parsing duplicated across components), weaker framing.

tokio::task::spawn in Drop panics if no runtime is active at drop time

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:146-164
Caught by: Code quality agent
Original severity: ⚠️ Important
Original confidence: 85/100
Dismissed at: Step 5 severity audit
Dismissed because: Duplicate of sec-3 — same underlying issue (tokio::task::spawn in Drop panics if no runtime), weaker framing.

stopAccessingBrowser resolves a new NSURL instance instead of reusing the one from startAccessingBrowser

apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:156-175
Caught by: Code quality agent
Original severity: ⚠️ Important
Original confidence: 82/100
Dismissed at: Step 5 severity audit
Dismissed because: Duplicate of sec-2 — same underlying issue (NSURL start/stop symmetry), weaker framing.

Raw system error messages displayed to users as toast content

libs/importer/src/components/chrome/import-chrome.component.ts:193-195
Caught by: Bug analysis agent
Original severity: ⚠️ Important
Original confidence: 85/100
Dismissed at: Step 5 severity audit
Dismissed because: Duplicate of sec-1 — same underlying issue (raw error fallback reaching user-facing toast), weaker framing.

---

Opus Code Review: PM-26250 Explore options to enable direct importer for mac app store build (#17479)

Date: 2026-05-05 | Reviewed by: Claude Code

Summary

Severity	Count
🛑 Blocker	0
⚠️ Important	0
♻️ Refactor	9

The PR enables the Chromium direct-importer flow for Mac App Store (sandboxed) desktop builds by introducing a security-scoped-bookmark request flow on the ObjC side and threading a new mas_build boolean through the napi → Rust → IPC → Angular call chain. No blocker- or important-tier issues survived validation: the most colorful claims (panic-in-Drop, mismatched-NSURL security-scope leak) were either dismissed by the validation pass or downgraded to refactor once the runtime guarantees and Apple's per-counter (not per-instance) contract were considered. What remains is structural debt: parallel browser arrays in three files, an inconsistent source of truth for the isMas flag spread across renderer, preload, and main, an unused bookmark field papered over with #[allow(dead_code)], and shared library code that reaches for desktop-only i18n keys. None of these block merge, but each is worth resolving before the next change in this area.

Findings

♻️ Refactor

Parallel browser arrays will drift when Chromium browsers are added

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:7-15
Caught by: Architecture agent

Details

BROWSER_BUNDLE_IDS (sandbox.rs lines 7-15) duplicates the browser-name keys already enumerated in SUPPORTED_BROWSERS (apps/desktop/desktop_native/chromium_importer/src/chromium/platform/macos.rs lines 14-43) and KEYCHAIN_CONFIG (macos.rs lines 68-104). Adding or removing a Chromium-based browser now requires synchronized edits across three parallel arrays in two files, with the only coupling being a string compare on the name field. The new sandbox path looks the bundle ID up by name; the natural home for that data is on BrowserConfig itself (e.g. an optional bundle_id: Option<&'static str> field next to data_dir) so the macOS browser table remains the single source of truth.

Inconsistent source of truth for isMas across IPC handlers

apps/desktop/src/main/tools/import/chromium-importer.service.ts:9-25
Caught by: Architecture agent

Details

Within a single class, two different patterns are used for the same mas_build parameter that is threaded all the way through napi → Rust:

• chromium_importer.getMetadata accepts isMas from the renderer (apps/desktop/src/app/tools/import/desktop-import-metadata.service.ts calls this.system.environment.isMacAppStore() and ships it across IPC; preload.ts forwards it).
• chromium_importer.requestBrowserAccess, getAvailableProfiles, and importLogins ignore any renderer hint and derive the value in the main process by calling isMacAppStore() from apps/desktop/src/utils.ts.

Pick one. The main-process isMacAppStore() utility is already used by every other consumer in apps/desktop/src/main/ and is the established pattern for this signal. Threading the boolean through preload and the renderer for getMetadata only adds a parameter to the IPC contract that the main process does not trust for the other three handlers — and the preload-side change in apps/desktop/src/app/tools/preload.ts only updated getMetadata and requestBrowserAccess to accept new args, leaving the other two preload signatures unchanged, which makes the asymmetry visible in three layers.

Shared lib component depends on desktop-only i18n keys

libs/importer/src/components/chrome/import-chrome.component.ts:181-205
Caught by: Code quality agent

Details

translateValidationError (in libs/importer) calls i18nService.t("chromiumImporterBrowserNotInstalled", ...) and i18nService.t("browserAccessDenied"). Both keys are added only to apps/desktop/src/locales/en/messages.json in this PR — they don't exist in libs/common or apps/web/apps/browser locales.

This violates the apps/libs boundary documented in the repo's CLAUDE.md ("Strict boundaries must be maintained between apps and libraries"). Today the shared component is wired only by the desktop callback so the codepath isn't reached in web/browser, but a sibling app that wires onLoadProfilesFromBrowser will silently render the raw key. Move the strings into a shared locale, or move the translation step into the desktop caller and have the shared component pass through already-translated messages.

Inconsistent MAS-flag origin between IPC handlers (duplicate framing)

apps/desktop/src/main/tools/import/chromium-importer.service.ts:8-25
Caught by: Code quality agent

Details

Three of four IPC handlers (requestBrowserAccess, getAvailableProfiles, importLogins) call isMacAppStore() themselves in the main process and ignore any renderer-supplied flag. getMetadata, however, accepts isMas from the renderer. Pick one. The main-process isMacAppStore() utility is the established pattern for this signal; threading the boolean through preload and the renderer for getMetadata only adds a parameter to the IPC contract that the main process does not trust for the other three handlers. (Same root issue as the architecture agent's finding above; surfaced here from the renderer-side perspective.)

ScopedBrowserAccess::Drop spawns fire-and-forget tokio task

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:147-164
Caught by: Code quality agent

Details

Drop calls tokio::task::spawn to send a stop_access cleanup. Two concerns:

1. tokio::task::spawn panics if no current runtime is available. In the Drop path that becomes a double-unwind and aborts the process. The construction sites today are all async fn resume() calls, so the runtime is reliably present when the value is constructed and dropped — but a future refactor that constructs/drops ScopedBrowserAccess synchronously (e.g., for testing) would silently introduce abort-in-Drop.
2. The returned JoinHandle is discarded and the result of desktop_objc::run_command is ignored (let _ = ...). Cleanup is fire-and-forget; if the runtime tears down before the task runs, the security-scoped resource is not released until process exit.

Recommend an explicit async fn close(self) consumed by import_logins before returning, with Drop retained only as a best-effort fallback gated by tokio::runtime::Handle::try_current() so it cannot panic.

Dead bookmark field in RequestAccessResponse

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:32-36
Caught by: Code quality agent

Details

#[derive(Debug, Deserialize)]
struct RequestAccessResponse {
    #[allow(dead_code)]
    bookmark: String,
}

The field is parsed from the ObjC response but never read — the only consumer matches CommandResult::Success { .. } with ... The #[allow(dead_code)] annotation papers over a true unused field added by this PR rather than removing it. Either delete the field (the ObjC side already saves the bookmark to NSUserDefaults, so there's no need to round-trip it across FFI) or have a consumer actually read it.

ScopedBrowserAccess::Drop calls tokio::task::spawn (duplicate framing)

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:144
Caught by: Bug analysis agent

Details

Same Drop + tokio::task::spawn concern as flagged by the code quality and security agents. The bug-analysis perspective: the JoinHandle is discarded and let _ = desktop_objc::run_command(input_json).await swallows any error, so any failure to release the security-scoped resource is invisible. In practice the runtime is always present when the value is dropped (because the construction site is async), so this lands at refactor severity rather than important. See the quality-3 finding above for the suggested fix.

preload.requestBrowserAccess signature does not forward isMas

apps/desktop/src/app/tools/preload.ts:12
Caught by: Bug analysis agent

Details

The napi .d.ts declares requestBrowserAccess(browser: string, masBuild: boolean): Promise<void> with masBuild required. The preload bridge sends only browser:

requestBrowserAccess: (browser: string): Promise<void> =>
  ipcRenderer.invoke("chromium_importer.requestBrowserAccess", browser),

The ipcMain handler in chromium-importer.service.ts calls chromium_importer.requestBrowserAccess(browser, isMacAppStore()), so functionally this works today. However, the inconsistent shape — getMetadata forwards isMas from the renderer, requestBrowserAccess/getAvailableProfiles/importLogins do not — is the same root issue surfaced by the architecture agent. A future change that wires requestBrowserAccess to honor a renderer-supplied flag will silently break.

Drop cleanup is fire-and-forget — implicates P05 (controlled access)

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:149
Caught by: Security & logic agent

Details

The only mechanism intended to release the security scope at the end of an import is Drop → tokio::task::spawn → stop_access. The result is discarded (let _ = ...), so any failure to release the scope is silent. The doc comment in import_logins ("releases the scope on Drop after the reads complete") advertises a contract that is not reliably honored.

The runtime presence guarantee at the construction site means this does not cross the Important bar in practice — but the cleanup pattern is the kind of P05 (Controlled access) concern worth tightening: an explicit awaited close() with the panic-prone Drop reduced to a guarded fallback would make the contract observable.

Reviewed and Dismissed

🔍 5 initial findings dismissed after validation

Strict path-equality check returns generic error on mis-selection

apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:65-71
Caught by: Code quality agent
Original severity: ⚠️ Important
Original confidence: 82/100
Dismissed at: Step 4 validation
Dismissed because: UX nit, not a bug — the Local-State JSON validation provides defense-in-depth and the dialog pre-selects the expected directoryURL, so the common path passes the equality check; users selecting a different path getting a generic error is a UX polish item below Refactor severity.

Trailing whitespace and out-of-order entry in messages.json

apps/desktop/src/locales/en/messages.json:4806
Caught by: Code quality agent
Original severity: ♻️ Refactor
Original confidence: 80/100
Dismissed at: Step 4 validation
Dismissed because: Trailing whitespace in JSON is harmless and a linter would catch it; the out-of-order placement of the new key is sub-Refactor noise that a senior reviewer would not block on.

is_browser_installed returns true for unknown browser names

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:197
Caught by: Bug analysis agent
Original severity: ⚠️ Important
Original confidence: 90/100
Dismissed at: Step 4 validation
Dismissed because: The only caller request_only validates browser_name against SUPPORTED_BROWSERS before invoking is_browser_installed, and BROWSER_BUNDLE_IDS contains the same seven entries as macOS SUPPORTED_BROWSERS, so the unknown-name branch is unreachable today; the structural drift risk is already covered by arch-1.

stopAccessingBrowser resolves bookmark to a fresh NSURL — start/stop unbalanced

apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:154
Caught by: Bug analysis agent
Original severity: ⚠️ Important
Original confidence: 85/100
Dismissed at: Step 4 validation
Dismissed because: Apple's documented contract for security-scoped resources balances start/stop calls on the URL but does not unambiguously require the same NSURL instance — the underlying counter is on the resolved security scope, and per the validation directive without conclusive proof of an instance-identity requirement this is dismissed.

stopAccessingBrowser calls stop on different NSURL instance — scoped resource not released

apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:153
Caught by: Security & logic agent
Original severity: ⚠️ Important
Original confidence: 85/100
Dismissed at: Step 4 validation
Dismissed because: Same reasoning as the bug-analysis duplicate above — cannot conclusively confirm Apple's contract requires the identical NSURL instance for stopAccessingSecurityScopedResource; the security-scope counter is associated with the resolved bookmark scope, not strictly the Obj-C object identity.

[theMickster]

Hi John 👋🏼
Trying to reconcile the fact from fiction regarding the Claude findings (so that future engineers benefit from tuning the context that Claude has during coding + code reviewing) has made for an enjoyable morning 🤣

Have you consider or ran into the following?

apps/desktop/resources/entitlements.mas.plist

⚠️ Missing com.apple.security.files.bookmarks.app-scope entitlement — the persistent bookmark flow this PR is built around will silently fail in a signed MAS build without it. Same fix applies to entitlements.mas.autofill-enabled.plist.

Details and fix

Apple's documentation article "Enabling Security-Scoped Bookmark and URL Access" states:

"If you want to provide your sandboxed app with persistent access to file-system resources, you must enable security-scoped bookmark and URL access."

It defines two entitlement keys:

Entitlement key	Description
com.apple.security.files.bookmarks.app-scope	Enables use of app-scoped bookmarks and URLs.
com.apple.security.files.bookmarks.document-scope	Enables use of document-scoped bookmarks and URLs.

The new BrowserAccessManager calls bookmarkDataWithOptions:NSURLBookmarkCreationWithSecurityScope to create a bookmark and URLByResolvingBookmarkData:options:NSURLBookmarkResolutionWithSecurityScope to resume it on subsequent launches. Both operations are gated on com.apple.security.files.bookmarks.app-scope.

What currently exists in both MAS plist files:

<key>com.apple.security.files.user-selected.read-write</key>
<true/>

user-selected.read-write covers the one-time NSOpenPanel access. It does not cover persisted bookmark creation or resolution. In a real signed MAS binary (not a development-signed build — dev signing does not strictly enforce sandbox entitlements), bookmarkDataWithOptions: will return nil or an error without the bookmark entitlement. The resume path silently fails and the user is re-prompted on every launch, defeating the entire purpose of the PR.

Fix — add to both entitlements.mas.plist and entitlements.mas.autofill-enabled.plist:

<key>com.apple.security.files.bookmarks.app-scope</key>
<true/>

How to verify on a real MAS build:

1. Sign the app with a valid MAS provisioning profile (not a dev certificate).
2. Grant browser access on first import.
3. Quit and relaunch the app.
4. Trigger import again — the picker should not appear if the bookmark resolved correctly.

If the picker reappears every launch, the entitlement is missing or not taking effect.

I tracked down some Apple specific documentation here: Enabling Security-Scoped Bookmark and URL Access

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
7.7% Duplication on New Code

See analysis details on SonarQube Cloud

[coroiu]

No changes to platform code