Merge: R1 S1 — backend safety quick wins (7 fixes)

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(
+        update(AuthToken)
-            AuthToken.token_hash == hash_token(raw), AuthToken.kind == kind
+        .where(
+            AuthToken.token_hash == hash_token(raw),
+            AuthToken.kind == kind,
+            AuthToken.used_at.is_(None),
+            AuthToken.expires_at >= now,
         )
-    ).scalar_one_or_none()
+        .values(used_at=now)
-    if row is None or row.used_at is not None or row.expires_at < datetime.now(timezone.utc):
+        .returning(AuthToken.user_id)
-        return None
+    ).first()
-    row.used_at = datetime.now(timezone.utc)
     db.commit()
+    if row is None:
+        return None
     return db.get(User, row.user_id)
 
 

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:
+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)."""
+    """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}"
 
 

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:
+    # One bulk DELETE rather than a get+delete per row (the ids are already in hand).
-        db.delete(db.get(Notification, nid))
+    db.execute(delete(Notification).where(Notification.id.in_(ids)))
     db.commit()
     return len(ids)

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)
 

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])

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)

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):

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 && <DemoBanner />}
-                <VersionBanner onOpen={() => openReleaseNotes(CURRENT_VERSION)} />
+                <VersionBanner onOpen={() => openReleaseNotes(FRONTEND_VERSION)} />
               </>
             }
           >

frontend/src/lib/releaseNotes.ts

@@ -923,5 +923,3 @@ export const RELEASE_NOTES: ReleaseEntry[] = [
     ],
   },
 ];
-
-export const CURRENT_VERSION = RELEASE_NOTES[0]?.version ?? "dev";