fix(browser): import Chromium cookies from Network DBs (#6875)#7882
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Chromium cookie import path now reads the source browser’s live cookies SQLite database directly instead of copying it to a temporary file. Target partition cookie database discovery resolves Network or legacy cookie paths and creates the database when missing. Decryption key extraction is deferred until encrypted values are detected, with cleanup on failure. Session persistence and tests cover Network paths, WAL databases, encrypted cookies, and staging cleanup. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/browser/browser-cookie-import.ts (1)
1473-1474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVestigial
tmpDirand misleadingtmpCookiesPathname.With temp snapshotting removed,
tmpDiris created but nothing is ever written into it — it's onlyrmSync'd in the various exit paths (Lines 1512, 1526, 1559, 1573, 1695, 1781), so it's now dead scaffolding. LikewisetmpCookiesPathno longer points at a temp copy; it's the live source path, which obscures the WAL-lock reasoning above. Consider droppingtmpDir(and its cleanup calls) and renaming the variable to reflect the live source path.♻️ Rename and drop the unused temp dir
- const tmpDir = mkdtempSync(join(tmpdir(), 'orca-cookie-import-')) - const tmpCookiesPath = browser.cookiesPath + const sourceCookiesPath = browser.cookiesPathThen remove the corresponding
rmSync(tmpDir, ...)cleanup calls and updatenew DatabaseSync(tmpCookiesPath, ...)(Line 1536) to usesourceCookiesPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ecf1589b-917a-4cbe-ab5f-4ce11e323bfa
📒 Files selected for processing (2)
src/main/browser/browser-cookie-import.test.tssrc/main/browser/browser-cookie-import.ts
rodboev
force-pushed
the
pr/chromium-cookie-import-db-paths
branch
from
6a4fec2 to
8f49611
Compare
Co-authored-by: Orca <help@stably.ai>
Jinwoo-H
left a comment
Jinwoo-H
left a comment
There was a problem hiding this comment.
Thanks — reviewed against #6875 and added the remaining maintainer fixes:
- keep modern
Network/Cookiesimport and restart replay on the same database - clean staging data after key-access failures
- cover live WAL, plaintext, encrypted, cleanup, and legacy fallback paths
Focused tests, full typecheck/lint, and the real Electron import flow pass.
# Conflicts: # src/main/browser/browser-session-registry.persistence.test.ts
Jinwoo-H
left a comment
Jinwoo-H
left a comment
There was a problem hiding this comment.
Re-approved after merging the current main into the PR branch and re-running the focused tests, full typecheck, and full lint.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 48c9d075-c2c0-42a9-b27d-724c33c7635e
📒 Files selected for processing (6)
src/main/browser/browser-cookie-import-test-database.tssrc/main/browser/browser-cookie-import.test.tssrc/main/browser/browser-cookie-import.tssrc/main/browser/browser-session-registry.persistence.test.tssrc/main/browser/browser-session-registry.tssrc/main/browser/chromium-cookie-path.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/browser/browser-cookie-import.ts
| export function encryptMacChromiumCookie(value: string, password: string): Buffer { | ||
| const key = pbkdf2Sync(password, 'saltysalt', 1003, 16, 'sha1') | ||
| const cipher = createCipheriv('aes-128-cbc', key, Buffer.alloc(16, ' ')) | ||
| return Buffer.concat([ | ||
| Buffer.from('v10'), | ||
| cipher.update(Buffer.from(value, 'latin1')), | ||
| cipher.final() | ||
| ]) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a "Why" comment for Chromium macOS encryption parameters.
The encryptMacChromiumCookie function uses non-obvious, Chromium-specific encryption parameters (PBKDF2 with 'saltysalt'/1003 iterations, AES-128-CBC with a 16-space IV, v10 prefix). Per coding guidelines, code driven by a non-obvious constraint should include a brief comment explaining why.
💡 Suggested comment
export function encryptMacChromiumCookie(value: string, password: string): Buffer {
+ // Why: Chromium on macOS derives the cookie encryption key via PBKDF2
+ // (salt 'saltysalt', 1003 iterations, SHA1) and encrypts with AES-128-CBC
+ // using a fixed 16-byte IV of spaces, prefixing the ciphertext with 'v10'.
const key = pbkdf2Sync(password, 'saltysalt', 1003, 16, 'sha1')
const cipher = createCipheriv('aes-128-cbc', key, Buffer.alloc(16, ' '))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function encryptMacChromiumCookie(value: string, password: string): Buffer { | |
| const key = pbkdf2Sync(password, 'saltysalt', 1003, 16, 'sha1') | |
| const cipher = createCipheriv('aes-128-cbc', key, Buffer.alloc(16, ' ')) | |
| return Buffer.concat([ | |
| Buffer.from('v10'), | |
| cipher.update(Buffer.from(value, 'latin1')), | |
| cipher.final() | |
| ]) | |
| } | |
| export function encryptMacChromiumCookie(value: string, password: string): Buffer { | |
| // Why: Chromium on macOS derives the cookie encryption key via PBKDF2 | |
| // (salt 'saltysalt', 1003 iterations, SHA1) and encrypts with AES-128-CBC | |
| // using a fixed 16-byte IV of spaces, prefixing the ciphertext with 'v10'. | |
| const key = pbkdf2Sync(password, 'saltysalt', 1003, 16, 'sha1') | |
| const cipher = createCipheriv('aes-128-cbc', key, Buffer.alloc(16, ' ')) | |
| return Buffer.concat([ | |
| Buffer.from('v10'), | |
| cipher.update(Buffer.from(value, 'latin1')), | |
| cipher.final() | |
| ]) | |
| } |
Source: Coding guidelines
Jinwoo-H
left a comment
Jinwoo-H
left a comment
There was a problem hiding this comment.
Final re-approval on the conflict-resolved head after PR Checks passed, including tests, unpacked Electron build, and packaged CLI smoke.
2 participants
Summary
Network/Cookiesfallback used for source browser profiles instead of assuming the legacyCookiespath.Closes #6875.
Screenshots
No visual change.
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildFocused validation:
pnpm exec vitest run --config config/vitest.config.ts src/main/browser/browser-cookie-import.test.ts- passed,Test Files 1 passed (1),Tests 17 passed (17).pnpm run typecheck:node- passed.pnpm exec oxlint src/main/browser/browser-cookie-import.ts src/main/browser/browser-cookie-import.test.ts- passed.pnpm exec oxfmt --check src/main/browser/browser-cookie-import.ts src/main/browser/browser-cookie-import.test.ts- passed,All matched files use the correct format.AI Review Report
Security Audit
Notes
No visual change. Live Chrome-on-Windows locking remains manual-validation scope; regression coverage uses real SQLite files for source and target
Network/Cookiespaths.