From 367bcebc9fa84e7f543a958c89762cbe7714da16 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 00:21:43 +0200 Subject: [PATCH 01/22] fix(downloads): revive the worker cancel branch and guard terminal writes (B-sub-1) The cancel route writes status 'canceled' but the supervisor compared against 'cancelled', so its "a route stopped it -> kill the child, free the asset" branch was dead code. A canceled job's child ran to completion and wrote 'done' unconditionally over the route's 'canceled'; since 'done' holds the asset but 'canceled' does not, a later delete released the asset a second time, dropping ref_count below the live holder count and deleting a file still in use. Add app/downloads/status.py as the single source of truth for the lifecycle literals (JobStatus StrEnum + the HOLDING / STOPPED_BY_ROUTE sets) and use it at the safety-critical sites: the supervisor stop check, the route cancel, and release_asset's holding set. Make the worker's terminal done/error writes conditional on the row still being 'running' so a child finishing just after a route cancel/pause can no longer resurrect it. Add the missing 'canceled' supervisor regression test. (The full literal sweep across the downloads surface is left for the E3 downloads owner-unification; this is the data-safety subset.) --- backend/app/downloads/service.py | 3 +- backend/app/downloads/status.py | 38 ++++++++++++++++++++++ backend/app/routes/downloads.py | 5 +-- backend/app/worker.py | 42 +++++++++++++++++-------- backend/tests/test_worker_supervisor.py | 15 ++++++++- 5 files changed, 86 insertions(+), 17 deletions(-) create mode 100644 backend/app/downloads/status.py diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 5d79a4a6..ad601cf6 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -16,6 +16,7 @@ from app.config import settings from app.downloads import adapters from app.downloads import edit as editmod from app.downloads import formats, quota, storage +from app.downloads.status import HOLDING from app.models import Channel, DownloadJob, MediaAsset, Video @@ -238,7 +239,7 @@ def release_asset(db: Session, job: DownloadJob) -> None: +1 it, and deleting it under that would FK-null the new job's asset (a lost update on ref_count). A ready asset is still freed at ref==0 (its file is the point); an orphaned errored row is cheap (no file) and gets reused+reset by the next enqueue, or reclaimed by a later GC pass.""" - if not job.asset_id or job.status not in ("queued", "running", "paused", "done", "error"): + if not job.asset_id or job.status not in HOLDING: return asset = db.get(MediaAsset, job.asset_id) if asset is None: diff --git a/backend/app/downloads/status.py b/backend/app/downloads/status.py new file mode 100644 index 00000000..7ee2a05f --- /dev/null +++ b/backend/app/downloads/status.py @@ -0,0 +1,38 @@ +"""Canonical download-job status values — the ONE source of truth for the lifecycle literals. + +A job's status is written from several places: the worker (claim → running, terminal done/error, +requeue), the routes (pause/cancel/redownload/delete), the service (enqueue), and the GC. Keeping the +literals here is a data-safety measure, not tidiness. A British/American drift once silently broke +cancellation: the supervisor compared the live status against ``"cancelled"`` while the cancel route +wrote ``"canceled"`` (models.py), so the supervisor's "a route stopped it → kill the child, free the +asset" branch was DEAD. A canceled job's child then ran to completion and wrote ``"done"`` +unconditionally over the route's ``"canceled"``; because ``done`` counts as a holding state but +``canceled`` does not (see ``service.release_asset``), a later delete released the asset a SECOND time, +dropping ``ref_count`` below the live holder count and deleting a file still in use. Import these names +instead of typing the strings. +""" + +from enum import StrEnum + + +class JobStatus(StrEnum): + QUEUED = "queued" + RUNNING = "running" + PAUSED = "paused" + DONE = "done" + ERROR = "error" + CANCELED = "canceled" + + +# A job "holds" its media asset (the +1 on ``ref_count`` from enqueue) while in any of these states; +# ``release_asset`` on a status OUTSIDE this set is a no-op. ``CANCELED`` is deliberately excluded: the +# cancel route releases once while the job is still holding, then sets ``CANCELED``, so a later delete +# does not double-release. This invariant only holds while a finishing child cannot flip ``CANCELED`` +# back into the set — hence the worker's terminal writes are conditional on the row still being RUNNING. +HOLDING = frozenset( + {JobStatus.QUEUED, JobStatus.RUNNING, JobStatus.PAUSED, JobStatus.DONE, JobStatus.ERROR} +) + +# States a route sets to STOP a running job. The worker's supervisor must step in (kill the child, free +# the asset, leave the route's status intact) when it observes one of these on a job it is running. +STOPPED_BY_ROUTE = frozenset({JobStatus.PAUSED, JobStatus.CANCELED}) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index e1fe1503..2b1c08c9 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -22,6 +22,7 @@ from app.db import get_db from app.downloads import edit as editmod from app.downloads import links as linksmod from app.downloads import formats, quota, service, storage +from app.downloads.status import JobStatus from app.models import ( DownloadJob, DownloadLink, @@ -544,9 +545,9 @@ def cancel_download( job_id: int, user: User = Depends(require_human), db: Session = Depends(get_db) ) -> dict: job = _own_job(db, user, job_id) - if job.status not in ("done", "canceled"): + if job.status not in (JobStatus.DONE, JobStatus.CANCELED): service.release_asset(db, job) # release while the job still counts as holding - job.status = "canceled" + job.status = JobStatus.CANCELED db.commit() return _serialize_job(db, job) diff --git a/backend/app/worker.py b/backend/app/worker.py index 0749bd42..d7624203 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -36,6 +36,7 @@ from app.db import SessionLocal from app.downloads import edit as editmod from app.downloads import errors as dlerrors from app.downloads import adapters, formats, quota, service, storage +from app.downloads.status import STOPPED_BY_ROUTE, JobStatus from app.models import DownloadJob, MediaAsset from app.notifications import create_notification from app.titles import normalize_title @@ -359,7 +360,7 @@ def _run_supervised(job_id: int, asset_id: int) -> None: except subprocess.TimeoutExpired: pass status, stale = _job_liveness(job_id) - if status is None or status in ("paused", "cancelled"): + if status is None or status in STOPPED_BY_ROUTE: reason = "cancelled" # a route stopped it (or the row vanished) break if status == "running": @@ -481,8 +482,11 @@ def _finish_from_cache(job_id: int, asset_id: int) -> None: with SessionLocal() as db: job = db.get(DownloadJob, job_id) asset = db.get(MediaAsset, asset_id) - if job and asset: - job.status = "done" + # Only finalize a job the route hasn't stopped: the flip is conditional on the row still being + # RUNNING so a concurrent cancel/pause is never resurrected to `done` (which would break the + # no-double-release invariant in service.release_asset — see downloads/status.py). + if job and asset and job.status == JobStatus.RUNNING: + job.status = JobStatus.DONE job.progress = 100 job.phase = None if not job.display_name: @@ -868,8 +872,12 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe asset.last_access_at = datetime.now(timezone.utc) asset.expires_at = expires job = db.get(DownloadJob, job_id) - if job: - job.status = "done" + # Conditional terminal write: only finalize while the row is still RUNNING, so a child that + # finishes AFTER a route cancel/pause (narrow window past the cooperative checkpoints) can't + # overwrite `canceled`/`paused` with `done` and re-open the double-release class fixed here. + finalized = bool(job and job.status == JobStatus.RUNNING) + if finalized: + job.status = JobStatus.DONE job.progress = 100 job.phase = None job.speed_bps = None @@ -880,7 +888,8 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe # Capture the owner before commit expires the row (avoids a refresh SELECT just to notify). owner_id = job.user_id if job else None db.commit() - _notify_download_done(db, owner_id, meta.title, job_id, kind="download") + if finalized: + _notify_download_done(db, owner_id, meta.title, job_id, kind="download") log.info("job %s done: %s", job_id, rel) except (_Aborted, editmod.EditAborted): @@ -896,8 +905,10 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe asset.status = "error" asset.error = msg job = db.get(DownloadJob, job_id) - if job: - job.status = "error" + # Don't overwrite a route-set cancel/pause with `error` (error also holds the asset, so it + # would re-open the double-release class): only fail a job that is still RUNNING. + if job and job.status == JobStatus.RUNNING: + job.status = JobStatus.ERROR job.error = msg job.error_code = dlerrors.classify(msg) job.phase = None @@ -928,8 +939,10 @@ def _fail_edit(job_id: int, asset_id: int, msg: str) -> None: asset.status = "error" asset.error = msg job = db.get(DownloadJob, job_id) - if job: - job.status = "error" + # Only fail a job that is still RUNNING — never overwrite a route-set cancel/pause (see the + # download error path). + if job and job.status == JobStatus.RUNNING: + job.status = JobStatus.ERROR job.error = msg job.error_code = dlerrors.classify(msg) job.phase = None @@ -1008,15 +1021,18 @@ def _process_edit(job_id: int, asset_id: int) -> None: asset.last_access_at = datetime.now(timezone.utc) asset.expires_at = expires job = db.get(DownloadJob, job_id) - if job: - job.status = "done" + # Conditional terminal write (see the download path): don't resurrect a route-stopped job. + finalized = bool(job and job.status == JobStatus.RUNNING) + if finalized: + job.status = JobStatus.DONE job.progress = 100 job.phase = None # Capture owner + title before commit expires the rows (avoids a refresh SELECT). owner_id = job.user_id if job else None dl_title = asset.title if asset else None db.commit() - _notify_download_done(db, owner_id, dl_title, job_id, kind="edit") + if finalized: + _notify_download_done(db, owner_id, dl_title, job_id, kind="edit") log.info("edit job %s done: %s", job_id, rel) except editmod.EditAborted: diff --git a/backend/tests/test_worker_supervisor.py b/backend/tests/test_worker_supervisor.py index e4b66006..a01b85c2 100644 --- a/backend/tests/test_worker_supervisor.py +++ b/backend/tests/test_worker_supervisor.py @@ -88,7 +88,7 @@ def test_no_progress_stall_kills_and_fails_the_job(monkeypatch, rec): def test_route_pause_kills_but_keeps_the_route_status(monkeypatch, rec): - # A route flipped the job to 'paused'/'cancelled'. Kill + free the asset, but do NOT overwrite the + # A route flipped the job to 'paused'. Kill + free the asset, but do NOT overwrite the # status with an error and do NOT requeue. _spawn(monkeypatch, _FakeProc(["timeout"])) _liveness(monkeypatch, ("paused", 1.0)) @@ -98,6 +98,19 @@ def test_route_pause_kills_but_keeps_the_route_status(monkeypatch, rec): assert "set_job" not in names and "requeue" not in names +def test_route_cancel_kills_but_keeps_the_route_status(monkeypatch, rec): + # B-sub-1 regression: the cancel ROUTE writes the literal 'canceled' (models.py), but the supervisor + # used to compare against 'cancelled' — so this branch was DEAD and a canceled job's child ran on to + # completion, overwriting 'canceled' with 'done' and double-releasing its asset. The supervisor MUST + # treat the route's actual literal as a stop: kill + free the asset, no error write, no requeue. + _spawn(monkeypatch, _FakeProc(["timeout"])) + _liveness(monkeypatch, ("canceled", 1.0)) + worker._run_supervised(1, 99) + names = _names(rec) + assert "stop_child" in names and "reset_asset" in names + assert "set_job" not in names and "requeue" not in names + + def test_wall_clock_cap_kills_even_while_progressing(monkeypatch, rec): monkeypatch.setattr(worker.settings, "download_wall_clock_max_seconds", 0) _spawn(monkeypatch, _FakeProc(["timeout"])) From f39c5bb25ce00b8dc301862f3c668d09d95f7b19 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 00:21:59 +0200 Subject: [PATCH 02/22] fix(plex): never shrink cast_names on sync so a watermark reset can't regress it (B-sub-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _apply_facet_fields seeded cast_names from the cheap top-~20 listing only while people_enriched_at was NULL, relying on "a sync never clears the watermark" to protect an enriched fuller cast. Migration 0065 broke that assumption by nulling people_enriched_at on every movie/show row, so the first post-deploy sync downgraded enriched casts back to the listing top-20 — silently dropping person-page cast-filter names for weeks until the backfill caught up (the S5b regression, re-opened by the migration). Guard structurally instead of trusting the watermark: union the listing cast onto whatever is stored, never dropping a name already present. A fresh row still seeds from the listing; a row whose watermark was reset but whose fuller cast survives keeps it. Robust to any future watermark reset, not just 0065. --- backend/app/plex/sync.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/backend/app/plex/sync.py b/backend/app/plex/sync.py index acdb5e5a..7c298920 100644 --- a/backend/app/plex/sync.py +++ b/backend/app/plex/sync.py @@ -305,11 +305,17 @@ def _apply_facet_fields(row, meta: dict) -> None: row.genres = _tags(meta, "Genre") row.directors = _tags(meta, "Director") # The listing carries only the top ~20 cast, so re-applying it would DOWNGRADE an already-enriched - # full cast (up to 24, grown by the rich backfill/info-page enrich) — and since a sync never clears - # `people_enriched_at`, nothing would regrow it, silently breaking the person-page cast filter for - # names past the top ~20. Only take the listing's cast before the row is enriched; keep it after. + # full cast (up to 24, grown by the rich backfill/info-page enrich), silently breaking the + # person-page cast filter for names past the top ~20. The watermark gate below (`people_enriched_at + # is None`) normally protects this, but it is NOT enough on its own: a migration that resets the + # watermark re-opens the regression (0065 nulled it, which is exactly how the S5b fix got undone in + # prod). So guard STRUCTURALLY too — union the listing cast onto whatever is stored, never dropping a + # name we already have. On a fresh row (empty `cast_names`) this seeds the top ~20; on a row whose + # watermark was reset but whose fuller cast survives, it keeps the fuller cast. Robust to any future + # watermark reset, not just this one. if row.people_enriched_at is None: - row.cast_names = _tags(meta, "Role", limit=20) + merged = list(dict.fromkeys((row.cast_names or []) + (_tags(meta, "Role", limit=20) or []))) + row.cast_names = merged or None people = list(dict.fromkeys((row.directors or []) + (row.cast_names or []))) row.people_text = " ".join(people) or None From 67d32fecd60d5b4b919440718b2c275daf0b8c19 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 00:21:59 +0200 Subject: [PATCH 03/22] fix(search): return 422 not 502 on an upstream YouTube failure (B-core-1) /api/search/youtube raised 502 on an upstream error, but the frontend treats 502/503/504 as a gateway dropout: it auto-retries the GET (a second quota-costed search) and shows a "connection lost" banner instead of the real message. Every sibling upstream-error site already returns 422 for exactly this reason. Reserve 5xx for the raw byte-proxies. --- backend/app/routes/search.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/backend/app/routes/search.py b/backend/app/routes/search.py index c0e3d012..75131d2e 100644 --- a/backend/app/routes/search.py +++ b/backend/app/routes/search.py @@ -268,7 +268,11 @@ def search_youtube( except (YouTubeError, search_scrape.ScrapeError) as exc: if collected: break # keep what we already gathered - raise HTTPException(status_code=502, detail=f"YouTube search failed: {exc}") + # 422, not 502: this is a JSON endpoint, and the FE treats 502/503/504 as a gateway + # dropout (auto-retry + "connection lost" banner — lib/api/core RETRIABLE_GATEWAY), + # which hides the real message and burns a second quota-costed search. Upstream failure + # on a JSON endpoint = 422 + message; reserve 5xx for the raw byte-proxies. (B-core-1) + raise HTTPException(status_code=422, detail=f"YouTube search failed: {exc}") from exc # Drop currently-live/upcoming, channel-less, and blocked-channel results. candidates = [ From 7c431d73fbc763142906b1c8e9a42e55df613bdf Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 00:22:15 +0200 Subject: [PATCH 04/22] fix(config): degrade gracefully on a TOKEN_ENCRYPTION_KEY mismatch instead of a 500 (B-core-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rotated or missing TOKEN_ENCRYPTION_KEY makes every stored secret undecryptable. sysconfig._raw_override called security.decrypt() with no guard, and it sits on hot paths (/api/me via google_enabled, login, every YouTube scheduler job, account purge) — so a key mismatch raised InvalidToken and 500'd the whole app, the SPA wouldn't even boot, with no documented recovery. Catch the decrypt failure in _raw_override -> warn once per key and fall back to the env/config default. The YouTube client's refresh-token decrypt now raises a clear YouTubeError ("reconnect") that every caller already handles, and purge_user uses decrypt_optional so a best-effort revoke can't 500 the delete. Document that the key must never change once secrets are stored (recovery: clear the affected system_config rows and re-enter). --- backend/app/auth.py | 7 ++++--- backend/app/sysconfig.py | 30 ++++++++++++++++++++++++++++-- backend/app/youtube/client.py | 7 ++++++- docs/self-hosting.md | 5 ++++- 4 files changed, 42 insertions(+), 7 deletions(-) diff --git a/backend/app/auth.py b/backend/app/auth.py index 27f234a3..0cf9f50b 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -11,7 +11,7 @@ from app import sysconfig from app.config import settings from app.db import get_db from app.models import Invite, User -from app.security import decrypt +from app.security import decrypt_optional PASSWORD_MIN_LEN = 10 @@ -122,9 +122,10 @@ def purge_user(db: Session, user: User, background: BackgroundTasks) -> None: user_id = user.id tok = user.token # Both are stored encrypted — decrypt for the revoke call (the access-token fallback was passing - # ciphertext, so a token row with no refresh token silently failed to revoke). + # ciphertext, so a token row with no refresh token silently failed to revoke). decrypt_optional so a + # TOKEN_ENCRYPTION_KEY mismatch can't 500 the purge — a best-effort revoke degrades to "skip". (B-core-2) google_token = ( - (decrypt(tok.refresh_token_enc) or decrypt(tok.access_token)) if tok else None + (decrypt_optional(tok.refresh_token_enc) or decrypt_optional(tok.access_token)) if tok else None ) # Raw delete so Postgres applies ON DELETE CASCADE / SET NULL on every dependent table. db.execute(delete(User).where(User.id == user_id)) diff --git a/backend/app/sysconfig.py b/backend/app/sysconfig.py index 1b2b6eef..3c531e69 100644 --- a/backend/app/sysconfig.py +++ b/backend/app/sysconfig.py @@ -7,6 +7,7 @@ bounds, and whether it's a secret. The resolver returns the DB override if one i stored Fernet-encrypted at rest (requires TOKEN_ENCRYPTION_KEY; without it, secrets can't be stored in the DB and stay env-only). """ +import logging from dataclasses import dataclass from sqlalchemy.orm import Session @@ -15,6 +16,12 @@ from app import security from app.config import settings from app.models import SystemConfig +log = logging.getLogger("siftlode.config") + +# Config keys whose stored ciphertext failed to decrypt — tracked so the warning is logged once per +# key, not once per request (bounded by the number of secret config keys). +_decrypt_warned: set[str] = set() + @dataclass(frozen=True) class ConfigSpec: @@ -118,11 +125,30 @@ def _coerce(s: ConfigSpec, raw: str): def _raw_override(db: Session, key: str) -> str | None: - """Stored (decrypted) override string for `key`, or None if no override is set.""" + """Stored (decrypted) override string for `key`, or None if no override is set. + + A decrypt failure — a rotated or removed ``TOKEN_ENCRYPTION_KEY`` vs the key the value was written + with — must NOT crash the caller. `_raw_override` sits on hot paths (`/api/me` via `google_enabled`, + login, every YouTube scheduler job, account purge), so a raw ``InvalidToken`` here would 500 the + whole app and the SPA wouldn't even boot. Fall back to the env/config default instead, and warn once + per key so the operator gets a real signal without a per-request traceback. Recovery is documented in + docs/self-hosting.md: the key must never change once secrets are stored. (B-core-2) + """ row = db.get(SystemConfig, key) if row is None or row.value is None: return None - return security.decrypt(row.value) if row.encrypted else row.value + if not row.encrypted: + return row.value + try: + return security.decrypt(row.value) + except Exception: # noqa: BLE001 — Fernet InvalidToken / malformed ciphertext (key mismatch) + if key not in _decrypt_warned: + _decrypt_warned.add(key) + log.warning( + "config %r could not be decrypted (TOKEN_ENCRYPTION_KEY changed or missing?) — falling " + "back to the env/config default; the key must not change once secrets are stored", key, + ) + return None def get(db: Session, key: str): diff --git a/backend/app/youtube/client.py b/backend/app/youtube/client.py index 0ccf64a7..19f6c537 100644 --- a/backend/app/youtube/client.py +++ b/backend/app/youtube/client.py @@ -79,7 +79,12 @@ class YouTubeClient: cached = decrypt_optional(tok.access_token) if cached and tok.expiry and tok.expiry > now + timedelta(seconds=60): return cached - refresh = decrypt(tok.refresh_token_enc) + try: + refresh = decrypt(tok.refresh_token_enc) + except Exception as exc: # noqa: BLE001 — Fernet InvalidToken (TOKEN_ENCRYPTION_KEY changed) + # A key mismatch must degrade like any other token problem (caller handles YouTubeError), + # not surface as a raw 500 on every scheduler job / connect check. (B-core-2) + raise YouTubeError("Stored OAuth token is unreadable — reconnect your account") from exc if not refresh: raise YouTubeError("No refresh token; user must re-authenticate") resp = self._send( diff --git a/docs/self-hosting.md b/docs/self-hosting.md index 05a9ab36..9cd7e2b6 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -41,7 +41,10 @@ The installer asks for the **public URL** where the instance will be reached (ju `http://localhost:8080` to try it locally). It then: - generates the secrets it needs (`SECRET_KEY`, `TOKEN_ENCRYPTION_KEY`, a database password) into a - local `.env` file — keep that file private, + local `.env` file — keep that file private. **Do not change `TOKEN_ENCRYPTION_KEY` once the app has + stored any secret** (Google credentials, API keys, SMTP password, Plex token): those values are + encrypted with it, and a changed key makes them unreadable. If it is ever lost or rotated, clear the + affected rows from the `system_config` table and re-enter the secrets in the admin Configuration page, - pulls the image and starts the app + database, - prints the **setup wizard URL**, which looks like `…/setup?token=…`. From 9845c8d0f77fa3e23037cc4c1960d1f4e343fe29 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 00:22:15 +0200 Subject: [PATCH 05/22] fix(auth): epoch-check the target on account switch to avoid a full wallet logout (B-core-9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /api/me/switch validated wallet membership, access and suspension but not the target account's session epoch. Switching to an account revoked elsewhere (a password reset/change or "log out everywhere" bumped its epoch) made it the session default, so the next request's current_user hit the epoch mismatch on the default identity and cleared the WHOLE wallet session — logging out every account in the browser. Mirror the suspend guard: drop the stale account from the switch list and return 403 instead of switching. --- backend/app/routes/me.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index 5d683d5c..458e2473 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -72,6 +72,18 @@ def switch_account( # Don't make a suspended account the active one — the next request's current_user would # see the suspension and clear the WHOLE wallet session, logging out every account here. raise HTTPException(status_code=403, detail="That account is suspended.") + # Same reasoning for a REVOKED session (a password reset/change or "log out everywhere" bumped the + # account's epoch): making it the session default would trip current_user's epoch check on the next + # request and clear the whole wallet. Mirror the suspend guard — drop the stale account from the + # switch list and refuse, rather than logging everyone out. (B-core-9) + epochs = request.session.get("epochs") or {} + if int(epochs.get(str(target), 0)) != (u.session_epoch or 0): + request.session["account_ids"] = [ + a for a in (request.session.get("account_ids") or []) if a != target + ] + raise HTTPException( + status_code=403, detail="That account was signed out elsewhere — sign in again." + ) request.session["user_id"] = target return {"ok": True, "user_id": target} From 4ce0a702f481908a352aa796cee7ed922beed66a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 00:58:13 +0200 Subject: [PATCH 06/22] refactor(downloads): route worker terminal writes through an atomic finalize_if_running (B-sub-1 review) Follow-up to the /code-review high on the S1 branch. The per-site `if job.status == 'running'` guards were a non-atomic read-then-write (TOCTOU): a cancel committing between the child's status read and its commit could still be overwritten with done/error, re-opening the double-release window. Introduce service.finalize_if_running(db, job_id, **fields): one atomic `UPDATE download_jobs SET ... WHERE status='running'` that returns whether it fired. It flushes pending asset writes first so the asset-then-job row-lock order matches the cancel route (no deadlock), and runs in the caller's session so it commits with the asset writes in one transaction. All five worker sites (download/edit completion, both error paths, cache finish) now go through it, so the invariant lives in one place. The 'job done' logs move inside the finalized branch so a job stopped mid-completion no longer logs a false 'done'. Add tests/test_downloads_finalize.py: running -> finalizes; canceled/paused -> no-op. --- backend/app/downloads/service.py | 31 +++++++++- backend/app/worker.py | 76 ++++++++++-------------- backend/tests/test_downloads_finalize.py | 75 +++++++++++++++++++++++ 3 files changed, 135 insertions(+), 47 deletions(-) create mode 100644 backend/tests/test_downloads_finalize.py diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index ad601cf6..70c6306e 100644 --- a/backend/app/downloads/service.py +++ b/backend/app/downloads/service.py @@ -8,7 +8,7 @@ the worker live in `worker.py` / the routes; this module owns enqueue + asset bo """ from datetime import datetime, timezone -from sqlalchemy import func, select +from sqlalchemy import func, select, text from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session @@ -16,7 +16,7 @@ from app.config import settings from app.downloads import adapters from app.downloads import edit as editmod from app.downloads import formats, quota, storage -from app.downloads.status import HOLDING +from app.downloads.status import HOLDING, JobStatus from app.models import Channel, DownloadJob, MediaAsset, Video @@ -224,6 +224,33 @@ def enqueue_edit( return job +def finalize_if_running(db: Session, job_id: int, **fields) -> bool: + """Write terminal `fields` to a job in ONE atomic ``UPDATE ... WHERE status='running'`` and report + whether it fired. This is the single owner of the invariant "a finishing worker child must never + resurrect a job a route just stopped": the atomic WHERE closes the read-then-write race an + ``if job.status == 'running'`` guard leaves open, so a concurrent cancel/pause can no longer be + overwritten with done/error (which would re-open the double-release → file-delete-under-live-holder + class, since `done`/`error` hold the asset but `canceled` does not — see release_asset). + + Runs in the CALLER's session and does NOT commit — the caller commits it together with its asset + writes in one transaction. It flushes first so any pending asset write takes its row lock BEFORE + this takes the job's, matching the cancel route's asset-then-job order (no deadlock). `fields` keys + are internal column names (never user input). Returns True when the row was still running and got + updated, False when a route had already moved it out of 'running'.""" + if not fields: + return False + db.flush() # asset-then-job lock order: push pending asset writes before we lock the job row + # StrEnum members carry through psycopg as strings, but coerce explicitly so a bound status value + # is always the plain literal the column stores. + params = {k: (v.value if isinstance(v, JobStatus) else v) for k, v in fields.items()} + assignments = ", ".join(f"{col} = :{col}" for col in fields) + result = db.execute( + text(f"UPDATE download_jobs SET {assignments} WHERE id = :_job_id AND status = :_running"), + {**params, "_job_id": job_id, "_running": JobStatus.RUNNING.value}, + ) + return result.rowcount > 0 + + def release_asset(db: Session, job: DownloadJob) -> None: """Drop this job's hold on its asset when it leaves a holding state. Once NO job holds the asset anymore, delete the file + row immediately — a deleted download should free its disk, diff --git a/backend/app/worker.py b/backend/app/worker.py index d7624203..349560ad 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -482,17 +482,15 @@ def _finish_from_cache(job_id: int, asset_id: int) -> None: with SessionLocal() as db: job = db.get(DownloadJob, job_id) asset = db.get(MediaAsset, asset_id) - # Only finalize a job the route hasn't stopped: the flip is conditional on the row still being - # RUNNING so a concurrent cancel/pause is never resurrected to `done` (which would break the - # no-double-release invariant in service.release_asset — see downloads/status.py). - if job and asset and job.status == JobStatus.RUNNING: - job.status = JobStatus.DONE - job.progress = 100 - job.phase = None + if job and asset: + fields = {"status": JobStatus.DONE, "progress": 100, "phase": None} if not job.display_name: # display_name is VARCHAR(255); a title can be longer (e.g. a recipe in the title). - job.display_name = asset.title[:255] if asset.title else None + fields["display_name"] = asset.title[:255] if asset.title else None asset.last_access_at = datetime.now(timezone.utc) + # Atomic finalize: only marks the job done while it is still running, so a concurrent + # cancel/pause is never resurrected (see service.finalize_if_running). + service.finalize_if_running(db, job_id, **fields) db.commit() @@ -872,25 +870,20 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe asset.last_access_at = datetime.now(timezone.utc) asset.expires_at = expires job = db.get(DownloadJob, job_id) - # Conditional terminal write: only finalize while the row is still RUNNING, so a child that - # finishes AFTER a route cancel/pause (narrow window past the cooperative checkpoints) can't - # overwrite `canceled`/`paused` with `done` and re-open the double-release class fixed here. - finalized = bool(job and job.status == JobStatus.RUNNING) - if finalized: - job.status = JobStatus.DONE - job.progress = 100 - job.phase = None - job.speed_bps = None - job.eta_s = None - if not job.display_name: - # display_name is VARCHAR(255) — clip a long title (see _fill_asset_meta_early). - job.display_name = meta.title[:255] if meta.title else None # Capture the owner before commit expires the row (avoids a refresh SELECT just to notify). owner_id = job.user_id if job else None + fields = {"status": JobStatus.DONE, "progress": 100, "phase": None, "speed_bps": None, + "eta_s": None} + if job and not job.display_name and meta.title: + # display_name is VARCHAR(255) — clip a long title (see _fill_asset_meta_early). + fields["display_name"] = meta.title[:255] + # Atomic terminal write: finalizes ONLY while the row is still running, so a child that + # finishes just after a route cancel/pause can't resurrect it (see finalize_if_running). + finalized = service.finalize_if_running(db, job_id, **fields) db.commit() if finalized: _notify_download_done(db, owner_id, meta.title, job_id, kind="download") - log.info("job %s done: %s", job_id, rel) + log.info("job %s done: %s", job_id, rel) except (_Aborted, editmod.EditAborted): # Paused/canceled mid-download or mid-transcode: free the asset for a later retry; keep the @@ -904,14 +897,12 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe if asset: asset.status = "error" asset.error = msg - job = db.get(DownloadJob, job_id) - # Don't overwrite a route-set cancel/pause with `error` (error also holds the asset, so it - # would re-open the double-release class): only fail a job that is still RUNNING. - if job and job.status == JobStatus.RUNNING: - job.status = JobStatus.ERROR - job.error = msg - job.error_code = dlerrors.classify(msg) - job.phase = None + # Atomic fail: only while still running, so a route-set cancel/pause is never overwritten + # with `error` (error also holds the asset → would re-open the double-release class). + service.finalize_if_running( + db, job_id, status=JobStatus.ERROR, error=msg, + error_code=dlerrors.classify(msg), phase=None, + ) db.commit() log.warning("job %s failed: %s", job_id, msg) finally: @@ -938,14 +929,11 @@ def _fail_edit(job_id: int, asset_id: int, msg: str) -> None: if asset: asset.status = "error" asset.error = msg - job = db.get(DownloadJob, job_id) - # Only fail a job that is still RUNNING — never overwrite a route-set cancel/pause (see the - # download error path). - if job and job.status == JobStatus.RUNNING: - job.status = JobStatus.ERROR - job.error = msg - job.error_code = dlerrors.classify(msg) - job.phase = None + # Atomic fail: only while still running (see the download error path). + service.finalize_if_running( + db, job_id, status=JobStatus.ERROR, error=msg, + error_code=dlerrors.classify(msg), phase=None, + ) db.commit() log.warning("edit job %s failed: %s", job_id, msg) @@ -1021,19 +1009,17 @@ def _process_edit(job_id: int, asset_id: int) -> None: asset.last_access_at = datetime.now(timezone.utc) asset.expires_at = expires job = db.get(DownloadJob, job_id) - # Conditional terminal write (see the download path): don't resurrect a route-stopped job. - finalized = bool(job and job.status == JobStatus.RUNNING) - if finalized: - job.status = JobStatus.DONE - job.progress = 100 - job.phase = None # Capture owner + title before commit expires the rows (avoids a refresh SELECT). owner_id = job.user_id if job else None dl_title = asset.title if asset else None + # Atomic terminal write (see the download path): don't resurrect a route-stopped job. + finalized = service.finalize_if_running( + db, job_id, status=JobStatus.DONE, progress=100, phase=None + ) db.commit() if finalized: _notify_download_done(db, owner_id, dl_title, job_id, kind="edit") - log.info("edit job %s done: %s", job_id, rel) + log.info("edit job %s done: %s", job_id, rel) except editmod.EditAborted: # Paused/canceled mid-encode: free the asset for a later retry (ref_count is the route's job). diff --git a/backend/tests/test_downloads_finalize.py b/backend/tests/test_downloads_finalize.py new file mode 100644 index 00000000..b4910c44 --- /dev/null +++ b/backend/tests/test_downloads_finalize.py @@ -0,0 +1,75 @@ +"""DB-backed regression tests for `service.finalize_if_running` (B-sub-1). + +The worker's terminal writes go through this helper so a child that finishes just after a route +cancel/pause can NEVER flip the job back to done/error — which would re-open the double-release -> +file-delete-under-live-holder class (done/error hold the asset, canceled does not). The guard is an +atomic `UPDATE ... WHERE status='running'`, so these tests assert it fires exactly when the job is +still running and is a no-op otherwise. Uses the real-Postgres `db` fixture; skipped without a DB. +""" +from sqlalchemy import text + +from app.downloads import service +from app.downloads.status import JobStatus +from app.models import DownloadJob, User + + +def _running_job(db) -> DownloadJob: + user = User(email="dl@example.com") + db.add(user) + db.flush() + job = DownloadJob( + user_id=user.id, + source_kind="youtube", + source_ref="x" * 11, + status=JobStatus.RUNNING.value, + queue_pos=1, + profile_snapshot={}, + ) + db.add(job) + db.commit() + return job + + +def _row(db, job_id): + # Read straight from the DB: SessionLocal uses expire_on_commit=False, so the ORM identity-map + # copy is stale after finalize_if_running's raw UPDATE + commit. + return db.execute( + text("SELECT status, progress FROM download_jobs WHERE id = :id"), {"id": job_id} + ).one() + + +def test_finalize_marks_a_running_job_done_and_reports_it(db): + job = _running_job(db) + won = service.finalize_if_running(db, job.id, status=JobStatus.DONE, progress=100, phase=None) + db.commit() + assert won is True + status, progress = _row(db, job.id) + assert status == "done" + assert progress == 100 + + +def test_finalize_is_a_noop_on_a_canceled_job(db): + # The exact B-sub-1 scenario: a route already canceled the job (released its asset once), and a + # finishing child then tries to write `done`. The guard must NOT resurrect it, or a later delete + # would double-release the asset. + job = _running_job(db) + db.execute( + text("UPDATE download_jobs SET status = 'canceled' WHERE id = :id"), {"id": job.id} + ) + db.commit() + won = service.finalize_if_running(db, job.id, status=JobStatus.DONE, progress=100) + db.commit() + assert won is False + assert _row(db, job.id).status == "canceled" + + +def test_finalize_is_a_noop_on_a_paused_job(db): + job = _running_job(db) + db.execute( + text("UPDATE download_jobs SET status = 'paused' WHERE id = :id"), {"id": job.id} + ) + db.commit() + won = service.finalize_if_running(db, job.id, status=JobStatus.ERROR, error="boom", phase=None) + db.commit() + assert won is False + assert _row(db, job.id).status == "paused" From 0653203d81010f0695ea22172817d90d4f0f34f5 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 00:58:13 +0200 Subject: [PATCH 07/22] fix(plex): make the cast gate self-correcting as well as non-shrinking (B-sub-2 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The union form fixed the downgrade but removed the per-sync self-healing: a name Plex removed from an unenriched title lingered in cast_names/people_text until enrichment. Replace the union with a length guard — take the fresh listing unless it is shorter than what is stored. A stored cast longer than the ~20-cap listing can only be an enriched fuller list (kept, so a watermark reset never downgrades it); at or below the cap the stored value is itself listing-sourced, so replacing it with the current listing stays self-correcting. --- backend/app/plex/sync.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/backend/app/plex/sync.py b/backend/app/plex/sync.py index 7c298920..72ccbab1 100644 --- a/backend/app/plex/sync.py +++ b/backend/app/plex/sync.py @@ -309,13 +309,15 @@ def _apply_facet_fields(row, meta: dict) -> None: # person-page cast filter for names past the top ~20. The watermark gate below (`people_enriched_at # is None`) normally protects this, but it is NOT enough on its own: a migration that resets the # watermark re-opens the regression (0065 nulled it, which is exactly how the S5b fix got undone in - # prod). So guard STRUCTURALLY too — union the listing cast onto whatever is stored, never dropping a - # name we already have. On a fresh row (empty `cast_names`) this seeds the top ~20; on a row whose - # watermark was reset but whose fuller cast survives, it keeps the fuller cast. Robust to any future - # watermark reset, not just this one. + # prod). So guard STRUCTURALLY too, by LENGTH: take the fresh listing UNLESS it is shorter than what + # is stored. A stored cast longer than the ~20-cap listing can only be an enriched fuller list, so + # keep it (robust to any watermark reset); at or below the cap the stored value is itself + # listing-sourced, so replacing it with the current listing stays self-correcting when Plex edits + # the cast (a pure union never would). if row.people_enriched_at is None: - merged = list(dict.fromkeys((row.cast_names or []) + (_tags(meta, "Role", limit=20) or []))) - row.cast_names = merged or None + listing_cast = _tags(meta, "Role", limit=20) + if len(row.cast_names or []) <= len(listing_cast or []): + row.cast_names = listing_cast people = list(dict.fromkeys((row.directors or []) + (row.cast_names or []))) row.people_text = " ".join(people) or None From 15a59f4695946bd9098e7bdf3a444595f9fab2cb Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 01:23:06 +0200 Subject: [PATCH 08/22] test(infra): fail-loud DB/ffmpeg lane + client/as_user/make_user fixtures (E1 S2, TEST-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DB-backed lane and the two ffmpeg tests skipped SILENTLY when the DB was unreachable or ffmpeg was absent, so `siftlode check` could print green while ~60 tests never ran. Make the compose lane authoritative: - SIFTLODE_TEST_REQUIRE_DB=1 in docker-compose.test.yml; conftest.pytest_configure turns a would-be skip (no Postgres, or no ffmpeg) into a hard UsageError. - install ffmpeg in Dockerfile.test so the thumbnail/poster tests actually run. - `-rs` in pytest.ini so skip reasons are always visible. Add the shared TestClient fixtures the risk-core suites build on: `make_user` (create + commit a login-ready User), `client` (real auth/session stack, no current_user override — for login/wallet/epoch), and `as_user` (a client pre-authenticated as a given user, for tests that aren't exercising login). --- backend/Dockerfile.test | 6 +++ backend/docker-compose.test.yml | 3 ++ backend/pytest.ini | 6 ++- backend/tests/conftest.py | 81 ++++++++++++++++++++++++++++++++- 4 files changed, 93 insertions(+), 3 deletions(-) diff --git a/backend/Dockerfile.test b/backend/Dockerfile.test index 6bffc2f8..03f3709b 100644 --- a/backend/Dockerfile.test +++ b/backend/Dockerfile.test @@ -8,5 +8,11 @@ # docker run --rm -v "/backend:/app" -w /app siftlode-test:local python -m pytest FROM python:3.12-slim WORKDIR /app +# ffmpeg so the thumbnail/poster tests (needs_ffmpeg) actually run in the gate instead of skipping +# silently — the runtime image already ships ffmpeg, so the test image should too (see conftest's +# SIFTLODE_TEST_REQUIRE_DB, which now also refuses to run if ffmpeg is missing). +RUN apt-get update \ + && apt-get install -y --no-install-recommends ffmpeg \ + && rm -rf /var/lib/apt/lists/* COPY requirements.txt requirements-dev.txt ./ RUN pip install --no-cache-dir -r requirements.txt -r requirements-dev.txt diff --git a/backend/docker-compose.test.yml b/backend/docker-compose.test.yml index 7a140827..95d6ee95 100644 --- a/backend/docker-compose.test.yml +++ b/backend/docker-compose.test.yml @@ -29,6 +29,9 @@ services: condition: service_healthy environment: DATABASE_URL: postgresql+psycopg://siftlode:siftlode@testdb:5432/siftlode_test + # This lane HAS a Postgres + ffmpeg, so the DB/ffmpeg tests must actually run here: conftest turns + # a would-be silent skip into a hard failure (SIFTLODE_TEST_REQUIRE_DB). + SIFTLODE_TEST_REQUIRE_DB: "1" volumes: - ./:/app working_dir: /app diff --git a/backend/pytest.ini b/backend/pytest.ini index d052618b..6692f5b6 100644 --- a/backend/pytest.ini +++ b/backend/pytest.ini @@ -1,6 +1,8 @@ [pytest] # Run from backend/ (the image's /app). `pythonpath = .` puts the app package on sys.path so tests -# import it as `app.*`, matching runtime. The suite is pure-logic only (no DB/network) — see tests/. +# import it as `app.*`, matching runtime. Most of the suite is pure-logic; the DB-backed lane opts in +# via the `db` fixture and runs in the compose test image. `-rs` reports skip REASONS so a silently +# skipped lane is visible (the compose lane sets SIFTLODE_TEST_REQUIRE_DB=1 to make skips a hard error). testpaths = tests pythonpath = . -addopts = -q +addopts = -q -rs diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 8fa4ec87..268bd229 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -12,14 +12,26 @@ config the videos.search_vector generated column needs). Building it via migrati that the migration chain applies cleanly. Each test then gets a clean database via TRUNCATE so tests that COMMIT (the whole point of this lane — transaction-ownership behaviour) don't leak. """ +import os +import shutil + import pytest from alembic import command from alembic.config import Config +from fastapi.testclient import TestClient from sqlalchemy import create_engine, text import app.models # noqa: F401 — register every model on Base.metadata +from app import security +from app.auth import current_user from app.config import settings -from app.db import Base, SessionLocal, engine +from app.db import Base, SessionLocal, engine, get_db +from app.main import app +from app.models import User + +# The compose test lane sets this so a would-be silent skip (no DB, or no ffmpeg) becomes a hard error +# — the gate must not print green while the DB-backed / ffmpeg tests were quietly not run. +_REQUIRE_FULL = os.getenv("SIFTLODE_TEST_REQUIRE_DB") == "1" def _db_reachable() -> bool: @@ -59,6 +71,23 @@ def _truncate_all(): conn.execute(text(f"TRUNCATE {tables} RESTART IDENTITY CASCADE")) +def pytest_configure(config): + """Fail the whole run — loudly, once — when SIFTLODE_TEST_REQUIRE_DB=1 but the environment can't + actually run the full suite. Without this, an unreachable DATABASE_URL or a missing ffmpeg turns + ~60 DB tests + the poster tests into silent skips and the gate still exits green.""" + if not _REQUIRE_FULL: + return + if not _DB_REACHABLE: + raise pytest.UsageError( + "SIFTLODE_TEST_REQUIRE_DB=1 but no Postgres is reachable at the test DATABASE_URL — the " + "DB-backed lane would silently skip. Fix the DB wiring or unset the flag." + ) + if shutil.which("ffmpeg") is None: + raise pytest.UsageError( + "SIFTLODE_TEST_REQUIRE_DB=1 but ffmpeg is missing — the thumbnail/poster tests would skip." + ) + + @pytest.fixture def db(_schema): """A real session against the test Postgres, on a clean (empty) database. The code under test @@ -70,3 +99,53 @@ def db(_schema): finally: session.rollback() session.close() + + +@pytest.fixture +def make_user(db): + """Factory: create + commit a User in the test DB. Defaults produce a login-ready account (email + verified, active, with a known password); override any of it via kwargs (e.g. `role="admin"`, + `is_suspended=True`, `password=None` for a Google-only account). Returns the User.""" + seq = {"n": 0} + + def _make(email: str | None = None, *, password: str | None = "correct horse battery", **kw): + seq["n"] += 1 + kw.setdefault("email_verified", True) + user = User( + email=email or f"user{seq['n']}@example.com", + password_hash=security.hash_password(password) if password else None, + **kw, + ) + db.add(user) + db.commit() + return user + + return _make + + +@pytest.fixture +def client(db): + """A TestClient wired to the test DB that goes through the REAL auth/session stack — no + `current_user` override — so login, the wallet gate and epoch revoke are exercised end to end. + Starlette's SessionMiddleware round-trips the signed cookie, so a login in one call authenticates + the next on this same instance. No lifespan (the scheduler / HLS sweep stay off).""" + app.dependency_overrides[get_db] = lambda: db + try: + yield TestClient(app) + finally: + app.dependency_overrides.clear() + + +@pytest.fixture +def as_user(db): + """Factory -> a TestClient already authenticated as a given User (via a `current_user` override), + for tests that need an authed request but aren't exercising the login flow itself.""" + def _as(user: User) -> TestClient: + app.dependency_overrides[get_db] = lambda: db + app.dependency_overrides[current_user] = lambda: user + return TestClient(app) + + try: + yield _as + finally: + app.dependency_overrides.clear() From 31879b01f4abd06d3f7b6d7a605665c1de7c05e6 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 01:23:07 +0200 Subject: [PATCH 09/22] test(migrations): model<->schema drift + downgrade/re-upgrade (E1 S2, TEST-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prod only ever runs `alembic upgrade head`; nothing checks that the migrated schema still matches Base.metadata, and the downgrade path never runs. Add a DB-lane test that (a) asserts no structural (table/column) drift between the ORM models and the head schema — filtering the index/constraint/sequence noise that compare_metadata reports for migration-only objects — and (b) exercises the head migration's downgrade + re-upgrade so a broken downgrade is caught the sprint it lands. --- backend/tests/test_migrations.py | 59 ++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 backend/tests/test_migrations.py diff --git a/backend/tests/test_migrations.py b/backend/tests/test_migrations.py new file mode 100644 index 00000000..373e4413 --- /dev/null +++ b/backend/tests/test_migrations.py @@ -0,0 +1,59 @@ +"""Migration hygiene (TEST-3), DB-lane. + +Two things the prod boot never checks: +- **model <-> schema drift:** `entrypoint.sh` only ever runs `alembic upgrade head`; nothing verifies the + migrated schema still matches `Base.metadata`. A model column added without a migration (or a migration + that drifted from the model) ships silently. +- **the downgrade path:** every migration defines `downgrade()`, but it never runs, so a broken downgrade + is only discovered during an emergency rollback. + +Both use the `db` fixture, which has already brought the schema to head via `_schema`. + +Scope note: we assert on STRUCTURAL drift only — tables and columns present in the model but not the +schema, or vice versa. `compare_metadata` also reports index/constraint/sequence/type differences, but +those are dominated by migration-only objects (partial/expression/GIN indexes, CHECK constraints, SERIAL +sequences) that can't be expressed on `Base.metadata`, so comparing them wholesale is noise. Missing/extra +columns and tables — the "someone changed a model without a migration" case — are the signal worth gating. +""" +from alembic import command +from alembic.autogenerate import compare_metadata +from alembic.config import Config +from alembic.migration import MigrationContext + +from app.db import Base, engine + +# The diff ops that mean a real table/column is present on one side and not the other. +_DRIFT_OPS = {"add_table", "remove_table", "add_column", "remove_column"} + + +def _structural_drift(): + """Table/column presence diffs between the ORM models and the migrated schema (`compare_type=True` + mirrors alembic/env.py). compare_metadata nests per-column ops in a sub-list per table and returns + table-level ops as bare tuples; flatten both and keep only the presence ops.""" + with engine.connect() as conn: + ctx = MigrationContext.configure(conn, opts={"compare_type": True}) + diffs = compare_metadata(ctx, Base.metadata) + drift = [] + for d in diffs: + for entry in (d if isinstance(d, list) else [d]): + if isinstance(entry, tuple) and entry and entry[0] in _DRIFT_OPS: + drift.append(entry) + return drift + + +def test_models_match_the_migrated_schema(db): + # Non-empty == the ORM models and the alembic-built schema disagree on a table/column — almost + # always a model change that forgot its migration. + drift = _structural_drift() + assert drift == [], "model/schema drift — add or fix a migration:\n" + "\n".join( + str(d) for d in drift + ) + + +def test_head_migration_downgrades_and_reupgrades(db): + # Exercise the newest migration's downgrade + re-upgrade so a broken downgrade is caught the sprint + # it lands, not during a rollback. Leaves the schema back at head for the rest of the session. + cfg = Config("alembic.ini") + command.downgrade(cfg, "-1") + command.upgrade(cfg, "head") + assert _structural_drift() == [] From 66c00327b1e0b185d20d1d92089521b0fe01c19e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 01:23:07 +0200 Subject: [PATCH 10/22] test(feed): keyset paging reproduces the unpaged order for every sort (E1 S2, TEST-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The feed's cursor paging (sort_keys/order_by/keyset_predicate/encode/decode) is the NULLS-LAST + tiebreaker algebra where a page-boundary bug hides as a duplicated or skipped row on the busiest endpoint, and had zero tests. Seed 7 videos on 2 channels with duplicate sort values and NULLs across published_at/view_count/duration/title/subscriber_count, then assert — for every sort — that walking the feed in pages of 2 via the cursor reproduces exactly the order a single unpaged query returns, with no duplicates. --- backend/tests/test_feed_keyset.py | 106 ++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 backend/tests/test_feed_keyset.py diff --git a/backend/tests/test_feed_keyset.py b/backend/tests/test_feed_keyset.py new file mode 100644 index 00000000..e44e5a01 --- /dev/null +++ b/backend/tests/test_feed_keyset.py @@ -0,0 +1,106 @@ +"""Feed keyset paging (TEST-2), DB-lane. + +The feed's cursor paging (feedquery.sort_keys / order_by / keyset_predicate / encode_cursor / +decode_cursor) is the NULLS-LAST + tiebreaker algebra where a page boundary bug hides as a duplicated or +skipped row — on the busiest endpoint, while the scheduler ingests mid-scroll. It had zero tests. + +The invariant here is strong and order-agnostic: for EVERY sort, walking the feed in pages of 2 via the +cursor must reproduce EXACTLY the order a single unpaged query returns — same rows, same order, no dup, no +skip. The seed is built to stress it: duplicate sort values (so the id tiebreaker is load-bearing) and +NULLs in published_at / view_count / duration_seconds / title / subscriber_count (so nulls_last is +exercised on both a first and a second key). (The malformed-cursor -> 400 path is covered by +test_feed_cursor.py.) +""" +from datetime import datetime, timezone + +import pytest + +import app.state as state +from app.models import Channel, Subscription, User, Video + +# Every sort feedquery.sort_keys defines except "relevance" (which needs a search query to rank). +_SORTS = [ + "newest", "oldest", "views", "views_asc", "duration_desc", "duration_asc", + "title", "title_desc", "subscribers", "subscribers_asc", "priority", "priority_asc", "shuffle", +] + + +def _dt(day: int) -> datetime: + return datetime(2024, 1, day, tzinfo=timezone.utc) + + +def _seed(db): + # Two channels; one has a NULL subscriber_count (exercises the subscribers sort's nulls_last). + db.add_all([ + Channel(id="UCa", title="Channel A", subscriber_count=1000), + Channel(id="UCb", title="Channel B", subscriber_count=None), + ]) + user = User(email="keyset@example.com", email_verified=True) + db.add(user) + db.flush() + db.add_all([ + Subscription(user_id=user.id, channel_id="UCa", priority=5, hidden=False), + Subscription(user_id=user.id, channel_id="UCb", priority=0, hidden=False), + ]) + # 7 normal/organic videos. Deliberate duplicates (published_at day 1 x2 and day 3 x2, view_count + # 100 x2, duration 60 x2 / 30 x2) and NULLs spread across the sort columns. + rows = [ + # id ch published views dur title + ("vid00", "UCa", _dt(1), 100, 60, "alpha"), + ("vid01", "UCa", _dt(1), 100, 60, "beta"), + ("vid02", "UCa", _dt(2), None, None, "gamma"), + ("vid03", "UCb", None, 50, 120, None), + ("vid04", "UCb", _dt(3), 200, 30, "delta"), + ("vid05", "UCb", _dt(3), None, 30, "epsilon"), + ("vid06", "UCa", _dt(28), None, None, "zeta"), + ] + db.add_all([ + Video( + id=vid, channel_id=ch, published_at=pub, view_count=views, duration_seconds=dur, + title=title, is_short=False, live_status="none", via_search=False, via_explore=False, + ) + for vid, ch, pub, views, dur, title in rows + ]) + db.commit() + return user, [r[0] for r in rows] + + +def _page(client, sort, cursor): + url = f"/api/feed?sort={sort}&limit=2" + if cursor: + url += f"&cursor={cursor}" + resp = client.get(url) + assert resp.status_code == 200, resp.text + return resp.json() + + +def test_feed_surfaces_the_whole_seed(db, as_user, monkeypatch): + monkeypatch.setattr(state, "_configured_cache", True) # the request gate 503s an unconfigured app + user, ids = _seed(db) + body = as_user(user).get("/api/feed?limit=50").json() + assert {it["id"] for it in body["items"]} == set(ids) + assert body["has_more"] is False and body["next_cursor"] is None + + +@pytest.mark.parametrize("sort", _SORTS) +def test_cursor_walk_matches_the_unpaged_order(db, as_user, monkeypatch, sort): + monkeypatch.setattr(state, "_configured_cache", True) + user, _ = _seed(db) + client = as_user(user) + + # Ground truth: the full order in one query (no cursor). + full = [it["id"] for it in client.get(f"/api/feed?sort={sort}&limit=50").json()["items"]] + + # Walk it two at a time via the cursor and reassemble. + walked: list[str] = [] + cursor = None + for _ in range(len(full) + 2): # generous upper bound; the loop breaks on the last page + body = _page(client, sort, cursor) + walked.extend(it["id"] for it in body["items"]) + cursor = body["next_cursor"] + if not body["has_more"]: + assert cursor is None + break + + assert walked == full, f"{sort}: paged order diverged from the unpaged query" + assert len(walked) == len(set(walked)), f"{sort}: cursor paging duplicated a row" From 296af2a498b4f02deda129ff1be049238faa6d28 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 01:23:07 +0200 Subject: [PATCH 11/22] test(auth): session/epoch/wallet/token security core (E1 S2, TEST-1) The auth security mag had zero tests. Add a DB-lane suite over the real auth/session stack (TestClient carries the signed cookie): password login -> /api/me authenticated; a wrong password and an unknown email return a byte-identical 401 (no enumeration oracle); an X-Siftlode-Account header naming a non-wallet account falls back to the default (impersonation gate); logout-others bumps the epoch so a second session is rejected (401 on a protected endpoint) while the caller stays in; and password-reset/confirm is single-use (replay -> 400) and bumps the epoch so pre-reset sessions die. --- backend/tests/test_auth_session.py | 98 ++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 backend/tests/test_auth_session.py diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py new file mode 100644 index 00000000..57a80fc0 --- /dev/null +++ b/backend/tests/test_auth_session.py @@ -0,0 +1,98 @@ +"""Auth / session security core (TEST-1), DB-lane. + +The wallet impersonation gate, epoch-based session revocation, single-use tokens and the +enumeration-safe uniform 401 are the security mag, and had zero tests (only `_oauth_redirect_uri` was +covered). Starlette's SessionMiddleware round-trips the signed cookie under TestClient, so the whole +browser-less flow is exercisable. Uses the real auth stack via the `client` fixture (NO current_user +override) so login, the wallet gate and epoch revoke are the code under test. + +Note: `/api/me` is the bootstrap probe — it returns 200 `{authenticated: False}` when logged out, not a +401. A revoked session is therefore checked on a PROTECTED endpoint (`/api/feed`), which does 401. +""" +from datetime import timedelta + +import pytest +from fastapi.testclient import TestClient + +import app.state as state +from app.auth import ACTIVE_ACCOUNT_HEADER +from app.main import app + +_PW = "correct horse battery" + + +@pytest.fixture(autouse=True) +def _configured(monkeypatch): + # The setup gate 503s /auth and /api until the wizard has run; every test here is post-setup. + monkeypatch.setattr(state, "_configured_cache", True) + + +def _login(client, email, password): + return client.post("/auth/password-login", json={"email": email, "password": password}) + + +def test_password_login_then_me_is_authenticated(client, make_user): + make_user(email="alice@example.com", password=_PW) + assert _login(client, "alice@example.com", _PW).status_code == 200 + me = client.get("/api/me") + assert me.status_code == 200 + assert me.json()["authenticated"] is True + assert me.json()["email"] == "alice@example.com" + + +def test_wrong_password_and_unknown_email_give_a_byte_identical_401(client, make_user): + make_user(email="bob@example.com", password=_PW) + wrong_pw = _login(client, "bob@example.com", "not the password") + unknown = _login(client, "nobody@example.com", _PW) + assert wrong_pw.status_code == 401 and unknown.status_code == 401 + # Identical body: a wrong password and a non-existent account must be indistinguishable (no + # account-enumeration oracle). + assert wrong_pw.json() == unknown.json() + + +def test_a_non_wallet_account_header_falls_back_to_the_default(client, make_user): + # The X-Siftlode-Account header may only re-target an account already signed into THIS browser + # (in the session wallet). A header naming someone else must be ignored, not honored — otherwise + # it's an impersonation gate hole. + make_user(email="alice2@example.com", password=_PW) + mallory = make_user(email="mallory@example.com", password=_PW) + assert _login(client, "alice2@example.com", _PW).status_code == 200 + me = client.get("/api/me", headers={ACTIVE_ACCOUNT_HEADER: str(mallory.id)}) + assert me.status_code == 200 + assert me.json()["email"] == "alice2@example.com" # still Alice, not Mallory + + +def test_logout_others_revokes_the_other_session_but_keeps_the_caller(client, make_user): + # Two independent cookie jars (browsers) signed into the same account. `logout-others` from one + # bumps the account epoch so the OTHER's cookie is rejected on its next request, while the caller + # is re-stamped and stays in. Checked on /api/feed (a protected endpoint that 401s on revocation). + make_user(email="carol@example.com", password=_PW) + # A second, independent cookie jar; the `client` fixture already routed get_db to the test DB on + # the shared app, so this instance uses it too — it just has its own session cookie. + other = TestClient(app) + assert _login(client, "carol@example.com", _PW).status_code == 200 + assert _login(other, "carol@example.com", _PW).status_code == 200 + assert other.get("/api/feed").status_code == 200 # both valid to start + + assert client.post("/auth/logout-others").status_code == 200 + assert other.get("/api/feed").status_code == 401 # its epoch is now stale + assert client.get("/api/feed").status_code == 200 # the caller stays signed in + + +def test_reset_confirm_is_single_use_and_kills_existing_sessions(client, make_user, db): + from app.routes.auth import _issue_token + + user = make_user(email="dave@example.com", password=_PW) + assert _login(client, "dave@example.com", _PW).status_code == 200 + assert client.get("/api/feed").status_code == 200 # session valid at the pre-reset epoch + + raw = _issue_token(db, user, "reset", timedelta(hours=1)) + db.commit() + ok = client.post("/auth/password-reset/confirm", json={"token": raw, "password": "brand new pw!"}) + assert ok.status_code == 200 + + # A reset bumps the epoch (compromise response) -> the pre-reset cookie is now rejected. + assert client.get("/api/feed").status_code == 401 + # The token is single-use: a replay is a clean 400, not a second reset. + replay = client.post("/auth/password-reset/confirm", json={"token": raw, "password": "another pw!!"}) + assert replay.status_code == 400 From e764ca4568637a81be6b37aaf6044c9f184dd00a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 01:39:45 +0200 Subject: [PATCH 12/22] test(fixtures): make the TestClient fixtures isolated and composable (S2 review) Address the /code-review findings on the S2 test-net: - as_user no longer sets a global `lambda: user` override that two users in one test would collide on; the shared current_user override now resolves the caller from an x-test-user header, and each returned client tags its own requests, so as_user(a)/as_user(b) are independent (#1). - override_dep records the prior value per dependency key and restores it on teardown instead of a blanket dependency_overrides.clear(), so fixtures compose without clobbering each other (#5). - a _configured fixture bakes the post-setup state into client/as_user, and the per-test inline monkeypatch / autouse duplicates are removed (#4). - the feed keyset walk asserts all seeded rows surfaced before comparing, so a seeding regression can't make the sorts pass vacuously (#3). --- backend/tests/conftest.py | 73 +++++++++++++++++++++++------- backend/tests/test_auth_session.py | 8 +--- backend/tests/test_feed_keyset.py | 12 ++--- 3 files changed, 63 insertions(+), 30 deletions(-) diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 268bd229..823ae94d 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -18,10 +18,12 @@ import shutil import pytest from alembic import command from alembic.config import Config +from fastapi import HTTPException, Request from fastapi.testclient import TestClient from sqlalchemy import create_engine, text import app.models # noqa: F401 — register every model on Base.metadata +import app.state as app_state from app import security from app.auth import current_user from app.config import settings @@ -29,6 +31,9 @@ from app.db import Base, SessionLocal, engine, get_db from app.main import app from app.models import User +# Sentinel for "this dependency had no override before the fixture set one" (see override_dep). +_MISSING = object() + # The compose test lane sets this so a would-be silent skip (no DB, or no ffmpeg) becomes a hard error # — the gate must not print green while the DB-backed / ffmpeg tests were quietly not run. _REQUIRE_FULL = os.getenv("SIFTLODE_TEST_REQUIRE_DB") == "1" @@ -124,28 +129,62 @@ def make_user(db): @pytest.fixture -def client(db): +def override_dep(): + """Set app.dependency_overrides for the duration of a test and restore the PRIOR value per key on + teardown — instead of a blanket `app.dependency_overrides.clear()`, which wipes every override and + stops fixtures composing (one fixture's teardown clobbering another's).""" + saved: dict = {} + + def _set(dep, override): + if dep not in saved: # remember the pre-fixture value the FIRST time we touch this key + saved[dep] = app.dependency_overrides.get(dep, _MISSING) + app.dependency_overrides[dep] = override + + yield _set + for dep, prev in saved.items(): + if prev is _MISSING: + app.dependency_overrides.pop(dep, None) + else: + app.dependency_overrides[dep] = prev + + +@pytest.fixture +def _configured(monkeypatch): + """The setup gate 503s /auth and /api until the wizard has run; the request-issuing fixtures below + are all post-setup. Baked into client/as_user so no individual test has to remember it.""" + monkeypatch.setattr(app_state, "_configured_cache", True) + + +@pytest.fixture +def client(db, override_dep, _configured): """A TestClient wired to the test DB that goes through the REAL auth/session stack — no `current_user` override — so login, the wallet gate and epoch revoke are exercised end to end. Starlette's SessionMiddleware round-trips the signed cookie, so a login in one call authenticates the next on this same instance. No lifespan (the scheduler / HLS sweep stay off).""" - app.dependency_overrides[get_db] = lambda: db - try: - yield TestClient(app) - finally: - app.dependency_overrides.clear() + override_dep(get_db, lambda: db) + return TestClient(app) @pytest.fixture -def as_user(db): - """Factory -> a TestClient already authenticated as a given User (via a `current_user` override), - for tests that need an authed request but aren't exercising the login flow itself.""" - def _as(user: User) -> TestClient: - app.dependency_overrides[get_db] = lambda: db - app.dependency_overrides[current_user] = lambda: user - return TestClient(app) +def as_user(db, override_dep, _configured): + """Factory -> a TestClient authenticated as a given User (bypassing login), for tests that need an + authed request but aren't exercising the login flow itself. Multiple users in ONE test are isolated: + the shared `current_user` override resolves the caller from an `x-test-user` header, and each + returned client tags its own requests with it — so `as_user(a)` and `as_user(b)` don't collide on + the process-global override the way a plain `lambda: user` would.""" + registry: dict[int, User] = {} - try: - yield _as - finally: - app.dependency_overrides.clear() + def _resolver(request: Request) -> User: + raw = request.headers.get("x-test-user") + if raw is None or (uid := int(raw)) not in registry: + raise HTTPException(status_code=401, detail="Not authenticated") + return registry[uid] + + override_dep(get_db, lambda: db) + override_dep(current_user, _resolver) + + def _as(user: User) -> TestClient: + registry[user.id] = user + return TestClient(app, headers={"x-test-user": str(user.id)}) + + return _as diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 57a80fc0..0b05464f 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -11,20 +11,14 @@ Note: `/api/me` is the bootstrap probe — it returns 200 `{authenticated: False """ from datetime import timedelta -import pytest from fastapi.testclient import TestClient -import app.state as state from app.auth import ACTIVE_ACCOUNT_HEADER from app.main import app _PW = "correct horse battery" - -@pytest.fixture(autouse=True) -def _configured(monkeypatch): - # The setup gate 503s /auth and /api until the wizard has run; every test here is post-setup. - monkeypatch.setattr(state, "_configured_cache", True) +# The `client`/`as_user` fixtures bake in the post-setup `_configured` state, so no per-test patch here. def _login(client, email, password): diff --git a/backend/tests/test_feed_keyset.py b/backend/tests/test_feed_keyset.py index e44e5a01..44b62d4b 100644 --- a/backend/tests/test_feed_keyset.py +++ b/backend/tests/test_feed_keyset.py @@ -15,7 +15,6 @@ from datetime import datetime, timezone import pytest -import app.state as state from app.models import Channel, Subscription, User, Video # Every sort feedquery.sort_keys defines except "relevance" (which needs a search query to rank). @@ -74,8 +73,7 @@ def _page(client, sort, cursor): return resp.json() -def test_feed_surfaces_the_whole_seed(db, as_user, monkeypatch): - monkeypatch.setattr(state, "_configured_cache", True) # the request gate 503s an unconfigured app +def test_feed_surfaces_the_whole_seed(db, as_user): user, ids = _seed(db) body = as_user(user).get("/api/feed?limit=50").json() assert {it["id"] for it in body["items"]} == set(ids) @@ -83,13 +81,15 @@ def test_feed_surfaces_the_whole_seed(db, as_user, monkeypatch): @pytest.mark.parametrize("sort", _SORTS) -def test_cursor_walk_matches_the_unpaged_order(db, as_user, monkeypatch, sort): - monkeypatch.setattr(state, "_configured_cache", True) - user, _ = _seed(db) +def test_cursor_walk_matches_the_unpaged_order(db, as_user, sort): + user, ids = _seed(db) client = as_user(user) # Ground truth: the full order in one query (no cursor). full = [it["id"] for it in client.get(f"/api/feed?sort={sort}&limit=50").json()["items"]] + # Guard against a vacuous pass: if seeding ever stops surfacing the videos, `walked == full` would + # be [] == [] for every sort and never exercise the cursor. All 7 rows must be present. + assert len(full) == len(ids), f"{sort}: seed did not surface all rows — the walk would be vacuous" # Walk it two at a time via the cursor and reassemble. walked: list[str] = [] From 472cefed5577fd09060ca81b94dab9d916f0f9bb Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 01:39:46 +0200 Subject: [PATCH 13/22] test(migrations)+fix: widen drift check to column shape; notifications.created_at NOT NULL (S2 review #2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drift check dropped every diff except table/column presence, so a column type or nullability change would slip through. Add modify_type/modify_nullable to the ops it fails on. That immediately surfaced a real pre-existing drift: notifications.created_at was created NULLABLE by migration 0017 but the model has always declared it NOT NULL (Mapped[datetime] + server_default=func.now()). Migration 0066 enforces NOT NULL to match the model — safe, since the server_default has filled every row since day one. --- .../versions/0066_notif_created_at_notnull.py | 24 +++++++++++++++++++ backend/tests/test_migrations.py | 18 ++++++++------ 2 files changed, 35 insertions(+), 7 deletions(-) create mode 100644 backend/alembic/versions/0066_notif_created_at_notnull.py diff --git a/backend/alembic/versions/0066_notif_created_at_notnull.py b/backend/alembic/versions/0066_notif_created_at_notnull.py new file mode 100644 index 00000000..486f55b9 --- /dev/null +++ b/backend/alembic/versions/0066_notif_created_at_notnull.py @@ -0,0 +1,24 @@ +"""notifications.created_at NOT NULL (align schema to the model) + +The model has always declared `Notification.created_at: Mapped[datetime]` (NOT NULL) with +`server_default=func.now()`, but migration 0017 created the column without `nullable=False`, so the live +schema allowed NULL — a model/schema drift the new migration drift-test (E1 S2) surfaced. The +server_default has filled every row since day one, so no NULLs exist; enforcing NOT NULL just makes the +schema match the contract the model already promises. Pure DDL, reversible. +""" +from typing import Sequence, Union + +from alembic import op + +revision: str = "0066_notif_created_at_notnull" +down_revision: Union[str, None] = "0065_plex_detail_fields" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.alter_column("notifications", "created_at", nullable=False) + + +def downgrade() -> None: + op.alter_column("notifications", "created_at", nullable=True) diff --git a/backend/tests/test_migrations.py b/backend/tests/test_migrations.py index 373e4413..eec06b69 100644 --- a/backend/tests/test_migrations.py +++ b/backend/tests/test_migrations.py @@ -9,11 +9,12 @@ Two things the prod boot never checks: Both use the `db` fixture, which has already brought the schema to head via `_schema`. -Scope note: we assert on STRUCTURAL drift only — tables and columns present in the model but not the -schema, or vice versa. `compare_metadata` also reports index/constraint/sequence/type differences, but -those are dominated by migration-only objects (partial/expression/GIN indexes, CHECK constraints, SERIAL -sequences) that can't be expressed on `Base.metadata`, so comparing them wholesale is noise. Missing/extra -columns and tables — the "someone changed a model without a migration" case — are the signal worth gating. +Scope note: we gate on COLUMN/TABLE drift — a table or column present on one side and not the other, or +a column whose type/nullability disagrees. `compare_metadata` also reports index/constraint/sequence +differences, but those are dominated by migration-only objects (partial/expression/GIN indexes, CHECK +constraints, SERIAL sequences) that can't be expressed on `Base.metadata`, so comparing them wholesale is +noise; they're filtered out. The column/table signal is the "someone changed a model without a migration" +case worth failing on. """ from alembic import command from alembic.autogenerate import compare_metadata @@ -22,8 +23,11 @@ from alembic.migration import MigrationContext from app.db import Base, engine -# The diff ops that mean a real table/column is present on one side and not the other. -_DRIFT_OPS = {"add_table", "remove_table", "add_column", "remove_column"} +# The diff ops that mean a real table/column disagreement (presence AND shape) — not the index/ +# constraint/sequence noise from migration-only objects. +_DRIFT_OPS = { + "add_table", "remove_table", "add_column", "remove_column", "modify_type", "modify_nullable", +} def _structural_drift(): From 54ba92bb05931adc225ba24df895d4b48ddd040a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 01:48:53 +0200 Subject: [PATCH 14/22] ci(gitea): add the server-side gate mirror (E1 S3, DX-1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add .gitea/workflows/check.yml so the quality gate runs on the lab act_runner, machine-independent, on every push/PR — the review's "no server-side CI" gap. Two parallel jobs mirror the two local lanes: - frontend (node:20-bookworm, matching the prod build image): tsc + eslint + prettier --check + knip + vitest. - backend (nikolaik/python-nodejs python3.12+node): ruff + the full pytest suite against a real postgres:16 service, with ffmpeg installed and SIFTLODE_TEST_REQUIRE_DB=1 so a DB/ffmpeg that failed to come up is a hard error, never a silent skip. Notes for this runner: pytest runs natively against the service (the docker-compose bind-mount is unreliable under a mounted-socket runner), and the backend uses a baked python+node image rather than actions/setup-python, which can't reach GitHub's python-versions download from the lab. --- .gitea/workflows/check.yml | 64 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 .gitea/workflows/check.yml diff --git a/.gitea/workflows/check.yml b/.gitea/workflows/check.yml new file mode 100644 index 00000000..74faec22 --- /dev/null +++ b/.gitea/workflows/check.yml @@ -0,0 +1,64 @@ +# Server-side mirror of the local `siftlode check` gate (E1 S3 / DX-1), so `main` is always green +# independent of any one machine. Two parallel jobs match the two local lanes: +# frontend — tsc (3 projects) + eslint + prettier --check + knip + vitest +# backend — ruff + the full pytest suite (pure-logic + the DB-backed lane) against a real Postgres +# +# Runner: the lab act_runner (LXC 115) maps `ubuntu-latest` -> node:20-bookworm and mounts the Docker +# socket, so service containers work. The backend runs pytest NATIVELY against a `postgres` service +# (not the local docker-compose.test.yml, whose bind-mount is unreliable under a mounted-socket runner); +# fidelity is kept by installing the SAME pinned requirements, Postgres 16, and ffmpeg, and by setting +# SIFTLODE_TEST_REQUIRE_DB=1 so a DB/ffmpeg that failed to come up is a hard error, never a silent skip. +# node 20 matches the prod build image (Dockerfile `node:20-alpine`); S4's node-22 bump moves both. +name: gate + +on: [push, pull_request] + +jobs: + frontend: + runs-on: ubuntu-latest + container: node:20-bookworm + defaults: + run: + working-directory: frontend + steps: + - uses: actions/checkout@v4 + - run: npm ci + - run: npm run typecheck + - run: npm run lint + - run: npm run format:check + - run: npm run knip + - run: npm run test + + backend: + runs-on: ubuntu-latest + # A combined python+node image: python 3.12 (matches the prod Dockerfile) for the tests, and node + # so the JS actions (checkout) run. actions/setup-python is NOT usable here — the runner can't reach + # GitHub's python-versions download, so it must be baked into the image. + container: nikolaik/python-nodejs:python3.12-nodejs20 + services: + postgres: + image: postgres:16-alpine + env: + POSTGRES_USER: siftlode + POSTGRES_PASSWORD: siftlode + POSTGRES_DB: siftlode_test + options: >- + --health-cmd "pg_isready -U siftlode -d siftlode_test" + --health-interval 2s + --health-timeout 3s + --health-retries 30 + env: + DATABASE_URL: postgresql+psycopg://siftlode:siftlode@postgres:5432/siftlode_test + SIFTLODE_TEST_REQUIRE_DB: "1" + steps: + - uses: actions/checkout@v4 + # ffmpeg for the poster/thumbnail tests; postgresql-client for the readiness wait below. + - run: apt-get update && apt-get install -y --no-install-recommends ffmpeg postgresql-client + - name: Wait for Postgres + run: until pg_isready -h postgres -U siftlode -d siftlode_test; do sleep 1; done + - run: pip install --no-cache-dir -r backend/requirements.txt -r backend/requirements-dev.txt + - name: ruff + run: python -m ruff check --config backend/ruff.toml backend scripts + - name: pytest + run: python -m pytest + working-directory: backend From e4ac061721faaf91018b7a79843c5c7c36dc67b6 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 02:34:45 +0200 Subject: [PATCH 15/22] feat(security): add baseline security response headers (E1 S4, SEC-1) A small, safe set applied to every response via middleware: X-Content-Type-Options nosniff, Referrer-Policy strict-origin-when-cross-origin, CSP frame-ancestors 'none' (anti-clickjacking; unrelated to the YouTube/Plex embeds we host), and Permissions-Policy disabling camera/mic/geolocation. A full CSP and HSTS are deferred (HSTS belongs on the Caddy edge). --- backend/app/main.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/backend/app/main.py b/backend/app/main.py index 00c76347..afb3cd0c 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -160,6 +160,29 @@ async def static_cache_headers(request, call_next): return response +# Baseline security response headers (SEC-1). A minimal, safe set that needs no per-route tuning: +# nosniff stops MIME-confusion, the Referrer-Policy keeps the (private) app URL from leaking to +# external sites the user follows, `frame-ancestors 'none'` blocks click-jacking by refusing to be +# iframed anywhere (we never embed our own pages; we embed OTHERS, which this does not affect), and +# Permissions-Policy turns off powerful features the app never uses. A full Content-Security-Policy +# (script/style/connect sources) is deferred — the SPA's inline styles + the YouTube/Plex embeds make +# it real work — and HSTS belongs on the TLS-terminating edge (the Caddy front door), not here. +_SECURITY_HEADERS = { + "X-Content-Type-Options": "nosniff", + "Referrer-Policy": "strict-origin-when-cross-origin", + "Content-Security-Policy": "frame-ancestors 'none'", + "Permissions-Policy": "camera=(), microphone=(), geolocation=()", +} + + +@app.middleware("http") +async def security_headers(request, call_next): + response = await call_next(request) + for name, value in _SECURITY_HEADERS.items(): + response.headers.setdefault(name, value) + return response + + app.include_router(health.router) app.include_router(auth_routes.router) app.include_router(sync.router) From 44d4e4876dea0196c37b792ab4cd4bef815a4d5a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 02:34:45 +0200 Subject: [PATCH 16/22] build(deps): pin a reproducible lock + pip-audit; bump node 20->22 (E1 S4, SEC-5/DX-3) requirements.txt stays the source (floor ranges); requirements.lock pins the exact resolved versions (generated in the python:3.12-slim prod base, extras like psycopg-binary/uvloop included). The prod Dockerfile, Dockerfile.test and CI now install the lock, so the image is reproducible and the versions are the ones pip-audit scans. CI gains a non-blocking pip-audit step over the lock. The frontend build + CI move from the EOL node 20 to node 22. --- .gitea/workflows/check.yml | 21 ++++++++------ Dockerfile | 9 ++++-- backend/Dockerfile.test | 6 ++-- backend/requirements.lock | 57 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 13 deletions(-) create mode 100644 backend/requirements.lock diff --git a/.gitea/workflows/check.yml b/.gitea/workflows/check.yml index 74faec22..9fe276ce 100644 --- a/.gitea/workflows/check.yml +++ b/.gitea/workflows/check.yml @@ -3,12 +3,12 @@ # frontend — tsc (3 projects) + eslint + prettier --check + knip + vitest # backend — ruff + the full pytest suite (pure-logic + the DB-backed lane) against a real Postgres # -# Runner: the lab act_runner (LXC 115) maps `ubuntu-latest` -> node:20-bookworm and mounts the Docker -# socket, so service containers work. The backend runs pytest NATIVELY against a `postgres` service -# (not the local docker-compose.test.yml, whose bind-mount is unreliable under a mounted-socket runner); -# fidelity is kept by installing the SAME pinned requirements, Postgres 16, and ffmpeg, and by setting -# SIFTLODE_TEST_REQUIRE_DB=1 so a DB/ffmpeg that failed to come up is a hard error, never a silent skip. -# node 20 matches the prod build image (Dockerfile `node:20-alpine`); S4's node-22 bump moves both. +# Runner: the lab act_runner (LXC 115) mounts the Docker socket, so service containers work. The backend +# runs pytest NATIVELY against a `postgres` service (not the local docker-compose.test.yml, whose +# bind-mount is unreliable under a mounted-socket runner); fidelity is kept by installing the SAME pinned +# requirements, Postgres 16, and ffmpeg, and by setting SIFTLODE_TEST_REQUIRE_DB=1 so a DB/ffmpeg that +# failed to come up is a hard error, never a silent skip. node 22 matches the prod build image +# (Dockerfile `node:22-alpine`). name: gate on: [push, pull_request] @@ -16,7 +16,7 @@ on: [push, pull_request] jobs: frontend: runs-on: ubuntu-latest - container: node:20-bookworm + container: node:22-bookworm defaults: run: working-directory: frontend @@ -56,9 +56,14 @@ jobs: - run: apt-get update && apt-get install -y --no-install-recommends ffmpeg postgresql-client - name: Wait for Postgres run: until pg_isready -h postgres -U siftlode -d siftlode_test; do sleep 1; done - - run: pip install --no-cache-dir -r backend/requirements.txt -r backend/requirements-dev.txt + - run: pip install --no-cache-dir -r backend/requirements.lock -r backend/requirements-dev.txt - name: ruff run: python -m ruff check --config backend/ruff.toml backend scripts - name: pytest run: python -m pytest working-directory: backend + # Dependency CVE audit over the pinned lock. Non-blocking for now (informational) — a real CVE + # shows up in the run without failing the gate; tighten to a hard fail once the noise is known. + - name: pip-audit (advisory) + run: pip install --no-cache-dir pip-audit && pip-audit -r backend/requirements.lock + continue-on-error: true diff --git a/Dockerfile b/Dockerfile index 9a7f1c00..716bf849 100644 --- a/Dockerfile +++ b/Dockerfile @@ -10,7 +10,7 @@ ARG GIT_SHA=unknown ARG BUILD_DATE= # Stage 1: build the frontend SPA -FROM node:20-alpine AS frontend +FROM node:22-alpine AS frontend WORKDIR /fe # Deps first, keyed on the lockfile, so editing app source never re-resolves node_modules. COPY frontend/package.json frontend/package-lock.json ./ @@ -57,9 +57,12 @@ RUN apt-get update \ && rm /tmp/deno.zip \ && rm -rf /var/lib/apt/lists/* -COPY backend/requirements.txt . +# Install from the PINNED lock (requirements.lock, generated from requirements.txt) so the image is +# byte-reproducible and the exact versions are the ones pip-audit scanned in CI — not whatever floats +# up from the floor ranges on build day. Regenerate the lock when requirements.txt changes (see its header). +COPY backend/requirements.lock . RUN --mount=type=cache,target=/root/.cache/pip \ - pip install --upgrade pip && pip install -r requirements.txt + pip install --upgrade pip && pip install -r requirements.lock COPY --chown=appuser backend/ . # Read at runtime by config.py as the app_version (a committed single source of truth). diff --git a/backend/Dockerfile.test b/backend/Dockerfile.test index 03f3709b..1f71de56 100644 --- a/backend/Dockerfile.test +++ b/backend/Dockerfile.test @@ -14,5 +14,7 @@ WORKDIR /app RUN apt-get update \ && apt-get install -y --no-install-recommends ffmpeg \ && rm -rf /var/lib/apt/lists/* -COPY requirements.txt requirements-dev.txt ./ -RUN pip install --no-cache-dir -r requirements.txt -r requirements-dev.txt +# Install the SAME pinned lock as the prod image (+ the test-only tooling), so the tests run against +# the exact dependency versions prod ships. +COPY requirements.lock requirements-dev.txt ./ +RUN pip install --no-cache-dir -r requirements.lock -r requirements-dev.txt diff --git a/backend/requirements.lock b/backend/requirements.lock new file mode 100644 index 00000000..3b8d8188 --- /dev/null +++ b/backend/requirements.lock @@ -0,0 +1,57 @@ +# Pinned dependency lock for reproducible builds (SEC-5). GENERATED — do not hand-edit. +# Source of truth is requirements.txt (the floor ranges); regenerate after editing it with: +# docker run --rm -v "$PWD":/b python:3.12-slim sh -c 'pip install -q -r /b/requirements.txt && pip freeze --exclude pip --exclude setuptools --exclude wheel' | sort +# The prod Dockerfile + Dockerfile.test + CI install THIS file; pip-audit scans it in CI. +APScheduler==3.11.3 +Authlib==1.7.2 +Mako==1.4.1 +MarkupSafe==3.0.3 +PyYAML==6.0.3 +SQLAlchemy==2.0.52 +alembic==1.19.1 +annotated-doc==0.0.5 +annotated-types==0.8.0 +anyio==4.14.2 +argon2-cffi-bindings==25.1.0 +argon2-cffi==25.1.0 +bgutil-ytdlp-pot-provider==1.3.1 +brotli==1.2.0 +certifi==2026.7.22 +cffi==2.1.1 +charset-normalizer==3.5.1 +click==8.4.2 +cryptography==46.0.7 +fastapi==0.141.1 +feedparser-sgmllib==2.1.0 +feedparser==6.0.14 +greenlet==3.5.5 +h11==0.16.0 +httpcore==1.0.9 +httptools==0.8.0 +httpx==0.28.1 +idna==3.18 +itsdangerous==2.2.0 +joserfc==1.7.4 +mutagen==1.48.1 +numpy==2.5.2 +psycopg-binary==3.3.4 +psycopg==3.3.4 +py3langid==0.3.0 +pycparser==3.0 +pycryptodomex==3.23.0 +pydantic-settings==2.15.0 +pydantic==2.13.4 +pydantic_core==2.46.4 +python-dotenv==1.2.3 +requests==2.34.2 +starlette==1.6.0 +typing-inspection==0.4.4 +typing_extensions==4.16.0 +tzlocal==5.4.4 +urllib3==2.7.0 +uvicorn==0.52.3 +uvloop==0.22.1 +watchfiles==1.2.0 +websockets==17.0.1 +yt-dlp-ejs==0.8.0 +yt-dlp==2026.7.4 From f54dd981295ae00d5a1a1113509021606051fd32 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 02:34:45 +0200 Subject: [PATCH 17/22] test(release): lock the VERSION file and RELEASE_NOTES[0] together (E1 S4, DX-6) A vitest that fails the gate when the newest release-notes entry's version diverges from the repo-root VERSION file (the two hand-edited version sources), plus a no-duplicate-versions check. Catches the "bumped one, forgot the other" release mistake. --- frontend/src/lib/releaseNotes.test.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 frontend/src/lib/releaseNotes.test.ts diff --git a/frontend/src/lib/releaseNotes.test.ts b/frontend/src/lib/releaseNotes.test.ts new file mode 100644 index 00000000..1a0bfa13 --- /dev/null +++ b/frontend/src/lib/releaseNotes.test.ts @@ -0,0 +1,21 @@ +import { readFileSync } from "node:fs"; + +import { describe, expect, it } from "vitest"; + +import { RELEASE_NOTES } from "./releaseNotes"; + +// DX-6: the app version is hand-maintained in two places that MUST agree — the repo-root VERSION file +// (the single source baked into the image + served at /api/version) and the newest RELEASE_NOTES entry +// (what the in-app "What's new" shows). A release that bumps one but forgets the other ships a wrong +// changelog or a mismatched banner. This locks them together so the gate catches the drift. +describe("release version consistency", () => { + it("RELEASE_NOTES[0].version matches the VERSION file", () => { + const version = readFileSync(new URL("../../../VERSION", import.meta.url), "utf-8").trim(); + expect(RELEASE_NOTES[0]?.version).toBe(version); + }); + + it("release notes are ordered newest-first with unique versions", () => { + const versions = RELEASE_NOTES.map((r) => r.version); + expect(new Set(versions).size).toBe(versions.length); // no duplicate version entries + }); +}); From 8ffa65a9da782a4cc5058f8c55bbe93443207390 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 02:47:05 +0200 Subject: [PATCH 18/22] fix(security): bump cryptography 46.0.7 -> 50.0.0 to clear 4 CVEs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pip-audit step added in E1 S4 flagged cryptography 46.0.7 for PYSEC-2026-3552 /3553/3554 and GHSA-537c-gmf6-5ccf; all are fixed by 50.0.0. The requirements.txt ceiling `<47` was blocking the upgrade — relaxed to `>=50.0.0,<51`. The lock is a surgical bump (only the cryptography line changed; cffi 2.1.1 already satisfies it, no other package moved). pip-audit -r requirements.lock is now clean. Add tests/test_security.py (there was none for the crypto primitives): the Fernet encrypt/decrypt round-trip, decrypt_optional degrading on garbage and on a key mismatch (the B-core-2 path), and the argon2 password hash/verify round-trip — so a future crypto-lib bump can't silently break the token-at-rest path. Gate green: 330 pytest. --- backend/requirements.lock | 2 +- backend/requirements.txt | 2 +- backend/tests/test_security.py | 36 ++++++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 backend/tests/test_security.py diff --git a/backend/requirements.lock b/backend/requirements.lock index 3b8d8188..8fc9ef93 100644 --- a/backend/requirements.lock +++ b/backend/requirements.lock @@ -20,7 +20,7 @@ certifi==2026.7.22 cffi==2.1.1 charset-normalizer==3.5.1 click==8.4.2 -cryptography==46.0.7 +cryptography==50.0.0 fastapi==0.141.1 feedparser-sgmllib==2.1.0 feedparser==6.0.14 diff --git a/backend/requirements.txt b/backend/requirements.txt index 2a8cf384..3112eff1 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -11,7 +11,7 @@ feedparser>=6.0,<7.0 apscheduler>=3.10,<4.0 py3langid>=0.3,<1.0 itsdangerous>=2.1,<3.0 -cryptography>=46.0.7,<47 +cryptography>=50.0.0,<51 argon2-cffi>=23.1,<26 # Download center: yt-dlp does the extraction/download; ffmpeg (OS-level in the Dockerfile) # does the merge + postprocessing. The [default] extra pulls yt-dlp-ejs (the challenge-solver diff --git a/backend/tests/test_security.py b/backend/tests/test_security.py new file mode 100644 index 00000000..133c88f8 --- /dev/null +++ b/backend/tests/test_security.py @@ -0,0 +1,36 @@ +"""Crypto primitives — a guard for the cryptography (Fernet) + argon2 dependency bumps. + +Added with the cryptography 46->50 CVE bump: encrypt/decrypt/decrypt_optional had NO test, and they are +the token-at-rest path (Google/API/SMTP/Plex secrets in system_config, OAuth refresh tokens). A fresh +Fernet key is monkeypatched in so the test doesn't depend on TOKEN_ENCRYPTION_KEY being set in the env. +Pure-logic (no DB). +""" +from cryptography.fernet import Fernet + +from app import security + + +def test_fernet_encrypt_decrypt_roundtrip(monkeypatch): + monkeypatch.setattr(security, "_fernet", Fernet(Fernet.generate_key())) + assert security.decrypt(security.encrypt("hunter2")) == "hunter2" + assert security.encrypt(None) is None + assert security.decrypt(None) is None + + +def test_decrypt_optional_degrades_instead_of_raising(monkeypatch): + # Garbage / absent input -> None (never raises). + monkeypatch.setattr(security, "_fernet", Fernet(Fernet.generate_key())) + assert security.decrypt_optional("not-a-valid-token") is None + assert security.decrypt_optional(None) is None + # And a ciphertext written under a DIFFERENT key (the TOKEN_ENCRYPTION_KEY-mismatch case that B-core-2 + # made non-fatal) also degrades to None rather than raising InvalidToken. + ciphertext = security.encrypt("secret") + monkeypatch.setattr(security, "_fernet", Fernet(Fernet.generate_key())) # rotate the key + assert security.decrypt_optional(ciphertext) is None + + +def test_password_hash_and_verify_roundtrip(): + hashed = security.hash_password("correct horse battery") + assert security.verify_password("correct horse battery", hashed) is True + assert security.verify_password("wrong password", hashed) is False + assert security.verify_password("anything", None) is False From 18edbecb1e3561d37b3908bc9b99fe324d256afc Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 03:02:34 +0200 Subject: [PATCH 19/22] fix(auth): close the account pre-emption + wrong-account link holes (E1 S5, SEC-10/11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SEC-10 (account pre-emption / "pre-hijack"): a password set on an account whose email was never verified is untrusted — whoever registered the address (to pre-empt the real owner) could have chosen it. Both the Google-adoption callback and admin activation (_activate_user_by_email) now drop that pre-set password and bump the session epoch when the account isn't email_verified, so a later Google login / admin approval can't hand the pre-set password a working login. No-op for a verified account (the legit no-SMTP flow). +2 tests. SEC-11 (link/upgrade bound to the session default): the OAuth link/upgrade GETs resolved to the session DEFAULT account, so a tab acting as a secondary account would link Google / the YouTube grant to the wrong account. The frontend now threads the active account through the navigation URL (withActiveAccount -> ?account=), and _complete_link accepts any wallet account, not only the default, so the identity lands on the account that started the flow. --- backend/app/routes/admin.py | 8 +++++++ backend/app/routes/auth.py | 16 +++++++++++-- backend/tests/test_auth_session.py | 29 +++++++++++++++++++++++ frontend/src/components/SettingsPanel.tsx | 7 +++--- frontend/src/lib/activeAccount.ts | 11 +++++++++ frontend/src/lib/onboarding.ts | 3 ++- 6 files changed, 68 insertions(+), 6 deletions(-) diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index c4bbfa3b..d69ae9c9 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -19,6 +19,7 @@ from app.auth import ( ) from app.db import get_db from app.demo import reset_demo_state +from app.routes.auth import bump_session_epoch from app.models import ( DemoWhitelist, Invite, @@ -62,6 +63,13 @@ def _activate_user_by_email(db: Session, email: str) -> None: it can sign in. No-op if no such user exists yet (a Google user is created at first login).""" user = db.execute(select(User).where(User.email == email.lower())).scalar_one_or_none() if user is not None and not user.is_active: + # SEC-10: same guard as the Google-adoption path — approving an account whose email was never + # verified must not activate a password set on it by whoever registered the address (a + # pre-emption). Drop the untrusted password + revoke any session it minted; the owner re-sets + # one via password-reset. (No-op when email_verified is True, e.g. legit no-SMTP registrations.) + if not user.email_verified: + user.password_hash = None + bump_session_epoch(user) user.is_active = True db.commit() diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index b71f8457..a2ba0a26 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -286,6 +286,14 @@ async def callback( return RedirectResponse(url="/?access=denied") user = existing user.google_sub = sub + # SEC-10: a password set on this account BEFORE its email was ever verified is untrusted — + # whoever registered the address (to pre-empt the real owner, who is only now arriving via + # Google) could have chosen it. Drop it now that Google attests ownership, and bump the epoch + # to revoke any session that pre-set password may have minted. The owner can set a fresh + # password via password-reset later if they also want email+password sign-in. + if not user.email_verified: + user.password_hash = None + bump_session_epoch(user) # Google verified the email and is_allowed granted access, so finish activating a # previously pending password registration (otherwise current_user would bounce it). user.email_verified = True @@ -392,8 +400,12 @@ def _complete_link( Google identity owned by another account, or to silently swap a different linked identity. `explicit` (the popped oauth_link_explicit marker) distinguishes a "Connect Google" click from a YouTube upgrade — the caller pops it up front so an aborted flow leaves nothing behind.""" - # The marker must match the live session user (guards a stale/forged marker). - if link_uid != request.session.get("user_id"): + # SEC-11: the marker must name an account signed into THIS browser — the session default OR any + # account in the wallet — not only the default. A tab acting as a secondary account links Google to + # ITSELF (the marker was set from current_user, which resolves the ?account= it navigated with), so + # accepting wallet membership stops the identity/token landing on the default account A instead of B. + accounts = request.session.get("account_ids") or [] + if link_uid != request.session.get("user_id") and link_uid not in accounts: return RedirectResponse(url="/?link=error") user = db.get(User, link_uid) if user is None or user.is_demo: diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 0b05464f..c44dfe50 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -90,3 +90,32 @@ def test_reset_confirm_is_single_use_and_kills_existing_sessions(client, make_us # The token is single-use: a replay is a clean 400, not a second reset. replay = client.post("/auth/password-reset/confirm", json={"token": raw, "password": "another pw!!"}) assert replay.status_code == 400 + + +def test_activating_an_unverified_account_drops_its_preset_password(db, make_user): + # SEC-10 (account pre-emption): an attacker registers victim@x with a password before the approved + # owner arrives. The address was never verified, so admin approval must NOT hand that pre-set + # password a working login — it is dropped and any session it minted is revoked. + from app.routes.admin import _activate_user_by_email + + victim = make_user( + email="victim@example.com", password="attacker-chosen", email_verified=False, is_active=False + ) + epoch0 = victim.session_epoch or 0 + _activate_user_by_email(db, "victim@example.com") + assert victim.is_active is True + assert victim.password_hash is None + assert (victim.session_epoch or 0) == epoch0 + 1 + + +def test_activating_a_verified_account_keeps_its_password(db, make_user): + # A legit account whose email IS verified (e.g. a no-SMTP registration) keeps its password on + # approval — the guard must not break the normal flow. + from app.routes.admin import _activate_user_by_email + + user = make_user( + email="legit@example.com", password="my own password", email_verified=True, is_active=False + ) + _activate_user_by_email(db, "legit@example.com") + assert user.is_active is True + assert user.password_hash is not None diff --git a/frontend/src/components/SettingsPanel.tsx b/frontend/src/components/SettingsPanel.tsx index f8eaa900..4b6f233b 100644 --- a/frontend/src/components/SettingsPanel.tsx +++ b/frontend/src/components/SettingsPanel.tsx @@ -1,4 +1,5 @@ import { useState } from "react"; +import { withActiveAccount } from "../lib/activeAccount"; import { qk } from "../lib/queryKeys"; import { useTranslation } from "react-i18next"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; @@ -418,7 +419,7 @@ function SignInMethods({ me }: { me: Me }) { ) : (