respond to review comments:

• replace usage of compile time flag with boolean for conditinal logic
• moved sandbox specific logic to contained module inside macos.rs
• remove redundant browser array from objc code and pass the target browser path as arg

apps/desktop/desktop_native/chromium_importer/src/chromium/mod.rs

@@ -51,25 +51,21 @@ pub enum LoginImportResult {
 }
 
 pub trait InstalledBrowserRetriever {
-    fn get_installed_browsers() -> Result<Vec<String>>;
+    fn get_installed_browsers(mas_build: bool) -> Result<Vec<String>>;
 }
 
 pub struct DefaultInstalledBrowserRetriever {}
 
 impl InstalledBrowserRetriever for DefaultInstalledBrowserRetriever {
-    fn get_installed_browsers() -> Result<Vec<String>> {
+    fn get_installed_browsers(mas_build: bool) -> Result<Vec<String>> {
         let mut browsers = Vec::with_capacity(SUPPORTED_BROWSER_MAP.len());
 
         #[allow(unused_variables)] // config only used outside of sandbox
         for (browser, config) in SUPPORTED_BROWSER_MAP.iter() {
-            #[cfg(all(target_os = "macos", feature = "sandbox"))]
-            {
-                // macOS sandbox mode: show all browsers, user will grant access when selected
+            if mas_build {
+                // show all browsers for MAS builds, user will grant access when selected
                 browsers.push((*browser).to_string());
-            }
-
-            #[cfg(not(all(target_os = "macos", feature = "sandbox")))]
-            {
+            } else {
                 // When not in sandbox check file system directly
                 let data_dir = get_browser_data_dir(config)?;
                 if data_dir.exists() {
@@ -91,7 +87,7 @@ pub fn get_available_profiles(browser_name: &str) -> Result<Vec<ProfileInfo>> {
 /// This shows the permission dialog and creates a security-scoped bookmark,
 #[cfg(all(target_os = "macos", feature = "sandbox"))]
 pub fn request_browser_access(browser_name: &str) -> Result<()> {
-    platform::ScopedBrowserAccess::request_only(browser_name)?;
+    platform::sandbox::ScopedBrowserAccess::request_only(browser_name)?;
 
     Ok(())
 }
@@ -99,7 +95,7 @@ pub fn request_browser_access(browser_name: &str) -> Result<()> {
 pub async fn import_logins(browser_name: &str, profile_id: &str) -> Result<Vec<LoginImportResult>> {
     // In sandbox mode, resume access to browser directory (use the formerly created bookmark)
     #[cfg(all(target_os = "macos", feature = "sandbox"))]
-    let _access = platform::ScopedBrowserAccess::resume(browser_name)?;
+    let _access = platform::sandbox::ScopedBrowserAccess::resume(browser_name)?;
 
     let (data_dir, local_state) = load_local_state_for_browser(browser_name)?;
 

apps/desktop/desktop_native/chromium_importer/src/chromium/platform/macos.rs

@@ -1,8 +1,3 @@
-#[cfg(feature = "sandbox")]
-use std::ffi::CString;
-#[cfg(feature = "sandbox")]
-use std::os::raw::c_char;
-
 use anyhow::{anyhow, Result};
 use async_trait::async_trait;
 use security_framework::passwords::get_generic_password;
@@ -17,64 +12,76 @@ use crate::{
 //
 
 #[cfg(feature = "sandbox")]
-extern "C" {
-    fn requestBrowserAccess(browser_name: *const c_char) -> *mut c_char;
-    fn hasStoredBrowserAccess(browser_name: *const c_char) -> bool;
-    fn startBrowserAccess(browser_name: *const c_char) -> *mut c_char;
-    fn stopBrowserAccess(browser_name: *const c_char);
-}
+pub mod sandbox {
+    use std::{ffi::CString, os::raw::c_char};
 
-#[cfg(feature = "sandbox")]
-pub struct ScopedBrowserAccess {
-    browser_name: String,
-}
+    use anyhow::{anyhow, Result};
 
-#[cfg(feature = "sandbox")]
-impl ScopedBrowserAccess {
-    /// Request access to browser directory and create a security bookmark if access is approved
-    pub fn request_only(browser_name: &str) -> Result<()> {
-        let c_name = CString::new(browser_name)?;
-
-        let bookmark_ptr = unsafe { requestBrowserAccess(c_name.as_ptr()) };
-        if bookmark_ptr.is_null() {
-            return Err(anyhow!(
-                "User denied access or selected an invalid browser directory"
-            ));
-        }
-        unsafe { libc::free(bookmark_ptr as *mut libc::c_void) };
-
-        Ok(())
+    extern "C" {
+        fn requestBrowserAccess(
+            browser_name: *const c_char,
+            relative_path: *const c_char,
+        ) -> *mut c_char;
+        fn hasStoredBrowserAccess(browser_name: *const c_char) -> bool;
+        fn startBrowserAccess(browser_name: *const c_char) -> *mut c_char;
+        fn stopBrowserAccess(browser_name: *const c_char);
     }
 
-    /// Resume browser directory access using previously created security bookmark
-    pub fn resume(browser_name: &str) -> Result<Self> {
-        let c_name = CString::new(browser_name)?;
-
-        if !unsafe { hasStoredBrowserAccess(c_name.as_ptr()) } {
-            return Err(anyhow!("Access has not been granted for this browser"));
-        }
-
-        let path_ptr = unsafe { startBrowserAccess(c_name.as_ptr()) };
-        if path_ptr.is_null() {
-            return Err(anyhow!(
-                "Failed to use browser existing security access, it may be stale"
-            ));
-        }
-        unsafe { libc::free(path_ptr as *mut libc::c_void) };
-
-        Ok(Self {
-            browser_name: browser_name.to_string(),
-        })
+    pub struct ScopedBrowserAccess {
+        browser_name: String,
     }
-}
 
-#[cfg(feature = "sandbox")]
-impl Drop for ScopedBrowserAccess {
-    fn drop(&mut self) {
-        let Ok(c_name) = CString::new(self.browser_name.as_str()) else {
-            return;
-        };
-        unsafe { stopBrowserAccess(c_name.as_ptr()) };
+    impl ScopedBrowserAccess {
+        /// Request access to browser directory and create a security bookmark if access is approved
+        pub fn request_only(browser_name: &str) -> Result<()> {
+            let config = crate::chromium::platform::SUPPORTED_BROWSERS
+                .iter()
+                .find(|b| b.name == browser_name)
+                .ok_or_else(|| anyhow!("Unsupported browser: {}", browser_name))?;
+
+            let c_name = CString::new(browser_name)?;
+            let c_path = CString::new(config.data_dir)?;
+
+            let bookmark_ptr = unsafe { requestBrowserAccess(c_name.as_ptr(), c_path.as_ptr()) };
+            if bookmark_ptr.is_null() {
+                return Err(anyhow!(
+                    "User denied access or selected an invalid browser directory"
+                ));
+            }
+            unsafe { libc::free(bookmark_ptr as *mut libc::c_void) };
+
+            Ok(())
+        }
+
+        /// Resume browser directory access using previously created security bookmark
+        pub fn resume(browser_name: &str) -> Result<Self> {
+            let c_name = CString::new(browser_name)?;
+
+            if !unsafe { hasStoredBrowserAccess(c_name.as_ptr()) } {
+                return Err(anyhow!("Access has not been granted for this browser"));
+            }
+
+            let path_ptr = unsafe { startBrowserAccess(c_name.as_ptr()) };
+            if path_ptr.is_null() {
+                return Err(anyhow!(
+                    "Failed to use browser existing security access, it may be stale"
+                ));
+            }
+            unsafe { libc::free(path_ptr as *mut libc::c_void) };
+
+            Ok(Self {
+                browser_name: browser_name.to_string(),
+            })
+        }
+    }
+
+    impl Drop for ScopedBrowserAccess {
+        fn drop(&mut self) {
+            let Ok(c_name) = CString::new(self.browser_name.as_str()) else {
+                return;
+            };
+            unsafe { stopBrowserAccess(c_name.as_ptr()) };
+        }
     }
 }
 

apps/desktop/desktop_native/chromium_importer/src/metadata.rs

@@ -17,11 +17,12 @@ pub struct NativeImporterMetadata {
 /// Only browsers listed in PLATFORM_SUPPORTED_BROWSERS will have the "chromium" loader.
 /// All importers will have the "file" loader.
 pub fn get_supported_importers<T: InstalledBrowserRetriever>(
+    mas_build: bool,
 ) -> HashMap<String, NativeImporterMetadata> {
     let mut map = HashMap::new();
 
     // Check for installed browsers
-    let installed_browsers = T::get_installed_browsers().unwrap_or_default();
+    let installed_browsers = T::get_installed_browsers(mas_build).unwrap_or_default();
 
     const IMPORTERS: &[(&str, &str)] = &[
         ("chromecsv", "Chrome"),
@@ -67,7 +68,7 @@ mod tests {
     pub struct MockInstalledBrowserRetriever {}
 
     impl InstalledBrowserRetriever for MockInstalledBrowserRetriever {
-        fn get_installed_browsers() -> Result<Vec<String>, anyhow::Error> {
+        fn get_installed_browsers(_mas_build: bool) -> Result<Vec<String>, anyhow::Error> {
             Ok(SUPPORTED_BROWSER_MAP
                 .keys()
                 .map(|browser| browser.to_string())
@@ -91,7 +92,7 @@ mod tests {
     #[cfg(target_os = "macos")]
     #[test]
     fn macos_returns_all_known_importers() {
-        let map = get_supported_importers::<MockInstalledBrowserRetriever>();
+        let map = get_supported_importers::<MockInstalledBrowserRetriever>(false);
 
         let expected: HashSet<String> = HashSet::from([
             "chromecsv".to_string(),
@@ -114,7 +115,7 @@ mod tests {
     #[cfg(target_os = "macos")]
     #[test]
     fn macos_specific_loaders_match_const_array() {
-        let map = get_supported_importers::<MockInstalledBrowserRetriever>();
+        let map = get_supported_importers::<MockInstalledBrowserRetriever>(false);
         let ids = [
             "chromecsv",
             "chromiumcsv",

apps/desktop/desktop_native/napi/index.d.ts

@@ -253,7 +253,7 @@ export declare namespace chromium_importer {
     instructions: string
   }
   /** Returns OS aware metadata describing supported Chromium based importers as a JSON string. */
-  export function getMetadata(): Record<string, NativeImporterMetadata>
+  export function getMetadata(masBuild: boolean): Record<string, NativeImporterMetadata>
   export function getAvailableProfiles(browser: string): Array<ProfileInfo>
   export function importLogins(browser: string, profileId: string): Promise<Array<LoginImportResult>>
   export function requestBrowserAccess(browser: string): void

apps/desktop/desktop_native/napi/src/lib.rs

@@ -1166,11 +1166,13 @@ pub mod chromium_importer {
 
     #[napi]
     /// Returns OS aware metadata describing supported Chromium based importers as a JSON string.
-    pub fn get_metadata() -> HashMap<String, NativeImporterMetadata> {
-        chromium_importer::metadata::get_supported_importers::<DefaultInstalledBrowserRetriever>()
-            .into_iter()
-            .map(|(browser, metadata)| (browser, NativeImporterMetadata::from(metadata)))
-            .collect()
+    pub fn get_metadata(mas_build: bool) -> HashMap<String, NativeImporterMetadata> {
+        chromium_importer::metadata::get_supported_importers::<DefaultInstalledBrowserRetriever>(
+            mas_build,
+        )
+        .into_iter()
+        .map(|(browser, metadata)| (browser, NativeImporterMetadata::from(metadata)))
+        .collect()
     }
 
     #[napi]

apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access.h

@@ -6,7 +6,7 @@
 // Request user permission to access browser directory
 // Returns base64-encoded bookmark data, or NULL if declined
 // Caller must free returned string
-char* requestBrowserAccess(const char* browserName);
+char* requestBrowserAccess(const char* browserName, const char* relativePath);
 
 // Check if we have stored bookmark (doesn't verify validity)
 bool hasStoredBrowserAccess(const char* browserName);

apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access.m

@@ -14,10 +14,11 @@ static BrowserAccessManager* getManager() {
     return sharedManager;
 }
 
-char* requestBrowserAccess(const char* browserName) {
+char* requestBrowserAccess(const char* browserName, const char* relativePath) {
     @autoreleasepool {
         NSString* name = [NSString stringWithUTF8String:browserName];
-        NSString* result = [getManager() requestAccessToBrowserDir:name];
+        NSString* path = [NSString stringWithUTF8String:relativePath];
+        NSString* result = [getManager() requestAccessToBrowserDir:name relativePath:path];
 
         if (result == nil) {
             return NULL;

apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.h

@@ -8,7 +8,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 /// Request access to a specific browser's directory
 /// Returns security bookmark data (used to persist permissions) as base64 string, or nil if user declined
-- (nullable NSString *)requestAccessToBrowserDir:(NSString *)browserName;
+- (nullable NSString *)requestAccessToBrowserDir:(NSString *)browserName relativePath:(NSString *)relativePath;
 
 /// Check if we have stored bookmark for browser (doesn't verify it's still valid)
 - (BOOL)hasStoredAccess:(NSString *)browserName;

apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m

@@ -3,31 +3,19 @@
 
 @implementation BrowserAccessManager {
     NSString *_bookmarkKey;
-    NSDictionary<NSString *, NSString *> *_browserPaths;
 }
 
 - (instancetype)init {
     self = [super init];
     if (self) {
         _bookmarkKey = @"com.bitwarden.chromiumImporter.bookmarks";
-
-        _browserPaths = @{
-            @"Chrome": @"Library/Application Support/Google/Chrome",
-            @"Chromium": @"Library/Application Support/Chromium",
-            @"Microsoft Edge": @"Library/Application Support/Microsoft Edge",
-            @"Brave": @"Library/Application Support/BraveSoftware/Brave-Browser",
-            @"Arc": @"Library/Application Support/Arc/User Data",
-            @"Opera": @"Library/Application Support/com.operasoftware.Opera",
-            @"Vivaldi": @"Library/Application Support/Vivaldi"
-        };
     }
     return self;
 }
 
-- (NSString *)requestAccessToBrowserDir:(NSString *)browserName {
+- (NSString *)requestAccessToBrowserDir:(NSString *)browserName relativePath:(NSString *)relativePath {
     // NSLog(@"[OBJC] requestAccessToBrowserDir called for: %@", browserName);
 
-    NSString *relativePath = _browserPaths[browserName];
     if (!relativePath) {
         // NSLog(@"[OBJC] Unknown browser: %@", browserName);
         return nil;

apps/desktop/src/app/tools/import/chromium-importer.service.ts

@@ -4,8 +4,8 @@ import { chromium_importer } from "@bitwarden/desktop-napi";
 
 export class ChromiumImporterService {
   constructor() {
-    ipcMain.handle("chromium_importer.getMetadata", async (event) => {
-      return await chromium_importer.getMetadata();
+    ipcMain.handle("chromium_importer.getMetadata", async (event, isMas: boolean) => {
+      return await chromium_importer.getMetadata(isMas);
     });
 
     // Used on Mac OS App Store builds to request permissions to browser entries outside the sandbox

apps/desktop/src/app/tools/import/desktop-import-metadata.service.ts

@@ -20,7 +20,8 @@ export class DesktopImportMetadataService
   }
 
   async init(): Promise<void> {
-    const metadata = await ipc.tools.chromiumImporter.getMetadata();
+    const isMas = ipc.platform.isMacAppStore;
+    const metadata = await ipc.tools.chromiumImporter.getMetadata(isMas);
     await this.parseNativeMetaData(metadata);
     await super.init();
   }

apps/desktop/src/app/tools/preload.ts

@@ -3,8 +3,10 @@ import { ipcRenderer } from "electron";
 import type { chromium_importer } from "@bitwarden/desktop-napi";
 
 const chromiumImporter = {
-  getMetadata: (): Promise<Record<string, chromium_importer.NativeImporterMetadata>> =>
-    ipcRenderer.invoke("chromium_importer.getMetadata"),
+  getMetadata: (
+    isMas: boolean,
+  ): Promise<Record<string, chromium_importer.NativeImporterMetadata>> =>
+    ipcRenderer.invoke("chromium_importer.getMetadata", isMas),
   // Request browser access for Mac OS App Store (sandboxed) builds (no-op in non-sandboxed builds)
   requestBrowserAccess: (browser: string): Promise<void> =>
     ipcRenderer.invoke("chromium_importer.requestBrowserAccess", browser),