From 358d722ce93e378d9b4ef00c183ee8c2ee4d1050 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 5 Sep 2026 04:02:57 +0200 Subject: [PATCH] fix(e12): close what round one broke, and revert two fixes that overreached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/app/downloads/errors.py | 16 +- backend/app/downloads/pipeline.py | 31 +++- backend/app/downloads/quota.py | 11 +- backend/app/downloads/urlguard.py | 29 ++-- backend/app/routes/downloads.py | 10 +- backend/tests/test_abuse_limits.py | 2 +- backend/tests/test_download_limits.py | 45 +++++- backend/tests/test_urlguard.py | 15 +- docs/self-hosting.md | 34 +++-- frontend/src/components/AdminUsers.tsx | 1 - frontend/src/components/OnboardingWizard.tsx | 5 - frontend/src/components/Stats.tsx | 11 +- frontend/src/components/VerifyBanner.tsx | 9 +- .../src/components/accountPanel.dom.test.tsx | 144 ++++++++++++++++++ frontend/src/i18n/locales/en/downloads.json | 2 +- frontend/src/i18n/locales/en/onboarding.json | 3 +- frontend/src/i18n/locales/hu/downloads.json | 2 +- frontend/src/i18n/locales/hu/onboarding.json | 3 +- frontend/src/lib/adminUsers.test.ts | 55 +++++++ frontend/src/lib/adminUsers.ts | 6 +- frontend/src/lib/useVerifyEntry.ts | 26 ++-- frontend/src/lib/verifyEntry.dom.test.tsx | 35 ++--- scripts/backup.ps1 | 10 +- scripts/restore.ps1 | 45 +++--- scripts/restore.sh | 18 ++- 25 files changed, 443 insertions(+), 125 deletions(-) create mode 100644 frontend/src/components/accountPanel.dom.test.tsx create mode 100644 frontend/src/lib/adminUsers.test.ts diff --git a/backend/app/downloads/errors.py b/backend/app/downloads/errors.py index 335af9cd..d4f8f403 100644 --- a/backend/app/downloads/errors.py +++ b/backend/app/downloads/errors.py @@ -8,10 +8,18 @@ explanation (and decides whether a retry makes sense). The raw text stays availa `_RETRIABLE_MARKERS` — kept here as the single source of truth for "what kind of failure is this". """ -# Failure texts the WORKER itself raises, as opposed to ones yt-dlp/ffmpeg produce. They live here, -# beside the rules that classify them, because a message and its category drifting apart is exactly -# how a category silently becomes unreachable — the rule below matches a fragment of each, and -# test_download_errors pins that they still classify as intended. +# Two worker-authored strings that this module owns because both are coupled to `_RULES`. +# +# QUOTA_FULL_MESSAGE is raised by the worker and must classify back to `quota_exceeded`; its rule +# below matches a fragment of it, and test_download_errors pins the pair, because a message and its +# rule drifting apart is exactly how a category silently becomes unreachable. +# +# NETWORK_REDACTED_MESSAGE is the opposite direction: it REPLACES a message after classification, so +# it deliberately has no rule. Nothing re-classifies a stored `job.error` today; if anything ever +# does, it must carry the original code rather than re-deriving one from this text. +# +# Other worker-raised texts (ffmpeg, "produced no output file", the urlguard refusal) are still +# inline literals at their raise sites — pinned by markers here, but not owned here. QUOTA_FULL_MESSAGE = "Your download storage is full — free up space, then try again." NETWORK_REDACTED_MESSAGE = "The server could not reach that address." diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index a83512b0..4cc3ef27 100644 --- a/backend/app/downloads/pipeline.py +++ b/backend/app/downloads/pipeline.py @@ -37,6 +37,18 @@ class _Aborted(Exception): """Raised inside the progress hook when the job was paused/canceled mid-download.""" +class _OwnerRefused(Exception): + """A refusal that belongs to THIS job's owner, not to the source. + + MediaAssets are shared: two users asking for the same video and format get one row (see + `service.get_or_create_asset`), and the generic failure handler writes the message onto it — + correctly, because an upstream failure really is everyone's. A per-user refusal is not. Written + to the shared row it would fail a second user's queued job for the same video with the first + user's private reason (`worker._process_job` reads `asset.error` verbatim), telling someone with + an empty library to free up space. So this is caught separately: the job fails, the asset goes + back to `pending` for whoever else is holding it.""" + + # Both progress sources call back FAR faster than the database should be touched: yt-dlp fires its # hook per chunk, and ffmpeg's `-progress pipe:1` emits a block of ~12 lines every ~0.5s while # run_ffmpeg calls its callbacks once per LINE. Every callback here is a round-trip (`_job_status` @@ -452,7 +464,7 @@ def _download( # the only one downloading unbounded. Fail the job here instead. Not retryable until they # free space, which is what the `quota_exceeded` category tells them. if remaining_bytes == 0: - raise RuntimeError(dlerrors.QUOTA_FULL_MESSAGE) + raise _OwnerRefused(dlerrors.QUOTA_FULL_MESSAGE) info = _media_info( _extract_with_fallback( job_id, @@ -554,6 +566,23 @@ def _download( # job's status (the route set it to paused/canceled). ref_count is adjusted by the route. _reset_asset_pending(asset_id) log.info("job %s aborted (pause/cancel)", job_id) + except _OwnerRefused as exc: + # Fail this job only. The asset is untouched and goes back to `pending`, so another user's + # job for the same video is claimed and downloaded normally instead of inheriting a reason + # that is about someone else's account. + _reset_asset_pending(asset_id) + msg = str(exc)[:500] + with SessionLocal() as db: + service.finalize_if_running( + db, + job_id, + status=JobStatus.ERROR, + error=msg, + error_code=dlerrors.classify(msg), + phase=None, + ) + db.commit() + log.info("job %s refused for its owner: %s", job_id, msg) except Exception as exc: # noqa: BLE001 — surface any failure to the user, keep worker alive msg = str(exc)[:500] code = dlerrors.classify(msg) diff --git a/backend/app/downloads/quota.py b/backend/app/downloads/quota.py index caf26e00..2fcd174e 100644 --- a/backend/app/downloads/quota.py +++ b/backend/app/downloads/quota.py @@ -6,9 +6,14 @@ Limits come from a per-user DownloadQuota row if the admin set one, else the sys `unlimited` (e.g. the admin's own account) bypasses the byte cap. Enforcement split: max_jobs + max_bytes are checked at enqueue (they bound holdings); per-user -max_concurrent is enforced by the worker at claim time (it bounds simultaneous downloads). max_bytes -is checked a SECOND time in the worker, via `remaining_bytes` — enqueue can only compare the -footprint as it stood when the job was created, and earlier jobs may have filled the cap since. +max_concurrent is enforced by the worker at claim time (it bounds simultaneous downloads). + +DOWNLOAD jobs additionally consult `remaining_bytes` in the worker, because enqueue can only compare +the footprint as it stood when the job was created and earlier jobs may have filled the cap since. +That is a per-job ceiling, not an aggregate ledger: concurrent jobs of one user each read the same +remainder (in-flight bytes are not `ready`, so `footprint` cannot see them), so a wave of +max_concurrent downloads can still land over the cap — the deliberate trade, since a download's size +is unknown until it finishes. Edit jobs do not consult it at all (tracked as SEC-19). """ from dataclasses import dataclass diff --git a/backend/app/downloads/urlguard.py b/backend/app/downloads/urlguard.py index 63d11d30..3ca5a946 100644 --- a/backend/app/downloads/urlguard.py +++ b/backend/app/downloads/urlguard.py @@ -17,9 +17,12 @@ the connection itself with a custom socket factory inside yt-dlp, a much larger 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 — but it still buys only what a - media-shaped internal response yields, since a non-media body fails the mp4 remux and stores - nothing. Tracked as SEC-17. + 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. @@ -36,7 +39,7 @@ from urllib.parse import urlparse # vocabulary (frontend `localizeDetail`) — keep them stable. REASON_SCHEME = "scheme" # not http(s) REASON_NO_HOST = "no_host" # URL carries no hostname -REASON_BAD_PORT = "bad_port" # ":99999" / ":abc" — urlparse only raises on ACCESS, so guard it +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 @@ -82,20 +85,20 @@ 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()) + # `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) - # `.port` is a lazily-parsed property: it raises ValueError for ":99999" or ":abc" on ACCESS, - # not at urlparse() time, and `.hostname` above happily returns first. Unguarded, one typo - # escapes the caller's `except BlockedUrl` as a 500 instead of the structured 422 this whole - # module exists to return. - try: - port = u.port - except ValueError as exc: - raise BlockedUrl(REASON_BAD_PORT) from exc for addr in _resolve(host, port): try: ip = ipaddress.ip_address(addr) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 7d55121e..2333389e 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -66,7 +66,15 @@ def resolve_source(raw: str) -> tuple[str, str]: if _VIDEO_ID.match(raw): return "youtube", raw if raw.startswith(("http://", "https://")): - u = urlparse(raw) + # `urlparse` raises ValueError on a malformed authority ("http://[::1/x"), and this runs + # BEFORE the urlguard check that would otherwise turn it into a structured refusal — so + # unguarded, that typo escapes as a 500. Same answer as any other unusable link. + try: + u = urlparse(raw) + except ValueError as exc: + raise HTTPException( + status_code=400, detail="That doesn't look like a valid video link." + ) from exc host = u.netloc.lower() if "youtube.com" in host: if u.path.startswith(("/shorts/", "/embed/", "/live/")): diff --git a/backend/tests/test_abuse_limits.py b/backend/tests/test_abuse_limits.py index 3ebd011b..a6e9c80e 100644 --- a/backend/tests/test_abuse_limits.py +++ b/backend/tests/test_abuse_limits.py @@ -95,7 +95,7 @@ def test_the_single_playlist_repull_shares_the_pull_cooldown(db, as_user, make_u from app.routes import playlists as plroutes user = make_user() - pl = Playlist(user_id=user.id, name="Linked", kind="youtube", yt_playlist_id="PL123") + pl = Playlist(user_id=user.id, name="Linked", source="youtube", yt_playlist_id="PL123") db.add(pl) db.commit() monkeypatch.setattr(plroutes, "has_read_scope", lambda u: True) diff --git a/backend/tests/test_download_limits.py b/backend/tests/test_download_limits.py index 918b72fc..30689251 100644 --- a/backend/tests/test_download_limits.py +++ b/backend/tests/test_download_limits.py @@ -10,7 +10,8 @@ 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.config import settings +from app.downloads import pipeline, quota from app.models import DownloadJob, DownloadQuota, MediaAsset, User from app.worker import _claim_job @@ -228,3 +229,45 @@ def test_remaining_bytes_is_zero_once_the_cap_is_met(db): _job(db, u.id, queue_pos=0, status="done", asset_id=a.id) db.flush() assert quota.remaining_bytes(db, u.id) == 0 + + +def test_a_refusal_for_one_user_does_not_poison_the_shared_asset(db, monkeypatch, tmp_path): + """A user at their cap fails their own job, and another user's queued job for the same video + still downloads. + + MediaAssets are deduped across users, and the generic failure handler writes the message onto + that shared row — correct for an upstream failure, wrong for "YOUR storage is full", which + `worker._process_job` would then hand verbatim to someone with an empty library. + """ + monkeypatch.setattr(settings, "download_root", str(tmp_path)) + over, other = User(email="over@example.com"), User(email="other@example.com") + db.add_all([over, other]) + db.flush() + db.add( + DownloadQuota( + user_id=over.id, max_bytes=1000, max_concurrent=2, max_jobs=100, unlimited=False + ) + ) + full = _sized_asset(db, 1500, "already-held") + _job(db, over.id, queue_pos=0, status="done", asset_id=full.id) + + shared = MediaAsset( + source_kind="youtube", source_ref="x" * 11, format_sig="s", status="pending", ref_count=2 + ) + db.add(shared) + db.flush() + mine = _job(db, over.id, queue_pos=1, status="running", asset_id=shared.id) + theirs = _job(db, other.id, queue_pos=2, asset_id=shared.id) + db.commit() + + # The refusal happens before extraction, so nothing needs stubbing — if it ever stops happening + # first, this test starts reaching the network, which is its own signal. + pipeline._download(mine.id, shared.id, "youtube", "x" * 11, {}, over.id) + + db.expire_all() + assert db.get(DownloadJob, mine.id).status == "error" + assert db.get(DownloadJob, mine.id).error_code == "quota_exceeded" + # The asset is released, NOT marked error — otherwise the next line's job inherits the refusal. + assert db.get(MediaAsset, shared.id).status == "pending" + assert db.get(MediaAsset, shared.id).error is None + assert db.get(DownloadJob, theirs.id).status == "queued" diff --git a/backend/tests/test_urlguard.py b/backend/tests/test_urlguard.py index 85e63909..9a9ca11d 100644 --- a/backend/tests/test_urlguard.py +++ b/backend/tests/test_urlguard.py @@ -87,10 +87,13 @@ 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. +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_BAD_PORT - assert _blocked_reason("http://example.com:abc/v.mp4") == urlguard.REASON_BAD_PORT + 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 diff --git a/docs/self-hosting.md b/docs/self-hosting.md index 085fd17b..b6b0731d 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -183,7 +183,7 @@ mkdir -p backups && out="backups/siftlode-$(date +%Y%m%d-%H%M%S).dump" && \ docker compose -f docker-compose.selfhost.yml exec -T db \ pg_dump -U siftlode -Fc siftlode > "$out.part" \ && [ -s "$out.part" ] && mv "$out.part" "$out" && echo "wrote $out" \ - || { rm -f "$out.part"; echo "BACKUP FAILED"; } + || { rm -f "$out.part"; echo "BACKUP FAILED" >&2; false; } # Restore. Stop the app first: pg_restore --clean DROPs each object in its own transaction, and a # running api holds locks that make those DROPs queue — abort the wait and you are left with a @@ -192,7 +192,11 @@ mkdir -p backups && out="backups/siftlode-$(date +%Y%m%d-%H%M%S).dump" && \ docker compose -f docker-compose.selfhost.yml stop api worker docker compose -f docker-compose.selfhost.yml exec -T db \ pg_restore -U siftlode -d siftlode --clean --if-exists < backups/siftlode-.dump -docker compose -f docker-compose.selfhost.yml up -d + +# Start it again only once the restore reported no errors — and, if you are rolling a release back, +# only AFTER pinning the older IMAGE_TAG. Every start runs `alembic upgrade head`, so starting the +# newer image here would re-apply the migrations you just restored away. +docker compose -f docker-compose.selfhost.yml start api worker ``` On Windows, do **not** pipe the dump through PowerShell — it corrupts binary data. Write it inside the @@ -200,13 +204,24 @@ container and copy it out instead. Check the exit code between the two steps and name: the dump inside the container keeps its name, so a `cp` that runs after a *failed* dump would copy out the previous run's file and quietly overwrite your only backup with a stale one. +Paste it as ONE block. `throw` only aborts the statement it is in, so as separate pasted lines the +copy would still run after a failed dump — and copy out the previous run's file: + ```powershell -New-Item -ItemType Directory -Force -Path backups | Out-Null -$out = "backups\siftlode-$(Get-Date -Format yyyyMMdd-HHmmss).dump" -docker compose -f docker-compose.selfhost.yml exec -T db pg_dump -U siftlode -Fc siftlode -f /tmp/s.dump -if ($LASTEXITCODE -ne 0) { throw "BACKUP FAILED - pg_dump returned $LASTEXITCODE" } -docker compose -f docker-compose.selfhost.yml cp db:/tmp/s.dump $out -docker compose -f docker-compose.selfhost.yml exec -T db rm -f /tmp/s.dump +& { + $compose = "docker-compose.selfhost.yml" + New-Item -ItemType Directory -Force -Path backups | Out-Null + $out = "backups\siftlode-$(Get-Date -Format yyyyMMdd-HHmmss).dump" + docker compose -f $compose exec -T db pg_dump -U siftlode -Fc siftlode -f /tmp/s.dump + $code = $LASTEXITCODE + if ($code -ne 0) { throw "BACKUP FAILED - pg_dump returned $code" } + docker compose -f $compose cp db:/tmp/s.dump $out + $code = $LASTEXITCODE + docker compose -f $compose exec -T db rm -f /tmp/s.dump + if ($code -ne 0) { throw "BACKUP FAILED - could not copy the dump out (exit $code)" } + if ((Get-Item $out).Length -eq 0) { Remove-Item $out; throw "BACKUP FAILED - the dump came out empty" } + "wrote $out" +} ``` ## Upgrading @@ -250,7 +265,8 @@ exactly where the old image expects it, in one step, with no revision arithmetic ```bash # 1. stop the app (leave the database up) docker compose -f docker-compose.selfhost.yml stop api worker -# 2. restore the dump you took before upgrading (see Backups above) +# 2. restore the dump you took before upgrading (see Backups above). Leave the app stopped: +# starting it here would run the NEWER image's migrations against the database you just restored. docker compose -f docker-compose.selfhost.yml exec -T db \ pg_restore -U siftlode -d siftlode --clean --if-exists < backups/siftlode-.dump # 3. pin the previous version in .env, then start again diff --git a/frontend/src/components/AdminUsers.tsx b/frontend/src/components/AdminUsers.tsx index 6b9fc0bc..51479081 100644 --- a/frontend/src/components/AdminUsers.tsx +++ b/frontend/src/components/AdminUsers.tsx @@ -387,7 +387,6 @@ function PendingAccess() { ); } -/** Did the resend fail because this instance has no working SMTP? */ // One stuck sign-up. The row names WHICH gate is shut rather than just offering buttons — the same // reasoning as the sign-in card's blocked panel, from the admin's side: an admin about to let someone // in should see that the address has not been confirmed yet, because on a no-SMTP instance that is diff --git a/frontend/src/components/OnboardingWizard.tsx b/frontend/src/components/OnboardingWizard.tsx index 13b6ff8a..c963bf0b 100644 --- a/frontend/src/components/OnboardingWizard.tsx +++ b/frontend/src/components/OnboardingWizard.tsx @@ -5,7 +5,6 @@ import { qk } from "../lib/queryKeys"; import { AlertTriangle, Check, Loader2, RefreshCw, ShieldCheck, X, Youtube } from "lucide-react"; import { api, type Me } from "../lib/api"; import { beginGrant, endOnboarding } from "../lib/onboarding"; -import { notifyActionError } from "../lib/actionErrors"; import Overlay from "./Overlay"; // A first-login wizard that walks the user through granting YouTube access one scope at a @@ -79,10 +78,6 @@ export default function OnboardingWizard({ me, onClose }: { me: Me; onClose: () qc.invalidateQueries({ queryKey: qk.channels() }); qc.invalidateQueries({ queryKey: qk.myStatus() }); }, - // Say something rather than nothing: this route is rate-limited (SEC-14) and can 403 without a - // read grant, and a swallowed rejection here leaves the wizard looking like it simply did not - // run. The server's own reason wins over the fallback. - onError: (e) => notifyActionError(e, t("onboarding.importFailed")), }); // First time we see read access with zero imported channels, pull the user's diff --git a/frontend/src/components/Stats.tsx b/frontend/src/components/Stats.tsx index a2265897..6a843697 100644 --- a/frontend/src/components/Stats.tsx +++ b/frontend/src/components/Stats.tsx @@ -6,7 +6,7 @@ import { History, Pause, Play, RefreshCw } from "lucide-react"; import { api, type AdminQuotaRow, type Me } from "../lib/api"; import { formatEta, quotaActionLabel } from "../lib/format"; import { notify } from "../lib/notifications"; -import { notifyActionError } from "../lib/actionErrors"; +import { notifyYouTubeActionError } from "../lib/youtubeErrors"; import { LS, useAccountPersistedState } from "../lib/storage"; import Tooltip from "./Tooltip"; import { LoadingState, StateMessage } from "./QueryState"; @@ -65,10 +65,11 @@ function Overview({ me }: { me: Me }) { message: t("stats.sync.synced", { count: r.subscriptions ?? 0 }), }); }, - // Via notifyActionError so the server's own reason wins over the generic fallback: this route - // is rate-limited (SEC-14), and "Sync failed" would report a pace limit as a fault while the - // same button on the Channels page explains it. A zero-arg onError discards that detail. - onError: (e) => notifyActionError(e, t("stats.sync.syncFailed")), + // The same helper the Channels page uses, for the same two reasons: it translates the 403 + // ("connect YouTube") instead of surfacing the route's bare English detail under a Hungarian + // UI, and it delegates everything else to notifyActionError so the server's own reason wins — + // which is what makes the SEC-14 cooldown read as a pace limit rather than "Sync failed". + onError: (e) => notifyYouTubeActionError(e, t("stats.sync.syncFailed")), }); const deepAll = useMutation({ mutationFn: () => api.deepAll(true), diff --git a/frontend/src/components/VerifyBanner.tsx b/frontend/src/components/VerifyBanner.tsx index ae566d1c..b38fae61 100644 --- a/frontend/src/components/VerifyBanner.tsx +++ b/frontend/src/components/VerifyBanner.tsx @@ -18,10 +18,11 @@ export default function VerifyBanner({ entry }: { entry: VerifyEntry }) { // silence is precisely what made the verify half a bug. The token stays in the address bar, so // signing out from this same tab picks it up. // - // Tested on `resetPending` ALONE, not `!outcome && resetPending`: the hook keeps the two states - // mutually exclusive (each new link clears the other), so an extra `!outcome` here would only add - // an ordering dependency on the early return below — tidy that return upwards one day and the - // banner silently becomes unreachable, which is the bug this branch was added to fix. + // Tested on `resetPending` ALONE, not `!outcome && resetPending`. The two can both be set (a + // reset link pasted over an unread confirmation, or a slow verify resolving after one), and this + // one goes first because it is the banner with something to DO. `dismiss` then peels it and the + // outcome shows underneath — where `!outcome &&` would have hidden this one until the other was + // dismissed, and dismissing that used to clear both. if (entry.resetPending) { return ( { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), + }; +}); + +const notifyMock = vi.fn(); +vi.mock("../lib/notifications", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, notify: (...a: unknown[]) => notifyMock(...a) }; +}); + +const access = { + email_verified: false, + email_configured: true, + is_active: false, + is_allowed: true, + can_activate: true, + has_password: false, + google_email: null, + invite_status: null, +}; + +const apiMock = { + adminUserAccess: vi.fn(async () => access), + createSetPasswordLink: vi.fn(), + resendUserVerification: vi.fn(), + activateUser: vi.fn(), +}; +vi.mock("../lib/api", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, api: apiMock }; +}); + +const { HttpError } = await import("../lib/api"); +const AccountPanel = (await import("./AccountPanel")).default; + +const user = { + id: 7, + email: "stuck@example.com", + display_name: null, + role: "user", + is_demo: false, + is_active: false, + is_suspended: false, +}; + +function mount() { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + {/* eslint-disable-next-line @typescript-eslint/no-explicit-any */} + {}} /> + , + ); +} + +/** Click the recovery button whose label is `key`, once the readout has loaded. */ +async function clickAction(key: string) { + mount(); + await screen.findByText("users.account.recovery"); + fireEvent.click(screen.getByText(key)); +} + +beforeEach(() => { + notifyMock.mockClear(); + Object.values(apiMock).forEach((m) => m.mockClear()); + apiMock.adminUserAccess.mockResolvedValue(access); +}); +afterEach(cleanup); + +const errorMessages = () => + notifyMock.mock.calls + .map(([a]) => a as { level?: string; message?: string }) + .filter((a) => a.level === "error") + .map((a) => a.message); + +describe("the account panel reports a refused recovery action", () => { + it("says the link could not be minted instead of leaving the button inert", async () => { + apiMock.createSetPasswordLink.mockRejectedValue(new HttpError(404, "gone", null)); + + await clickAction("users.account.setPassword"); + + await waitFor(() => expect(errorMessages()).toContain("users.account.linkFailed")); + // …and no link block was rendered, so the toast is the only thing that happened. + expect(screen.queryByText("users.account.copy")).toBeNull(); + }); + + it("names the no-mail case, so a no-SMTP instance does not read as a bug", async () => { + apiMock.resendUserVerification.mockRejectedValue( + new HttpError(409, "email off", { code: "resend", reason: "email_off" }), + ); + + await clickAction("users.account.resend"); + + await waitFor(() => + expect(errorMessages()).toContain("users.pendingAccess.resendEmailOff"), + ); + }); + + it("reports a genuine send failure as a failure, not as a sent mail", async () => { + apiMock.resendUserVerification.mockRejectedValue( + new HttpError(422, "send failed", { code: "resend", reason: "send_failed" }), + ); + + await clickAction("users.account.resend"); + + await waitFor(() => expect(errorMessages()).toContain("users.pendingAccess.resendFailed")); + // The success toast must NOT also fire — that is the exact lie the backend guards against. + expect(notifyMock.mock.calls.map(([a]) => (a as { message?: string }).message)).not.toContain( + "users.account.resendSent", + ); + }); + + it("reports a refused activation", async () => { + apiMock.activateUser.mockRejectedValue( + new HttpError(409, "not allowed", { code: "activate", reason: "not_allowed" }), + ); + + await clickAction("users.account.activate"); + + await waitFor(() => + expect(errorMessages()).toContain("users.pendingAccess.activateFailed"), + ); + }); +}); diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index bbd1b720..693adfb3 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -235,7 +235,7 @@ "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.", - "bad_port": "That link's port number isn't valid." + "malformed": "That link isn't a valid web address." }, "fail": { "title": "Download failed", diff --git a/frontend/src/i18n/locales/en/onboarding.json b/frontend/src/i18n/locales/en/onboarding.json index 886033df..4a726175 100644 --- a/frontend/src/i18n/locales/en/onboarding.json +++ b/frontend/src/i18n/locales/en/onboarding.json @@ -25,6 +25,5 @@ "title": "All set", "body": "Read and unsubscribe access are both granted. You can review or revoke either one anytime in Settings → Account, or from your Google Account.", "done": "Done" - }, - "importFailed": "Couldn't import your subscriptions. You can try again from Statistics." + } } diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index dfaf1942..f845cc2c 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -235,7 +235,7 @@ "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.", - "bad_port": "Ennek a hivatkozásnak érvénytelen a portszáma." + "malformed": "Ez a hivatkozás nem érvényes webcím." }, "fail": { "title": "A letöltés nem sikerült", diff --git a/frontend/src/i18n/locales/hu/onboarding.json b/frontend/src/i18n/locales/hu/onboarding.json index 7c9581fd..acc586a1 100644 --- a/frontend/src/i18n/locales/hu/onboarding.json +++ b/frontend/src/i18n/locales/hu/onboarding.json @@ -25,6 +25,5 @@ "title": "Minden kész", "body": "Az olvasási és a leiratkozási hozzáférés is engedélyezve van. Bármelyiket bármikor áttekintheted vagy visszavonhatod a Beállítások → Fiók menüben, vagy a Google-fiókodban.", "done": "Kész" - }, - "importFailed": "Nem sikerült importálni a feliratkozásaidat. Újrapróbálhatod a Statisztika oldalon." + } } diff --git a/frontend/src/lib/adminUsers.test.ts b/frontend/src/lib/adminUsers.test.ts new file mode 100644 index 00000000..75e88d41 --- /dev/null +++ b/frontend/src/lib/adminUsers.test.ts @@ -0,0 +1,55 @@ +// @vitest-environment jsdom +// +// jsdom because the shared module reaches lib/api, which pulls in i18n's localStorage probe. +import { describe, it, expect, vi } from "vitest"; +import { QueryClient } from "@tanstack/react-query"; +import { invalidateUserAccess, isEmailOff } from "./adminUsers"; +import { HttpError } from "./api"; +import { qk } from "./queryKeys"; + +// The set matters, not the call: letting someone in changes three things a user can see at once — +// the row, the pending queue, and the /api/me badge — and the account panel shipped invalidating +// only the first two, so the badge went on claiming the person was stuck until a reload. Pinning the +// keys here is what stops the two call sites drifting apart again, since neither component test +// asserts on invalidation. +describe("invalidateUserAccess", () => { + it("refreshes the row, the pending queue and the badge", () => { + const qc = new QueryClient(); + const spy = vi.spyOn(qc, "invalidateQueries").mockImplementation(() => Promise.resolve()); + + invalidateUserAccess(qc); + + const keys = spy.mock.calls.map(([a]) => JSON.stringify(a?.queryKey)); + expect(keys).toEqual( + [qk.adminUsers(), qk.adminPendingAccess(), qk.me()].map((k) => JSON.stringify(k)), + ); + }); + + it("also refreshes one account's gate readout when given an id", () => { + const qc = new QueryClient(); + const spy = vi.spyOn(qc, "invalidateQueries").mockImplementation(() => Promise.resolve()); + + invalidateUserAccess(qc, 7); + + expect(spy.mock.calls.map(([a]) => JSON.stringify(a?.queryKey))).toContain( + JSON.stringify(qk.adminUserAccess(7)), + ); + }); +}); + +// The admin-side resend deliberately reports failures (the public one is silent), so it has to be +// able to say WHICH — a no-SMTP instance is a configuration answer, not a bug. +describe("isEmailOff", () => { + it("recognises the no-mail refusal", () => { + expect(isEmailOff(new HttpError(409, "x", { code: "resend", reason: "email_off" }))).toBe(true); + }); + + it("is false for any other failure, including a non-HttpError", () => { + expect(isEmailOff(new HttpError(422, "x", { code: "resend", reason: "send_failed" }))).toBe( + false, + ); + expect(isEmailOff(new HttpError(500, "x", null))).toBe(false); + expect(isEmailOff(new TypeError("Failed to fetch"))).toBe(false); + expect(isEmailOff(undefined)).toBe(false); + }); +}); diff --git a/frontend/src/lib/adminUsers.ts b/frontend/src/lib/adminUsers.ts index f7e2afc0..f8bc82d8 100644 --- a/frontend/src/lib/adminUsers.ts +++ b/frontend/src/lib/adminUsers.ts @@ -9,7 +9,11 @@ import { qk } from "./queryKeys"; // Grouped invalidation lives per domain rather than in `qk` (see the contract note there); // `invalidateDownloads` is the same pattern for the downloads domain. -/** Everything that goes stale when a user's ACCESS state changes (activate, approve, suspend). +/** Everything that goes stale when an account is LET IN (activate, approve an invite). + * + * Suspend and delete move the same counters and are deliberately NOT wired here yet — they have + * their own pre-existing staleness, tracked separately, and widening this in passing would put an + * unreviewed change in a review-fix diff. * * `me` is in the set because the admin badge counts pending activations, and `admin-pending-access` * because the queue is exactly the list of accounts this action removes one from — refreshing only diff --git a/frontend/src/lib/useVerifyEntry.ts b/frontend/src/lib/useVerifyEntry.ts index 411f8fc9..4952cb95 100644 --- a/frontend/src/lib/useVerifyEntry.ts +++ b/frontend/src/lib/useVerifyEntry.ts @@ -59,15 +59,7 @@ export function useVerifyEntry(): VerifyEntry { // Both halves of the mailed pair arrive the same way — pasted into a running tab, which is a // same-document navigation the import-time snapshot never sees. The verify half got this // listener in E9.5 S2; the reset half was left out, so it stayed silent (DX-17). - if (hash.get("reset")) { - setResetPending(true); - // Same rule as a second verify link below: a newly pasted link must not be shadowed by a - // banner still standing from the previous one. Without this the reset guidance is - // unreachable whenever an unread verify outcome is on screen, and `dismiss` then clears the - // pending flag along with the outcome — the DX-17 silence, one state later. - setOutcome(null); - setEmail(null); - } + if (hash.get("reset")) setResetPending(true); }; // replaceState (how clearEntryUrl strips the fragment) does NOT fire hashchange, so this cannot // feed itself. @@ -82,12 +74,9 @@ export function useVerifyEntry(): VerifyEntry { clearEntryUrl(); if (!token || consumed.current.has(token)) return; consumed.current.add(token); - // A second link arriving over an unread first outcome must not leave the OLD banner standing — - // whichever kind that outcome was. Clearing both keeps the two states mutually exclusive, which - // is what lets the single `dismiss` below clear "whatever is showing" without eating the other. + // A second link arriving over an unread first outcome must not leave the OLD banner standing. setOutcome(null); setEmail(null); - setResetPending(false); api .verifyEmail(token) .then((r) => { @@ -112,9 +101,18 @@ export function useVerifyEntry(): VerifyEntry { outcome, email, resetPending, + // Dismiss peels ONE layer. The two states are independent — a reset link can arrive over an + // unread confirmation, and a slow /auth/verify can resolve after one did — so clearing both at + // once silently discards whichever the user had not read yet. The banner shows the reset + // guidance first (it is the one with something to do), and dismissing it reveals the outcome + // underneath rather than taking it with it. dismiss: () => { + if (resetPending) { + setResetPending(false); + return; + } setOutcome(null); - setResetPending(false); + setEmail(null); }, }; } diff --git a/frontend/src/lib/verifyEntry.dom.test.tsx b/frontend/src/lib/verifyEntry.dom.test.tsx index 4f6802a5..e2d80349 100644 --- a/frontend/src/lib/verifyEntry.dom.test.tsx +++ b/frontend/src/lib/verifyEntry.dom.test.tsx @@ -34,6 +34,7 @@ async function mountAt(url: string, verifyEmail: ReturnType) { <>
{`${entry.outcome ?? "none"}:${entry.email ?? "-"}`}
{entry.resetPending ? "reset-pending" : "-"}
+