prevent shared browser session leaks - #562
Conversation
Phase 1 of the shared-browser session lifecycle fix. Atomic quota slots (unique-constraint claim on bf_key_fingerprint) are NOT yet implemented; the count-based pre-check remains advisory and the Browser Fabric server stays the final quota arbiter. - persist Browser Fabric credential identity without storing plaintext keys - preserve and track remote sessions across restart and worker changes - retry failed session cleanup with CAS-based maintenance claims - retain sessions after navigation or event-pipeline failures - add structured quota and credential errors - add browser leak and PostgreSQL-gated concurrency tests Tests: test_browser_tab_leak.py 38 passed; PostgreSQL concurrency tests 2 skipped (TEST_DATABASE_URL not configured); full backend suite 325 passed, 20 failed, 2 skipped — the 20 failures are byte-identical to the clean develop baseline (no local PostgreSQL, pre-existing mock/await and init.sql parity issues), zero regressions.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zomux
left a comment
zomux
left a comment
There was a problem hiding this comment.
Reviewed in depth — the leak fix is well-designed: credential references (SHA-256 fingerprint) instead of plaintext keys, confirmed-vs-unknown close semantics, tombstoning of unreleased sessions, a CAS sweeper that never holds a lock across a BF HTTP call, correct authz (resolve tab → authorize against its own workspace), cross-workspace isolation, redaction of key material in logs/errors/responses, and real Postgres concurrency tests. Strong, substantive tests (727-line leak suite + cross-connection CAS).
Approving, but ONE blocking coordination item before merge: this PR's migration revision 026 (026_browser_tab_session_tracking) COLLIDES with open PR #558, which also adds a 026 off 025. Whichever lands second breaks Alembic ('Duplicate revision identifiers: 026' → boot fails). One of the two must renumber to 027 (down_revision='026'). Please coordinate with #558 before both merge.
Non-blocking nits: (1) make 026 idempotent (op with IF NOT EXISTS, matching 025) given prod's history of stuck/partial migrations; (2) add last_active_at < idle_cutoff to the idle-reaper claim UPDATE — the SELECT filters on it but the claim only re-guards status=='active', a tiny TOCTOU that could reap a just-touched ephemeral tab.
…on 026 Merged the browser session-leak fix (credential references instead of plaintext keys, confirmed-vs-unknown close semantics, tombstoning, CAS sweeper, per-workspace isolation, redaction, real Postgres concurrency tests). DB SAFETY: made migration 026 idempotent + dialect-agnostic — it now checks the existing browser_tabs columns via the inspector and only adds/drops what's missing/present, instead of bare op.add_column/op.drop_column. This repo has a history of stuck/partial migrations in prod (see open #558), so a re-run on a partially-applied DB must not hard-fail with "column already exists". Revision chain unchanged (026 -> 025, the current develop head). Note: 026's revision id will collide with open PR #558 (also 026); whichever lands after this must renumber to 027 (down_revision="026"). Follow-up (minor, not done here): add `last_active_at < idle_cutoff` to the idle-reaper claim UPDATE in browser_maintenance.py to close a small TOCTOU.
2 participants
Phase 1 of the shared-browser session lifecycle fix. Atomic quota slots (unique-constraint claim on bf_key_fingerprint) are NOT yet implemented; the count-based pre-check remains advisory and the Browser Fabric server stays the final quota arbiter.
Tests: test_browser_tab_leak.py 38 passed; PostgreSQL concurrency tests 2 skipped (TEST_DATABASE_URL not configured); full backend suite 325 passed, 20 failed, 2 skipped — the 20 failures are byte-identical to the clean develop baseline (no local PostgreSQL, pre-existing mock/await and init.sql parity issues), zero regressions.