fix(e12): close the eighteen findings the E12 review round left standing
The deep review of the branch raised 47 findings, 18 of which survived adversarial verification. Grouped by what they actually were: **The size ceiling inverted at its boundary.** `remaining_bytes` clamped to 0 for a user already at their storage cap, and `build_ydl_opts` reads 0 as "no limit" — so the one user over quota was the only one downloading with no ceiling at all. `check_enqueue` could not catch it: it ran while the job was queued and the user was still under the cap. The arithmetic moves into `quota.remaining_bytes` (where every other max_bytes decision lives, and where it can be tested), and the worker now fails the job outright rather than proceeding unbounded — as a non-retryable `quota_exceeded`, since retrying cannot help until space is freed. **Silent failures in the new account panel.** All three actions went through `quiet: true` endpoints, which suppress the global error dialog on the promise that the caller reports inline — with no `onError`, that promise was broken and a refusal rendered as nothing. On the broken-SMTP instance this panel exists for, an admin would read silence as "the mail went out". The co-invalidation set and the email-off predicate move to `lib/adminUsers`, shared with the pending-access queue, so the two copies of these mutations cannot drift apart again — which is how half of this was lost in the first place. **A rollback recipe that destroyed data without working.** `alembic downgrade -1` steps back one migration, but a release routinely ships several (0.80.0 shipped four): it drops the newest one's columns and leaves the schema at a revision the older image also cannot resolve, i.e. the crash loop it was meant to escape, minus the data. The dump restore is now the recommended path, and the alembic route asks the target version for its own head revision instead of counting backwards. Also: redirects named in the urlguard docstring beside the DNS-rebinding residual they share a fix with (SEC-17); a malformed port refused as `bad_port` instead of escaping as a 500; the pull cooldown extended to the sibling route spending the same QuotaAction; restore stops the app before dropping its tables; the PowerShell cleanup can no longer decide the script's verdict on 5.1, nor report the wrong exit code; the documented one-liners hardened to match the scripts; the registry's retention window stated where pinning is advertised; the two banners made mutually exclusive so neither can shadow the other; `hover:bg-hover` (a utility this project never generated) replaced by the `selectable` recipe; the clipboard helper reused; and the sync cooldown's copy swept to the two callers that discarded it. Verified: gate green (vitest 678, pytest 949), and the panel, the row hover and the reset banner UAT'd on localdev.
This commit is contained in:
@@ -85,3 +85,28 @@ def test_the_cooldown_is_per_user_not_global(db, as_user, make_user, monkeypatch
|
||||
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
|
||||
|
||||
|
||||
def test_the_single_playlist_repull_shares_the_pull_cooldown(db, as_user, make_user, monkeypatch):
|
||||
# SEC-14, the sweep half: `revert-youtube` spends under the SAME QuotaAction.PLAYLISTS_PULL key
|
||||
# as the bulk `sync-youtube`, so limiting only one of them left the identical drain open one
|
||||
# route over. Both must draw on the one allowance.
|
||||
from app.models import Playlist
|
||||
from app.routes import playlists as plroutes
|
||||
|
||||
user = make_user()
|
||||
pl = Playlist(user_id=user.id, name="Linked", kind="youtube", yt_playlist_id="PL123")
|
||||
db.add(pl)
|
||||
db.commit()
|
||||
monkeypatch.setattr(plroutes, "has_read_scope", lambda u: True)
|
||||
monkeypatch.setattr(plroutes, "repull_playlist", lambda d, u, p: {"ok": True})
|
||||
monkeypatch.setattr(plroutes, "sync_user_playlists", lambda d, u: {"ok": True})
|
||||
|
||||
c = as_user(user)
|
||||
codes = [c.post(f"/api/playlists/{pl.id}/revert-youtube").status_code for _ in range(4)]
|
||||
assert codes[:3] == [200, 200, 200]
|
||||
assert codes[3] == 429
|
||||
# One allowance, not one each: the bulk route is now on cooldown too.
|
||||
r = c.post("/api/playlists/sync-youtube")
|
||||
assert r.status_code == 429
|
||||
assert r.json()["detail"] == {"code": "sync_cooldown"}
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
"""Category classification of raw yt-dlp / ffmpeg download-failure strings (S2)."""
|
||||
|
||||
from app.downloads import errors as dlerrors
|
||||
from app.downloads.errors import classify
|
||||
|
||||
|
||||
@@ -54,3 +55,22 @@ def test_login_beats_generic_unavailable_when_both_present():
|
||||
"This video is unavailable"
|
||||
)
|
||||
assert classify(msg) == "login_required"
|
||||
|
||||
|
||||
def test_our_own_worker_refusals_classify_as_themselves():
|
||||
# The worker raises these itself (not yt-dlp), and the rule that catches each one matches only a
|
||||
# fragment of the text. Pinning message and rule together is what stops an edit to the wording
|
||||
# from silently orphaning the category — the UI would then fall back to "Download failed".
|
||||
assert classify(dlerrors.QUOTA_FULL_MESSAGE) == "quota_exceeded"
|
||||
|
||||
|
||||
def test_a_url_jobs_network_failure_is_redacted_but_youtube_keeps_its_text():
|
||||
# SEC-12: the raw transport text for a user-chosen host:port is a port-scanner readout, so it is
|
||||
# replaced for `url` jobs. Everything else must survive verbatim, or a real diagnosis is lost.
|
||||
raw = "ERROR: Unable to download webpage: <urlopen error [Errno 111] Connection refused>"
|
||||
assert dlerrors.user_error_text("url", raw, "network") == dlerrors.NETWORK_REDACTED_MESSAGE
|
||||
# Same failure, but the destination was never user-chosen — nothing to leak, keep the detail.
|
||||
assert dlerrors.user_error_text("youtube", raw, "network") == raw
|
||||
# A user-chosen host, but a category that says something real about the CONTENT, not the network.
|
||||
login = "ERROR: [facebook] 123: You must be logged in to see this post"
|
||||
assert dlerrors.user_error_text("url", login, "login_required") == login
|
||||
|
||||
@@ -10,6 +10,7 @@ Uses the `db` fixture (real Postgres) — skipped when no database is reachable.
|
||||
its OWN session, so every test COMMITS its setup before calling it.
|
||||
"""
|
||||
|
||||
from app.downloads import quota
|
||||
from app.models import DownloadJob, DownloadQuota, MediaAsset, User
|
||||
from app.worker import _claim_job
|
||||
|
||||
@@ -169,3 +170,61 @@ def test_list_downloads_is_capped(db, monkeypatch):
|
||||
monkeypatch.setattr(dl, "_MAX_LIST", 2)
|
||||
rows = dl.list_downloads(user=u, db=db)
|
||||
assert len(rows) == 2 # bounded by the cap, not the 3 rows on disk
|
||||
|
||||
|
||||
# --- E12 · S3 · the worker-side max_bytes remainder ------------------------------------------
|
||||
|
||||
|
||||
def _sized_asset(db, size_bytes, ref):
|
||||
a = MediaAsset(
|
||||
source_kind="url",
|
||||
source_ref=ref,
|
||||
format_sig="s",
|
||||
status="ready",
|
||||
ref_count=1,
|
||||
size_bytes=size_bytes,
|
||||
)
|
||||
db.add(a)
|
||||
db.flush()
|
||||
return a
|
||||
|
||||
|
||||
def test_remaining_bytes_is_none_for_an_unlimited_user(db):
|
||||
u = User(email="unlimited@example.com")
|
||||
db.add(u)
|
||||
db.flush()
|
||||
_quota(db, u.id, max_concurrent=2, unlimited=True)
|
||||
db.flush()
|
||||
# None, not a number: "no cap" and "no budget left" must never be the same value downstream —
|
||||
# build_ydl_opts leaves max_filesize unset for both, so only the caller can tell them apart.
|
||||
assert quota.remaining_bytes(db, u.id) is None
|
||||
|
||||
|
||||
def test_remaining_bytes_is_the_unspent_part_of_the_cap(db):
|
||||
u = User(email="partial@example.com")
|
||||
db.add(u)
|
||||
db.flush()
|
||||
db.add(
|
||||
DownloadQuota(user_id=u.id, max_bytes=1000, max_concurrent=2, max_jobs=100, unlimited=False)
|
||||
)
|
||||
a = _sized_asset(db, 400, "partial-1")
|
||||
_job(db, u.id, queue_pos=0, status="done", asset_id=a.id)
|
||||
db.flush()
|
||||
assert quota.remaining_bytes(db, u.id) == 600
|
||||
|
||||
|
||||
def test_remaining_bytes_is_zero_once_the_cap_is_met(db):
|
||||
# THE case this exists for: `check_enqueue` let these jobs in while the user was under the cap,
|
||||
# and an earlier one has filled it since. Zero is what makes the worker refuse; if this ever
|
||||
# returned the raw negative or None instead, the user already over quota would be the only one
|
||||
# downloading with no size ceiling at all.
|
||||
u = User(email="atcap@example.com")
|
||||
db.add(u)
|
||||
db.flush()
|
||||
db.add(
|
||||
DownloadQuota(user_id=u.id, max_bytes=1000, max_concurrent=2, max_jobs=100, unlimited=False)
|
||||
)
|
||||
a = _sized_asset(db, 1500, "atcap-1") # overshot: one job finished larger than the remainder
|
||||
_job(db, u.id, queue_pos=0, status="done", asset_id=a.id)
|
||||
db.flush()
|
||||
assert quota.remaining_bytes(db, u.id) == 0
|
||||
|
||||
@@ -85,3 +85,12 @@ def test_non_http_schemes_are_refused_before_any_lookup(monkeypatch, url):
|
||||
|
||||
def test_a_url_without_a_host_is_refused():
|
||||
assert _blocked_reason("http:///just/a/path") == urlguard.REASON_NO_HOST
|
||||
|
||||
|
||||
def test_a_malformed_port_is_a_refusal_not_a_crash(monkeypatch):
|
||||
# `urlparse(...).port` raises ValueError on ACCESS, not at parse time, and `.hostname` returns
|
||||
# first — so an unguarded read 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_BAD_PORT
|
||||
assert _blocked_reason("http://example.com:abc/v.mp4") == urlguard.REASON_BAD_PORT
|
||||
|
||||
Reference in New Issue
Block a user