From dc9b05b6204761817d7ac3a29e860f5a23d79621 Mon Sep 17 00:00:00 2001 From: John Harrington <84741727+harr1424@users.noreply.github.com> Date: Tue, 2 Dec 2025 15:19:22 -0700 Subject: [PATCH] eval Claude's comments and address legitimate findings --- .../src/chromium/platform/macos.rs | 2 +- apps/desktop/desktop_native/objc/README.md | 30 ------------------- .../browser_access_manager.m | 22 +------------- .../tools/import/import-desktop.component.ts | 14 +++++++-- apps/desktop/src/locales/en/messages.json | 3 ++ 5 files changed, 16 insertions(+), 55 deletions(-) delete mode 100644 apps/desktop/desktop_native/objc/README.md 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 3ddd64a71f..f2fc994f4c 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 @@ -63,7 +63,7 @@ pub mod sandbox { 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" + "Existing security scoped access has become stale" )); } unsafe { libc::free(path_ptr as *mut libc::c_void) }; diff --git a/apps/desktop/desktop_native/objc/README.md b/apps/desktop/desktop_native/objc/README.md deleted file mode 100644 index 6dc7ece8fc..0000000000 --- a/apps/desktop/desktop_native/objc/README.md +++ /dev/null @@ -1,30 +0,0 @@ -# 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 to support this in CI builds: -``` - - 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. - -- 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_manager.m b/apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m index ef96ab2997..24e938352f 100644 --- a/apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m +++ b/apps/desktop/desktop_native/objc/src/native/chromium_importer/browser_access_manager.m @@ -14,18 +14,14 @@ } - (NSString *)requestAccessToBrowserDir:(NSString *)browserName relativePath:(NSString *)relativePath { - // NSLog(@"[OBJC] requestAccessToBrowserDir called for: %@", browserName); - + if (!relativePath) { - // NSLog(@"[OBJC] Unknown browser: %@", browserName); return nil; } NSURL *homeDir = [[NSFileManager defaultManager] homeDirectoryForCurrentUser]; NSURL *browserPath = [homeDir URLByAppendingPathComponent:relativePath]; - // NSLog(@"[OBJC] Browser path: %@", browserPath.path); - // NSOpenPanel must be run on the main thread __block NSURL *selectedURL = nil; __block NSModalResponse panelResult = NSModalResponseCancel; @@ -41,31 +37,22 @@ openPanel.canChooseFiles = NO; openPanel.directoryURL = browserPath; - // NSLog(@"[OBJC] About to call runModal"); panelResult = [openPanel runModal]; selectedURL = openPanel.URL; - // NSLog(@"[OBJC] runModal returned: %ld", (long)panelResult); }; if ([NSThread isMainThread]) { - // NSLog(@"[OBJC] Already on main thread"); showPanel(); } else { - // NSLog(@"[OBJC] Dispatching to main queue..."); dispatch_sync(dispatch_get_main_queue(), showPanel); } if (panelResult != NSModalResponseOK || !selectedURL) { - // NSLog(@"[OBJC] User cancelled access request or panel failed"); return nil; } - // NSLog(@"[OBJC] User selected URL: %@", selectedURL.path); - NSURL *localStatePath = [selectedURL URLByAppendingPathComponent:@"Local State"]; if (![[NSFileManager defaultManager] fileExistsAtPath:localStatePath.path]) { - // NSLog(@"[OBJC] Selected folder doesn't appear to be a valid %@ directory", browserName); - NSAlert *alert = [[NSAlert alloc] init]; alert.messageText = @"Invalid Folder"; alert.informativeText = [NSString stringWithFormat: @@ -85,12 +72,10 @@ error:&error]; if (!bookmarkData) { - // NSLog(@"[OBJC] Failed to create bookmark: %@", error); return nil; } [self saveBookmark:bookmarkData forBrowser:browserName]; - // NSLog(@"[OBJC] Successfully created and saved bookmark"); return [bookmarkData base64EncodedStringWithOptions:0]; } @@ -113,19 +98,16 @@ error:&error]; if (!url) { - // NSLog(@"Failed to resolve bookmark: %@", error); return nil; } if (isStale) { - // NSLog(@"Security bookmark for %@ is stale, attempting to re-create it", browserName); NSData *newBookmarkData = [url bookmarkDataWithOptions:NSURLBookmarkCreationWithSecurityScope includingResourceValuesForKeys:nil relativeToURL:nil error:&error]; if (!newBookmarkData) { - // NSLog(@"Failed to create bookmark: %@", error); return nil; } @@ -133,7 +115,6 @@ } if (![url startAccessingSecurityScopedResource]) { - // NSLog(@"Failed to start accessing security-scoped resource"); return nil; } @@ -155,7 +136,6 @@ error:&error]; if (!url) { - // NSLog(@"Failed to resolve bookmark for stop: %@", error); return; } diff --git a/apps/desktop/src/app/tools/import/import-desktop.component.ts b/apps/desktop/src/app/tools/import/import-desktop.component.ts index 768746f94f..f46c3889cc 100644 --- a/apps/desktop/src/app/tools/import/import-desktop.component.ts +++ b/apps/desktop/src/app/tools/import/import-desktop.component.ts @@ -2,6 +2,7 @@ import { CommonModule } from "@angular/common"; import { Component } from "@angular/core"; import { JslibModule } from "@bitwarden/angular/jslib.module"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { DialogRef, AsyncActionsModule, ButtonModule, DialogModule } from "@bitwarden/components"; import type { chromium_importer } from "@bitwarden/desktop-napi"; import { ImportMetadataServiceAbstraction } from "@bitwarden/importer-core"; @@ -42,7 +43,10 @@ export class ImportDesktopComponent { protected readonly onLoadProfilesFromBrowser = this._onLoadProfilesFromBrowser.bind(this); protected readonly onImportFromBrowser = this._onImportFromBrowser.bind(this); - constructor(public dialogRef: DialogRef) {} + constructor( + public dialogRef: DialogRef, + private i18nService: I18nService, + ) {} /** * Callback that is called after a successful import. @@ -54,8 +58,12 @@ export class ImportDesktopComponent { private async _onLoadProfilesFromBrowser( browser: string, ): Promise { - // Request browser access (required for sandboxed builds, no-op otherwise) - await ipc.tools.chromiumImporter.requestBrowserAccess(browser); + try { + // Request browser access (required for sandboxed builds, no-op otherwise) + await ipc.tools.chromiumImporter.requestBrowserAccess(browser); + } catch { + throw new Error(this.i18nService.t("browserAccessDenied")); + } return ipc.tools.chromiumImporter.getAvailableProfiles(browser); } diff --git a/apps/desktop/src/locales/en/messages.json b/apps/desktop/src/locales/en/messages.json index 757059c4e4..2003fa075f 100644 --- a/apps/desktop/src/locales/en/messages.json +++ b/apps/desktop/src/locales/en/messages.json @@ -4237,5 +4237,8 @@ }, "sessionTimeoutHeader": { "message": "Session timeout" + }, + "browserAccessDenied": { + "message": "Either an invalid browser directory was selected, or access was denied" } }