From 0cc450d1c9e704b24d8ec38a0c5c451ac592c72f Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 5 Sep 2026 01:02:56 +0200 Subject: [PATCH] fix(downloads,sync): bound what one user can make the server fetch, download and spend MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SEC-12: the download box accepts an arbitrary http(s) URL and yt-dlp fetches it from the WORKER, so any approved user could aim the server at the compose network, the LAN or a cloud metadata endpoint — and read the result off job.error, where "connection refused" vs "HTTP Error 401" vs "timed out" is a port scanner. downloads/urlguard.py refuses a URL whose host resolves to a loopback/private/link-local/reserved address, at enqueue AND again in the worker (DNS can move in between, and the worker is where the socket opens). A network-class failure on a `url` job now reports one fixed sentence instead of the raw transport text; the error CODE is still classified from the original, so the localized explanation is unchanged. DNS rebinding is explicitly NOT closed — that needs the connection gated inside yt-dlp — and the module says so rather than implying more than it does. SEC-13: `noplaylist` only demotes a video-inside-a-playlist URL to that video, so a URL that is purely a playlist or channel still expanded into every entry — one enqueue, unbounded downloads. `playlist_items` pins it to one. Size was unbounded too: `check_enqueue` compares the CURRENT footprint against the cap, and a download's size is unknown until it lands, so one job could overshoot by any amount; the owner's remaining budget is now handed to yt-dlp as `max_filesize`, applied AFTER the adapter merge so no source adapter can raise a limit the quota layer set. A join's cut-list is capped at 100: an accurate join emits two filters per segment into one ffmpeg filter_complex. SEC-14: the YouTube budget is instance-wide, so one user holding down a manual sync spent the day for everyone. Per-user (not per-IP — the cost belongs to the account) 3-per-10-minutes limiters on the two bulk pulls, subscriptions and playlists, with separate budgets so one does not close the other. The review also listed /rss, /backfill and /enrich; E3 deleted them, and search stays unlimited on purpose — it is interactive, not a bulk pull. Gate: pytest 896 -> 924 (none skipped), vitest 671. --- backend/app/downloads/edit.py | 10 +++ backend/app/downloads/formats.py | 18 ++++- backend/app/downloads/pipeline.py | 40 +++++++++- backend/app/downloads/urlguard.py | 88 +++++++++++++++++++++ backend/app/routes/downloads.py | 12 +++ backend/app/routes/playlists.py | 9 +++ backend/app/routes/sync.py | 10 +++ backend/tests/test_abuse_limits.py | 87 ++++++++++++++++++++ backend/tests/test_formats.py | 49 ++++++++++++ backend/tests/test_urlguard.py | 87 ++++++++++++++++++++ frontend/src/i18n/locales/en/downloads.json | 7 ++ frontend/src/i18n/locales/en/errors.json | 1 + frontend/src/i18n/locales/hu/downloads.json | 7 ++ frontend/src/i18n/locales/hu/errors.json | 1 + frontend/src/lib/api/core.ts | 8 ++ 15 files changed, 429 insertions(+), 5 deletions(-) create mode 100644 backend/app/downloads/urlguard.py create mode 100644 backend/tests/test_abuse_limits.py create mode 100644 backend/tests/test_urlguard.py diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index a9028ff5..aabeba7b 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -54,6 +54,10 @@ class EditAborted(Exception): # --- edit spec ------------------------------------------------------------------------------ +# SEC-13: ceiling on a join's cut-list. An accurate join emits two filters per segment into a single +# ffmpeg `filter_complex`, so "however many the caller sent" is an unbounded command line. +_MAX_SEGMENTS = 100 + def normalize_edit_spec(spec: dict | None) -> dict: """Canonicalize a raw edit spec: clamp/round trim, coerce crop to ints, drop empties. @@ -88,6 +92,12 @@ def normalize_edit_spec(spec: dict | None) -> dict: if en > st: norm.append({"start_s": round(st, 3), "end_s": round(en, 3)}) norm.sort(key=lambda x: x["start_s"]) + # SEC-13: an accurate join builds one ffmpeg `filter_complex` with two filters per segment, + # so an unbounded list becomes an unbounded command line and an ffmpeg that never finishes. + # The cap is far above any real cut-list (the UI's own splitter produces a handful) and the + # excess is DROPPED rather than raising: the surplus segments are the attack, the first + # _MAX_SEGMENTS are a legitimate edit. + del norm[_MAX_SEGMENTS:] if norm: out["segments"] = norm # A join always chooses a codec path: accurate=filter-concat re-encode, diff --git a/backend/app/downloads/formats.py b/backend/app/downloads/formats.py index cf836462..56c51163 100644 --- a/backend/app/downloads/formats.py +++ b/backend/app/downloads/formats.py @@ -116,6 +116,7 @@ def build_ydl_opts( progress_hook, extra: dict | None = None, cookiefile: str | None = None, + max_bytes: int | None = None, ) -> dict: """Assemble yt-dlp options for a single download to `outtmpl` (a full path template). @@ -125,12 +126,20 @@ def build_ydl_opts( `cookiefile` is the acting user's per-host cookies.txt path (S6), attached ONLY by the worker after the host-scope + private-asset gate in `downloads.credentials`. It is set AFTER the `extra` - merge so a source adapter can never clobber it.""" + merge so a source adapter can never clobber it. + + `max_bytes` (SEC-13) is the caller's remaining storage budget; yt-dlp refuses a format larger + than it rather than filling the disk and only then being caught by the quota accounting.""" s = normalize(spec) h = s["max_height"] opts: dict = { "outtmpl": outtmpl, "noplaylist": True, + # SEC-13: `noplaylist` only demotes a video-inside-a-playlist URL to the single video. A + # URL that is PURELY a playlist or channel has no such video to fall back to, so it still + # expands into every entry — one enqueue, an unbounded number of downloads. One job means + # one item; a user who wants the whole list enqueues it item by item, under their quota. + "playlist_items": "1", "quiet": True, "no_warnings": True, "noprogress": True, @@ -154,6 +163,13 @@ def build_ydl_opts( if extra: opts.update(extra) + # SEC-13: hard ceiling on a single download. AFTER the adapter merge, for the same reason + # `cookiefile` is — a source adapter must not be able to raise a limit the quota layer set. A + # non-positive budget is left unset: "0 bytes left" is a refusal for enqueue to make, not + # something to express here as an impossible yt-dlp filter. + if max_bytes and max_bytes > 0: + opts["max_filesize"] = max_bytes + # The acting user's per-host cookie (S6). AFTER the adapter merge on purpose: an adapter must not # be able to override the authenticated session the worker resolved for this specific user. if cookiefile: diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 2ed78b94..cb5cd7b4 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -15,7 +15,8 @@ from app.config import settings from app.db import SessionLocal from app.downloads import edit as editmod from app.downloads import errors as dlerrors -from app.downloads import adapters, credentials, formats, mediakind, probe, service, storage +from app.downloads import adapters, credentials, formats, mediakind, probe, quota, service, storage +from app.downloads import urlguard from app.downloads.jobdb import ( _edit_staging_dir, _job_status, @@ -305,13 +306,17 @@ def _media_info(info: dict) -> dict: return {**info, **chosen} -def _extract_with_fallback(job_id, asset_id, url, spec, staging, adapter, cookiefile=None): +def _extract_with_fallback( + job_id, asset_id, url, spec, staging, adapter, cookiefile=None, max_bytes=None +): """Download via yt-dlp using the source `adapter`'s client sets + options. For YouTube the adapter supplies the primary player-client set then a POT-backed fallback (retried on a bot/DRM/format error); other sources make a single attempt. Returns the info dict of the successful attempt. `cookiefile` (S6) is the acting user's per-host cookies.txt, already gated by - `credentials.resolve_cookiefile`; passed to every attempt so the fallback stays authenticated.""" + `credentials.resolve_cookiefile`; passed to every attempt so the fallback stays authenticated. + + `max_bytes` (SEC-13) is the owner's remaining storage budget, applied to every attempt.""" client_sets = adapter.client_sets() outtmpl = str(staging / "%(id)s.%(ext)s") last_exc: Exception | None = None @@ -322,6 +327,7 @@ def _extract_with_fallback(job_id, asset_id, url, spec, staging, adapter, cookie _make_progress_hook(job_id, asset_id), extra=adapter.ydl_opts_extra(clients), cookiefile=cookiefile, + max_bytes=max_bytes, ) opts["postprocessor_hooks"] = [_make_pp_hook(job_id)] _touch_job(job_id) # stall-watchdog heartbeat: extraction writes no progress, so a slow / @@ -415,8 +421,17 @@ def _download( _set_job(job_id, phase="downloading", progress=0) adapter = adapters.resolve_adapter(source_kind, source_ref) url = service.source_url(source_kind, source_ref) + # SEC-12, second half: the enqueue guard ran when the job was created, which may have been + # hours ago and against a different DNS answer. This is where the socket actually gets + # opened, so the check is repeated here and the job fails rather than fetching. + if source_kind == "url": + try: + urlguard.check_url(url) + except urlguard.BlockedUrl as e: + raise RuntimeError(f"Refusing to download from this address ({e.reason}).") from e # S6: attach the acting user's per-host cookie ONLY when it's host-scoped to this URL AND # this asset is that user's private, salted identity (the double gate lives in credentials). + remaining_bytes: int | None = None with SessionLocal() as cdb: asset_sig = cdb.execute( select(MediaAsset.format_sig).where(MediaAsset.id == asset_id) @@ -425,6 +440,13 @@ def _download( cookiefile = credentials.resolve_cookiefile( cdb, user_id, url, asset_sig, spec, staging ) + # SEC-13: what this user has left. `check_enqueue` only compares the CURRENT footprint + # against the cap — a download's size is unknown until it finishes, so a single job could + # previously blow through the cap by any amount. Handing the remainder to yt-dlp as + # `max_filesize` makes the format selector refuse an oversized file up front instead. + lim = quota.resolve(cdb, user_id) + if not lim.unlimited: + remaining_bytes = max(0, lim.max_bytes - quota.footprint(cdb, user_id)) info = _media_info( _extract_with_fallback( job_id, @@ -434,6 +456,7 @@ def _download( staging, adapter, cookiefile=str(cookiefile) if cookiefile else None, + max_bytes=remaining_bytes, ) ) # Re-resolve now that yt-dlp's info reveals the true source (a bare/redirected url only @@ -527,6 +550,15 @@ def _download( log.info("job %s aborted (pause/cancel)", job_id) except Exception as exc: # noqa: BLE001 — surface any failure to the user, keep worker alive msg = str(exc)[:500] + code = dlerrors.classify(msg) + # SEC-12: for a user-supplied URL, a transport-level failure text distinguishes "connection + # refused" from "HTTP Error 401" from "timed out" per host:port — which is a port scanner + # with extra steps, since the raw text is shown to the user behind the details disclosure. + # The urlguard above stops it aiming at our own network; this stops the remaining readout. + # Only the network class, and only for `url` jobs: a YouTube job has no user-chosen host, and + # every other class (login_required, geo_blocked, drm, …) is real, useful diagnosis. + if source_kind == "url" and code == "network": + msg = "The server could not reach that address." with SessionLocal() as db: asset = db.get(MediaAsset, asset_id) if asset: @@ -539,7 +571,7 @@ def _download( job_id, status=JobStatus.ERROR, error=msg, - error_code=dlerrors.classify(msg), + error_code=code, phase=None, ) db.commit() diff --git a/backend/app/downloads/urlguard.py b/backend/app/downloads/urlguard.py new file mode 100644 index 00000000..2d9d633f --- /dev/null +++ b/backend/app/downloads/urlguard.py @@ -0,0 +1,88 @@ +"""Refuse a user-supplied download URL that points back into our own network (SEC-12). + +The Download Center deliberately accepts an arbitrary ``http(s)`` URL — that is the feature — and +hands it to yt-dlp running on the WORKER. Without a guard, any approved user can make the server +fetch anything the server can reach: other containers on the compose network, the LAN, a cloud +metadata endpoint. The failure text then comes back to them in ``job.error``, so "connection +refused" vs "HTTP Error 401" vs "timed out" turns the download box into an internal port scanner. + +What this closes: a URL whose host resolves to a loopback / private / link-local / reserved address +is refused, at enqueue AND again in the worker before extraction. Both, because DNS can answer +differently between the two moments, and the worker is where the socket is actually opened. + +What it does NOT close, stated plainly rather than implied: a host that passes the worker's check +and then answers yt-dlp's own resolution with an internal address (DNS rebinding). Closing that +needs the connection itself to be gated — a custom socket factory inside yt-dlp — which is a much +larger change than this one. The two checks here stop the straightforward case (typing +``http://127.0.0.1:6379`` or an internal hostname into the box), which is the reachable one. + +Not used for YouTube jobs: those resolve to a fixed public host from an 11-char id, so there is no +user-controlled destination to guard. +""" + +import ipaddress +import socket +from urllib.parse import urlparse + +# Reason codes travel to the client inside the structured 422 detail, so they are part of the API +# vocabulary (frontend `localizeDetail`) — keep them stable. +REASON_SCHEME = "scheme" # not http(s) +REASON_NO_HOST = "no_host" # URL carries no hostname +REASON_UNRESOLVABLE = "unresolvable" # DNS said nothing (honest: not the same as "blocked") +REASON_PRIVATE = "private" # resolves into a network the server must not fetch from + + +class BlockedUrl(Exception): + """A user-supplied URL that must not be fetched server-side. ``reason`` is one of the + REASON_* codes above.""" + + def __init__(self, reason: str): + super().__init__(reason) + self.reason = reason + + +def _is_forbidden(ip: ipaddress.IPv4Address | ipaddress.IPv6Address) -> bool: + """Every address family we must not open a connection to. Checked explicitly rather than + leaning on ``is_private`` alone, whose coverage differs between IPv4 and IPv6.""" + # ::ffff:127.0.0.1 is loopback wearing an IPv6 hat — unwrap before judging it. + mapped = getattr(ip, "ipv4_mapped", None) + if mapped is not None: + ip = mapped + return bool( + ip.is_private + or ip.is_loopback + or ip.is_link_local + or ip.is_multicast + or ip.is_reserved + or ip.is_unspecified + ) + + +def _resolve(host: str, port: int | None) -> list[str]: + """Every address ``host`` resolves to. Raises BlockedUrl(REASON_UNRESOLVABLE) when DNS has no + answer — reported as its own outcome, because "we could not look that up" and "we refuse to + fetch that" are different things to tell a user.""" + try: + infos = socket.getaddrinfo(host, port, proto=socket.IPPROTO_TCP) + except socket.gaierror as exc: + raise BlockedUrl(REASON_UNRESOLVABLE) from exc + return [info[4][0] for info in infos] + + +def check_url(raw: str) -> None: + """Raise BlockedUrl unless ``raw`` is an http(s) URL whose host resolves only to addresses the + server is allowed to fetch from. A host with even ONE forbidden address is refused: a + round-robin answer must not become a coin flip.""" + u = urlparse((raw or "").strip()) + if u.scheme not in ("http", "https"): + raise BlockedUrl(REASON_SCHEME) + host = u.hostname + if not host: + raise BlockedUrl(REASON_NO_HOST) + for addr in _resolve(host, u.port): + try: + ip = ipaddress.ip_address(addr) + except ValueError: # getaddrinfo returned something unparseable; treat as unusable + raise BlockedUrl(REASON_PRIVATE) from None + if _is_forbidden(ip): + raise BlockedUrl(REASON_PRIVATE) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 7ba0e12c..7d55121e 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -28,6 +28,7 @@ from app.downloads import edit as editmod from app.downloads import links as linksmod from app.downloads import quota, service, storage from app.downloads import upload as uploadmod +from app.downloads import urlguard from app.models import ( DownloadJob, DownloadLink, @@ -352,6 +353,17 @@ def enqueue_download( body: EnqueueDownloadIn, user: User = Depends(require_human), db: Session = Depends(get_db) ) -> dict: source_kind, source_ref = resolve_source(body.source) + # SEC-12: a non-YouTube source is a raw URL the worker will fetch server-side, so refuse the + # ones pointing back into our own network BEFORE a job exists. The worker re-checks (DNS can + # move between here and the download); see downloads/urlguard.py for what that does and does + # not cover. + if source_kind == "url": + try: + urlguard.check_url(source_ref) + except urlguard.BlockedUrl as e: + raise HTTPException( + status_code=422, detail={"code": "url_blocked", "reason": e.reason} + ) from e spec, profile_id = _resolve_spec(db, user, body.profile_id, body.spec) display_name = (body.display_name or "").strip() or None try: diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 397df8fb..b9a4a008 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -20,10 +20,17 @@ from app.sync.playlists import ( repull_playlist, sync_user_playlists, ) +from app.ratelimit import RateLimiter from app.youtube.client import YouTubeClient, YouTubeError router = APIRouter(prefix="/api/playlists", tags=["playlists"]) +# SEC-14, same reasoning as the sync module's limiter: a playlist pull lists every playlist the +# caller owns and then every item in each, against the instance-wide YouTube budget. Its own +# limiter rather than a shared one — spending the manual subscription-import allowance must not +# also close the playlist button, and vice versa. +_playlist_pull_limiter = RateLimiter(max_events=3, window_seconds=600) + def _own_playlist(db: Session, user: User, playlist_id: int) -> Playlist: pl = db.get(Playlist, playlist_id) @@ -284,6 +291,8 @@ def sync_youtube( status_code=403, detail="Connect YouTube (read access) to sync your playlists.", ) + if not _playlist_pull_limiter.allow(str(user.id)): + raise HTTPException(status_code=429, detail={"code": "sync_cooldown"}) with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PULL): return sync_user_playlists(db, user) diff --git a/backend/app/routes/sync.py b/backend/app/routes/sync.py index 9a70bc11..8b1e7462 100644 --- a/backend/app/routes/sync.py +++ b/backend/app/routes/sync.py @@ -7,12 +7,20 @@ from app.audit import AuditAction from app.auth import admin_user, current_user, has_read_scope, require_human from app.db import get_db from app.models import Channel, Subscription, User, Video +from app.ratelimit import RateLimiter from app.sync.runner import estimate_deep_backfill from app.sync.subscriptions import import_subscriptions from app.scheduler import running_job_ids router = APIRouter(prefix="/api/sync", tags=["sync"]) +# SEC-14: the YouTube API budget is SHARED across the whole instance, and a subscription import +# walks every page of the caller's subscriptions — so one user holding down a manual sync button can +# spend the day's units for everyone. Keyed by user id, not client IP: the cost belongs to the +# account, and the same person on a second device is the same drain. 3 per 10 minutes leaves manual +# re-syncing entirely usable (the scheduler does the routine work anyway) while removing the loop. +_sync_limiter = RateLimiter(max_events=3, window_seconds=600) + @router.post("/subscriptions") def sync_subscriptions(user: User = Depends(require_human), db: Session = Depends(get_db)) -> dict: @@ -22,6 +30,8 @@ def sync_subscriptions(user: User = Depends(require_human), db: Session = Depend status_code=403, detail="Connect your YouTube account (read access) to import subscriptions.", ) + if not _sync_limiter.allow(str(user.id)): + raise HTTPException(status_code=429, detail={"code": "sync_cooldown"}) with quota.measured(db, user.id, quota.QuotaAction.SUBSCRIPTIONS_PULL) as m: result = import_subscriptions(db, user) result["quota_used_estimate"] = m.used diff --git a/backend/tests/test_abuse_limits.py b/backend/tests/test_abuse_limits.py new file mode 100644 index 00000000..85193b5d --- /dev/null +++ b/backend/tests/test_abuse_limits.py @@ -0,0 +1,87 @@ +"""E12 · S3 — the route-level halves of SEC-12 and SEC-14. + +`test_urlguard.py` pins the address judgement itself; this pins that the ENQUEUE route actually +consults it and answers in the structured `{code, reason}` shape the frontend localizes, and that a +manual sync trigger stops spending the instance-wide YouTube budget when held down. + +DB-backed (the `as_user` fixture needs real rows) — skipped when no database is reachable. +""" + +from app.downloads import urlguard + + +def test_enqueueing_a_loopback_url_is_refused_with_a_structured_detail( + db, as_user, make_user, monkeypatch +): + user = make_user() + db.commit() + monkeypatch.setattr( + urlguard.socket, "getaddrinfo", lambda *a, **kw: [(None, None, None, "", ("127.0.0.1", 0))] + ) + r = as_user(user).post("/api/downloads", json={"source": "http://redis.internal:6379/x"}) + assert r.status_code == 422 + # The shape matters as much as the refusal: a bare English string under a bilingual UI is the + # thing the canonical-patterns table forbids. + assert r.json()["detail"] == {"code": "url_blocked", "reason": urlguard.REASON_PRIVATE} + + +def test_a_public_url_gets_past_the_guard(db, as_user, make_user, monkeypatch): + # Mutation check for the test above: with a public answer the request must fail LATER (or + # succeed), never with url_blocked — otherwise the guard would be refusing everything and the + # first test would still pass. + user = make_user() + db.commit() + monkeypatch.setattr( + urlguard.socket, + "getaddrinfo", + lambda *a, **kw: [(None, None, None, "", ("93.184.216.34", 0))], + ) + r = as_user(user).post("/api/downloads", json={"source": "https://example.com/v"}) + detail = r.json().get("detail") if r.status_code >= 400 else None + assert not (isinstance(detail, dict) and detail.get("code") == "url_blocked") + + +def test_youtube_sources_never_consult_the_guard(db, as_user, make_user, monkeypatch): + # A YouTube job resolves to a fixed public host from an 11-char id, so there is no + # user-controlled destination — resolving one would be pure latency on every enqueue. + def boom(*a, **kw): + raise AssertionError("the URL guard resolved DNS for a YouTube source") + + user = make_user() + db.commit() + monkeypatch.setattr(urlguard.socket, "getaddrinfo", boom) + as_user(user).post("/api/downloads", json={"source": "dQw4w9WgXcQ"}) + + +def test_manual_subscription_sync_goes_on_cooldown(db, as_user, make_user, monkeypatch): + from app.routes import sync as syncroutes + + user = make_user() + db.commit() + # Neither the YouTube grant nor the import itself is under test — the limiter is. + monkeypatch.setattr(syncroutes, "has_read_scope", lambda u: True) + monkeypatch.setattr(syncroutes, "import_subscriptions", lambda d, u: {"imported": 0}) + + c = as_user(user) + codes = [c.post("/api/sync/subscriptions").status_code for _ in range(4)] + # 3 per 10 minutes: the first three pass, the fourth is refused. + assert codes[:3] == [200, 200, 200] + assert codes[3] == 429 + assert c.post("/api/sync/subscriptions").json()["detail"] == {"code": "sync_cooldown"} + + +def test_the_cooldown_is_per_user_not_global(db, as_user, make_user, monkeypatch): + # Keyed by user id on purpose: the shared YouTube budget is spent per account, and one user + # exhausting their allowance must not lock everyone else out of their own sync button. + from app.routes import sync as syncroutes + + a, b = make_user(), make_user() + db.commit() + monkeypatch.setattr(syncroutes, "has_read_scope", lambda u: True) + monkeypatch.setattr(syncroutes, "import_subscriptions", lambda d, u: {"imported": 0}) + + ca = as_user(a) + for _ in range(4): + ca.post("/api/sync/subscriptions") + assert ca.post("/api/sync/subscriptions").status_code == 429 + assert as_user(b).post("/api/sync/subscriptions").status_code == 200 diff --git a/backend/tests/test_formats.py b/backend/tests/test_formats.py index b5803d05..6c1fbbdd 100644 --- a/backend/tests/test_formats.py +++ b/backend/tests/test_formats.py @@ -109,3 +109,52 @@ def test_browser_transcode_flags_universal_playability(): assert F("h264", "aac", False) == (True, False, False) # A failed probe / missing stream is never re-encoded. assert F(None, None, True) == (False, False, False) + + +# --- SEC-13 · one enqueue is one bounded download ------------------------------------------- + + +def test_a_playlist_url_can_only_yield_one_item(): + # `noplaylist` alone only demotes a video-inside-a-playlist URL to that video; a URL that is + # purely a playlist or channel has no such video, so without this it expands into every entry. + opts = _opts({}) + assert opts["noplaylist"] is True + assert opts["playlist_items"] == "1" + + +def test_the_remaining_quota_becomes_a_hard_size_ceiling(): + opts = formats.build_ydl_opts({}, "/tmp/x", lambda d: None, max_bytes=5_000_000) + assert opts["max_filesize"] == 5_000_000 + + +def test_no_budget_means_no_size_filter(): + # Unlimited users pass None; a non-positive remainder is the quota layer's refusal to make at + # enqueue, not an impossible yt-dlp filter to express here. + for budget in (None, 0, -1): + assert "max_filesize" not in formats.build_ydl_opts( + {}, "/tmp/x", lambda d: None, max_bytes=budget + ) + + +def test_a_source_adapter_cannot_raise_the_size_ceiling(): + # The adapter merge happens after max_filesize is set, so this pins that no adapter option can + # widen a storage cap — the mirror of the cookiefile rule, in the opposite direction. + opts = formats.build_ydl_opts( + {}, "/tmp/x", lambda d: None, extra={"max_filesize": 999_999_999}, max_bytes=5_000_000 + ) + assert opts["max_filesize"] == 5_000_000 + + +def test_a_join_cut_list_is_capped(): + # An accurate join emits two filters per segment into one ffmpeg filter_complex, so an unbounded + # list is an unbounded command line. The surplus is dropped; the legitimate head survives. + segs = [{"start_s": i, "end_s": i + 1} for i in range(500)] + out = editmod.normalize_edit_spec({"segments": segs}) + assert len(out["segments"]) == editmod._MAX_SEGMENTS + assert out["segments"][0] == {"start_s": 0.0, "end_s": 1.0} + + +def test_a_normal_cut_list_is_untouched(): + segs = [{"start_s": 0, "end_s": 5}, {"start_s": 10, "end_s": 12}] + out = editmod.normalize_edit_spec({"segments": segs}) + assert len(out["segments"]) == 2 diff --git a/backend/tests/test_urlguard.py b/backend/tests/test_urlguard.py new file mode 100644 index 00000000..9cbd5abf --- /dev/null +++ b/backend/tests/test_urlguard.py @@ -0,0 +1,87 @@ +"""SEC-12 · the download URL guard. + +The Download Center hands a user-supplied URL to yt-dlp on the worker, so the server fetches +whatever the user typed. These pin that a URL resolving into our own network is refused, that the +refusal reasons stay distinct (an unresolvable host is not the same message as a blocked one), and +that a normal public URL still goes through. + +DNS is stubbed rather than live: the point under test is the address JUDGEMENT, and a test that +resolves real names would be slow, offline-fragile, and would silently start passing if the guard +stopped resolving at all. +""" + +import pytest + +from app.downloads import urlguard + + +def _stub_dns(monkeypatch, addrs): + """Make every lookup answer with `addrs` (a list of textual IPs).""" + + def fake_getaddrinfo(host, port, *a, **kw): + return [(None, None, None, "", (addr, port or 0)) for addr in addrs] + + monkeypatch.setattr(urlguard.socket, "getaddrinfo", fake_getaddrinfo) + + +def _blocked_reason(url): + with pytest.raises(urlguard.BlockedUrl) as e: + urlguard.check_url(url) + return e.value.reason + + +@pytest.mark.parametrize( + "addr", + [ + "127.0.0.1", # loopback + "10.1.2.3", # RFC1918 + "192.168.0.5", # RFC1918 + "172.16.9.9", # RFC1918 + "169.254.169.254", # link-local — the cloud metadata endpoint + "0.0.0.0", # unspecified + "::1", # IPv6 loopback + "fe80::1", # IPv6 link-local + "fc00::1", # IPv6 unique-local + "::ffff:127.0.0.1", # loopback wearing an IPv6 hat + ], +) +def test_addresses_on_our_own_network_are_refused(monkeypatch, addr): + _stub_dns(monkeypatch, [addr]) + assert _blocked_reason("http://whatever.example/video") == urlguard.REASON_PRIVATE + + +def test_one_bad_answer_in_a_round_robin_blocks_the_whole_host(monkeypatch): + # A host answering with both a public and an internal address must not become a coin flip: + # yt-dlp would pick for itself, so the guard refuses if ANY answer is forbidden. + _stub_dns(monkeypatch, ["93.184.216.34", "127.0.0.1"]) + assert _blocked_reason("https://mixed.example/v") == urlguard.REASON_PRIVATE + + +def test_a_public_address_passes(monkeypatch): + _stub_dns(monkeypatch, ["93.184.216.34"]) + urlguard.check_url("https://example.com/watch?v=abc") # does not raise + + +def test_unresolvable_is_its_own_reason(monkeypatch): + import socket as _socket + + def boom(*a, **kw): + raise _socket.gaierror("nope") + + monkeypatch.setattr(urlguard.socket, "getaddrinfo", boom) + # Distinct from PRIVATE on purpose: "we couldn't look that up" and "we refuse to fetch that" + # are different things to tell a user, and the frontend renders them differently. + assert _blocked_reason("https://nx.example/v") == urlguard.REASON_UNRESOLVABLE + + +@pytest.mark.parametrize("url", ["file:///etc/passwd", "ftp://host/x", "gopher://host/1"]) +def test_non_http_schemes_are_refused_before_any_lookup(monkeypatch, url): + def boom(*a, **kw): # a scheme rejection must not have resolved anything first + raise AssertionError("DNS was consulted for a non-http scheme") + + monkeypatch.setattr(urlguard.socket, "getaddrinfo", boom) + assert _blocked_reason(url) == urlguard.REASON_SCHEME + + +def test_a_url_without_a_host_is_refused(): + assert _blocked_reason("http:///just/a/path") == urlguard.REASON_NO_HOST diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index 6bd6082f..f8da3a4a 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -229,6 +229,13 @@ "tooLarge": "That file is too large (max {{size}}).", "empty": "That file is empty." }, + "urlBlocked": { + "scheme": "Only http:// and https:// links can be downloaded.", + "no_host": "That link doesn't contain a website address.", + "unresolvable": "That address couldn't be looked up. Check the link and try again.", + "private": "That address is on the server's own network, so it can't be downloaded from.", + "generic": "That address can't be downloaded from." + }, "fail": { "title": "Download failed", "detailsLink": "details", diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index 3a61b69b..d595a028 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -3,6 +3,7 @@ "generic": "The server couldn't carry out that action. Please try again.", "validation": "Some of the values you entered weren't valid. Please check them and try again.", "server": "Server error", + "syncCooldown": "You've synced a few times just now — give it a few minutes. The YouTube budget is shared with everyone on this instance, and background syncing keeps running either way.", "ok": "OK", "offline": { "title": "Connection lost", diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 56ab3e54..4570dc3e 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -229,6 +229,13 @@ "tooLarge": "Ez a fájl túl nagy (legfeljebb {{size}}).", "empty": "Ez a fájl üres." }, + "urlBlocked": { + "scheme": "Csak http:// és https:// hivatkozás tölthető le.", + "no_host": "Ez a hivatkozás nem tartalmaz webcímet.", + "unresolvable": "Ezt a címet nem sikerült feloldani. Ellenőrizd a hivatkozást, és próbáld újra.", + "private": "Ez a cím a szerver saját hálózatán van, ezért nem tölthető le róla.", + "generic": "Erről a címről nem lehet letölteni." + }, "fail": { "title": "A letöltés nem sikerült", "detailsLink": "részletek", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index a8847c5c..3fcd3f93 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -3,6 +3,7 @@ "generic": "A szerver nem tudta végrehajtani a műveletet. Próbáld újra.", "validation": "Néhány megadott érték érvénytelen volt. Ellenőrizd, majd próbáld újra.", "server": "Szerverhiba", + "syncCooldown": "Az imént többször is szinkronizáltál — várj pár percet. A YouTube-keret közös az instance összes felhasználójával, a háttérszinkron pedig így is fut tovább.", "ok": "OK", "offline": { "title": "Megszakadt a kapcsolat", diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index 31af97ee..74b6db05 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -72,6 +72,14 @@ function localizeDetail(d: unknown): string { if (o?.code === "upload_too_large") return t("downloads.errors.upload.tooLarge", { size: formatBytes(o.limit) }); if (o?.code === "upload_empty") return t("downloads.errors.upload.empty"); + // SEC-12: a download URL the server refuses to fetch. The reasons are deliberately distinct — + // "we could not look that host up" is a different thing to tell someone than "we will not fetch + // from that address" — so an unknown reason falls back to the generic refusal rather than a key. + if (o?.code === "url_blocked") + return t(`downloads.errors.urlBlocked.${o.reason}`, t("downloads.errors.urlBlocked.generic")); + // SEC-14: a manual sync trigger on cooldown. Not an error the user caused so much as a pace + // limit, and the copy says so. + if (o?.code === "sync_cooldown") return t("errors.syncCooldown"); return t("errors.generic"); }