From 98ec1ffcabdc7b9b16461ebdedd83d1b6c4d7046 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 13 Sep 2026 22:31:49 +0200 Subject: [PATCH] fix(downloads): Retry and Re-download take a cookie added after the failure The login-wall copy tells the user to add their cookies and press Retry, but Retry kept the job on the shared asset the worker never attaches a cookie to, so it failed on the same wall. Resume now moves a pending/errored job onto the asset enqueue would pick with the cookie (never off a file or a download in flight). Re-download salted its fresh asset with the job's previous asset id, which the cookie gate cannot recompute, so even a cookie download rebuilt without the cookie. The rebuild salt is now the new asset's own id (plus the user's cookie salt when one applies), and is_authed_asset recognises it. --- backend/app/downloads/credentials.py | 35 +++-- backend/app/downloads/pipeline.py | 9 +- backend/app/downloads/service.py | 61 ++++++++- backend/app/routes/downloads.py | 5 +- backend/app/worker.py | 2 +- backend/tests/test_download_credentials.py | 145 ++++++++++++++++++++- 6 files changed, 229 insertions(+), 28 deletions(-) diff --git a/backend/app/downloads/credentials.py b/backend/app/downloads/credentials.py index 5594143c..917ad96e 100644 --- a/backend/app/downloads/credentials.py +++ b/backend/app/downloads/credentials.py @@ -1,10 +1,12 @@ """Per-user, per-host download cookies — the single source of truth for "does the acting user's cookie apply to this download, and how does that keep the result private". -Two call sites depend on ONE consistent answer (house rule: an invariant lives in shared code, not +Two sides depend on ONE consistent answer (house rule: an invariant lives in shared code, not in prose): * `enqueue` salts the MediaAsset cache signature with the user id WHEN a cookie applies, so the - authed download becomes a PRIVATE asset that is never cache-served to another user. + authed download becomes a PRIVATE asset that is never cache-served to another user. Retry + (`service.resume`) and Re-download (`service.redownload`) ask the same question again, so a + cookie the user added after a failure goes with the next attempt. * the worker attaches the cookie file to yt-dlp ONLY when a cookie still applies AND the asset it is downloading into is that private, salted identity — so a cookie can never poison a shared asset (the load-bearing cross-user privacy fix). @@ -30,7 +32,7 @@ from sqlalchemy.orm import Session from app.downloads import formats from app.downloads.adapters import _host_is -from app.models import DownloadCredential +from app.models import DownloadCredential, MediaAsset from app.security import decrypt_optional, encrypt log = logging.getLogger("siftlode.downloads.credentials") @@ -104,12 +106,27 @@ def authed_format_sig(spec: dict, user_id: int, host_key: str) -> str: return formats.format_sig(spec, salt=cookie_salt(user_id, host_key)) -def is_authed_asset(asset_format_sig: str, spec: dict, user_id: int, url: str | None) -> bool: +def rebuild_salt(asset_id: int, cookie: str | None) -> str: + """The salt of a Re-download's fresh asset: its OWN row id (so every rebuild is a new identity no + enqueue can ever compute), prefixed with the user's `cookie_salt` when a cookie applies. The id is + what makes it recognisable — the worker has the asset row, so `is_authed_asset` can recompute this + one, where a salt built from the job's previous asset id could not be.""" + return f"{cookie}:rebuild:{asset_id}" if cookie else f"rebuild:{asset_id}" + + +def is_authed_asset(asset: MediaAsset, spec: dict, user_id: int, url: str | None) -> bool: """Whether an asset is `user_id`'s private, cookie-salted identity for `url`'s host — the one - kind of asset a cookie is ever attached to. The worker's cookie gate asks it before a download, + kind of asset a cookie is ever attached to: the one enqueue creates (`authed_format_sig`), or a + Re-download's rebuild of it (`rebuild_salt`). The worker's cookie gate asks it before a download, and the failure diagnosis asks it after one (a failure on this asset happened WITH the cookie).""" host_key = host_key_for_url(url) - return host_key is not None and asset_format_sig == authed_format_sig(spec, user_id, host_key) + if host_key is None: + return False + if asset.format_sig == authed_format_sig(spec, user_id, host_key): + return True + return asset.id is not None and asset.format_sig == formats.format_sig( + spec, salt=rebuild_salt(asset.id, cookie_salt(user_id, host_key)) + ) def salt_for_enqueue(db: Session, user_id: int, url: str | None) -> str | None: @@ -211,7 +228,7 @@ def resolve_cookiefile( db: Session, user_id: int, url: str | None, - asset_format_sig: str, + asset: MediaAsset, spec: dict, staging: Path, ) -> Path | None: @@ -220,7 +237,7 @@ def resolve_cookiefile( The double gate that prevents cross-user leakage: 1. a credential must still exist for `url`'s host (host-scoped via `host_key_for_url`), AND 2. the asset being downloaded into must BE this user's private, salted identity - (`asset_format_sig == authed_format_sig(...)`). + (`is_authed_asset`). If (2) fails the asset is the SHARED (unsalted) one — attaching the cookie would poison the global cache with a private-group video, so we attach nothing. An unreadable blob (rotated key → @@ -232,7 +249,7 @@ def resolve_cookiefile( cred = get_credential(db, user_id, host_key) if cred is None: return None - if not is_authed_asset(asset_format_sig, spec, user_id, url): + if not is_authed_asset(asset, spec, user_id, url): return None # shared asset — never attach a cookie to a non-private identity raw = decrypt_optional(cred.cookies_enc) if not raw: diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index a29dcc66..e683913f 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -8,7 +8,6 @@ from datetime import date, datetime, timedelta, timezone from pathlib import Path import yt_dlp -from sqlalchemy import select from app import sysconfig from app.config import settings @@ -446,12 +445,10 @@ def _download( # this asset is that user's private, salted identity (the double gate lives in credentials). remaining_bytes: int | None = None with SessionLocal() as cdb: - asset_sig = cdb.execute( - select(MediaAsset.format_sig).where(MediaAsset.id == asset_id) - ).scalar_one_or_none() - if asset_sig is not None: + target = cdb.get(MediaAsset, asset_id) + if target is not None: cookiefile = credentials.resolve_cookiefile( - cdb, user_id, url, asset_sig, spec, staging + cdb, user_id, url, target, spec, staging ) # SEC-13: what this user has left. `check_enqueue` only compared the footprint as it # stood when the job was QUEUED — a download's size is unknown until it finishes, so a diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 8f1aa971..77ff9fd2 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -7,6 +7,7 @@ handles everything else. State transitions used by the routes (pause/resume/canc the worker live in `worker.py` / the routes; this module owns enqueue + asset bookkeeping. """ +import uuid from datetime import datetime, timezone from sqlalchemy import func, select, update @@ -341,7 +342,11 @@ def pause(db: Session, job: DownloadJob) -> None: def resume(db: Session, job: DownloadJob) -> None: """Requeue a paused/errored job for a fresh attempt, clearing its error. If the SHARED asset itself failed, reset it to pending so the worker actually re-downloads — otherwise the requeued job would - instantly re-inherit the asset's stale error (the worker short-circuits an errored asset).""" + instantly re-inherit the asset's stale error (the worker short-circuits an errored asset). + + A cookie the user added AFTER the job was queued moves the job onto their private asset first + (`_follow_the_cookie`): the worker only ever attaches a cookie to that identity, so without the move + the Retry a login-wall message asks for would fail on the same wall.""" if job.status in (JobStatus.PAUSED, JobStatus.ERROR): job.progress = 0 job.error = None @@ -349,6 +354,8 @@ def resume(db: Session, job: DownloadJob) -> None: job.error_detail = None if job.asset_id: asset = db.get(MediaAsset, job.asset_id) + if asset is not None: + asset = _follow_the_cookie(db, job, asset) if asset and asset.status == "error": # MediaAsset.status (no enum), not a JobStatus asset.status = "pending" asset.error = None @@ -356,6 +363,32 @@ def resume(db: Session, job: DownloadJob) -> None: db.commit() +def _follow_the_cookie(db: Session, job: DownloadJob, asset: MediaAsset) -> MediaAsset: + """Re-point a requeued job at the asset enqueue would pick NOW, when the user has a cookie for the + link's host but the job still holds an asset the worker won't attach it to (queued before the + cookie existed). Returns the asset the job holds afterwards. + + Only a `pending` / `error` asset moves — neither has a file nor a download in flight. A `ready` one + already has the file, and a `downloading` one may be this job's own child still being stopped by + the pause; the next Retry after it settles moves the job then. Removing a cookie never moves a job + back: the worker attaches nothing without a credential, so the private asset is as good as any.""" + if asset.status not in ("pending", "error"): + return asset + url = source_url(job.source_kind, job.source_ref) + salt = credentials.salt_for_enqueue(db, job.user_id, url) + spec = formats.normalize(job.profile_snapshot) + if salt is None or credentials.is_authed_asset(asset, spec, job.user_id, url): + return asset + target = get_or_create_asset( + db, job.source_kind, job.source_ref, formats.format_sig(spec, salt=salt) + ) + populate_from_catalog(db, target) + release_asset(db, job) # drop the old hold BEFORE re-pointing — it keys off job.asset_id + incr_ref_count(db, target.id) + job.asset_id = target.id + return target + + def cancel(db: Session, job: DownloadJob) -> None: """Cancel a job unless it's already done/canceled, releasing its asset hold WHILE it still counts as holding — `canceled` ∉ HOLDING, so a later delete won't double-release.""" @@ -374,13 +407,31 @@ def delete(db: Session, job: DownloadJob) -> None: def redownload(db: Session, job: DownloadJob) -> None: """Rebuild a finished/failed job's file into a FRESH, private asset (the cache sig is salted with - the job id + current asset id, so it never reuses the dedup pool) and requeue the SAME job row — + the new asset's own id, so it never reuses the dedup pool) and requeue the SAME job row — so any public share link (DownloadLink -> job_id) keeps resolving to the newly-downloaded file. The old hold is dropped first (freeing the old file iff this job was its last holder). The caller - has already rejected edit clips + still-in-progress jobs (HTTP 409).""" + has already rejected edit clips + still-in-progress jobs (HTTP 409). + + When the user has a cookie for the link's host, the salt carries it (`credentials.rebuild_salt`), + so the worker recognises the rebuild as their private identity and attaches the cookie — a cookie + download, or one the user added after a failure, rebuilds with the cookie.""" spec = formats.normalize(job.profile_snapshot) - sig = formats.format_sig(spec, salt=f"{job.id}:{job.asset_id}") - asset = get_or_create_asset(db, job.source_kind, job.source_ref, sig) + cookie = credentials.salt_for_enqueue( + db, job.user_id, source_url(job.source_kind, job.source_ref) + ) + # The salt names the row's own id, which exists only once the row does: insert it under a unique + # placeholder, then write the real signature in the same transaction (nobody sees the placeholder). + asset = MediaAsset( + source_kind=job.source_kind, + source_ref=job.source_ref, + format_sig=f"rebuild:{uuid.uuid4().hex}", + status="pending", + ref_count=0, + ) + db.add(asset) + db.flush() + asset.format_sig = formats.format_sig(spec, salt=credentials.rebuild_salt(asset.id, cookie)) + db.flush() populate_from_catalog(db, asset) # Drop the old hold BEFORE re-pointing the job — release_asset keys off the job's current asset. release_asset(db, job) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 3ef241e3..0ec65afa 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -630,8 +630,9 @@ def redownload_download( resolves to the newly-downloaded file — a re-download never breaks a link you already sent out (only a *failed* re-download leaves the job in `error`, which is the intended broken-link case). - The rebuild always re-fetches into a FRESH, private asset (the cache sig is salted with the job - id + current asset id) rather than the dedup pool. That means it (a) always genuinely rebuilds + The rebuild always re-fetches into a FRESH, private asset (the cache sig is salted with the new + asset's own id, and the user's cookie salt when one applies — `credentials.rebuild_salt`) rather + than the dedup pool. That means it (a) always genuinely rebuilds under the current format/naming rules — never silently reuses a shared cached file — and (b) never deletes a file another job/user still shares: the old asset is only removed if THIS job was its last holder.""" diff --git a/backend/app/worker.py b/backend/app/worker.py index ebc0c1d1..24138a11 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -410,7 +410,7 @@ def _process_job(job_id: int) -> None: asset_error, url=url, cookie_attached=credentials.is_authed_asset( - asset.format_sig, formats.normalize(job.profile_snapshot), job.user_id, url + asset, formats.normalize(job.profile_snapshot), job.user_id, url ), ) diff --git a/backend/tests/test_download_credentials.py b/backend/tests/test_download_credentials.py index 48e4f42c..b5bc2ab6 100644 --- a/backend/tests/test_download_credentials.py +++ b/backend/tests/test_download_credentials.py @@ -18,7 +18,7 @@ from cryptography.fernet import Fernet from app import security from app.downloads import credentials, formats, service -from app.models import DownloadCredential, MediaAsset, User +from app.models import DownloadCredential, DownloadJob, MediaAsset, User @pytest.fixture(autouse=True) @@ -189,7 +189,9 @@ class TestResolveCookiefile: db.commit() spec = formats.normalize({}) salted = credentials.authed_format_sig(spec, u.id, "facebook") - path = credentials.resolve_cookiefile(db, u.id, FB_URL, salted, spec, tmp_path) + path = credentials.resolve_cookiefile( + db, u.id, FB_URL, self._asset_sig(salted), spec, tmp_path + ) assert path is not None and Path(path).read_text() == COOKIES def test_never_attaches_to_a_shared_asset(self, db, tmp_path): @@ -200,7 +202,12 @@ class TestResolveCookiefile: db.commit() spec = formats.normalize({}) shared = formats.format_sig(spec) - assert credentials.resolve_cookiefile(db, u.id, FB_URL, shared, spec, tmp_path) is None + assert ( + credentials.resolve_cookiefile( + db, u.id, FB_URL, self._asset_sig(shared), spec, tmp_path + ) + is None + ) def test_non_matching_host_gets_no_cookie(self, db, tmp_path): u = _user(db) @@ -210,13 +217,141 @@ class TestResolveCookiefile: # A YouTube URL has no supported host_key -> no cookie regardless of what's stored. yt = "https://www.youtube.com/watch?v=abc" salted = credentials.authed_format_sig(spec, u.id, "facebook") - assert credentials.resolve_cookiefile(db, u.id, yt, salted, spec, tmp_path) is None + assert ( + credentials.resolve_cookiefile(db, u.id, yt, self._asset_sig(salted), spec, tmp_path) + is None + ) def test_no_credential_gets_no_cookie(self, db, tmp_path): u = _user(db) spec = formats.normalize({}) salted = credentials.authed_format_sig(spec, u.id, "facebook") - assert credentials.resolve_cookiefile(db, u.id, FB_URL, salted, spec, tmp_path) is None + assert ( + credentials.resolve_cookiefile( + db, u.id, FB_URL, self._asset_sig(salted), spec, tmp_path + ) + is None + ) + + +# --- Retry / Re-download take a cookie added later (DB) ----------------------------------------- +# Measured on dev (E11.5): a Facebook reel failed on the login wall, the user added their cookie as +# the failure copy tells them to, pressed Retry — and it failed again, because the job still held the +# shared asset the worker never attaches a cookie to. Re-download forked a salt the gate could not +# recognise, so even a job downloaded WITH a cookie rebuilt without it. + + +def _held_job(db, u, *, sig, asset_status="error", job_status="error"): + a = MediaAsset( + source_kind="url", source_ref=FB_URL, format_sig=sig, status=asset_status, ref_count=1 + ) + db.add(a) + db.flush() + j = DownloadJob( + user_id=u.id, + asset_id=a.id, + source_kind="url", + source_ref=FB_URL, + status=job_status, + queue_pos=0, + profile_snapshot={}, + ) + db.add(j) + db.commit() + return a, j + + +class TestRequeueTakesTheCookie: + spec = formats.normalize({}) + + def test_retry_after_adding_a_cookie_moves_the_job_to_the_private_asset(self, db, tmp_path): + u = _user(db) + shared, j = _held_job(db, u, sig=formats.format_sig(self.spec)) + credentials.upsert_credential(db, u.id, "facebook", COOKIES) + db.commit() + + service.resume(db, j) + + db.expire_all() + job = db.get(DownloadJob, j.id) + target = db.get(MediaAsset, job.asset_id) + assert job.status == "queued" and target.id != shared.id + assert target.format_sig == credentials.authed_format_sig(self.spec, u.id, "facebook") + assert (target.status, target.ref_count) == ("pending", 1) + assert db.get(MediaAsset, shared.id).ref_count == 0 # the old hold went with the move + assert credentials.resolve_cookiefile(db, u.id, FB_URL, target, self.spec, tmp_path) + + def test_retry_without_a_cookie_keeps_the_shared_asset(self, db): + u = _user(db) + shared, j = _held_job(db, u, sig=formats.format_sig(self.spec)) + + service.resume(db, j) + + db.expire_all() + assert db.get(DownloadJob, j.id).asset_id == shared.id + assert db.get(MediaAsset, shared.id).ref_count == 1 + + @pytest.mark.parametrize( + ("asset_status", "job_status"), [("ready", "error"), ("downloading", "paused")] + ) + def test_retry_never_moves_off_a_file_or_a_download_in_flight( + self, db, asset_status, job_status + ): + u = _user(db) + shared, j = _held_job( + db, + u, + sig=formats.format_sig(self.spec), + asset_status=asset_status, + job_status=job_status, + ) + credentials.upsert_credential(db, u.id, "facebook", COOKIES) + db.commit() + + service.resume(db, j) + + db.expire_all() + assert db.get(DownloadJob, j.id).asset_id == shared.id + + def test_redownload_with_a_cookie_rebuilds_into_an_asset_the_cookie_goes_with( + self, db, tmp_path + ): + u = _user(db) + other = _user(db, "other@example.com") + credentials.upsert_credential(db, u.id, "facebook", COOKIES) + credentials.upsert_credential(db, other.id, "facebook", COOKIES) + authed = credentials.authed_format_sig(self.spec, u.id, "facebook") + first, j = _held_job(db, u, sig=authed, asset_status="ready", job_status="done") + + seen = {first.id} + for _ in range(2): # a rebuild of a rebuild is still recognised + service.redownload(db, j) + db.expire_all() + job = db.get(DownloadJob, j.id) + target = db.get(MediaAsset, job.asset_id) + assert target.id not in seen and target.format_sig != authed + seen.add(target.id) + assert credentials.resolve_cookiefile(db, u.id, FB_URL, target, self.spec, tmp_path) + # Private: another user's cookie is never attached to it. + assert not credentials.is_authed_asset(target, self.spec, other.id, FB_URL) + + # A Retry of a failed rebuild keeps the rebuild — it is already the cookie's identity. + target.status, job.status = "error", "error" + db.commit() + service.resume(db, job) + db.expire_all() + assert db.get(DownloadJob, j.id).asset_id == target.id + + def test_a_rebuild_made_without_a_cookie_is_no_cookie_identity(self, db, tmp_path): + u = _user(db) + _, j = _held_job(db, u, sig=formats.format_sig(self.spec)) + service.redownload(db, j) + credentials.upsert_credential(db, u.id, "facebook", COOKIES) + db.commit() + + db.expire_all() + target = db.get(MediaAsset, db.get(DownloadJob, j.id).asset_id) + assert not credentials.is_authed_asset(target, self.spec, u.id, FB_URL) # --- API: write-only, never echoes the blob (DB) ----------------------------------------------