diff --git a/.gitea/workflows/check.yml b/.gitea/workflows/check.yml new file mode 100644 index 00000000..9fe276ce --- /dev/null +++ b/.gitea/workflows/check.yml @@ -0,0 +1,69 @@ +# 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) 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] + +jobs: + frontend: + runs-on: ubuntu-latest + container: node:22-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.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/VERSION b/VERSION index 367d4880..24c41e03 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.66.0 \ No newline at end of file +0.67.0 \ No newline at end of file diff --git a/backend/Dockerfile.test b/backend/Dockerfile.test index 6bffc2f8..1f71de56 100644 --- a/backend/Dockerfile.test +++ b/backend/Dockerfile.test @@ -8,5 +8,13 @@ # docker run --rm -v "/backend:/app" -w /app siftlode-test:local python -m pytest FROM python:3.12-slim WORKDIR /app -COPY requirements.txt requirements-dev.txt ./ -RUN pip install --no-cache-dir -r requirements.txt -r requirements-dev.txt +# 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/* +# 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/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/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/downloads/service.py b/backend/app/downloads/service.py index 5d79a4a6..39af6a58 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, update from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session @@ -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, JobStatus from app.models import Channel, DownloadJob, MediaAsset, Video @@ -223,6 +224,35 @@ 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` are + mapped-column kwargs (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 + # A Core UPDATE with the guard as a WHERE — same atomic "only while still running" write as the + # rest of the codebase's 2.0 statements, no hand-built SQL string. synchronize_session=False: this + # is a terminal write, so we don't pay to sync the (about-to-be-discarded) ORM identity map. Bound + # StrEnum values bind as their plain str literal (StrEnum is a str subclass). + result = db.execute( + update(DownloadJob) + .where(DownloadJob.id == job_id, DownloadJob.status == JobStatus.RUNNING.value) + .values(**fields) + .execution_options(synchronize_session=False) + ) + 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, @@ -238,7 +268,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/main.py b/backend/app/main.py index 00c76347..19825bd0 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -160,6 +160,32 @@ 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. `X-Frame-Options: DENY` duplicates +# the frame-ancestors intent for legacy engines that never implemented CSP frame-ancestors (belt and +# suspenders — modern browsers use the CSP directive and ignore the header). 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'", + "X-Frame-Options": "DENY", + "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) diff --git a/backend/app/plex/sync.py b/backend/app/plex/sync.py index acdb5e5a..9ed6cad2 100644 --- a/backend/app/plex/sync.py +++ b/backend/app/plex/sync.py @@ -294,6 +294,11 @@ def _paginate(plex: PlexClient, key: str, item_type: int): break +# The section listing exposes at most the top ~20 cast (Plex's own cap); a stored cast longer than this +# can only have come from the rich enrich, never the listing. +_LISTING_CAST_CAP = 20 + + def _apply_facet_fields(row, meta: dict) -> None: """Set the filterable/orderable metadata columns shared by plex_items + plex_shows (identical column names) from the cheap section listing: rating, content_rating, studio, release date, @@ -305,11 +310,19 @@ 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, by LENGTH against the listing cap: a stored cast LONGER than the + # cap can only be an enriched fuller list, so keep it (robust to any watermark reset). A stored cast + # at or below the cap is itself listing-sourced, so take the fresh listing — which tracks Plex cast + # EDITS in EITHER direction (comparing against this listing's length instead would silently keep a + # stale longer cast when Plex TRIMS one). if row.people_enriched_at is None: - row.cast_names = _tags(meta, "Role", limit=20) + listing_cast = _tags(meta, "Role", limit=_LISTING_CAST_CAP) + if len(row.cast_names or []) <= _LISTING_CAST_CAP: + row.cast_names = listing_cast people = list(dict.fromkeys((row.directors or []) + (row.cast_names or []))) row.people_text = " ".join(people) or None diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index c4bbfa3b..f1dfa860 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -24,6 +24,7 @@ from app.models import ( Invite, User, ) +from app.routes.auth import revoke_preset_password_if_unverified from app.utils import iso, valid_email router = APIRouter(prefix="/api/admin", tags=["admin"]) @@ -62,6 +63,10 @@ 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 hand a working login to a password set on it by whoever registered the + # address (a pre-emption). Shared single-source helper so the two paths can't drift. + revoke_preset_password_if_unverified(user) user.is_active = True db.commit() diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index b71f8457..0f0290a2 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -286,6 +286,9 @@ async def callback( return RedirectResponse(url="/?access=denied") user = existing user.google_sub = sub + # SEC-10: drop any password pre-set on this account while its email was still unverified, + # now that Google attests ownership (bumps the epoch to revoke any session it minted). + revoke_preset_password_if_unverified(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 +395,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: @@ -805,6 +812,18 @@ def bump_session_epoch(user: User) -> None: user.session_epoch = (user.session_epoch or 0) + 1 +def revoke_preset_password_if_unverified(user: User) -> None: + """SEC-10 (account pre-emption): 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. When + ownership is finally attested (Google adoption, or admin approval), drop that password and bump the + epoch to revoke any session it minted; the owner re-sets one via password-reset. No-op for an + already-verified account, so a legit no-SMTP registration is unaffected. The single source for this + rule — both the Google-callback and admin-activation paths call it so they can't drift.""" + if not user.email_verified: + user.password_hash = None + bump_session_epoch(user) + + @router.post("/logout-others") def logout_others( request: Request, user: User = Depends(current_user), db: Session = Depends(get_db) 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/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} 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 = [ 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/worker.py b/backend/app/worker.py index 0749bd42..3aa9fe94 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": @@ -482,13 +483,14 @@ def _finish_from_cache(job_id: int, asset_id: int) -> None: job = db.get(DownloadJob, job_id) asset = db.get(MediaAsset, asset_id) if job and asset: - job.status = "done" - job.progress = 100 - job.phase = None - if not job.display_name: + fields = {"status": JobStatus.DONE, "progress": 100, "phase": None} + if not job.display_name and asset.title: # 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] 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() @@ -868,20 +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) - if job: - job.status = "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() - _notify_download_done(db, owner_id, meta.title, job_id, kind="download") - log.info("job %s done: %s", job_id, rel) + 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): # Paused/canceled mid-download or mid-transcode: free the asset for a later retry; keep the @@ -895,12 +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) - if job: - job.status = "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: @@ -927,12 +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) - if job: - job.status = "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) @@ -1008,16 +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) - if job: - job.status = "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() - _notify_download_done(db, owner_id, dl_title, job_id, kind="edit") - log.info("edit job %s done: %s", job_id, rel) + 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: # Paused/canceled mid-encode: free the asset for a later retry (ref_count is the route's job). 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/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/requirements.lock b/backend/requirements.lock new file mode 100644 index 00000000..8fc9ef93 --- /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==50.0.0 +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 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/conftest.py b/backend/tests/conftest.py index 8fa4ec87..823ae94d 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -12,14 +12,31 @@ 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 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 -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 + +# 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" def _db_reachable() -> bool: @@ -59,6 +76,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 +104,87 @@ 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 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).""" + override_dep(get_db, lambda: db) + return TestClient(app) + + +@pytest.fixture +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] = {} + + 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 new file mode 100644 index 00000000..c44dfe50 --- /dev/null +++ b/backend/tests/test_auth_session.py @@ -0,0 +1,121 @@ +"""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 + +from fastapi.testclient import TestClient + +from app.auth import ACTIVE_ACCOUNT_HEADER +from app.main import app + +_PW = "correct horse battery" + +# The `client`/`as_user` fixtures bake in the post-setup `_configured` state, so no per-test patch here. + + +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 + + +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/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" diff --git a/backend/tests/test_feed_keyset.py b/backend/tests/test_feed_keyset.py new file mode 100644 index 00000000..44b62d4b --- /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 + +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): + 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, 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] = [] + 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" diff --git a/backend/tests/test_migrations.py b/backend/tests/test_migrations.py new file mode 100644 index 00000000..b09b03c3 --- /dev/null +++ b/backend/tests/test_migrations.py @@ -0,0 +1,72 @@ +"""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 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 +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 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(): + """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") + reupgraded = False + try: + command.upgrade(cfg, "head") + reupgraded = True + assert _structural_drift() == [] + finally: + # If the re-upgrade itself failed, restore the SHARED schema to head so the failure doesn't + # cascade into every later DB-lane test. Skip it when the upgrade already succeeded — re-running + # it would be a redundant no-op and could mask a failing assert with the finally's own error. + if not reupgraded: + command.upgrade(cfg, "head") diff --git a/backend/tests/test_plex_detail.py b/backend/tests/test_plex_detail.py index d5d0bba3..ae46e868 100644 --- a/backend/tests/test_plex_detail.py +++ b/backend/tests/test_plex_detail.py @@ -128,6 +128,35 @@ def test_apply_facet_fields_preserves_enriched_cast_names(): assert "Diane Keaton" in enriched.people_text and "Francis Ford Coppola" in enriched.people_text +def test_apply_facet_fields_keeps_over_cap_cast_after_a_watermark_reset(): + """The enriched-cast guard must survive a watermark RESET (people_enriched_at nulled, e.g. by a + migration like 0065). A stored cast LONGER than the listing cap can only be an enriched fuller list, + so it's kept even though the row now reads as un-enriched — comparing against the cap, not the + current listing's length, is what makes this robust (regression guard for the review fix).""" + thin = {"Role": [{"tag": f"Actor {i}"} for i in range(20)]} # a full 20-name listing + over_cap = SimpleNamespace( + people_enriched_at=None, # watermark was reset + directors=None, + cast_names=[f"Enriched {i}" for i in range(22)], # 22 > the 20 cap => enriched + ) + _apply_facet_fields(over_cap, thin) + assert over_cap.cast_names == [f"Enriched {i}" for i in range(22)] # kept, not downgraded + + +def test_apply_facet_fields_tracks_a_trimmed_listing_when_unenriched(): + """A listing-sourced cast (at or below the cap, never enriched) must TRACK Plex cast edits in both + directions. When Plex trims the cast, the shorter fresh listing replaces the stored one — the old + length-vs-this-listing guard would have wrongly kept the stale longer cast.""" + stored = SimpleNamespace( + people_enriched_at=None, + directors=None, + cast_names=["A", "B", "C", "D", "E"], # 5, at/below the cap => listing-sourced + ) + trimmed = {"Role": [{"tag": "A"}, {"tag": "B"}]} # Plex removed 3 cast members + _apply_facet_fields(stored, trimmed) + assert stored.cast_names == ["A", "B"] + + def test_enrich_people_batch_persists_detail(db): lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) db.add(lib) 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 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"])) 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=…`. 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 }) { ) : (