From 4f31ae797d1913696cdabab8fd4c44e25058bf03 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 21 Jul 2026 21:47:26 +0200 Subject: [PATCH 1/6] fix(auth): rate-limit request-access + close two token races (C-3.5, C-3.11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-3.5: /auth/request-access had no rate limiter (unlike register/login/reset/demo) and answered "approved" for an allow-listed email — a public enumeration oracle for the allow-list, and an unthrottled invite/admin-mail firehose. It now rate-limits per client IP and returns a uniform "pending" for every outcome (allowed, pending, new, throttled); an already-allowed user can still just sign in with Google. C-3.11: _consume_token was a read-check-write, so two concurrent requests could both consume the same verify/reset token. It's now a single conditional UPDATE (...WHERE used_at IS NULL AND expires_at >= now RETURNING user_id) — only the request that flips used_at gets a user back. --- backend/app/auth.py | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/backend/app/auth.py b/backend/app/auth.py index cdd84904..cb5b83ec 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -8,7 +8,7 @@ from authlib.integrations.starlette_client import OAuth, OAuthError from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request from fastapi.responses import JSONResponse, RedirectResponse from starlette.requests import HTTPConnection -from sqlalchemy import delete, func, select +from sqlalchemy import delete, func, select, update from sqlalchemy.orm import Session from app import email as email_mod @@ -31,6 +31,7 @@ _DECOY_PASSWORD_HASH = hash_password(secrets.token_urlsafe(16)) _register_limiter = RateLimiter(max_events=5, window_seconds=300) _login_limiter = RateLimiter(max_events=10, window_seconds=300) _reset_limiter = RateLimiter(max_events=5, window_seconds=300) +_request_access_limiter = RateLimiter(max_events=5, window_seconds=300) # At most one "your account is suspended" email per address per hour, so a suspended user who # keeps retrying (or a script with their valid credentials) can't be used to mailbomb them. _suspend_email_limiter = RateLimiter(max_events=1, window_seconds=3600) @@ -512,16 +513,26 @@ async def logout(request: Request): @router.post("/request-access") async def request_access( payload: dict, + request: Request, background: BackgroundTasks, db: Session = Depends(get_db), ) -> dict: """Public: ask for access. Idempotent — repeat requests for the same email don't - duplicate or re-spam admins (upsert returns None for an already-pending row).""" + duplicate or re-spam admins (upsert returns None for an already-pending row). + + Answers a uniform ``"pending"`` for EVERY outcome — already-allowed, newly-pending, + already-pending, and rate-limited alike. The old ``"approved"`` reply for an allow-listed + address made this a public enumeration oracle for the allow-list (which the rest of the module + is careful to avoid, cf. demo_login); an already-allowed user can still just sign in with Google. + Rate-limited per client IP like register/login/reset, and a block returns "pending" too so the + endpoint can't be probed for throttling either.""" email = (payload.get("email") or "").strip().lower() if not valid_email(email): raise HTTPException(status_code=400, detail="Enter a valid email address.") + if not _request_access_limiter.allow(_client_ip(request)): + return {"status": "pending"} if is_allowed(db, email): - return {"status": "approved"} # already allowed — just sign in + return {"status": "pending"} # already allowed — don't reveal it inv = upsert_pending_invite(db, email) admins = admin_notify_emails(db) if inv is not None and admins: @@ -583,18 +594,27 @@ def _issue_token(db: Session, user: User, kind: str, ttl: timedelta) -> str: def _consume_token(db: Session, raw: str | None, kind: str) -> User | None: - """Validate + burn a token. Returns its user, or None if missing/expired/used/wrong kind.""" + """Validate + burn a token atomically. Returns its user, or None if missing/expired/used/wrong + kind. The burn is a single conditional UPDATE (…WHERE used_at IS NULL…RETURNING) rather than a + read-check-write, so two concurrent requests can't both consume the same verify/reset token — + only the one whose UPDATE flips used_at gets a row back.""" if not raw: return None + now = datetime.now(timezone.utc) row = db.execute( - select(AuthToken).where( - AuthToken.token_hash == hash_token(raw), AuthToken.kind == kind + update(AuthToken) + .where( + AuthToken.token_hash == hash_token(raw), + AuthToken.kind == kind, + AuthToken.used_at.is_(None), + AuthToken.expires_at >= now, ) - ).scalar_one_or_none() - if row is None or row.used_at is not None or row.expires_at < datetime.now(timezone.utc): - return None - row.used_at = datetime.now(timezone.utc) + .values(used_at=now) + .returning(AuthToken.user_id) + ).first() db.commit() + if row is None: + return None return db.get(User, row.user_id) From dd99206ed24781193e33340f72f2edd435ad1ae3 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 21 Jul 2026 21:47:26 +0200 Subject: [PATCH 2/6] fix(worker): don't revive a cancelled job when requeuing a busy asset (C-3.10) The asset-busy and claim-fail branches wrote status='queued' unconditionally, so a pause/cancel the route had just written was clobbered back to queued and the job silently revived. New _requeue_if_running does the requeue as a conditional UPDATE (...WHERE status='running'), a no-op once the route has moved the job off 'running'. --- backend/app/worker.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/backend/app/worker.py b/backend/app/worker.py index b35d6f15..d8080468 100644 --- a/backend/app/worker.py +++ b/backend/app/worker.py @@ -66,6 +66,22 @@ def _job_status(job_id: int) -> str | None: ).scalar() +def _requeue_if_running(job_id: int, phase: str) -> None: + """Put the job back to 'queued' for a later retry, but ONLY while it is still 'running'. The + route flips status to paused/cancelled to stop a job; a blind `status='queued'` here would + clobber that and silently revive a job the user just cancelled. The conditional WHERE makes the + requeue a no-op unless we still own a running job.""" + with SessionLocal() as db: + db.execute( + text( + "UPDATE download_jobs SET status='queued', phase=:phase, updated_at=now() " + "WHERE id=:id AND status='running'" + ), + {"id": job_id, "phase": phase}, + ) + db.commit() + + def _parse_upload_date(raw) -> date | None: if not raw: return None @@ -179,13 +195,13 @@ def _process_job(job_id: int) -> None: return if asset_status == "downloading": # Another worker owns this asset; wait and let the job be reclaimed later. - _set_job(job_id, status="queued", phase="waiting") + _requeue_if_running(job_id, "waiting") time.sleep(2) return # asset_status == "pending": try to win the download / edit. if not _claim_asset(asset_id): - _set_job(job_id, status="queued", phase="waiting") + _requeue_if_running(job_id, "waiting") time.sleep(2) return @@ -414,7 +430,7 @@ def _download(job_id: int, asset_id: int, source_kind: str, source_ref: str, spe ) layout, retention_days = _download_settings() - rel = storage.rel_path(meta, ext, layout) + rel = storage.rel_path(meta, ext, layout, asset_id=asset_id) thumb = _find_thumbnail(staging, produced) storage.place_file(produced, settings.download_root, rel) nfo_ok = storage.write_sidecars(settings.download_root, rel, meta, thumb) From 3dfa35f3964c7b67e78aa1a875656e2f36346d1b Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 21 Jul 2026 21:47:26 +0200 Subject: [PATCH 3/6] perf(notifications): bulk-delete trimmed read notifications (C-4.1) trim_read looped a db.get()+db.delete() per id (N+1). The ids are already in hand, so one DELETE ... WHERE id IN (...) replaces the loop. --- backend/app/notifications.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/app/notifications.py b/backend/app/notifications.py index c4ce1209..1e7b6cec 100644 --- a/backend/app/notifications.py +++ b/backend/app/notifications.py @@ -5,7 +5,7 @@ survive reloads/devices and are produced server-side. First producer is the main job. Unread rows never auto-expire; read rows are trimmed past a soft per-user cap so the table can't grow without bound for a heavy user. """ -from sqlalchemy import select +from sqlalchemy import delete, select from sqlalchemy.orm import Session from app.models import Notification @@ -52,7 +52,7 @@ def trim_read(db: Session, user_id: int, cap: int = READ_SOFT_CAP) -> int: ) if not ids: return 0 - for nid in ids: - db.delete(db.get(Notification, nid)) + # One bulk DELETE rather than a get+delete per row (the ids are already in hand). + db.execute(delete(Notification).where(Notification.id.in_(ids))) db.commit() return len(ids) From 0dfc5e9ea222505044514b1ecf124591abe01265 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 21 Jul 2026 21:47:45 +0200 Subject: [PATCH 4/6] fix(quota): keep the demo account off three quota-burning paths (C-3.6) The demo account could spend real YouTube quota where the human-only guard was missing (the channel_detail enrichment already had it): - channels PATCH: an immediate run_recent_backfill when deep_requested is turned on; - discovery: the channel-detail enrich on the Discover tab; - feed get_video_detail: the off-catalog videos.list lookup for a video we don't store (the DB-hit path stays free; demo now gets a 404 for unknown videos). --- backend/app/routes/channels.py | 12 ++++++++++-- backend/app/routes/feed.py | 5 +++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/backend/app/routes/channels.py b/backend/app/routes/channels.py index dca498e3..7919e9a8 100644 --- a/backend/app/routes/channels.py +++ b/backend/app/routes/channels.py @@ -180,7 +180,8 @@ def discover_channels( # (fresh subscriber counts/thumbnails carry through without a re-query). Only the title can # change, which affects the tie-break order → re-sort in Python instead of re-hitting the DB. need = [ch.id for ch, _, _ in rows if ch.details_synced_at is None][:DISCOVERY_ENRICH_CAP] - if need: + # Skip the quota-spending enrich for the demo account (it just sees the stub metadata). + if need and not user.is_demo: try: with quota.attribute(user.id, quota.QuotaAction.CHANNELS_DISCOVER), YouTubeClient(db, user) as yt: apply_channel_details(db, yt.get_channels(need)) @@ -402,7 +403,14 @@ def update_channel( # Opting a channel into full history should give an immediate feed: if we haven't even # fetched its recent uploads yet, do that one channel now (cheap) instead of waiting for # the scheduler. Deep paging still follows on the scheduler's next run (recent-then-deep). - if deep_turned_on and channel is not None and channel.recent_synced_at is None: + # Not for the demo account: it must not be able to trigger an immediate quota-burning backfill + # (the scheduler's deep paths are already human-only). Same guard as channel_detail's enrichment. + if ( + deep_turned_on + and channel is not None + and channel.recent_synced_at is None + and not user.is_demo + ): with quota.attribute(user.id, quota.QuotaAction.VIDEOS_BACKFILL_RECENT): run_recent_backfill(db, [channel], max_channels=1) diff --git a/backend/app/routes/feed.py b/backend/app/routes/feed.py index e5d29525..443ee19b 100644 --- a/backend/app/routes/feed.py +++ b/backend/app/routes/feed.py @@ -737,6 +737,11 @@ def get_video_detail( "duration_seconds": v.duration_seconds, } + # The DB path above is free; the off-catalog YouTube lookup below spends a quota unit, so the + # demo account can't reach it — it just sees "unknown" for a video we don't already store. + if user.is_demo: + raise HTTPException(status_code=404, detail="Unknown video") + try: with quota.attribute(user.id, quota.QuotaAction.VIDEOS_LOOKUP), YouTubeClient(db, user) as yt: items = yt.get_videos([video_id]) From 0317f53b1e73fb4b2067e28698d3cc249d37b72f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 21 Jul 2026 21:47:45 +0200 Subject: [PATCH 5/6] fix(downloads): disambiguate the media path by asset id (C-3.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A MediaAsset is unique on (source_kind, source_ref, format_sig), so the same video in two formats is two assets — but rel_path keyed only on video_id, so they computed the same path: the second download silently overwrote the first, and deleting either removed the shared file. rel_path now takes an optional asset_id and appends "_a{id}" (which Plex ignores — the .nfo sidecar carries the metadata); the worker passes the asset id. Only affects NEW downloads (existing assets keep their stored rel_path). Covered by a new test (two ids → two paths; omitted → the plain name, unchanged). --- backend/app/downloads/storage.py | 15 +++++++++++---- backend/tests/test_storage.py | 10 ++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/backend/app/downloads/storage.py b/backend/app/downloads/storage.py index 1aee7b85..d8e12790 100644 --- a/backend/app/downloads/storage.py +++ b/backend/app/downloads/storage.py @@ -74,16 +74,23 @@ def display_filename(name: str) -> str: return text or "download" -def rel_path(meta: MediaMeta, ext: str, layout: str = "plex") -> str: - """Path of the media file relative to DOWNLOAD_ROOT (forward slashes).""" +def rel_path(meta: MediaMeta, ext: str, layout: str = "plex", asset_id: int | None = None) -> str: + """Path of the media file relative to DOWNLOAD_ROOT (forward slashes). + + `asset_id` disambiguates the file. A MediaAsset is unique on (source_kind, source_ref, + format_sig), so the SAME video downloaded in two formats is two distinct assets — but without + the id they compute the identical path, so the second silently overwrites the first and deleting + either removes the shared file. The `_a{id}` suffix (which Plex ignores — the .nfo sidecar carries + the metadata) keeps them apart. Optional so callers/tests that don't need it keep the plain name.""" title = sanitize(meta.title) vid = meta.video_id + disc = f"_a{asset_id}" if asset_id is not None else "" if layout == "flat": - return f"{title}_[{vid}].{ext}" + return f"{title}_[{vid}]{disc}.{ext}" channel = sanitize(meta.uploader or "Unknown_Channel", 80) year = meta.upload_date.year if meta.upload_date else 0 d = meta.upload_date.isoformat() if meta.upload_date else "0000-00-00" - epname = f"{channel}_-_{d}_-_{title}_[{vid}].{ext}" + epname = f"{channel}_-_{d}_-_{title}_[{vid}]{disc}.{ext}" return f"{channel}/Season_{year}/{epname}" diff --git a/backend/tests/test_storage.py b/backend/tests/test_storage.py index ab2c8e74..f1ce6716 100644 --- a/backend/tests/test_storage.py +++ b/backend/tests/test_storage.py @@ -54,6 +54,16 @@ class TestRelPath: out = rel_path(meta, "mp4") assert "Season_0/" in out and "0000-00-00" in out + def test_asset_id_disambiguates_two_formats_of_the_same_video(self): + # Same video, two assets (e.g. 1080p vs 720p) must NOT collide on one path. + a = rel_path(self.meta, "mp4", asset_id=5) + b = rel_path(self.meta, "mp4", asset_id=6) + assert a != b + assert a.endswith("_[abc123]_a5.mp4") and b.endswith("_[abc123]_a6.mp4") + # Omitting it keeps the plain name (unchanged for callers/tests that don't pass one). + assert rel_path(self.meta, "mp4", layout="flat") == "Cool_Clip_[abc123].mp4" + assert rel_path(self.meta, "mp4", layout="flat", asset_id=7) == "Cool_Clip_[abc123]_a7.mp4" + class TestDownloadFilename: def test_appends_the_container_extension(self): From 3d5662df22d3ea6e8c6eea7ff77e578f16fef14d Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 21 Jul 2026 21:47:45 +0200 Subject: [PATCH 6/6] perf(frontend): keep the changelog out of the eager App chunk (C-3.19) App.tsx imported CURRENT_VERSION from releaseNotes, which pulled its ~900-line changelog into the eager App chunk and defeated the lazy ReleaseNotes split. App now uses FRONTEND_VERSION (the built VITE_APP_VERSION, equal to RELEASE_NOTES[0].version by release convention) from lib/version, and CURRENT_VERSION is removed. Verified in the built bundle: the changelog text now lives only in the ReleaseNotes lazy chunk. --- frontend/src/App.tsx | 7 +++++-- frontend/src/lib/releaseNotes.ts | 2 -- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index b84dea2e..4c4c06dc 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -35,7 +35,10 @@ import ErrorDialog from "./components/ErrorDialog"; import VersionBanner from "./components/VersionBanner"; import DemoBanner from "./components/DemoBanner"; import { focusAccessRequestsTab } from "./lib/adminUsersTab"; -import { CURRENT_VERSION } from "./lib/releaseNotes"; +// The built app version, NOT imported from releaseNotes — importing anything from that module +// pulls its whole ~900-line changelog into the eager App chunk and defeats the lazy ReleaseNotes +// split. FRONTEND_VERSION equals RELEASE_NOTES[0].version by release convention (both bump together). +import { FRONTEND_VERSION } from "./lib/version"; // The page modules (and their side rails) are code-split per route via the module registry // (components/moduleRegistry). App keeps only the persistent-shell chunks + the channel page and the @@ -389,7 +392,7 @@ export default function App() { banners={ <> {meQuery.data!.is_demo && } - openReleaseNotes(CURRENT_VERSION)} /> + openReleaseNotes(FRONTEND_VERSION)} /> } > diff --git a/frontend/src/lib/releaseNotes.ts b/frontend/src/lib/releaseNotes.ts index c9846456..09d8beb7 100644 --- a/frontend/src/lib/releaseNotes.ts +++ b/frontend/src/lib/releaseNotes.ts @@ -923,5 +923,3 @@ export const RELEASE_NOTES: ReleaseEntry[] = [ ], }, ]; - -export const CURRENT_VERSION = RELEASE_NOTES[0]?.version ?? "dev";