peter
358d722ce9
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.
100 lines
4.1 KiB
Python
100 lines
4.1 KiB
Python
"""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
|
|
|
|
|
|
def test_a_malformed_url_is_a_refusal_not_a_crash(monkeypatch):
|
|
# Both shapes raise ValueError LATE — `urlparse` on an unclosed bracket, `.port` only when the
|
|
# property is READ — and `.hostname` happily returns first. Unguarded, either turns a user typo
|
|
# into a 500 that escapes the caller's `except BlockedUrl` and loses the localized detail this
|
|
# module exists to produce.
|
|
_stub_dns(monkeypatch, ["93.184.216.34"])
|
|
assert _blocked_reason("http://example.com:99999/v.mp4") == urlguard.REASON_MALFORMED
|
|
assert _blocked_reason("http://example.com:abc/v.mp4") == urlguard.REASON_MALFORMED
|
|
assert _blocked_reason("http://[::1/v.mp4") == urlguard.REASON_MALFORMED
|
|
assert _blocked_reason("http://[example.com]/v.mp4") == urlguard.REASON_MALFORMED
|