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" : "-"}
+