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 |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: 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
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR adds a substantial macOS Safari cookie-import workflow that reads protected browser data, transfers authentication cookies, and introduces new parser, permission, UI, and IPC behavior. It also adds a static-analysis suppression directive, so the scope and review-sensitive changes require human review. Not approved because:
Review your spending limits in Billing settings, or comment |
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
|
Macroscope skipped reviewing this pull request. Per-PR cost limit exceeded (workspace setting). Reviews on this PR have cost $48.44 so far. This review would add an estimated $1.85, bringing the total to $50.29 — above your per-PR limit of $50.00. Tip To get this pull request reviewed, you can:
|
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>
… 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>
The step's "Open System Settings" button did nothing: it routed the `x-apple.systempreferences:` deep link through `openExternal`, whose allowlist only passes http(s) and remote-editor schemes, so the link was silently dropped. Adds a dedicated `openSystemSettings(pane)` path instead. The renderer sends a known pane identifier, not a URL, and the main process maps it to an app-fixed deep link and opens it directly — so the general link allowlist stays locked down while this one trusted destination gets through. Uses the post-Ventura Full Disk Access anchor. Also reworks the step's copy to say why the permission is needed and that it can be revoked once the import is done. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two review findings. Safari's parser interpolated a bare IPv6 host into the cookie URL (`http://::1/`), which Electron rejects; hosts with a colon are now bracketed, matching the Chromium reader's cookieScope. And the cookie-database candidate probes accepted any stat-able entry, so a directory squatting on `Network/Cookies` would be picked, fail the SQLite open, and shadow a valid legacy `Cookies` file behind it; every candidate probe now requires a regular file, in the listing, the counts, and the importer alike. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d route only TCC to Full Disk Access Two review findings on the Safari reader. The parser validated each declared page but never checked that the declared pages account for the file, so a jar with extra undeclared pages imported "successfully" with those cookies silently missing; it now requires the remainder to be exactly what Safari writes after the pages — nothing, the 8-byte checksum, or checksum plus a length-prefixed property list — and refuses anything else. And `isPermissionDenied` sent ordinary EACCES failures (mode bits, ACLs) to the Full Disk Access grant, which cannot fix them; only TCC's EPERM routes there now, and the TCC test injects a real EPERM instead of relying on chmod's EACCES. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ch other A record offset was only checked against the page's end, so one pointing into the page header/offset table, or back into an already-parsed record, would read those bytes as a fabricated cookie. Offsets must now land past the offset table and records may not overlap any accepted so far. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The listing found Safari's jar with `stat`, which TCC permits without Full Disk Access, so Safari always listed as ready and the wizard opened on the configure step — the permission-first flow, the "still required" recheck and its `resume: "configure"` path could never run, and a refresh could not tell granted from denied. `unavailableReason` now probes the jar by opening it for read; TCC's EPERM reports `needsFullDiskAccess`, while a readable jar or any other failure does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Safari reader set `domain` on every cookie, but `ImportedCookie.domain` is only for domain cookies (leading dot): Electron treats any supplied `domain` as one and re-adds the dot, widening host-only cookies to every subdomain and rejecting `__Host-` cookies outright. Route through the shared `cookieScope` like the Chromium and Firefox readers do, which also owns the IPv6 bracketing. Co-Authored-By: Claude Code <noreply@anthropic.com>
… denied macOS applies a Full Disk Access grant to the process identity at launch, so the running app can keep failing the probe after the toggle. Say so in the still-required panel. Fold the two recheck handlers into one, drop the unreachable no-localApi branch and its second toast, and document Safari import in the user guide. Co-Authored-By: Claude Code <noreply@anthropic.com>
macOS previewThe preview download was removed because this PR closed or the preview label was removed. |
- Add Safari profile discovery from SafariTabs metadata - Fall back to WebKit cookie stores when metadata is unavailable
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Reviewed by Cursor Bugbot for commit c6cb164. Configure here.
| } | ||
| } | ||
| return profiles; | ||
| }); |
There was a problem hiding this comment.
FDA check skips named Safari jars
Medium Severity
listSafariProfiles can mark Safari installed from a named-profile jar, but the listing-time Full Disk Access probe still only opens the default "." jar. If that default jar is missing, TCC denial is not reported up front, so Safari looks ready or notInstalled instead of opening the permission step.
Reviewed by Cursor Bugbot for commit c6cb164. Configure here.


Adds Safari as a browser import source on macOS.
Safari keeps its cookies unencrypted in
Cookies.binarycookies; the protection is Full Disk Access (TCC), not a key. The reader parses the binary format with every declared structure bounds-checked and refuses a jar the page table does not describe rather than importing part of it. Timestamps are rebased from the 2001 epoch, and host-only vs domain cookies go through the samecookieScoperule as the Chromium and Firefox readers.TCC denies the read with
EPERMwhile still allowingstat, so listing probes the jar by opening it and reportsneedsFullDiskAccessup front. The wizard gains a Full Disk Access step: Open System Settings deep-links to the Privacy & Security → Full Disk Access pane through a dedicatedopenSystemSettingsIPC method keyed by pane id (the genericopenExternalallowlist is unchanged), and after granting the wizard rechecks or retries the selected import. Only Safari's primary profile jar is read; additional Safari profiles are out of scope.Validation: binary parser fixtures and malformed-record boundaries, an
EPERM-vs-EACCESdistinction test, the fixed System Settings destination, and wizard state transitions. The parser was also run against referencebinarycookiesfixtures and structurally checked against a live Safari 27 jar.Original implementation: Claude Code. Review fixes: GPT-5.6 Sol agents via Codex, and Claude Fable 5 via Claude Code.
Note
Add Safari cookie import with Full Disk Access permission flow
cookcookie jar instead of SQLite databases used by Chromium and FirefoxparseBinaryCookiesin SafariCookies.ts to decode the big-endian page table, little-endian records, Safari epoch timestamps, URL/domain/path scope, and secure/HTTP-only flags; structurally invalid jars fail withSafariCookieReadErrorSafariTabsmetadata with fallback to scanning UUID-named WebKit data stores when metadata is unreadableneedsFullDiskAccessunavailability reason and a wizard step that lets users open the macOS Full Disk Access System Settings pane, recheck permission, and resume the importneedsFullDiskAccesswhen the cookie jar exists but opening it yieldsEPERM;EACCESand other errors remain generic read failuresMacroscope summarized c6cb164.
Note
Medium Risk
Touches macOS TCC-sensitive cookie reads and new IPC that opens fixed System Settings URLs; parser mistakes could drop or mis-import cookies, but URLs are allowlisted and malformed jars fail closed.
Overview
Adds Safari as a macOS browser-import source, reading
Cookies.binarycookieswith a bounds-checked parser that maps records into preview cookies (epoch rebase, host-only/cookieScope, secure/httpOnly flags) and surfaces TCC denials asneedsFullDiskAccess(EPERM) rather than generic read failures.Availability and import probe Safari by opening the jar (since
statcan succeed without Full Disk Access), wire the Safari engine throughBrowserImport, and extend source discovery to list the default jar plus named profiles fromSafariTabs.db/ WebKit data stores, resolvingCookies.binarycookiesper selected profile. Safari is treated as safe to import while running (no lock probe).Desktop shell / UX: new
openSystemSettings("full-disk-access")IPC (pane id, not renderer URLs) deep-links to Privacy & Security → Full Disk Access; the import wizard gains a Full Disk Access step with recheck/retry, and Integrations calls the local API to open settings. Contracts add thesafarisource id andneedsFullDiskAccesscopy; user docs note the FDA flow.Reviewed by Cursor Bugbot for commit c6cb164. Bugbot is set up for automated code reviews on this repo. Configure here.