mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 08:03:39 +00:00
Brennan Benson
32ee3b0536
* reland(browser): restore CDP-identity cookie-import writes (#14729) Reverts the revert3c8410d927(#14942) to restore the reviewedbf6dc6fcbatree. The defect that caused that revert is NOT fixed by this commit — it is fixed in the commits that follow, so the delta a reviewer must scrutinise stays small instead of hiding inside a 2000-line re-add. Conflict resolutions, all keep-both: - browser-cookie-import.ts: two hunks around the zero-import early return. #14683's undecryptable warning and decryptedCookies.length === 0 condition are kept alongside the reland's old-client gate and partitionSkippedCookies. The old-client gate stays above the early return, and so above any mutation. - en.json: both string sets (#14683's undecryptable copy and partitionSkipped). - crashpad-capture.test.ts: took HEAD wholesale; unrelated to this ticket. getStoragePath, and its positive-write assertion moves from cookies.set to the CDP identity store — adapted, not weakened, and now strictly stronger because it also asserts cookies.set carries no imported user data. Every decryption assertion is untouched. * fix(browser): add registrableFamily, one definition of a cookie family STA-4300, change 1 of the reland fixes. No behaviour change yet — this only adds the helper the later commits derive every skip scope from. Deriving "family" inline in several places is what let the removal scope and the write set disagree in STA-4090 and STA-4170, so there is exactly one. The IP check runs on normalizeCookieDomain's OUTPUT, never the raw string. psl treats an IPv4 literal as a dotted DNS name — psl.parse('127.0.0.1').domain is '0.1' — and Chromium accepts many spellings of one address. new URL() inside normalizeCookieDomain canonicalises 127.1 / 2130706433 / 0x7f.1 / 010.0.0.1 and a trailing dot to a dotted quad first, so isIP() then catches all of them. Mutation-proved: moving the check ahead of normalisation reddens the five alternate-spelling cases and nothing else. Bracketed IPv6 needs its own branch because isIP('[::1]') is 0; without it the value falls through to psl, which throws, which returns the host — the right answer for the wrong reason. Returns null for a bare public suffix: naming `com` as a family would preserve an entire TLD from removal and silently turn an import into a no-op. * fix(browser): plan every cookie's fate before any jar mutation STA-4300, change 2. Adds planImportWrites — pure, no I/O — so the write set and every removal scope can derive from one value instead of being computed twice. Not yet wired into the import paths; that is the next commits. TWO passes, and the second is not optional. Family-atomic skip is a property of the whole input: with a readable mixed.example row BEFORE an unreadable sub.mixed.example row, a per-row guard emits the readable one before anything knows the family will be skipped. Pass 1 classifies and collects the skipped families; pass 2 re-filters the provisional writes. Mutation-proved with a faithful one-pass guard (consult skippedFamilies as you go): the readable-before-unreadable case goes red and the unreadable-before- readable case still passes. That asymmetry is why both orders are tested and why only the first is the named detector. Family closure is at the registrable boundary because that is the boundary the removal scope actually expands to: importedDomainScopes turns an imported mixed.example into descendant roots, so a readable apex cookie drags a skipped subdomain's live session into the removal scope (STA-4300 §2b). hasUnrepresentableSkip surfaces a skip whose family cannot be named. A family we cannot name is one we cannot exclude from the removal plan, and clearing a family we cannot protect is the P0 this ticket exists to stop — so the caller refuses before mutating rather than proceeding and hoping. * fix(browser): derive path A's removal scope from the write plan (STA-4300 §2b) Change 3. importValidatedCookies now plans before it opens the jar, and the replacement scope and the write set are the SAME array.bf6dc6fcbafiltered replacementDomains per exact cookie (partition.status !== 'unreadable'). That is not sufficient: replaceCookiesForImportedDomains expands each imported domain into its descendant roots, so a readable cookie on mixed.example pulls sub.mixed.example into the removal scope — and a skipped cookie living there has its session removed with nothing written back. Same erasure as the P0, different path. plan.writes is family-closed, and because path A passes the very same array to replaceCookiesForImportedDomains and to writeImportedCookies, the two sets cannot drift apart. That is stronger than keeping them in sync: there is only one set. Also here, both before any mutation: - an unrepresentable skip (registrableFamily → null) refuses the import, because a family we cannot name is one we cannot exclude from the removal scope; - the old-client gate now keys off plan.skips rather than re-deriving the condition. Counters: partitionSkippedCookies is a BREAKDOWN of skippedCookies — the unreadable rows plus their family-suppressed siblings — added in exactly once, so totalCookies === importedCookies + skippedCookies keeps holding. Counting it separately is how a summary silently stops adding up. Full src/main/browser suite: 69 files, 729 tests, green. * fix(browser): split path B into scan and emit, and never stage a skip (STA-4300) Change 4 — this is the commit that fixes the shipped P0.bf6dc6fcbadid: decryptedCookies.push({ ..., partition }) // write set built HERE if (partition.status === 'unreadable') { ... continue } // guard AFTER so an unreadable row discovered late could not retract a sibling already emitted, while removeTransplantableCookies cleared the ENTIRE jar. The mutation set was the whole jar and the write set a strict subset of it, which is how a mixed readable/unreadable source emptied a populated jar and repopulated only part of it. Now the row loop SCANS only. Nothing is emitted inside it — not decryptedCookies, not domainSet, not a staging row, not the imported count. Between scan and emit, planImportWrites closes the skip over whole registrable families, and emit walks the plan. Each scanned candidate carries its raw source row because buildChromiumCookieInsertParams needs it; a record holding only the derived fields compiles and then silently cannot stage. Also between scan and emit, both before any mutation: - an unrepresentable skip refuses the import; - any skipped family calls disableStaging. A staged image is a whole-database replacement on the next start, so it cannot express "preserve this family"; main's existing memoryFailed arm then reports restart-fallback-unavailable. Test contract change, deliberately stronger: "never stages a partitioned row whose ancestor bit is unreadable" asserted that an image WAS registered and merely omitted the row. That image would still have erased the preserved family on replay. It now asserts no image is registered at all — plus a new paired case proving a no-skip import still stages, so "disable on skip" cannot be implemented as "disable always" unnoticed. Honest note on detection: reverting the emit filter currently reddens only the staging assertion, because every end-to-end fixture in this module still starts with an EMPTY jar — where clear-then-write-all and clear-then-write-some are indistinguishable. That is the blind spot that let this ship. The populated-jar suite in the next commit is what actually detects the erasure. Full src/main/browser: 69 files, 730 tests, green. * fix(browser): never remove a family the import declined to write (STA-4300 I2) Change 5, and the one that makes preservation observable. removeTransplantableCookies takes preserveFamilies. removableCookieEntries filters those families out, which keeps them out of the removal plan AND — since the CDP snapshot is taken from that same list — out of the restore set, so a preserved coordinate is never submitted to any mutation at all. When anything is preserved, the bulk clearData path is not used. clearData removes everything outside excludeOrigins, and its own comment concedes a rejection may already have emptied part of the jar; a preserved family is deliberately absent from the snapshot, so a partial delete followed by a rejection would destroy it with no identity able to restore it. Handing a dynamically derived preserve list to that primitive cannot be made safe, so a skip-bearing clear runs the frozen per-coordinate plan — which already excludes those families — as its primary path instead. Nothing is preserved on an ordinary import, so that path keeps today's single clearData call unchanged. New populated-jar suite. Every end-to-end fixture in this module starts with cookies.get returning [], and against an empty jar "clear then write all" and "clear then write some" are indistinguishable — which is why four independent gates missed the P0. These fixtures start populated. Mutation-proved, and this is the first detector in the change set that catches the actual erasure rather than a proxy: - drop the preserved-family filter → 6 of 8 red - use bulk clearData while preserving → 6 of 8 red, including "leaves a preserved family untouched", which is the erasure itself IPv4-literal and single-label host families are covered explicitly: psl reads 127.0.0.1 as the dotted DNS name '0.1', so a wrong family here would fail to match the preserve set and erase the live loopback session. Full src/main/browser: 70 files, 738 tests, green. * test(browser): pin STA-4300 end to end in real Electron, both write paths Ports the pre-implementation reproduction into the PR. Verified RED against main's product code and GREEN with the fix, on both paths — I re-ran it both ways rather than assuming. Two assertions are INVERTED rather than deleted, and both are now stronger. The repro proved the defect by showing cookies.set was called WITHOUT a partitionKey; after the reland the import write path does not touch cookies.set for user data at all, so seeing the imported cookie there is itself the regression. The fixture's internal guard is inverted the same way. Non-vacuity, asserted in-run rather than in a report: ok:true with a nonzero importedCookies, the importer reaching its terminal step, the source really carrying its partition fields, and a CONTROL cookie written via CDP *with* a partitionKey and read back with it — so "partitionKey absent" cannot be confused with "the oracle cannot see partitions". electronVersion proves Electron ran. Fixture correction worth recording, because the original reproduction was invalid on this path and I did not catch it on first read: readJsonCookiePartition reads a NESTED partitionKey object, but the fixture wrote topLevelSite and hasCrossSiteAncestor at the entry's top level, where the importer never looks. The file/paste case therefore failed on main for a fixture-shape reason rather than for the defect — and its own non-vacuity check validated what the fixture wrote instead of what the parser reads, which is exactly how a check that looks rigorous proves nothing. Only the native half was ever a real reproduction. Partition keys use a schemeful SITE, not an origin: Chromium canonicalises https://app.example.com to https://example.com, measured via CDP. * fix(browser): fail closed on lossy cookie identity recovery * fix(browser): preserve unreadable families before decrypt * fix(browser): reject malformed cookie partition sites * fix(browser): validate recovered cookie partition sites * fix(browser): reject empty JSON partition identities * fix(browser): fail closed and serialize cookie imports * fix(browser): reject malformed CDP opaque flags * fix(browser): roll back outcome-unknown cookie writes * revert(browser): move import serialization out to STA-4601 Reverts only the concurrency-serialization half off7c27b71abso #15030 stays scoped to the STA-4300 reland. That commit mixed two concerns; this removes one of them and keeps the other. REMOVED (moves to STA-4601, a separate pre-existing P1): - acquireCookieMutationLock / withCookieMutationLock and the mutationLocks map; clear.ts goes back to withCookieClearLock - the import-wide lock in path A (mutationOwner on the target, acquire/release around replace + writes + rollback) - the widened lock around path B's clear + memory writes - browser-cookie-import-concurrency.test.ts KEPT (genuinely STA-4300 scope, not concurrency): - partitionKeyOpaque handling in readJsonCookiePartition and the CDP snapshot. An opaque partition key cannot be represented faithfully, so it reads as unreadable and its family is preserved — exactly the rule this ticket adds. 194e57a628's refinement of that shape stays too. The concurrent-import interleaving is real and reachable (nothing serialises imports per partition, and the clear lock is released before the memory writes), but it predates this change and is not caused or worsened by it. It gets its own PR and its own review rather than riding a P0 reland whose history is that every additional fix introduced a new defect. src/main/browser: 71 files, 772 tests, green. Electron oracle still passes on both write paths and still goes red against main's product code.