diff --git a/.github/workflows/build-desktop.yml b/.github/workflows/build-desktop.yml index 27a7bfe4124..cd5a392bb6b 100644 --- a/.github/workflows/build-desktop.yml +++ b/.github/workflows/build-desktop.yml @@ -1681,6 +1681,9 @@ jobs: - name: Build Native Module if: steps.cache.outputs.cache-hit != 'true' working-directory: apps/desktop/desktop_native + env: + # required to bypass sandbox (MAS only) + SANDBOX_BUILD: 1 run: node build.js cross-platform - name: Build 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 44687a5abd3..952618cc853 100644 --- a/apps/desktop/desktop_native/chromium_importer/src/chromium/mod.rs +++ b/apps/desktop/desktop_native/chromium_importer/src/chromium/mod.rs @@ -59,7 +59,7 @@ impl InstalledBrowserRetriever for DefaultInstalledBrowserRetriever { fn get_installed_browsers() -> Result> { let mut browsers = Vec::with_capacity(SUPPORTED_BROWSER_MAP.len()); - #[allow(unused_variables)] // config only used outside of sandbox + #[allow(unused_variables)] // config only used outside of sandbox for (browser, config) in SUPPORTED_BROWSER_MAP.iter() { #[cfg(all(target_os = "macos", feature = "sandbox"))] { @@ -90,12 +90,8 @@ pub fn get_available_profiles(browser_name: &String) -> Result> /// 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: &String) -> Result<()> { - println!("request_browser_access() called for: {}", browser_name); - platform::ScopedBrowserAccess::request_only(browser_name)?; - println!("request_browser_access() completed successfully"); - Ok(()) } 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 f3077e42420..a89f0b5ee78 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 @@ -31,13 +31,13 @@ pub struct ScopedBrowserAccess { impl ScopedBrowserAccess { /// Request access to browser directory and create a security bookmark if access is approved pub fn request_only(browser_name: &str) -> Result<()> { - println!("request_only() called for {}", browser_name); - 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 declined access")); + return Err(anyhow!( + "User denied access or selected an invalid browser directory" + )); } unsafe { libc::free(bookmark_ptr as *mut libc::c_void) }; @@ -46,8 +46,6 @@ impl ScopedBrowserAccess { /// Resume browser directory access using previously created security bookmark pub fn resume(browser_name: &str) -> Result { - println!("resume() called for {}", browser_name); - let c_name = CString::new(browser_name)?; if !unsafe { hasStoredBrowserAccess(c_name.as_ptr()) } { @@ -56,7 +54,9 @@ impl ScopedBrowserAccess { 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")); + return Err(anyhow!( + "Failed to use browser existing security access, it may be stale" + )); } unsafe { libc::free(path_ptr as *mut libc::c_void) }; @@ -65,17 +65,13 @@ impl ScopedBrowserAccess { }) } - /// First requests access to browser directory and then ensures access is still usable + /// First requests access to browser directory and then ensures access is still usable pub fn request_and_start(browser_name: &str) -> Result { - println!("request_and_start() called for {}", browser_name); - Self::request_only(browser_name)?; Self::resume(browser_name) } pub fn has_stored_access(browser_name: &str) -> bool { - println!("has_stored_access() called for {}", browser_name); - let Ok(c_name) = CString::new(browser_name) else { return false; }; @@ -86,8 +82,6 @@ impl ScopedBrowserAccess { #[cfg(feature = "sandbox")] impl Drop for ScopedBrowserAccess { fn drop(&mut self) { - println!("drop ScopedBrowserAccess has been called"); - let Ok(c_name) = CString::new(self.browser_name.as_str()) else { return; }; 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 06f3da04fc9..fe497de0773 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 @@ -6,4 +6,4 @@ mod native; // Windows exposes public const #[allow(unused_imports)] -pub use native::*; \ No newline at end of file +pub use native::*; diff --git a/apps/desktop/desktop_native/napi/index.d.ts b/apps/desktop/desktop_native/napi/index.d.ts index 750d1cb3486..75cb012f1c4 100644 --- a/apps/desktop/desktop_native/napi/index.d.ts +++ b/apps/desktop/desktop_native/napi/index.d.ts @@ -249,7 +249,6 @@ export declare namespace chromium_importer { export function getMetadata(): Record export function getAvailableProfiles(browser: string): Array export function importLogins(browser: string, profileId: string): Promise> - // used only on Mac OS App Store builds, no-op on other platforms export function requestBrowserAccess(browser: string): void } export declare namespace autotype { diff --git a/apps/desktop/desktop_native/napi/src/lib.rs b/apps/desktop/desktop_native/napi/src/lib.rs index d60bae51378..f8ec4fbec25 100644 --- a/apps/desktop/desktop_native/napi/src/lib.rs +++ b/apps/desktop/desktop_native/napi/src/lib.rs @@ -1184,8 +1184,6 @@ pub mod chromium_importer { #[napi] pub fn request_browser_access(browser: String) -> napi::Result<()> { - println!("request_browser_access() was called from napi"); - #[cfg(all(target_os = "macos", feature = "sandbox"))] { chromium_importer::chromium::request_browser_access(&browser) diff --git a/apps/desktop/desktop_native/objc/README.md b/apps/desktop/desktop_native/objc/README.md new file mode 100644 index 00000000000..63da54110a9 --- /dev/null +++ b/apps/desktop/desktop_native/objc/README.md @@ -0,0 +1,34 @@ +# File Scoped Security Access on Mac App Store Builds +## Accessing filesystem entries outside of the app sandbox + +### TLDR +#### Build & run locally using `npm run electron:sandbox` +#### `build-desktop.yml` section `macos-package-mas` was modified: +``` + - name: Build Native Module + if: steps.cache.outputs.cache-hit != 'true' + working-directory: apps/desktop/desktop_native + env: + # required to bypass sandbox (MAS only) + SANDBOX_BUILD: 1 + run: node build.js cross-platform +``` +### The Rust code uses a feature named `sandbox` that is used to conditionally compile almost all of the logic introduced in this PR: `#[cfg(all(target_os = "macos", feature = "sandbox"))]` + +### How it works +- `apps/desktop/src/app/tools/import/chromium-importer.service.ts` serves as the entrypoint to this logic. __Regardless of the `SANDBOX_BUILD` env var__, this service will call `request_browser_access()` +in `apps/desktop/desktop_native/napi/src/lib.rs` which returns an `Ok` unit variant (no-op) when the `sandbox` feature is not enabled. Outside of sandbox builds, `ChromiumImporterService` will avoid any of the logic added in this PR and continue the chromium importer workflow exactly as before. + +- When the `sandbox` feature is enabled the call chain advances to `apps/desktop/desktop_native/chromium_importer/src/chromium/mod.rs` where another conditionally compiled (cfg gated) `request_browser_access()` function is defined. This function is used to advance the call chain into the platform specific (Mac OS) Rust code inside `apps/desktop/desktop_native/chromium_importer/src/chromium/platform/macos.rs`. + +- When the `sandbox` feature is enabled, a series of cfg gated functions are used in this file to interface with native Objective-C bridges and Swift code responsible for requesting access to browser specific directories. + +- `request_only()` is responsible for requesting access to a given browser's directory AND creating the security scoped bookmark, the mechanism Mac uses to persist these kinds of permissions. It corresponds to `requestAccessToBroswerDir()` in the file `apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access.swift`. This Swift code will determine the appropriate directory to request access to, based upon the browser name passed as argument, and will then display an `NSOpenPanel` with a message instructing the user to select the browser's directory where profile information is stored. + +- `resume()` is responsible for determining if a security scoped bookmark exists and is valid, if so, it will be used to access the browser directory. + +- `request_and_start()` encapsulates both of the functions described above into a single function call. + +- `has_stored_access()` and `drop()` are self-explanatory helper functions. + +- See directory `apps/desktop/desktop_native/objc/src/native/chromium_importer` for the native Objective-C and Swift code. Instead of removing debug output I've commented out `NSLog` statements since they may be useful in the future. \ No newline at end of file diff --git a/apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access.swift b/apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access.swift index 53bf14aa06b..5bcdd4835f6 100644 --- a/apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access.swift +++ b/apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access.swift @@ -17,24 +17,24 @@ import Foundation /// Request access to a specific browser's directory /// Returns security bookmark data (used to persist permissions) as base64 string, or nil if user declined @objc public func requestAccessToBroswerDir(_ browserName: String) -> String? { - NSLog("[SWIFT] requestAccessToBroswerDir called for: \(browserName)") + // NSLog("[SWIFT] requestAccessToBroswerDir called for: \(browserName)") guard let relativePath = browserPaths[browserName] else { - NSLog("[SWIFT] Unknown browser: \(browserName)") + // NSLog("[SWIFT] Unknown browser: \(browserName)") return nil } let homeDir = FileManager.default.homeDirectoryForCurrentUser let browserPath = homeDir.appendingPathComponent(relativePath) - NSLog("[SWIFT] Browser path: \(browserPath.path)") + // NSLog("[SWIFT] Browser path: \(browserPath.path)") // NSOpenPanel must be run on the main thread var selectedURL: URL? var panelResult: NSApplication.ModalResponse = .cancel if Thread.isMainThread { - NSLog("[SWIFT] Already on main thread") + // NSLog("[SWIFT] Already on main thread") let openPanel = NSOpenPanel() openPanel.message = "Please select your \(browserName) data folder\n\nExpected location:\n\(browserPath.path)" @@ -42,16 +42,16 @@ import Foundation openPanel.allowsMultipleSelection = false openPanel.canChooseDirectories = true openPanel.canChooseFiles = false - openPanel.directoryURL = browserPath.deletingLastPathComponent() + openPanel.directoryURL = browserPath - NSLog("[SWIFT] About to call openPanel.runModal()") + // NSLog("[SWIFT] About to call openPanel.runModal()") panelResult = openPanel.runModal() selectedURL = openPanel.url - NSLog("[SWIFT] runModal returned: \(panelResult.rawValue)") + // NSLog("[SWIFT] runModal returned: \(panelResult.rawValue)") } else { - NSLog("[SWIFT] Dispatching to main queue...") + // NSLog("[SWIFT] Dispatching to main queue...") DispatchQueue.main.sync { - NSLog("[SWIFT] Inside main queue dispatch block") + // NSLog("[SWIFT] Inside main queue dispatch block") let openPanel = NSOpenPanel() openPanel.message = "Please select your \(browserName) data folder\n\nExpected location:\n\(browserPath.path)" @@ -59,25 +59,25 @@ import Foundation openPanel.allowsMultipleSelection = false openPanel.canChooseDirectories = true openPanel.canChooseFiles = false - openPanel.directoryURL = browserPath.deletingLastPathComponent() + openPanel.directoryURL = browserPath - NSLog("[SWIFT] About to call openPanel.runModal()") + // NSLog("[SWIFT] About to call openPanel.runModal()") panelResult = openPanel.runModal() selectedURL = openPanel.url - NSLog("[SWIFT] runModal returned: \(panelResult.rawValue)") + // NSLog("[SWIFT] runModal returned: \(panelResult.rawValue)") } } guard panelResult == .OK, let url = selectedURL else { - NSLog("[SWIFT] User cancelled access request or panel failed") + // NSLog("[SWIFT] User cancelled access request or panel failed") return nil } - NSLog("[SWIFT] User selected URL: \(url.path)") + // NSLog("[SWIFT] User selected URL: \(url.path)") let localStatePath = url.appendingPathComponent("Local State") guard FileManager.default.fileExists(atPath: localStatePath.path) else { - NSLog("[SWIFT] Selected folder doesn't appear to be a valid \(browserName) directory") + // NSLog("[SWIFT] Selected folder doesn't appear to be a valid \(browserName) directory") let alert = NSAlert() alert.messageText = "Invalid Folder" @@ -98,10 +98,10 @@ import Foundation ) saveBookmark(bookmarkData, forBrowser: browserName) - NSLog("[SWIFT] Successfully created and saved bookmark") + // NSLog("[SWIFT] Successfully created and saved bookmark") return bookmarkData.base64EncodedString() } catch { - NSLog("[SWIFT] Failed to create bookmark: \(error)") + // NSLog("[SWIFT] Failed to create bookmark: \(error)") return nil } } @@ -128,7 +128,7 @@ import Foundation ) if isStale { - NSLog("Security bookmark for \(browserName) is stale, attempting to re-create it") + // NSLog("Security bookmark for \(browserName) is stale, attempting to re-create it") do { let newBookmarkData = try url.bookmarkData( options: .withSecurityScope, @@ -138,19 +138,19 @@ import Foundation saveBookmark(newBookmarkData, forBrowser: browserName) } catch { - NSLog("Failed to create bookmark: \(error)") + // NSLog("Failed to create bookmark: \(error)") return nil } } guard url.startAccessingSecurityScopedResource() else { - NSLog("Failed to start accessing security-scoped resource") + // NSLog("Failed to start accessing security-scoped resource") return nil } return url.path } catch { - NSLog("Failed to resolve bookmark: \(error)") + // NSLog("Failed to resolve bookmark: \(error)") return nil } } @@ -172,7 +172,7 @@ import Foundation url.stopAccessingSecurityScopedResource() } catch { - NSLog("Failed to resolve bookmark for stop: \(error)") + // NSLog("Failed to resolve bookmark for stop: \(error)") } } diff --git a/apps/desktop/src/app/tools/import/chromium-importer.service.ts b/apps/desktop/src/app/tools/import/chromium-importer.service.ts index 196333fe69b..dbfba95d695 100644 --- a/apps/desktop/src/app/tools/import/chromium-importer.service.ts +++ b/apps/desktop/src/app/tools/import/chromium-importer.service.ts @@ -1,4 +1,3 @@ -/* eslint-disable no-console */ import { ipcMain } from "electron"; import { chromium_importer } from "@bitwarden/desktop-napi"; @@ -11,18 +10,10 @@ export class ChromiumImporterService { // Used on Mac OS App Store builds to request permissions to browser entries outside the sandbox ipcMain.handle("chromium_importer.requestBrowserAccess", async (event, browser: string) => { - console.log("[IPC] requestBrowserAccess handler called for:", browser); - console.log("[IPC] chromium_importer keys:", Object.keys(chromium_importer)); - console.log( - "[IPC] requestBrowserAccess exists?", - typeof chromium_importer.requestBrowserAccess, - ); - if (chromium_importer.requestBrowserAccess) { - console.log("[IPC] Calling native requestBrowserAccess"); return await chromium_importer.requestBrowserAccess(browser); } - console.log("[IPC] requestBrowserAccess not found, returning no-op"); + // requestBrowserAccess not found, returning with no-op return; });