Repository navigation
fix(desktop): don't abort the login import on a cookie row that isn't a buffer - #6788
Conversation
… a buffer decryptCookieValue's "v10" scheme check ran before its own try/catch, so a cookie row whose encrypted_value came back as a string threw `encryptedValue.subarray is not a function` out of mapCookieRow, killed the read loop, and failed the whole import. Move the check inside the guard so such a row is skipped like any other undecryptable one. Refs DESKTOP-11J
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe cookie import path now rejects text-valued encrypted fields by returning ChangesCookie validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR skips malformed cookie rows instead of aborting the entire login import while preserving successful decryption and existing loud failures; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Problem
Someone opens Import settings from another browser, ticks logins, and presses Import. The mutation throws and they get a red
Could not import from browser — Could not import browser cookies: encryptedValue.subarray is not a functiontoast. Nothing is imported, and retrying is futile: the same profile row fails every time. The reporting person pressed the button 6 times in 33 seconds and got nowhere. If they also ticked history, the history import runs first and succeeds, but the cookie throw happens before the success toast — so a partly-successful import is still reported to them as a total failure.Root cause
decryptCookieValuedocuments that it "returns null for values in any other scheme", and has atry/catchimplementing exactly that. But the "v10" scheme check sat above thattry:.lengthsucceeds for anything with a length;.subarrayonly exists on a Buffer. The rows come from a rawbetter-sqlite3.all()cast straight toChromeCookieRow[], soencrypted_value: Bufferis an assumption about the profile database, not a guarantee — SQLite is dynamically typed, and a value stored in that column as TEXT comes back as a JavaScript string. On such a row theTypeErrorescapeddecryptCookieValue, escapedmapCookieRow, aborted theforloop inreadCookiesFromProfile, and surfaced as a 500 from the tRPC mutation.Fix
Move the scheme check inside the existing
try. One row that isn't a decryptable buffer is now skipped like any other undecryptable row — which is what the function already promised. Nothing else changes: thetrybody is untouched, and a successfully decrypted cookie produces exactly what it did before.The only behaviour that changes is for input that previously threw. No row-shape validation, no per-field error handling, no new capture.
What still fails loudly
Deliberately unchanged, because these mean "we imported nothing", not "we skipped one cookie":
new Database(..., { fileMustExist: true })) — throws, reaches the tRPC catch, error toast.copyFileSync) — same.cookiestable/columns aren't what we expect (SELECT fails) — same.NOT_FOUND, "That browser profile is no longer available."One correction on the framing worth stating plainly: a denied Keychain key never threw, before or after this change.
readSafeStorageKeyreturns null on denial,readCookiesFromProfilereturns[], andimportCookiesIntoSessionreportskeyUnavailable: true, which the dialog renders as "logins skipped (Keychain access denied)". That's a distinct, visible signal rather than an error toast, and it is preserved untouched — it is not swallowed by this fix.Typed causes
None added. Nothing reads a
cause.kindon this path — the renderer branches on thekeyUnavailablefield, not on error shape — so the existing plainnew TRPCError({ code, message })at the router stays as it is.Verification
bunx biome checkon both changed files:cd apps/desktop && bun run typecheck:Two tests added, using the shape the failure actually arrives in (a string where a Buffer was declared). Before the change, both fail with the exact error from the issue:
After, the file passes, and so does the whole
src/main/lib/browser/directory — including the pre-existing tests that assert real v10 buffers still decrypt, so the skip has not widened into swallowing valid cookies:Repo-wide
bun run lintwas not run (it hangs on cross-worktree bun locks);biome checkon the changed files stands in for it.Refs DESKTOP-11J
https://claude.ai/code/session_b1c13bee-c201-4c37-bf2a-d43d29438086
Summary by cubic
Skips non-buffer cookie rows during login import to prevent a hard failure. Previously,
decryptCookieValuechecked the "v10" scheme before its try/catch and threw on string rows, aborting the import; now the check runs inside the guard, so undecryptable or non-buffer rows return null and are skipped.decryptCookieValue. Valid "v10" buffers still decrypt unchanged.keyUnavailable: true.encrypted_valuefrombetter-sqlite3.Written for commit 8aa0bc4. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests