Round two of the review loop, on round one's own diff. Two of its findings were regressions the fixes themselves introduced, which is exactly what the loop is for: **A per-user refusal was writing onto a shared row.** MediaAssets are deduped across users, and the generic failure handler puts the message on the asset — right for an upstream failure, wrong for "your storage is full". Two users queuing the same video share one asset, so the first user hitting their cap failed the second user's job with the first one's private reason, telling someone with an empty library to free up space. The refusal now raises `_OwnerRefused`, caught separately: the job fails, the asset goes back to `pending` for whoever else holds it. Pinned by a test that drives `_download` directly — the refusal happens before extraction, so nothing needs stubbing. **The restore scripts restarted the app, undoing the restore.** `docker compose start` re-runs the container command, and that runs `alembic upgrade head` — so on the rollback path this documents as recommended (restore the dump, THEN pin the older tag) the newer image immediately re-applied the migrations the restore had just undone, and on a FAILED restore it put the app back on a half-restored schema. Both are the opposite of why the stop was added. Nothing is restarted automatically now; the scripts say what to run and when, and the docs' snippets say the same. **Reverted rather than patched:** the OnboardingWizard toast (the wizard already renders that failure inline, next to a Retry button — three recoveries for one failure), and `useVerifyEntry`'s "each new link clears the other". The latter blanked the signed-out card's only banner, where nothing renders the reset flag to replace it. The two states are independent again; what changes is that `dismiss` peels ONE layer, which is what the original finding was actually about. Also: `Stats` uses the YouTube helper rather than the bare one, so the route's English 403 detail no longer beats the translated fallback under a Hungarian UI; `urlparse` guarded alongside `.port` (an unclosed bracket raised just as late), the reason renamed `malformed` since "bad port" misdescribes it; three docstrings narrowed to what the code actually does (the worker's byte check is per-job, not an aggregate ledger, and not on the edit path; the redirect residual leaks a reachability oracle, which the note had denied; the invalidation helper does not cover suspend); the pasted Windows recipe wrapped so `throw` can stop it; the bash one-liner's failure branch no longer exits 0; and the tests that were missing for the round-one fixes — the account panel's error paths, the invalidation set, and the shared-asset case above. Gate green: vitest 685, pytest 950.
109 lines
5.4 KiB
Python
109 lines
5.4 KiB
Python
"""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, because both need the same fix — gating
|
|
the connection itself with a custom socket factory inside yt-dlp, a much larger change than this:
|
|
|
|
* DNS rebinding: a host that passes the worker's check and then answers yt-dlp's own resolution
|
|
with an internal address.
|
|
* HTTP redirects: a public host that answers 3xx with an internal ``Location``. yt-dlp follows it,
|
|
and only the URL the user TYPED was ever judged here. An attacker-controlled host or any public
|
|
open redirect reaches it, so this is the cheaper of the two. It does NOT usually yield the
|
|
response body — a non-media body fails the mp4 remux and stores nothing — but it does hand back
|
|
the readout above: the redaction in ``errors.user_error_text`` fires only for the ``network``
|
|
class, so a dead internal port (refused → ``network`` → redacted) is distinguishable from a live
|
|
one (an HTTP answer that is not media → ``unsupported_host``/``format_unavailable``, raw text
|
|
kept). Tracked as SEC-17.
|
|
|
|
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_MALFORMED = "malformed" # unparseable authority or port; urlparse raises late, so guard it
|
|
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."""
|
|
# `urlparse` itself raises ValueError on a malformed authority ("http://[::1/x" — an unclosed
|
|
# bracket), and `.port` raises on ":99999" / ":abc" when READ rather than at parse time. Both
|
|
# escape the caller's `except BlockedUrl` as a 500 unless caught here, which is the opposite of
|
|
# what this module exists to do: turn a bad URL into a structured, localizable refusal.
|
|
try:
|
|
u = urlparse((raw or "").strip())
|
|
port = u.port
|
|
except ValueError as exc:
|
|
raise BlockedUrl(REASON_MALFORMED) from exc
|
|
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, 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)
|