feat(desktop): import cookies from Safari - #7262
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: unavailable · PR result: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
88cf9f7 to
31db6ab
Compare
github-actions
Bot
added
the
📱 Native Change
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. New feature adding Safari cookie import with ~460 lines of new parsing and UI code. Unresolved High severity finding about cookie domain handling that could widen cookies to subdomains, plus the "Open System Settings" button appears non-functional due to URL scheme not being allowlisted. You can customize Macroscope's approvability policy. Learn more. |
31db6ab to
3df1bbe
Compare
3df1bbe to
79df949
Compare
79df949 to
d66d4fb
Compare
There was a problem hiding this comment.
Effect service conventions review: one finding on the new Safari cookie error. Everything else (subpath namespace imports, Effect.fn boundaries, tagged-error definition, pass-through of the already structured error in Effect.try, no hidden runtimes) matches the conventions.
Posted via Macroscope — Effect Service Conventions
d66d4fb to
06d5c22
Compare
There was a problem hiding this comment.
Effect service conventions review: one finding on the new Safari cookie error. Everything else (subpath namespace imports, Effect.fn boundaries, tagged-error definition, pass-through of the already structured error in Effect.try, no hidden runtimes) matches the conventions.
Posted via Macroscope — Effect Service Conventions
06d5c22 to
06722a1
Compare
06722a1 to
a8129c1
Compare
a8129c1 to
be13d9f
Compare
6b977b6 to
7181e16
Compare
7181e16 to
8cdfcef
Compare
8cdfcef to
2fffb54
Compare
| url: `${secure ? "https" : "http"}://${host}${path || "/"}`, | ||
| name, | ||
| value, | ||
| domain, |
There was a problem hiding this comment.
🟠 High BrowserImport/SafariCookies.ts:146
Host-only Safari cookies are imported with ImportedCookie.domain set to the bare host, so Electron treats them as domain cookies and widens them to subdomains (and rejects __Host- cookies). Set domain to undefined unless the source domain starts with ..
| domain, | |
| domain: domain.startsWith(".") ? domain : undefined, |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/desktop/src/preview/BrowserImport/SafariCookies.ts around line 146:
Host-only Safari cookies are imported with `ImportedCookie.domain` set to the bare host, so Electron treats them as domain cookies and widens them to subdomains (and rejects `__Host-` cookies). Set `domain` to `undefined` unless the source domain starts with `.`.
2fffb54 to
51247f4
Compare
51247f4 to
180da0b
Compare
There was a problem hiding this comment.
One finding: the Full Disk Access dialog's primary action is silently a no-op on desktop because the x-apple.systempreferences: scheme is blocked by the main-process external-URL allowlist.
Posted via Macroscope — UI Consistency
| <AlertDialogClose | ||
| render={<Button />} | ||
| onClick={() => { | ||
| void readLocalApi() | ||
| ?.shell.openExternal(FULL_DISK_ACCESS_SETTINGS_URL) | ||
| .catch(() => undefined); | ||
| }} |
There was a problem hiding this comment.
The dialog's primary action can't open anything today: ElectronShell.parseSafeExternalUrl (apps/desktop/src/electron/ElectronShell.ts) only allows http:, https: and the editor remote schemes, so x-apple.systempreferences: is rejected, desktopBridge.openExternal resolves false, localApi.shell.openExternal throws "Unable to open link.", and .catch(() => undefined) swallows it. AlertDialogClose still dismisses the dialog, so the user is left with a button that closes the sheet and does nothing — the one recovery path this dialog exists to offer.
Smallest fix: add "x-apple.systempreferences:" to SAFE_EXTERNAL_PROTOCOLS (macOS-only is fine) and, so a future block isn't invisible, replace the bare .catch(() => undefined) with the existing failure toast telling the user to open System Settings → Privacy & Security → Full Disk Access manually.
Posted via Macroscope — UI Consistency
675447a to
47e56cd
Compare
There was a problem hiding this comment.
UI consistency review: one finding on the new Full Disk Access dialog in apps/web/src/components/settings/IntegrationsSettings.tsx.
Posted via Macroscope — UI Consistency
47e56cd to
1a6149b
Compare
There was a problem hiding this comment.
One finding: the Full Disk Access step's "Open System Settings" button cannot open anything today, and the failure is swallowed. Details inline.
Posted via Macroscope — UI Consistency
| onOpenFullDiskAccessSettings={() => | ||
| void readLocalApi() | ||
| ?.shell.openExternal(FULL_DISK_ACCESS_SETTINGS_URL) | ||
| .catch(() => undefined) | ||
| } |
There was a problem hiding this comment.
This still can't open anything, and after the move into BrowserImportWizard the button is now a complete no-op (it no longer dismisses the dialog either). ElectronShell.parseSafeExternalUrl (apps/desktop/src/electron/ElectronShell.ts) allows only http:, https: and the remote editor schemes, so x-apple.systempreferences: is rejected, desktopBridge.openExternal resolves false, localApi.shell.openExternal throws "Unable to open link.", and .catch(() => undefined) discards it. The user clicks the step's action and nothing happens or is said.
Smallest fix: allow the x-apple.systempreferences: scheme in SAFE_EXTERNAL_PROTOCOLS (darwin only), and surface a failure here instead of swallowing it — e.g. a toast in the catch, as ThreadTerminalDrawer does — so the step degrades to readable instructions rather than a dead control.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Bugbot Autofix is ON, but a cloud agent failed to start.
Reviewed by Cursor Bugbot for commit 1a6149b. Configure here.
| * later. | ||
| */ | ||
| const FULL_DISK_ACCESS_SETTINGS_URL = | ||
| "x-apple.systempreferences:com.apple.preference.security?Privacy_AllFilesAccess"; |
There was a problem hiding this comment.
Wrong Full Disk Access URL
Medium Severity
FULL_DISK_ACCESS_SETTINGS_URL uses the query fragment Privacy_AllFilesAccess. Documented macOS deep links for Full Disk Access use Privacy_AllFiles. Even after the scheme is allowlisted, this URL may open the wrong pane or fail to land on Full Disk Access.
Reviewed by Cursor Bugbot for commit 1a6149b. Configure here.
Safari does not encrypt its cookies. It stores them in a proprietary `Cookies.binarycookies` file whose protection is TCC rather than cryptography: the file sits inside the app container, which only apps with Full Disk Access may read. So the gate is a permission the user grants in System Settings, and a denial is reported as exactly that rather than as a generic read failure. Two details the format forces: Timestamps count seconds from 2001-01-01, not the UNIX epoch, so every expiry needs rebasing or cookies import as long expired. The format predates SameSite and carries no equivalent field. Imported cookies are marked Lax, the modern browser default — claiming "none" would widen the scope of every cookie Safari ever set. Safari keeps one jar for the whole app rather than per-profile, so it exposes a single implicit profile, and it has no observable lock file since the jar is written atomically. The parser is covered by tests that build the binary format byte for byte, including a multi-page file — Safari pages its jar, and a single-page reader would silently return only the first slice. That coverage matters because the real file cannot be read on this machine without the very permission the feature asks for; the TCC path itself was verified against the live file, which denies with EPERM and reports `needsFullDiskAccess`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every `nodeBuiltinImport:off` in the import module now says which builtin it covers and why Effect has no equivalent, matching the neighbouring preload and Playwright modules. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… of it `Buffer.subarray` clamps rather than throwing, so every declared structure in the binary format was taken on trust. An overlong page swallowed the following page's bytes and pushed the cursor past the end, dropping every cookie after the boundary from an import that still reported success. A record whose declared size overran its page left its string offsets free to read the next record's bytes as this cookie's value. Pages, records, and string offsets are now bounds-checked against what the file actually contains, and a mismatch fails the read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches the Chromium and Firefox readers: the failure carries which jar it was for, so a Full Disk Access refusal is traceable rather than anonymous. Optional because the parser raises before a path is in hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A Safari import with no Full Disk Access failed with the generic "cookie database could not be read" instead of telling the user to grant access — and no prompt appears, because macOS never prompts for Full Disk Access; the app is added by hand. The denial arrives as EPERM, which Effect tags `Unknown`, not `PermissionDenied` (that is EACCES), so checking the tag alone never matched. The underlying errno is checked too. Verified against the real jar: the reason is now `needsFullDiskAccess`, which the renderer maps to the System Settings instruction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Safari's cookies sit behind Full Disk Access, which no one has granted before their first import — so it is a step in the flow, not a failure. When an import comes back needing it, the wizard shows a screen that says what it's for, links to the right System Settings pane, and — from an "I've turned it on" button — runs the import itself, so the user never restarts from the menu. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1a6149b to
dbaf712
Compare
There was a problem hiding this comment.
One follow-up on the Safari cookie error context; everything else in the Safari path (namespace imports, Schema.TaggedErrorClass with structural attributes and a message derived from them, Effect.catchTags in BrowserImport.ts, Effect.fn construction) matches the conventions used by the sibling Chromium/Firefox readers.
Posted via Macroscope — Effect Service Conventions
| return yield* Effect.try({ | ||
| try: () => parseBinaryCookies(Buffer.from(contents)), | ||
| catch: (cause) => | ||
| isSafariCookieReadError(cause) | ||
| ? cause | ||
| : new SafariCookieReadError({ | ||
| reason: "readFailed", | ||
| cookieDatabasePath: cookiePath, | ||
| cause, | ||
| }), |
There was a problem hiding this comment.
Parse failures still lose the path they occurred on: parseBinaryCookies throws without cookieDatabasePath, and the pass-through branch returns that error unchanged even though cookiePath is in hand here, so a corrupt jar logs Could not read Safari cookies: readFailed. with no file named. Consider threading the path into the parser (e.g. parseBinaryCookies(buffer, cookiePath) used at each throw new SafariCookieReadError({ reason: "readFailed", cookieDatabasePath })), which also lets cookieDatabasePath become required like ChromiumCookieReadError's.
Posted via Macroscope — Effect Service Conventions
1 participant
Adds Safari, completing the source list.
Safari does not encrypt its cookies. It stores them in a proprietary
Cookies.binarycookiesfile whose protection is TCC rather than cryptography: the file sits inside the app container, readable only by apps with Full Disk Access. So the gate is a permission you grant in System Settings, and a denial is reported as exactly that.Two details the format forces
Timestamps count from 2001-01-01, not the UNIX epoch. Without rebasing, every cookie imports as ~31 years expired.
The format predates SameSite and carries no equivalent field. Imported cookies are marked
Lax, the modern browser default — claimingnonewould widen the scope of every cookie Safari ever set.Safari also keeps one jar for the whole app rather than per-profile, so it exposes a single implicit profile, and it has no observable lock file since the jar is written atomically.
Testing
The parser is covered by tests that build the binary format byte for byte, including a multi-page file — Safari pages its jar, and a single-page reader would silently return only the first slice.
That coverage matters because the real file cannot be read on this machine without the very permission the feature asks for. The TCC path itself was verified against the live file: it denies with
EPERMand surfacesneedsFullDiskAccesswith an actionable remedy rather than a generic failure.Not verified against a real
Cookies.binarycookies. The layout follows the documented format and round-trips through a byte-accurate fixture, but no genuine Safari jar has been parsed — worth one read with Full Disk Access granted before relying on it.Note
Add Safari cookie import support to the browser import wizard
Cookies.binarycookiesformat inSafariCookies.ts, including bounds checking, Apple-to-Unix epoch conversion, and structuredSafariCookieReadErrorwithneedsFullDiskAccessorreadFailedreasons.Sources.ts, pointing to~/Library/Containers/com.apple.Safari/Data/Library/Cookies/Cookies.binarycookies, with a single pseudo-profile and no running-browser lock check.FullDiskAccessStepto the import wizard that guides users to grant Full Disk Access in macOS System Settings and retry the import whenneedsFullDiskAccessis returned.browserImport.tsto accept"safari"as a source ID and"needsFullDiskAccess"as an unavailable reason with user-facing copy.EPERM/EACCESerror codes andPlatformErrortagging; other OS-level read failures fall back to the genericreadFailedreason.Macroscope summarized dbaf712.
Note
Medium Risk
Imports real session cookies from a protected system path and relies on a reverse-engineered binary format not yet validated against a live Safari jar; mis-parsing or wrong metadata could silently skip or mis-set cookies.
Overview
Safari cookie import is wired end-to-end on macOS: a new
safariengine readsCookies.binarycookiesfrom Safari’s container, maps Apple-epoch timestamps and flags into the sharedImportedCookieshape (defaulting SameSite tolax), and plugs intoBrowserImportalongside Chromium and Firefox.Desktop plumbing extends
Sources(single implicit profile, no lock-file “running” check,Cookies.binarycookiespath) and treats TCCEPERMasneedsFullDiskAccessinstead of a generic read failure. The parser rejects malformed or truncated jars so partial imports cannot look successful.UI and contracts add
safariandneedsFullDiskAccess, route that reason to afullDiskAccesswizard step with Open System Settings, and exportimportFailureReasonfor tests. Coverage includes byte-built fixtures (multi-page jars) and permission-denial behavior.Reviewed by Cursor Bugbot for commit dbaf712. Bugbot is set up for automated code reviews on this repo. Configure here.