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[bot]]

Checkmarx One – Scan Summary & Details – 70a6fdd1-e33b-4e40-9104-a762a36562ed

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

[codecov[bot]]

Codecov Report

:x: Patch coverage is 3.21101% with 211 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 42.05%. Comparing base (3bd9ee1) to head (7b897b4). :white_check_mark: All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...chromium_importer/src/chromium/platform/sandbox.rs	0.00%	109 Missing :warning:
...sktop_native/chromium_importer/src/chromium/mod.rs	0.00%	37 Missing :warning:
apps/desktop/desktop_native/napi/src/lib.rs	0.00%	19 Missing :warning:
...r/src/components/chrome/import-chrome.component.ts	0.00%	19 Missing :warning:
...p/src/app/tools/import/import-desktop.component.ts	0.00%	13 Missing :warning:
...src/main/tools/import/chromium-importer.service.ts	0.00%	7 Missing :warning:
...pp/tools/import/desktop-import-metadata.service.ts	0.00%	2 Missing :warning:
apps/desktop/src/app/tools/preload.ts	0.00%	2 Missing :warning:
...e/chromium_importer/src/chromium/platform/macos.rs	0.00%	1 Missing :warning:
apps/desktop/src/main.ts	0.00%	1 Missing :warning:
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17479      +/-   ##
==========================================
- Coverage   42.12%   42.05%   -0.07%     
==========================================
  Files        3592     3593       +1     
  Lines      104316   104489     +173     
  Branches    15741    15747       +6     
==========================================
+ Hits        43944    43945       +1     
- Misses      58505    58677     +172     
  Partials     1867     1867              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude[bot]]

Claude finished @harr1424's task in 2m 36s —— View job

---

Code Review Complete

I've completed a thorough review of the Mac App Store sandbox implementation for the direct importer. The architecture is solid, but there are critical issues that need to be addressed before merge.

---

Critical Issues (❌)

1. Async operation in Drop implementation (sandbox.rs:155-173)

Location: apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:155

The Drop implementation spawns an async task that may not complete before program termination, potentially leaving security-scoped resources unreleased.

impl Drop for ScopedBrowserAccess {
    fn drop(&mut self) {
        tokio::task::spawn(async move {
            // This task may never complete!
            let _ = desktop_objc::run_command(input_json).await;
        });
    }
}

Problem: The spawned task is fire-and-forget. If the program terminates immediately after Drop, stopAccessingSecurityScopedResource may never be called, violating Apple's sandbox requirements.

Fix: Add an explicit async fn close() method for proper cleanup and use Drop only for logging warnings:

impl ScopedBrowserAccess {
    pub async fn close(self) {
        // Explicit async cleanup
        let _ = desktop_objc::run_command(input_json).await;
    }
}

impl Drop for ScopedBrowserAccess {
    fn drop(&mut self) {
        eprintln!("Warning: ScopedBrowserAccess dropped without calling close()");
    }
}

2. Missing error handling for stale bookmarks (browser_access_manager.m:122-133)

Location: apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:122

When a bookmark becomes stale, the code attempts to create a new one but doesn't verify it's valid. The user may need to re-grant access.

if (isStale) {
    NSData *newBookmarkData = [url bookmarkDataWithOptions:...];
    if (!newBookmarkData) {
        return nil;  // No way to distinguish why this failed
    }
    [self saveBookmark:newBookmarkData forBrowser:browserName];
}

Fix: Clear the stale bookmark when recreation fails so the UI knows to re-request access:

if (!newBookmarkData) {
    [self saveBookmark:nil forBrowser:browserName];  // Clear stale bookmark
    return nil;
}

3. Race condition with NSURL instances (browser_access_manager.m:142-161)

Location: apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:142

Apple's documentation requires calling startAccessingSecurityScopedResource and stopAccessingSecurityScopedResource on the same NSURL instance. Currently, stopAccessingBrowser creates a new NSURL by re-resolving the bookmark.

Problem: If the bookmark was updated due to staleness between start and stop, you're calling stop on a different URL instance.

Fix: Cache the resolved NSURL instances:

@implementation BrowserAccessManager {
    NSMutableDictionary<NSString *, NSURL *> *_activeURLs;
}

- (NSString *)startAccessingBrowser:(NSString *)browserName {
    // ... resolve bookmark ...
    if (![url startAccessingSecurityScopedResource]) {
        return nil;
    }
    _activeURLs[browserName] = url;  // Cache for later
    return url.path;
}

- (void)stopAccessingBrowser:(NSString *)browserName {
    NSURL *url = _activeURLs[browserName];
    if (url) {
        [url stopAccessingSecurityScopedResource];
        [_activeURLs removeObjectForKey:browserName];
    }
}

---

Important Issues (⚠️)

4. No validation of Local State readability (browser_access_manager.m:62)

The code checks if "Local State" exists but not if it's readable.

Fix: Add readability check:

if (![fileManager isReadableFileAtPath:localStatePath.path]) {
    return nil;
}

5. Silent error swallowing in Drop (sandbox.rs:168)

Errors are ignored with let _ =, making debugging difficult.

Fix: Add logging:

if let Err(e) = desktop_objc::run_command(input_json).await {
    eprintln!("Warning: Failed to stop accessing browser {}: {}", browser_name, e);
}

---

Technical Debt (♻️)

6. BrowserAccessManager recreated on every command

Each command creates a new instance instead of using a singleton, which prevents proper state tracking for active URLs (needed for Fix #3).

Recommendation: Implement singleton pattern:

+ (instancetype)sharedManager {
    static BrowserAccessManager *sharedManager = nil;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        sharedManager = [[self alloc] init];
    });
    return sharedManager;
}

---

Questions (❓)

7. Handling revoked permissions

hasStoredAccess only checks if a bookmark exists, not if it's still valid. If a user revokes permissions via System Settings, the app won't know until attempting import.

Suggestion: Add a validate_access method that proactively checks bookmark validity and prompts for re-authorization before import attempts.

---

Positive Observations

1. Correct use of security-scoped bookmarks for App Store sandbox
2. Proper bookmark persistence with UserDefaults
3. Good user experience with file picker and clear instructions
4. Appropriate use of async/await throughout the call stack
5. ARC enabled in Objective-C code for memory safety

---

Overall Assessment

REQUEST CHANGES - The architecture is solid, but Findings 1, 2, and 3 are blocking issues that could cause resource leaks and violate Apple's API contracts. Please address these before merge.

After fixes, this will be a solid foundation for sandboxed direct import.

[Hinton]

Electron might support this out of the box, I found StartAccessingSecurityScopedResource, which seems to be quite similar to what we want to achieve, although we have to do this on the JS side.

[Hinton]

You still have the sandbox flag.

[harr1424]

You still have the sandbox flag.

Sorry, based upon this comment I thought it would make more sense to keep the flag for conditional compilation, but to use a bool to control conditional logic.

Just confirming you would like the flag removed completely? And always compile the sandbox related code for all Mac OS builds?

[Hinton]

You still have the sandbox flag.

Sorry, based upon this comment I thought it would make more sense to keep the flag for conditional compilation, but to use a bool to control conditional logic.

Just confirming you would like the flag removed completely? And always compile the sandbox related code for all Mac OS builds?

I see how that could be confusing. Personally I would prefer not gating this behind a flag. It's not something that probably needs to be behind a flag, and always compiling it should prevent any accidental enabling/disabling of the flag as it's not a pattern we use elsewhere at the moment.

[claude[bot]]

Overall Assessment: REQUEST CHANGES

Critical Issues:

• apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:159 - Unsafe async operation in Drop implementation
• apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:67 - Missing error handling for bookmark creation failures
• apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:102-152 - Race condition in security-scoped resource access

See detailed inline analysis below.

---

Finding 1: Unsafe async operation in Drop implementation

Location: apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:159

Severity: ❌ CRITICAL

Details and fix

The Drop implementation spawns a Tokio task without ensuring it completes:

impl Drop for ScopedBrowserAccess {
    fn drop(&mut self) {
        tokio::task::spawn(async move {
            // This task may never complete before the program exits
            let _ = desktop_objc::run_command(input_json).await;
        });
    }
}

Problem: When ScopedBrowserAccess is dropped, the spawned task is fire-and-forget. If the program terminates immediately after, stopAccessingSecurityScopedResource may never be called, leaving the security-scoped resource open and potentially causing:

• Resource leaks in macOS sandbox
• File descriptors remaining open
• Security audit failures

Fix: Use tokio::runtime::Handle::block_on or make cleanup synchronous via direct FFI:

impl Drop for ScopedBrowserAccess {
    fn drop(&mut self) {
        let browser_name = self.browser_name.clone();
        
        // Block on cleanup to ensure it completes
        if let Ok(handle) = tokio::runtime::Handle::try_current() {
            handle.block_on(async move {
                let input = CommandInput {
                    namespace: "chromium_importer".to_string(),
                    command: "stop_access".to_string(),
                    params: serde_json::json!({"browserName": browser_name}),
                };
                if let Ok(input_json) = serde_json::to_string(&input) {
                    let _ = desktop_objc::run_command(input_json).await;
                }
            });
        }
    }
}

---

Finding 2: Missing error handling for bookmark creation failures

Location: apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:67-68, 103-104

Severity: ❌ CRITICAL

Details and fix

Two locations silently return nil on bookmark creation failure without logging:

// Line 67-68
if (!bookmarkData) {
    return nil;  // No logging - user gets generic error
}

// Line 103-104  
if (!newBookmarkData) {
    return nil;  // Stale bookmark refresh failed silently
}

Problem: When bookmark creation fails, the error object contains critical debugging information (permissions, file system errors, etc.) but is discarded. Users and developers cannot diagnose why access failed.

Fix: Log the error before returning:

if (!bookmarkData) {
    NSLog(@"Failed to create security bookmark for %@: %@", browserName, error.localizedDescription);
    return nil;
}

if (!newBookmarkData) {
    NSLog(@"Failed to refresh stale bookmark for %@: %@", browserName, error.localizedDescription);
    return nil;
}

---

Finding 3: Race condition in security-scoped resource access

Location: apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:102-152

Severity: ⚠️ IMPORTANT

Details and fix

Multiple concurrent calls to resume() for the same browser could result in:

1. Multiple calls to startAccessingSecurityScopedResource() on the same URL
2. Unbalanced start/stop calls (N starts, 1 stop in Drop)
3. macOS sandbox errors or undefined behavior

Problem: The current implementation has no synchronization:

pub async fn resume(browser_name: &str) -> Result<Self> {
    // Multiple concurrent calls possible
    let start_output = desktop_objc::run_command(...).await?;
    // Now multiple ScopedBrowserAccess instances exist for same browser
    Ok(Self { browser_name: browser_name.to_string() })
}

Fix: Add a global mutex to ensure only one active access per browser:

use std::sync::Mutex;
use std::collections::HashSet;

static ACTIVE_ACCESSES: Mutex<HashSet<String>> = Mutex::new(HashSet::new());

impl ScopedBrowserAccess {
    pub async fn resume(browser_name: &str) -> Result<Self> {
        // Acquire lock
        let mut active = ACTIVE_ACCESSES.lock().unwrap();
        if active.contains(browser_name) {
            return Err(anyhow!("Browser access already active"));
        }
        active.insert(browser_name.to_string());
        drop(active);  // Release lock before async operation
        
        // Existing logic...
        
        Ok(Self { browser_name: browser_name.to_string() })
    }
}

impl Drop for ScopedBrowserAccess {
    fn drop(&mut self) {
        // Remove from active set
        if let Ok(mut active) = ACTIVE_ACCESSES.lock() {
            active.remove(&self.browser_name);
        }
        // Existing cleanup...
    }
}

---

Finding 4: Stale bookmark handling doesn't verify new bookmark validity

Location: apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:97-108

Severity: ⚠️ IMPORTANT

Details and fix

When a bookmark is detected as stale, the code creates a new bookmark and saves it without verifying the new bookmark actually works:

if (isStale) {
    NSData *newBookmarkData = [url bookmarkDataWithOptions:NSURLBookmarkCreationWithSecurityScope ...];
    if (!newBookmarkData) {
        return nil;  // OK - handles failure
    }
    [self saveBookmark:newBookmarkData forBrowser:browserName];
    // But continues with OLD url, not re-resolved from new bookmark
}

if (![url startAccessingSecurityScopedResource]) {
    return nil;
}

Problem: The url variable still points to the URL resolved from the OLD (stale) bookmark. The newly created bookmark is saved but never validated. If the stale bookmark worked but the new one doesn't, we save a broken bookmark.

Fix: Re-resolve the new bookmark before using it:

if (isStale) {
    NSData *newBookmarkData = [url bookmarkDataWithOptions:NSURLBookmarkCreationWithSecurityScope
                                        includingResourceValuesForKeys:nil
                                        relativeToURL:nil
                                        error:&error];
    if (!newBookmarkData) {
        NSLog(@"Failed to refresh stale bookmark: %@", error.localizedDescription);
        return nil;
    }
    
    // Re-resolve the NEW bookmark before saving
    BOOL newIsStale = NO;
    NSURL *newUrl = [NSURL URLByResolvingBookmarkData:newBookmarkData
                                options:NSURLBookmarkResolutionWithSecurityScope
                                relativeToURL:nil
                                bookmarkDataIsStale:&newIsStale
                                error:&error];
    
    if (!newUrl || newIsStale) {
        NSLog(@"New bookmark is invalid or immediately stale");
        return nil;
    }
    
    [self saveBookmark:newBookmarkData forBrowser:browserName];
    url = newUrl;  // Use the new URL
}

---

Finding 5: Deprecated API usage - NSUserDefaults synchronize

Location: apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m:148

Severity: 🎨 SUGGESTED

Details and improvement

[defaults synchronize];

Issue: synchronize has been deprecated since macOS 10.12. The system automatically synchronizes preferences at periodic intervals and on app termination.

Fix: Remove the line:

- (void)saveBookmark:(NSData *)data forBrowser:(NSString *)browserName {
    NSUserDefaults *defaults = [NSUserDefaults standardUserDefaults];
    NSString *key = [self bookmarkKeyFor:browserName];
    [defaults setObject:data forKey:key];
    // [defaults synchronize];  // Remove - handled automatically
}

This is a minor issue but eliminates deprecated API usage.

---

Finding 6: Inconsistent masBuild parameter propagation

Location: Multiple files

Severity: ♻️ DEBT

Details and rationale

The mas_build boolean is determined in multiple ways:

• TypeScript: isMacAppStore() from utils (line 5, chromium-importer.service.ts)
• Passed as parameter through entire call chain
• Checked at runtime in Rust

Problem: This creates:

1. Redundant parameter passing through multiple layers
2. Potential for mismatch if environment changes mid-execution
3. Makes code harder to test (must mock environment)

Suggestion: Consider determining once at application startup and storing in a global configuration rather than passing through every function call. This would simplify signatures and reduce coupling.

Current:

pub async fn import_logins(browser_name: &str, profile_id: &str, mas_build: bool)

Potential:

pub async fn import_logins(browser_name: &str, profile_id: &str)
// Internally checks config::is_mas_build()

This is technical debt but not blocking for this PR.

---

Finding 7: Browser configuration duplication

Location: apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs:9-17 and other platform files

Severity: ♻️ DEBT

Details and rationale

Browser bundle IDs are duplicated between:

• BROWSER_BUNDLE_IDS constant (sandbox.rs)
• Browser name strings throughout codebase
• Platform-specific browser configs

Problem: When adding a new browser, developers must update multiple locations. Risk of inconsistency.

Suggestion: Centralize browser metadata in a single source of truth, possibly extending the existing BrowserConfig struct to include optional bundle IDs. Not blocking for this PR but consider for future refactoring.

---

Finding 8: Missing null check in requestAccessCommand

Location: apps/desktop/desktop_native/objc/src/native/chromium_importer/commands/request_access.m:17-18

Severity: ❓ QUESTION

Question and context

if (bookmarkData == nil) {
    return _return(context, _error(@"browserAccessDenied"));
}

Question: The error message "browserAccessDenied" is generic. Can we differentiate between:

1. User explicitly declined permission (user cancelled dialog)
2. Invalid browser directory selected (validation failed at line 55-58)
3. System-level permission error

Each scenario needs different user guidance. Current implementation treats all failures identically.

Suggestion: Have requestAccessToBrowserDir return more specific error information (perhaps an NSError?) so we can provide better user feedback.

---

Positive Observations

1. Security-focused design: Using security-scoped bookmarks is the correct approach for Mac App Store sandbox
2. Proper ARC usage: Objective-C code enables ARC, reducing memory management risks
3. Async/await propagation: Proper use of async/await through the entire call stack
4. User experience: File picker with clear instructions helps users grant correct permissions
5. Bookmark persistence: Using UserDefaults for bookmark storage is appropriate

---

Testing Recommendations

Before merging, please verify:

1. Concurrent imports: Start two browser imports simultaneously - verify no crashes or leaked resources
2. Stale bookmark recovery: Manually modify bookmark in UserDefaults to trigger stale path - verify recovery works
3. Permission denial: Cancel permission dialog - verify error message is user-friendly
4. App termination during import: Kill app mid-import - verify no leaked security-scoped resources
5. Invalid browser selection: Select wrong folder - verify appropriate error message
6. Multiple browsers: Import from 3+ different browsers sequentially - verify bookmarks persist correctly