From d38aa1ae9004e821afac5ef9c42faf1135a99c61 Mon Sep 17 00:00:00 2001 From: John Harrington <84741727+harr1424@users.noreply.github.com> Date: Tue, 9 Dec 2025 09:07:28 -0700 Subject: [PATCH] address review comments --- .github/CODEOWNERS | 1 + apps/desktop/desktop_native/build.js | 5 +- .../chromium_importer/src/chromium/mod.rs | 53 +++-- .../src/chromium/platform/macos.rs | 214 ------------------ .../src/chromium/platform/mod.rs | 4 + .../src/chromium/platform/sandbox.rs | 206 +++++++++++++++++ .../chromium_importer/src/metadata.rs | 8 +- .../objc/src/native/bridging-header.h | 3 - 8 files changed, 242 insertions(+), 252 deletions(-) create mode 100644 apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs delete mode 100644 apps/desktop/desktop_native/objc/src/native/bridging-header.h diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 89fff27b217..fae6a19e1ee 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -33,6 +33,7 @@ apps/browser/src/tools @bitwarden/team-tools-dev apps/cli/src/tools @bitwarden/team-tools-dev apps/desktop/desktop_native/bitwarden_chromium_import_helper @bitwarden/team-tools-dev apps/desktop/desktop_native/chromium_importer @bitwarden/team-tools-dev +apps/desktop/desktop_native/objc/src/native/chromium_importer @bitwarden/team-tools-dev apps/desktop/src/app/tools @bitwarden/team-tools-dev apps/web/src/app/tools @bitwarden/team-tools-dev libs/angular/src/tools @bitwarden/team-tools-dev diff --git a/apps/desktop/desktop_native/build.js b/apps/desktop/desktop_native/build.js index dbbdde307d7..a7ed89a9c17 100644 --- a/apps/desktop/desktop_native/build.js +++ b/apps/desktop/desktop_native/build.js @@ -28,10 +28,7 @@ let crossPlatform = process.argv.length > 2 && process.argv[2] === "cross-platfo function buildNapiModule(target, release = true) { const targetArg = target ? `--target ${target}` : ""; const releaseArg = release ? "--release" : ""; - child_process.execSync(`npm run build -- ${releaseArg} ${targetArg}`, { - stdio: 'inherit', - cwd: path.join(__dirname, "napi") - }); + child_process.execSync(`npm run build -- ${releaseArg} ${targetArg}`, { stdio: 'inherit', cwd: path.join(__dirname, "napi") }); } function buildProxyBin(target, release = true) { diff --git a/apps/desktop/desktop_native/chromium_importer/src/chromium/mod.rs b/apps/desktop/desktop_native/chromium_importer/src/chromium/mod.rs index f98b67a5171..52288f6de80 100644 --- a/apps/desktop/desktop_native/chromium_importer/src/chromium/mod.rs +++ b/apps/desktop/desktop_native/chromium_importer/src/chromium/mod.rs @@ -51,28 +51,30 @@ pub enum LoginImportResult { } pub trait InstalledBrowserRetriever { - fn get_installed_browsers(mas_build: bool) -> Result>; + fn get_installed_browsers(mas_build: bool) -> Vec; } pub struct DefaultInstalledBrowserRetriever {} impl InstalledBrowserRetriever for DefaultInstalledBrowserRetriever { - fn get_installed_browsers(mas_build: bool) -> Result> { - let mut browsers = Vec::with_capacity(SUPPORTED_BROWSER_MAP.len()); - for (browser, config) in SUPPORTED_BROWSER_MAP.iter() { - if mas_build { - // show all browsers for MAS builds, user will grant access when selected - browsers.push((*browser).to_string()); - } else { - // When not in sandbox check file system directly - let data_dir = get_and_validate_data_dir(config)?; - if data_dir.exists() { - browsers.push((*browser).to_string()); - } - } + fn get_installed_browsers(mas_build: bool) -> Vec { + // Show all browsers for MAS builds, user will grant access when selected + if mas_build { + return SUPPORTED_BROWSER_MAP + .keys() + .map(|browser| (*browser).to_string()) + .collect(); } - - Ok(browsers) + // When not in sandbox, check file system directly + SUPPORTED_BROWSER_MAP + .iter() + .filter_map(|(browser, config)| { + get_and_validate_data_dir(config) + .ok() + .filter(|data_dir| data_dir.exists()) + .map(|_| (*browser).to_string()) + }) + .collect() } } @@ -83,11 +85,9 @@ pub async fn get_available_profiles( ) -> Result> { // MAS builds need to resume security-scoped access before reading browser files #[cfg(target_os = "macos")] - let _access = if mas_build { - Some(platform::sandbox::ScopedBrowserAccess::resume(browser_name).await?) - } else { - None - }; + if mas_build { + platform::sandbox::ScopedBrowserAccess::resume(browser_name).await?; + } let (_, local_state) = load_local_state_for_browser(browser_name)?; Ok(get_profile_info(&local_state)) @@ -103,18 +103,17 @@ pub async fn request_browser_access(browser_name: &str, mas_build: bool) -> Resu Ok(()) } +#[allow(unused_variables)] pub async fn import_logins( browser_name: &str, profile_id: &str, - _mas_build: bool, + mas_build: bool, ) -> Result> { // MAS builds will use the formerly created security bookmark #[cfg(target_os = "macos")] - let _access = if _mas_build { - Some(platform::sandbox::ScopedBrowserAccess::resume(browser_name).await?) - } else { - None - }; + if mas_build { + platform::sandbox::ScopedBrowserAccess::resume(browser_name).await?; + } let (data_dir, local_state) = load_local_state_for_browser(browser_name)?; diff --git a/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/macos.rs b/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/macos.rs index 05d37332cf9..ba940d67997 100644 --- a/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/macos.rs +++ b/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/macos.rs @@ -7,220 +7,6 @@ use crate::{ util, }; -// -// Sandbox specific (for Mac App Store Builds) -// - -pub mod sandbox { - use anyhow::{anyhow, Result}; - use serde::{Deserialize, Serialize}; - - // Before requesting access outside of sandbox, determine if browser is actually installed - const BROWSER_BUNDLE_IDS: &[(&str, &str)] = &[ - ("Chrome", "com.google.Chrome"), - ("Chromium", "org.chromium.Chromium"), - ("Microsoft Edge", "com.microsoft.edgemac"), - ("Brave", "com.brave.Browser"), - ("Arc", "company.thebrowser.Browser"), - ("Opera", "com.operasoftware.Opera"), - ("Vivaldi", "com.vivaldi.Vivaldi"), - ]; - - #[derive(Debug, Deserialize)] - #[serde(tag = "type")] - enum CommandResult { - #[serde(rename = "success")] - Success { value: T }, - #[serde(rename = "error")] - Error { error: String }, - } - - #[derive(Debug, Deserialize)] - struct RequestAccessResponse { - #[allow(dead_code)] - bookmark: String, - } - - #[derive(Debug, Deserialize)] - struct HasStoredAccessResponse { - #[serde(rename = "hasAccess")] - has_access: bool, - } - - #[derive(Debug, Deserialize)] - struct StartAccessResponse { - #[allow(dead_code)] - path: String, - } - - #[derive(Debug, Serialize)] - struct CommandInput { - namespace: String, - command: String, - params: serde_json::Value, - } - - pub struct ScopedBrowserAccess { - browser_name: String, - } - - impl ScopedBrowserAccess { - /// Request access to browser directory and create a security bookmark if access is approved - pub async 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))?; - - if !is_browser_installed(browser_name).await? { - return Err(anyhow!( - "chromiumImporterBrowserNotInstalled:{}", - browser_name - )); - } - - // For macOS, data_dir is always a single-element array - let relative_path = config - .data_dir - .first() - .ok_or_else(|| anyhow!("No data directory configured for browser"))?; - - let input = CommandInput { - namespace: "chromium_importer".to_string(), - command: "request_access".to_string(), - params: serde_json::json!({ - "browserName": browser_name, - "relativePath": relative_path, - }), - }; - - let output = desktop_objc::run_command(serde_json::to_string(&input)?) - .await - .map_err(|e| anyhow!("Failed to call ObjC command: {}", e))?; - - let result: CommandResult = serde_json::from_str(&output) - .map_err(|e| anyhow!("Failed to parse ObjC response: {}", e))?; - - match result { - CommandResult::Success { .. } => Ok(()), - CommandResult::Error { error } => Err(anyhow!("{}", error)), - } - } - - /// Resume browser directory access using previously created security bookmark - pub async fn resume(browser_name: &str) -> Result { - // First check if we have stored access - let has_access_input = CommandInput { - namespace: "chromium_importer".to_string(), - command: "has_stored_access".to_string(), - params: serde_json::json!({ - "browserName": browser_name, - }), - }; - - let has_access_output = - desktop_objc::run_command(serde_json::to_string(&has_access_input)?) - .await - .map_err(|e| anyhow!("Failed to call ObjC command: {}", e))?; - - let has_access_result: CommandResult = - serde_json::from_str(&has_access_output) - .map_err(|e| anyhow!("Failed to parse ObjC response: {}", e))?; - - let has_access = match has_access_result { - CommandResult::Success { value } => value.has_access, - CommandResult::Error { error } => return Err(anyhow!("{}", error)), - }; - - if !has_access { - return Err(anyhow!("Access has not been granted for this browser")); - } - - // Start accessing the browser - let start_input = CommandInput { - namespace: "chromium_importer".to_string(), - command: "start_access".to_string(), - params: serde_json::json!({ - "browserName": browser_name, - }), - }; - - let start_output = desktop_objc::run_command(serde_json::to_string(&start_input)?) - .await - .map_err(|e| anyhow!("Failed to call ObjC command: {}", e))?; - - let start_result: CommandResult = - serde_json::from_str(&start_output) - .map_err(|e| anyhow!("Failed to parse ObjC response: {}", e))?; - - match start_result { - CommandResult::Success { .. } => Ok(Self { - browser_name: browser_name.to_string(), - }), - CommandResult::Error { error } => Err(anyhow!("{}", error)), - } - } - } - - impl Drop for ScopedBrowserAccess { - fn drop(&mut self) { - let browser_name = self.browser_name.clone(); - - tokio::task::spawn(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; - } - }); - } - } - - async fn is_browser_installed(browser_name: &str) -> Result { - let bundle_id = BROWSER_BUNDLE_IDS - .iter() - .find(|(name, _)| *name == browser_name) - .map(|(_, id)| *id); - - let bundle_id = match bundle_id { - Some(id) => id, - None => return Ok(true), - }; - - let input = CommandInput { - namespace: "chromium_importer".to_string(), - command: "check_browser_installed".to_string(), - params: serde_json::json!({ - "bundleId": bundle_id, - }), - }; - - let output = desktop_objc::run_command(serde_json::to_string(&input)?) - .await - .map_err(|e| anyhow!("Failed to call ObjC command: {}", e))?; - - let result: CommandResult = serde_json::from_str(&output) - .map_err(|e| anyhow!("Failed to parse ObjC response: {}", e))?; - - match result { - CommandResult::Success { value } => Ok(value.is_installed), - CommandResult::Error { error } => Err(anyhow!("{}", error)), - } - } - - #[derive(Debug, Deserialize)] - struct CheckBrowserInstalledResponse { - #[serde(rename = "isInstalled")] - is_installed: bool, - } -} - // // Public API // diff --git a/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/mod.rs b/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/mod.rs index fe497de0773..0567e63a362 100644 --- a/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/mod.rs +++ b/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/mod.rs @@ -4,6 +4,10 @@ #[cfg_attr(target_os = "macos", path = "macos.rs")] mod native; +// Sandbox support (macOS only for MAS builds) +#[cfg(target_os = "macos")] +pub mod sandbox; + // Windows exposes public const #[allow(unused_imports)] pub use native::*; diff --git a/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs b/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs new file mode 100644 index 00000000000..d28989c85a3 --- /dev/null +++ b/apps/desktop/desktop_native/chromium_importer/src/chromium/platform/sandbox.rs @@ -0,0 +1,206 @@ +// +// Sandbox specific (for Mac App Store Builds) +// + +use anyhow::{anyhow, Result}; +use serde::{Deserialize, Serialize}; + +// Before requesting access outside of sandbox, determine if browser is actually installed +const BROWSER_BUNDLE_IDS: &[(&str, &str)] = &[ + ("Chrome", "com.google.Chrome"), + ("Chromium", "org.chromium.Chromium"), + ("Microsoft Edge", "com.microsoft.edgemac"), + ("Brave", "com.brave.Browser"), + ("Arc", "company.thebrowser.Browser"), + ("Opera", "com.operasoftware.Opera"), + ("Vivaldi", "com.vivaldi.Vivaldi"), +]; + +#[derive(Debug, Deserialize)] +#[serde(tag = "type")] +enum CommandResult { + #[serde(rename_all = "camelCase")] + Success { value: T }, + #[serde(rename_all = "camelCase")] + Error { error: String }, +} + +#[derive(Debug, Deserialize)] +struct RequestAccessResponse { + #[allow(dead_code)] + bookmark: String, +} + +#[derive(Debug, Deserialize)] +struct HasStoredAccessResponse { + #[serde(rename = "hasAccess")] + has_access: bool, +} + +#[derive(Debug, Deserialize)] +struct StartAccessResponse { + #[allow(dead_code)] + path: String, +} + +#[derive(Debug, Serialize)] +struct CommandInput { + namespace: String, + command: String, + params: serde_json::Value, +} + +pub struct ScopedBrowserAccess { + browser_name: String, +} + +impl ScopedBrowserAccess { + /// Request access to browser directory and create a security bookmark if access is approved + pub async 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))?; + + if !is_browser_installed(browser_name).await? { + return Err(anyhow!( + "chromiumImporterBrowserNotInstalled:{}", + browser_name + )); + } + + // For macOS, data_dir is always a single-element array + let relative_path = config + .data_dir + .first() + .ok_or_else(|| anyhow!("No data directory configured for browser"))?; + + let input = CommandInput { + namespace: "chromium_importer".to_string(), + command: "request_access".to_string(), + params: serde_json::json!({ + "browserName": browser_name, + "relativePath": relative_path, + }), + }; + + let output = desktop_objc::run_command(serde_json::to_string(&input)?) + .await + .map_err(|e| anyhow!("Failed to call ObjC command: {}", e))?; + + let result: CommandResult = serde_json::from_str(&output) + .map_err(|e| anyhow!("Failed to parse ObjC response: {}", e))?; + + match result { + CommandResult::Success { .. } => Ok(()), + CommandResult::Error { error } => Err(anyhow!("{}", error)), + } + } + + /// Resume browser directory access using previously created security bookmark + pub async fn resume(browser_name: &str) -> Result { + // First check if we have stored access + let has_access_input = CommandInput { + namespace: "chromium_importer".to_string(), + command: "has_stored_access".to_string(), + params: serde_json::json!({ + "browserName": browser_name, + }), + }; + + let has_access_output = + desktop_objc::run_command(serde_json::to_string(&has_access_input)?) + .await + .map_err(|e| anyhow!("Failed to call ObjC command: {}", e))?; + + let has_access_result: CommandResult = + serde_json::from_str(&has_access_output) + .map_err(|e| anyhow!("Failed to parse ObjC response: {}", e))?; + + let has_access = match has_access_result { + CommandResult::Success { value } => value.has_access, + CommandResult::Error { error } => return Err(anyhow!("{}", error)), + }; + + if !has_access { + return Err(anyhow!("Access has not been granted for this browser")); + } + + // Start accessing the browser + let start_input = CommandInput { + namespace: "chromium_importer".to_string(), + command: "start_access".to_string(), + params: serde_json::json!({ + "browserName": browser_name, + }), + }; + + let start_output = desktop_objc::run_command(serde_json::to_string(&start_input)?) + .await + .map_err(|e| anyhow!("Failed to call ObjC command: {}", e))?; + + let start_result: CommandResult = serde_json::from_str(&start_output) + .map_err(|e| anyhow!("Failed to parse ObjC response: {}", e))?; + + match start_result { + CommandResult::Success { .. } => Ok(Self { + browser_name: browser_name.to_string(), + }), + CommandResult::Error { error } => Err(anyhow!("{}", error)), + } + } +} + +impl Drop for ScopedBrowserAccess { + fn drop(&mut self) { + let browser_name = self.browser_name.clone(); + + tokio::task::spawn(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; + } + }); + } +} + +async fn is_browser_installed(browser_name: &str) -> Result { + let bundle_id = BROWSER_BUNDLE_IDS + .iter() + .find(|(name, _)| *name == browser_name) + .map(|(_, id)| *id) + .ok_or(true); + + let input = CommandInput { + namespace: "chromium_importer".to_string(), + command: "check_browser_installed".to_string(), + params: serde_json::json!({ + "bundleId": bundle_id, + }), + }; + + let output = desktop_objc::run_command(serde_json::to_string(&input)?) + .await + .map_err(|e| anyhow!("Failed to call ObjC command: {}", e))?; + + let result: CommandResult = serde_json::from_str(&output) + .map_err(|e| anyhow!("Failed to parse ObjC response: {}", e))?; + + match result { + CommandResult::Success { value } => Ok(value.is_installed), + CommandResult::Error { error } => Err(anyhow!("{}", error)), + } +} + +#[derive(Debug, Deserialize)] +struct CheckBrowserInstalledResponse { + #[serde(rename = "isInstalled")] + is_installed: bool, +} diff --git a/apps/desktop/desktop_native/chromium_importer/src/metadata.rs b/apps/desktop/desktop_native/chromium_importer/src/metadata.rs index b71d53a3a76..1beb3b61086 100644 --- a/apps/desktop/desktop_native/chromium_importer/src/metadata.rs +++ b/apps/desktop/desktop_native/chromium_importer/src/metadata.rs @@ -22,7 +22,7 @@ pub fn get_supported_importers( let mut map = HashMap::new(); // Check for installed browsers - let installed_browsers = T::get_installed_browsers(mas_build).unwrap_or_default(); + let installed_browsers = T::get_installed_browsers(mas_build); const IMPORTERS: &[(&str, &str)] = &[ ("chromecsv", "Chrome"), @@ -68,11 +68,11 @@ mod tests { pub struct MockInstalledBrowserRetriever {} impl InstalledBrowserRetriever for MockInstalledBrowserRetriever { - fn get_installed_browsers(_mas_build: bool) -> Result, anyhow::Error> { - Ok(SUPPORTED_BROWSER_MAP + fn get_installed_browsers(_mas_build: bool) -> Vec { + SUPPORTED_BROWSER_MAP .keys() .map(|browser| browser.to_string()) - .collect()) + .collect() } } diff --git a/apps/desktop/desktop_native/objc/src/native/bridging-header.h b/apps/desktop/desktop_native/objc/src/native/bridging-header.h deleted file mode 100644 index 4f6fb13bfc6..00000000000 --- a/apps/desktop/desktop_native/objc/src/native/bridging-header.h +++ /dev/null @@ -1,3 +0,0 @@ -#import -#import "utils.h" -#import "interop.h"