Fix persistent browser Keychain access in desktop onboarding#9427
Fix persistent browser Keychain access in desktop onboarding#9427skanderkaroui wants to merge 8 commits into
Conversation
…hem as garbage The Chromium cookie decrypt handled v10/v11 (macOS AES-128-CBC) and treated everything else as plaintext. A v20 (app-bound) or newer encrypted value is not plaintext, so decoding it as Latin-1 produced a garbage string that got sent as a bogus Google auth cookie. As browsers move past v10 this silently corrupts the import. Guard the plaintext branch: any 'v<dd>' tag we don't decrypt (v20, v24, ...) is skipped, not garbage-decoded. Genuinely unencrypted values still pass through. Deliberately minimal (ponytail): no v20/app-bound decoder and no per-version telemetry — on macOS today cookies are v10, and the durable path for newer schemes is Google OAuth, not a per-version scraper. Ceiling noted in-code. Test: xcrun swift build -c debug (Build complete, exit 0).
Scopes the migration away from the version-fragile browser cookie scraper to backend-mediated Google OAuth. Reuse-first: the backend already ships the OAuth flow, token refresh, connector pattern, and a live Google Calendar integration, so the work is mostly wiring the desktop fetch layer to it. Decisions captured: Calendar first (cheap/verified scope), Gmail via gmail.readonly (accepting restricted-scope/CASA cost), backend-mediated, standalone connector (not incremental-auth). Explicit ponytail cuts: no new token-encryption scheme, no MCP-style multi-client OAuth, keep the fixed cookie path as fallback.
…external blockers calendar_tools.py already fetches events from the Google Calendar API with the stored OAuth token, so Phase 0 is a thin read endpoint + desktop wiring, not new OAuth. Also record the external gates (Google client creds, connected test account, consent-screen scope config, Gmail CASA) that prevent live verification from the repo alone.
Git-on-my-level
added
security-review
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the careful write-up and tests here. I did a maintainer/security-focused review of the diff.
Positive signal on the code direction: moving the Keychain read in-process, matching the full Safe Storage service/account identity, avoiding a /usr/bin/security fallback, and skipping unknown versioned cookie blobs all look like the right shape for reducing confusing prompts and failing closed instead of emitting garbage cookie data. The focused tests cover the important mapping/query/regression cases.
I’m not formally approving this because it touches a security/privacy-sensitive onboarding path and adds a developer migration plan that can steer future AI/coding agents. Human maintainer review is still warranted before merge, especially for:
- confirming the in-process Keychain prompt / “Always Allow” persistence behavior on the signed Omi desktop app across supported macOS/browser combinations;
- validating the new OAuth migration document as maintainer-approved direction, since it describes “decisions locked,” Gmail restricted-scope/CASA tradeoffs, backend token storage assumptions, and the intended fallback strategy;
- deciding whether this docs plan belongs in this PR or should land separately after product/security sign-off.
No urgent security/supply-chain issue found in the diff. My recommendation is positive but maintainer-gated: treat the implementation as promising, keep the security/docs review labels, and have a human maintainer validate the Keychain behavior and OAuth roadmap language before merge.
2 participants
What changed and why
Replace browser Safe Storage reads through /usr/bin/security with an exact in-process Security framework lookup. Keychain prompts and Always Allow grants now belong to the signed Omi app and persist across relaunches. Unsupported versioned cookie blobs fail closed, and the durable Google OAuth migration path is documented.
Product invariants affected
None.
How it was verified
Tests
BrowserGoogleSessionTests covers the complete service/account mapping for every known browser target, exact Security query construction, and behavioral rejection of unsupported versioned cookie ciphertext. Adding a future browser target without a Keychain identity causes the mapping test to fail.
Scoped cleanups (optional)
Removed the /usr/bin/security read fallback and its retry policy. This keeps one native code path and prevents future prompt attribution regressions. The branch history uses one changed file per commit.