From 47d968eeeaf3d1bfea45ea062b915188977cd666 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 15 Sep 2026 02:53:22 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20review=20round=204=20=E2=80=94=20an=20ac?= =?UTF-8?q?count=20switch=20unloads=20as=20the=20old=20account,=20a=20shar?= =?UTF-8?q?e=20link's=20lockout=20costs=20five=20networks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep-review round 4, targeted at round 3's changes (f569b19c..e7f38396). Accounts - Switching, adding or signing out an account moved the tab's pin before the page unloaded, so the players' resume beacons fired on the way out under the NEW account: one account's playback position saved into the other. The pin is now handed to the next page (pinOnNextPage) and taken over at import; clearActiveAccount is gone. - Switching to a suspended account was refused but left it in the switcher, refused on every click; a deactivated one was not refused at all, and made the default it signed every account in the browser out on the next request. Every account _session_user would refuse — gone, deactivated, suspended, revoked — is refused and dropped from the list. Share page (/watch) - An IPv6 client is limited by its /64 (client_ip becomes client_key, for every limiter): keyed on the full address, one subscriber could rotate through endless identities. The comments now state what the per-link ceiling costs: guessing from five networks can keep a link shut for its window, and guesses already running when it fills are still checked. - The link is looked up before the limiter records the attempt, so made-up tokens no longer grow it by an entry each. Forms - Failing the same way twice is announced again: the sign-in, passphrase and setup errors are keyed on the attempt (a clear and a set in one handler left the alert node untouched). - The install wizard's own validation showed the generic error — it threw plain objects its catch did not read. It sets the step's message. Tests: a re-pin leaving the previous account's notices behind; the pin hand-over; switching to an ended account; the /64 viewer; made-up tokens; the repeated alerts. Each mutation-checked RED for its own reason. --- backend/app/config.py | 2 +- backend/app/ratelimit.py | 16 +++- backend/app/routes/auth.py | 16 ++-- backend/app/routes/me.py | 16 ++-- backend/app/routes/public.py | 17 ++-- backend/entrypoint.sh | 2 +- backend/tests/test_auth_session.py | 25 +++++ backend/tests/test_public_unlock_limit.py | 25 ++++- frontend/src/components/KeyGate.tsx | 9 +- frontend/src/components/NavSidebar.tsx | 12 ++- frontend/src/components/SetupWizard.tsx | 15 ++- frontend/src/components/Welcome.tsx | 6 +- .../src/components/formAlerts.dom.test.tsx | 91 +++++++++++++++++++ .../src/lib/accountPinnedStores.dom.test.ts | 54 +++++++---- frontend/src/lib/activeAccount.dom.test.ts | 37 ++++++++ frontend/src/lib/activeAccount.ts | 30 +++++- frontend/src/lib/api.ts | 10 +- frontend/src/lib/api/core.ts | 9 +- 18 files changed, 316 insertions(+), 76 deletions(-) create mode 100644 frontend/src/components/formAlerts.dom.test.tsx create mode 100644 frontend/src/lib/activeAccount.dom.test.ts diff --git a/backend/app/config.py b/backend/app/config.py index 323a33b0..0483011c 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -57,7 +57,7 @@ class Settings(BaseSettings): # VPS Caddy front door. A request from any other peer (someone hitting the app port directly) # is NOT trusted and its raw socket IP is used, so it can't spoof its rate-limit identity via # XFF. Empty = trust no proxy (use the direct peer) — correct for a directly-exposed / local-dev - # deploy. Comma-separated. See ratelimit.client_ip + the deploy docs. + # deploy. Comma-separated. See ratelimit.client_key + the deploy docs. trusted_proxy_ips: str = "" # --- Outbound email (onboarding: access-request + approval notices) --- diff --git a/backend/app/ratelimit.py b/backend/app/ratelimit.py index 9be1aa50..8a7d6ae1 100644 --- a/backend/app/ratelimit.py +++ b/backend/app/ratelimit.py @@ -1,6 +1,6 @@ """A tiny in-process sliding-window rate limiter. -Generic groundwork: keyed by an arbitrary string (e.g. a client IP — `client_ip`), so it can throttle any +Generic groundwork: keyed by an arbitrary string (e.g. a client — `client_key`), so it can throttle any endpoint. Single-worker uvicorn → in-memory state is sufficient; it resets on restart, which is fine for abuse throttling (not for anything that must survive a deploy). Not shared across processes — if we ever run multiple workers, swap the backing store for Redis behind the same @@ -74,8 +74,10 @@ def _norm_ip(s: str) -> ipaddress.IPv4Address | ipaddress.IPv6Address | None: return ip.ipv4_mapped if getattr(ip, "ipv4_mapped", None) else ip -def client_ip(request: Request) -> str: - """Best-effort client IP for rate limiting. X-Forwarded-For is client-controlled, so we trust it +def client_key(request: Request) -> str: + """Best-effort client identity for rate limiting: its IP, or for IPv6 the /64 the address sits in — a + subscriber is handed a whole /64, so keyed on the full address one client would have endless identities + to spread its attempts over. X-Forwarded-For is client-controlled, so we trust it ONLY when the request actually arrived from a configured reverse proxy (settings.trusted_proxy_ips — the address the proxy connects FROM, e.g. the VPS Caddy's WireGuard peer IP). Our proxy APPENDS the client it saw to XFF, so the RIGHTMOST entry is the real client and can't be forged by a @@ -86,10 +88,14 @@ def client_ip(request: Request) -> str: `--proxy-headers` (see entrypoint.sh) precisely so it never rewrites `request.client` from XFF — do NOT add that flag, or the trust check below would compare against a forgeable value.""" peer = request.client.host if request.client else "unknown" + client = peer trusted = {_norm_ip(p) for p in settings.trusted_proxy_ip_set} - {None} if _norm_ip(peer) in trusted: xff = request.headers.get("x-forwarded-for") parts = [p.strip() for p in (xff or "").split(",") if p.strip()] if parts: - return parts[-1] - return peer + client = parts[-1] + ip = _norm_ip(client) + if isinstance(ip, ipaddress.IPv6Address): + return str(ipaddress.IPv6Network((ip, 64), strict=False)) + return client diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index e3ce65e7..18c25e67 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -39,7 +39,7 @@ from app.auth import ( from app.config import settings from app.db import SessionLocal, get_db from app.models import AuthToken, DemoWhitelist, Invite, OAuthToken, User -from app.ratelimit import RateLimiter, client_ip +from app.ratelimit import RateLimiter, client_key from app.security import encrypt, hash_password, hash_token, verify_password from app.utils import canonical_email, valid_email @@ -642,7 +642,7 @@ async def request_access( email = (body.email or "").strip().lower() if not valid_email(email): raise HTTPException(status_code=400, detail="Enter a valid email address.") - if not _request_access_limiter.allow(client_ip(request)): + if not _request_access_limiter.allow(client_key(request)): return {"status": "pending"} if is_allowed(db, email): return {"status": "pending"} # already allowed — don't reveal it @@ -659,7 +659,7 @@ def demo_login(body: EmailIn, request: Request, db: Session = Depends(get_db)) - so there is no visible 'demo login' button. Rate-limited per IP; when blocked it answers exactly like a non-match (authenticated=false) so the field can't be hammered as an enumeration oracle. On a match it sets the session and the client reloads into the app.""" - if not _demo_limiter.allow(client_ip(request)): + if not _demo_limiter.allow(client_key(request)): return {"authenticated": False} email = (body.email or "").strip().lower() if valid_email(email) and is_demo_allowed(db, email): @@ -805,7 +805,7 @@ def register( ) if not sysconfig.get_bool(db, "allow_registration"): raise HTTPException(status_code=403, detail="Registration is currently closed.") - if not _register_limiter.allow(client_ip(request)): + if not _register_limiter.allow(client_key(request)): return {"status": "ok"} # silently throttle; uniform response # Do the existence check + account creation OFF the response path (a background task with its own @@ -973,7 +973,7 @@ def verify_email_resend( and re-registering an already-registered email is a silent no-op, so a user who lost the first mail had no way back. Uniform response + timing regardless of whether the email has a pending account (the lookup runs off the response path), so it can't probe for registered emails.""" - if not _verify_resend_limiter.allow(client_ip(request)): + if not _verify_resend_limiter.allow(client_key(request)): return {"status": "ok"} # silently throttle; uniform response email = (body.email or "").strip().lower() if valid_email(email): @@ -1032,7 +1032,7 @@ def password_login( """Email+password sign-in. Wrong email/password give a single uniform 401 (no enumeration). Once the password is proven correct, the owner gets a specific reason if their account is still pending or suspended — that's not an enumeration leak (they already hold the password).""" - if not _login_limiter.allow(client_ip(request)): + if not _login_limiter.allow(client_key(request)): raise _login_refused(429, "rate_limited") email = (body.email or "").strip().lower() password = body.password or "" @@ -1063,7 +1063,7 @@ def password_reset_request( ) -> dict: """Request a password-reset link. Uniform response + timing regardless of whether the email has a password account (the lookup runs off the response path), so it can't probe for registered emails.""" - if not _reset_limiter.allow(client_ip(request)): + if not _reset_limiter.allow(client_key(request)): return {"status": "ok"} email = (body.email or "").strip().lower() # Do the account lookup + token issue + email OFF the response path (a background task with its @@ -1112,7 +1112,7 @@ def password_reset_check(body: TokenIn, request: Request, db: Session = Depends( told `true`, never `false`: this check may only ever save a user a wasted form, so when it cannot answer it must degrade to the old behaviour instead of declaring a live link dead. """ - if not _reset_check_limiter.allow(client_ip(request)): + if not _reset_check_limiter.allow(client_key(request)): return {"ok": True} return {"ok": any(_peek_token(db, body.token, k) is not None for k in _PASSWORD_TOKEN_KINDS)} diff --git a/backend/app/routes/me.py b/backend/app/routes/me.py index 8274e5c9..6f50f2dd 100644 --- a/backend/app/routes/me.py +++ b/backend/app/routes/me.py @@ -66,19 +66,23 @@ def switch_account( target = body.user_id if target not in (request.session.get("account_ids") or []): raise HTTPException(status_code=403, detail="Not an account you've signed into here.") + # An account `_session_user` would refuse — gone, deactivated, suspended, or its session revoked — is + # refused here too, and dropped from the switch list the way `_session_user` drops it. Made the session + # default instead, the next request's current_user would refuse it as the DEFAULT and clear the whole + # wallet session, logging out every account here. (B-core-9) u = db.get(User, target) if u is None: + drop_from_wallet(request.session, target) raise HTTPException(status_code=404, detail="That account no longer exists.") if not is_allowed(db, u.email): raise HTTPException(status_code=403, detail="That account no longer has access.") + if not u.is_active: + drop_from_wallet(request.session, target) + raise HTTPException(status_code=403, detail="That account no longer has access.") if u.is_suspended: - # Don't make a suspended account the active one — the next request's current_user would - # see the suspension and clear the WHOLE wallet session, logging out every account here. + drop_from_wallet(request.session, target) raise HTTPException(status_code=403, detail="That account is suspended.") - # Same reasoning for a REVOKED session (a password reset/change or "log out everywhere" bumped the - # account's epoch): making it the session default would trip current_user's epoch check on the next - # request and clear the whole wallet. Mirror the suspend guard — drop the stale account from the - # switch list and refuse, rather than logging everyone out. (B-core-9) + # Revoked: a password reset/change or "log out everywhere" bumped the account's epoch. epochs = request.session.get("epochs") or {} if int(epochs.get(str(target), 0)) != (u.session_epoch or 0): drop_from_wallet(request.session, target) diff --git a/backend/app/routes/public.py b/backend/app/routes/public.py index 92976020..e580a77c 100644 --- a/backend/app/routes/public.py +++ b/backend/app/routes/public.py @@ -17,17 +17,20 @@ from app.db import get_db from app.downloads import links as linksmod from app.downloads import storage from app.models import DownloadJob, DownloadLink, MediaAsset -from app.ratelimit import RateLimiter, client_ip +from app.ratelimit import RateLimiter, client_key from app.security import verify_password router = APIRouter(prefix="/api/public", tags=["public"]) # Slow password guessing on a link (the token itself is 192-bit, so it needs no throttle). Counted per -# viewer — the link AND the client — so someone who has the link but not its password can't lock out the -# viewers who do, and a busy link's own unlocks don't count against its next viewer. +# viewer — the link AND the client (`client_key`) — so a client guessing wrong shuts out only itself, and a +# busy link's own unlocks don't count against its next viewer. _unlock_limit = RateLimiter(max_events=10, window_seconds=300) # ...under a ceiling on WRONG passwords for the link from every client together, so guessing spread over -# many addresses stays slow too. Viewers who have the password never add to it. +# many addresses stays slow too; unlocking with the password never adds to it. The price of the ceiling: +# someone with the link but not its password, guessing from 50 / 10 = 5 networks, can keep the link shut +# for everyone until the window passes. It is checked before the password and counted after it, so guesses +# already running when it fills are still checked — as many as the route's threadpool runs at once. _unlock_failures = RateLimiter(max_events=50, window_seconds=300) @@ -92,10 +95,12 @@ def watch_unlock( token: str, body: WatchUnlockIn, request: Request, db: Session = Depends(get_db) ) -> dict: """Exchange the link password for a signed grant carried by the media URL.""" - viewer = f"{token}|{client_ip(request)}" + # The link first: a token naming none must leave no limiter entry behind, or posting made-up tokens + # would grow the limiter without bound. + link = _link_or_404(db, token) + viewer = f"{token}|{client_key(request)}" if _unlock_failures.exhausted(token) or not _unlock_limit.allow(viewer): raise HTTPException(status_code=429, detail="Too many attempts. Try again later.") - link = _link_or_404(db, token) if not link.password_hash: # Not a password link — nothing to unlock; just hand back the plain meta. job, asset = _ready_target(db, link) diff --git a/backend/entrypoint.sh b/backend/entrypoint.sh index 2b2d6d0f..c2c52c1d 100644 --- a/backend/entrypoint.sh +++ b/backend/entrypoint.sh @@ -11,7 +11,7 @@ echo "Starting Siftlode API..." # on the next request -> 502. Non-idempotent POSTs (the player's progress/state writes, # fired every 5s) can't be auto-retried by the proxy, so those 502s reached the user. # -# SECURITY: do NOT add --proxy-headers / --forwarded-allow-ips here. ratelimit.client_ip trusts +# SECURITY: do NOT add --proxy-headers / --forwarded-allow-ips here. ratelimit.client_key trusts # X-Forwarded-For only when the TRUE socket peer is a configured proxy (TRUSTED_PROXY_IPS); those # flags would make uvicorn overwrite request.client from the forgeable XFF, defeating that check and # reopening the rate-limit bypass (SA3). diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index 855c9142..72fff9e6 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -102,6 +102,31 @@ def test_a_tab_pinned_to_an_account_whose_session_ended_comes_back_as_the_defaul assert client.get("/api/feed").status_code == 200 # Bob's session is intact +@pytest.mark.parametrize("ended", ["revoked", "suspended", "deactivated"]) +def test_switching_to_an_account_whose_session_ended_refuses_it_and_drops_it_from_the_list( + client, make_user, db, monkeypatch, ended +): + # The account switcher lists this browser's wallet. A revoked account was dropped from it on a refused + # switch, but a suspended one stayed, refused on every click; a deactivated one was not refused at all — + # made the default, the next request found it inactive and signed every account here out. + monkeypatch.setattr("app.routes.me.is_allowed", lambda _db, _email: True) # both hold an invite + alice = make_user(email="alice5@example.com", password=_PW) + make_user(email="bob5@example.com", password=_PW) + assert _login(client, "alice5@example.com", _PW).status_code == 200 + assert _login(client, "bob5@example.com", _PW).status_code == 200 + if ended == "revoked": + alice.session_epoch = (alice.session_epoch or 0) + 1 + elif ended == "suspended": + alice.is_suspended = True + else: + alice.is_active = False + db.commit() + + assert client.post("/api/me/switch", json={"user_id": alice.id}).status_code == 403 + assert [a["email"] for a in client.get("/api/me/accounts").json()] == ["bob5@example.com"] + assert client.get("/api/feed").status_code == 200 # Bob is still signed in + + def test_logout_others_revokes_the_other_session_but_keeps_the_caller(client, make_user): # Two independent cookie jars (browsers) signed into the same account. `logout-others` from one # bumps the account epoch so the OTHER's cookie is rejected on its next request, while the caller diff --git a/backend/tests/test_public_unlock_limit.py b/backend/tests/test_public_unlock_limit.py index c0e382f9..ffbdebd0 100644 --- a/backend/tests/test_public_unlock_limit.py +++ b/backend/tests/test_public_unlock_limit.py @@ -1,7 +1,8 @@ """The password unlock of a shared /watch link (routes/public.py), DB-lane. -Guesses are slowed per viewer (the link AND the client), under a per-link ceiling on wrong passwords, so -someone who holds a password link without its password can't lock out the viewers who have it. +Guesses are slowed per viewer (the link AND the client — an IPv6 client by its /64), under a per-link +ceiling on wrong passwords, so a client guessing wrong shuts out only itself until it spreads its guesses +over as many networks as the ceiling allows. """ import pytest @@ -46,6 +47,26 @@ def test_a_link_holder_without_the_password_cannot_lock_out_its_viewers(unlock_u assert _from("198.51.100.7").post(unlock_url, json={"password": _PW}).status_code == 200 +def test_an_ipv6_client_is_one_viewer_across_its_64(unlock_url): + # One subscriber gets a whole /64: keyed on the full address, it could rotate through endless viewers. + for _ in range(10): + wrong = _from("2001:db8:1:2::5").post(unlock_url, json={"password": "guess"}) + assert wrong.status_code == 403 + assert _from("2001:db8:1:2::6").post(unlock_url, json={"password": _PW}).status_code == 429 + + assert _from("2001:db8:1:3::5").post(unlock_url, json={"password": _PW}).status_code == 200 + + +def test_a_token_naming_no_link_leaves_nothing_in_the_limiter(unlock_url): + # The limiter used to record the attempt before looking the link up: posting made-up tokens grew it by + # an entry each, kept for the whole window. + keys = len(public._unlock_limit._hits) + for n in range(5): + missing = _from("203.0.113.5").post(unlock_url.replace("/watch/", f"/watch/x{n}"), json={}) + assert missing.status_code == 404 + assert len(public._unlock_limit._hits) == keys + + def test_wrong_passwords_from_many_clients_still_reach_the_links_ceiling(unlock_url, monkeypatch): monkeypatch.setattr(public, "_unlock_failures", RateLimiter(max_events=3, window_seconds=300)) viewer = _from("198.51.100.7") diff --git a/frontend/src/components/KeyGate.tsx b/frontend/src/components/KeyGate.tsx index 0796bfee..3683c045 100644 --- a/frontend/src/components/KeyGate.tsx +++ b/frontend/src/components/KeyGate.tsx @@ -32,9 +32,13 @@ export default function KeyGate({ meId, compact = false }: { meId: number; compa const [confirm, setConfirm] = useState(""); const [curPass, setCurPass] = useState(""); const [err, setErr] = useState(null); + // Keys the error: failing the same way twice is a new node, so a screen reader announces it again (a clear + // and a set in one handler commit together, leaving the node untouched). + const [attempt, setAttempt] = useState(0); const [busy, setBusy] = useState(false); async function submit() { + setAttempt((n) => n + 1); setErr(null); if (pass.length < 6) return setErr(t("messages.passTooShort")); if (!configured && pass !== confirm) return setErr(t("messages.passMismatch")); @@ -67,6 +71,7 @@ export default function KeyGate({ meId, compact = false }: { meId: number; compa } async function doReset() { + setAttempt((n) => n + 1); setErr(null); if (hasPassword && !curPass) return setErr(t("messages.resetNeedPass")); setBusy(true); @@ -107,7 +112,7 @@ export default function KeyGate({ meId, compact = false }: { meId: number; compa /> )} {err && ( -
+
{err}
)} @@ -158,7 +163,7 @@ export default function KeyGate({ meId, compact = false }: { meId: number; compa /> )} {err && ( -
+
{err}
)} diff --git a/frontend/src/components/NavSidebar.tsx b/frontend/src/components/NavSidebar.tsx index 2e9e10ea..42d29d2f 100644 --- a/frontend/src/components/NavSidebar.tsx +++ b/frontend/src/components/NavSidebar.tsx @@ -4,7 +4,8 @@ import { useTranslation } from "react-i18next"; import { useQuery } from "@tanstack/react-query"; import { useNavigation, useNavigationActions } from "./NavigationProvider"; import { Info, LogOut, Settings, Shield, UserPlus, type LucideIcon } from "lucide-react"; -import { api, clearActiveAccount, setActiveAccount, type Me } from "../lib/api"; +import { api, type Me } from "../lib/api"; +import { pinOnNextPage } from "../lib/activeAccount"; import * as e2ee from "../lib/e2ee"; import { useLiveQuery } from "../lib/useLiveQuery"; import { getUnreadCount, subscribe } from "../lib/notifications"; @@ -71,7 +72,7 @@ export default function NavSidebar({ } catch { /* IndexedDB unavailable (e.g. private mode) — never let it block the logout + reload */ } - clearActiveAccount(); + pinOnNextPage(null); location.reload(); } @@ -86,8 +87,9 @@ export default function NavSidebar({ function switchTo(id: number) { // Per-tab switch: point THIS tab at the account and reload; other tabs keep their own // identity. No server round-trip — req() sends the account header (validated against the - // browser wallet server-side), so the cookie's default account is left untouched. - setActiveAccount(id); + // browser wallet server-side), so the cookie's default account is left untouched. The pin moves with + // the reload, not before it: the page going away still unloads as the account it was built for. + pinOnNextPage(id); // Drop the previous account's in-module sub-view/overlay before the reload (which would // otherwise preserve history.state across the swap). Otherwise the new account lands on a // stale sub-view — e.g. an open chat thread whose partnerId belongs to the OLD identity @@ -334,7 +336,7 @@ export default function NavSidebar({ // Adding an account switches THIS tab to it: drop the tab's pin so that on // return from Google it adopts the freshly-added account (the new default) // and pins that. Other tabs keep their own pinned account. - clearActiveAccount(); + pinOnNextPage(null); window.location.href = "/auth/login"; }} className="selectable w-full flex items-center gap-2 text-sm px-2 py-2 rounded-lg text-muted hover:text-fg hover:bg-card transition" diff --git a/frontend/src/components/SetupWizard.tsx b/frontend/src/components/SetupWizard.tsx index 38b52861..7632e1fb 100644 --- a/frontend/src/components/SetupWizard.tsx +++ b/frontend/src/components/SetupWizard.tsx @@ -36,6 +36,9 @@ export default function SetupWizard() { const [stepIdx, setStepIdx] = useState(0); const [busy, setBusy] = useState(false); const [error, setError] = useState(""); + // Keys the error: failing the same way twice is a new node, so a screen reader announces it again (a clear + // and a set in one handler commit together, leaving the node untouched). + const [attempt, setAttempt] = useState(0); // Admin account. const [email, setEmail] = useState(""); const [password, setPassword] = useState(""); @@ -74,19 +77,21 @@ export default function SetupWizard() { }; // Each step's "Next" action persists what it collected, then advances. Optional steps with empty - // fields just advance (skip). Errors surface inline (the setup calls use the quiet flag). + // fields just advance (skip). Errors surface inline (the setup calls use the quiet flag); a step's own + // validation sets its message and stays. async function next() { + setAttempt((n) => n + 1); setError(""); setBusy(true); try { if (step === "admin") { if (!/^[^@\s]+@[^@\s]+\.[^@\s]+$/.test(email)) - throw { detail: t("setup.admin.invalidEmail") }; + return setError(t("setup.admin.invalidEmail")); if (password.length < PASSWORD_MIN) - throw { detail: t("setup.admin.shortPassword", { n: PASSWORD_MIN }) }; + return setError(t("setup.admin.shortPassword", { n: PASSWORD_MIN })); await api.setupAdmin(token, email, password); } else if (step === "google" && (gid.trim() || gsecret.trim())) { - if (!gid.trim() || !gsecret.trim()) throw { detail: t("setup.google.bothRequired") }; + if (!gid.trim() || !gsecret.trim()) return setError(t("setup.google.bothRequired")); await api.setupConfig(token, { google_client_id: gid.trim(), google_client_secret: gsecret.trim(), @@ -268,7 +273,7 @@ export default function SetupWizard() { )} {error && ( -

+

{error}

)} diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index b35aa251..789cca04 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -398,6 +398,9 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { const [password, setPassword] = useState(""); const [busy, setBusy] = useState(false); const [error, setError] = useState(""); + // Keys the error: failing the same way twice is a new node, so a screen reader announces it again (a clear + // and a set in one handler commit together, leaving the node untouched). + const [attempt, setAttempt] = useState(0); // Whether to offer "Continue with Google" — hidden when the instance has no Google OAuth // configured. Optimistic (most instances have it); the rare Google-less instance just sees it // disappear once the config loads. @@ -481,6 +484,7 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { async function onSubmit(e: React.FormEvent) { e.preventDefault(); + setAttempt((n) => n + 1); reset(); const mail = email.trim().toLowerCase(); if ( @@ -687,7 +691,7 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { /> )} {error && ( -

+

{error}

)} diff --git a/frontend/src/components/formAlerts.dom.test.tsx b/frontend/src/components/formAlerts.dom.test.tsx new file mode 100644 index 00000000..2a751a6c --- /dev/null +++ b/frontend/src/components/formAlerts.dom.test.tsx @@ -0,0 +1,91 @@ +// @vitest-environment jsdom +// +// A form's error is announced (role="alert"), and failing the same way again must be announced again. The +// clear and the set of a validation failure commit together, so the alert node used to stay untouched on +// the second attempt and a screen reader said nothing — a press that seemed to do nothing. +// +// i18n is mocked to a key-passthrough; the API and the E2EE module are stubbed so nothing reaches out. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), + }; +}); + +vi.mock("../lib/api", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + api: { + ...actual.api, + authConfig: () => Promise.resolve({ google_enabled: false }), + messageMyKey: () => Promise.resolve({ configured: true }), + me: () => Promise.resolve({ authenticated: false }), + setupInfo: () => Promise.resolve({ secrets_manageable: false }), + }, + }; +}); + +vi.mock("../lib/e2ee", () => ({ unlock: vi.fn(), setup: vi.fn(), clearDevice: vi.fn() })); + +import KeyGate from "./KeyGate"; +import SetupWizard from "./SetupWizard"; +import Welcome from "./Welcome"; + +afterEach(() => { + cleanup(); + window.history.replaceState(null, "", "/"); +}); + +// Fails twice the same way; the second alert must be a new node carrying the same message. +async function failTwice(fail: () => void, message: string) { + fail(); + const first = await screen.findByRole("alert"); + expect(first.textContent).toBe(message); + fail(); + const second = screen.getByRole("alert"); + expect(second.textContent).toBe(message); + expect(second).not.toBe(first); +} + +describe("a form failing the same way twice announces it twice", () => { + it("the message passphrase gate", async () => { + render( + + + , + ); + const unlock = await screen.findByText("messages.unlock"); + fireEvent.change(screen.getByPlaceholderText("messages.passphrase"), { + target: { value: "abc" }, + }); + await failTwice(() => fireEvent.click(unlock), "messages.passTooShort"); + }); + + it("the sign-in card", async () => { + render( + {} }} />, + ); + const email = await screen.findByPlaceholderText("welcome.auth.email"); + fireEvent.change(email, { target: { value: "not-an-address" } }); + await failTwice(() => fireEvent.submit(email.closest("form")!), "welcome.auth.invalidEmail"); + }); + + it("the install wizard, with the step's own message rather than the generic one", async () => { + window.history.replaceState(null, "", "/?token=setup-token"); + render(); + fireEvent.click(await screen.findByText("setup.nav.next")); // past the intro + fireEvent.change(await screen.findByPlaceholderText("setup.admin.email"), { + target: { value: "not-an-address" }, + }); + await failTwice( + () => fireEvent.click(screen.getByText("setup.nav.next")), + "setup.admin.invalidEmail", + ); + }); +}); diff --git a/frontend/src/lib/accountPinnedStores.dom.test.ts b/frontend/src/lib/accountPinnedStores.dom.test.ts index d849c726..04e9ab0c 100644 --- a/frontend/src/lib/accountPinnedStores.dom.test.ts +++ b/frontend/src/lib/accountPinnedStores.dom.test.ts @@ -6,6 +6,24 @@ // nobody's history, and the tab's first notice replaced the account's stored history with it. import { beforeEach, describe, expect, it, vi } from "vitest"; +const entry = (id: number, message: string) => ({ + id, + level: "info", + message, + ts: 1, + read: true, + dismissed: true, + transient: false, + repeat: 1, + requiresInteraction: false, +}); +const storedMessages = (account: number) => + ( + JSON.parse(localStorage.getItem(`siftlode.notifications.${account}`) ?? "[]") as { + message: string; + }[] + ).map((n) => n.message); + beforeEach(() => { localStorage.clear(); sessionStorage.clear(); @@ -14,18 +32,7 @@ beforeEach(() => { describe("stores read at import, in a tab not yet pinned", () => { it("keep the account's notification history and settings once the tab is pinned", async () => { - const older = { - id: 7, - level: "info", - message: "older", - ts: 1, - read: true, - dismissed: true, - transient: false, - repeat: 1, - requiresInteraction: false, - }; - localStorage.setItem("siftlode.notifications.5", JSON.stringify([older])); + localStorage.setItem("siftlode.notifications.5", JSON.stringify([entry(7, "older")])); localStorage.setItem("siftlode.notifSettings.5", JSON.stringify({ durationMs: 0 })); const notifications = await import("./notifications"); const { setActiveAccount } = await import("./api/core"); @@ -34,10 +41,7 @@ describe("stores read at import, in a tab not yet pinned", () => { setActiveAccount(5); notifications.notify({ message: "after the pin" }); - const stored = JSON.parse(localStorage.getItem("siftlode.notifications.5") ?? "[]") as { - message: string; - }[]; - expect(stored.map((n) => n.message)).toEqual(["older", "before the pin", "after the pin"]); + expect(storedMessages(5)).toEqual(["older", "before the pin", "after the pin"]); expect(notifications.getNotifSettings().durationMs).toBe(0); }); @@ -51,3 +55,21 @@ describe("stores read at import, in a tab not yet pinned", () => { expect(hints.hintsEnabled()).toBe(false); }); }); + +describe("a pinned tab pinned to another account", () => { + it("leaves the previous account's notices with that account", async () => { + // BootGate moves a tab whose account was signed out elsewhere onto the one the server answers as. + sessionStorage.setItem("siftlode.activeAccount", "4"); + localStorage.setItem("siftlode.notifications.4", JSON.stringify([entry(7, "four's")])); + localStorage.setItem("siftlode.notifications.5", JSON.stringify([entry(8, "five's")])); + const notifications = await import("./notifications"); + const { setActiveAccount } = await import("./api/core"); + + notifications.notify({ message: "as four" }); + setActiveAccount(5); + notifications.notify({ message: "as five" }); + + expect(storedMessages(5)).toEqual(["five's", "as five"]); + expect(storedMessages(4)).toEqual(["four's", "as four"]); + }); +}); diff --git a/frontend/src/lib/activeAccount.dom.test.ts b/frontend/src/lib/activeAccount.dom.test.ts new file mode 100644 index 00000000..0cc12ff0 --- /dev/null +++ b/frontend/src/lib/activeAccount.dom.test.ts @@ -0,0 +1,37 @@ +// @vitest-environment jsdom +// +// A switch or sign-out hands the tab's next page its account instead of moving the pin: the page being left +// still unloads as its own account. The players' resume beacons read the pin as they fire on the way out, +// and with the pin already moved they saved one account's playback position into the other. +import { beforeEach, describe, expect, it, vi } from "vitest"; + +beforeEach(() => { + sessionStorage.clear(); + sessionStorage.setItem("siftlode.activeAccount", "3"); + vi.resetModules(); // the hand-over is taken at import: each import below is a page load +}); + +describe("pinOnNextPage", () => { + it("leaves the pin alone until the next page, which takes the account over once", async () => { + const leaving = await import("./activeAccount"); + leaving.pinOnNextPage(7); + expect(leaving.activeAccountId()).toBe(3); // the unload still acts as 3 + + vi.resetModules(); + const next = await import("./activeAccount"); + expect(next.activeAccountId()).toBe(7); + + next.pinActiveAccount(9); // BootGate corrects it + vi.resetModules(); // and an F5 keeps the correction + expect((await import("./activeAccount")).activeAccountId()).toBe(9); + }); + + it("hands over no account at all, so the next page adopts the session's default", async () => { + const leaving = await import("./activeAccount"); + leaving.pinOnNextPage(null); + expect(leaving.activeAccountId()).toBe(3); + + vi.resetModules(); + expect((await import("./activeAccount")).activeAccountId()).toBeNull(); + }); +}); diff --git a/frontend/src/lib/activeAccount.ts b/frontend/src/lib/activeAccount.ts index f23cf381..c57493ff 100644 --- a/frontend/src/lib/activeAccount.ts +++ b/frontend/src/lib/activeAccount.ts @@ -4,7 +4,8 @@ // helpers that consume it. // // Deliberately NOT reactive: BootGate pins the account before any provider mounts, and a switch reloads -// the page, so nothing mounted ever sees the id change underneath it. The one exception is state read at +// the page (handing the new pin over through `pinOnNextPage`), so nothing mounted ever sees the id change +// underneath it. The one exception is state read at // module import (the notification and hints stores), which in a new tab runs before the pin: it reads // again through `onAccountPinned`. // @@ -27,6 +28,33 @@ export function pinActiveAccount(id: number): void { pinListeners.forEach((reload) => reload()); } +// The account the tab's NEXT page is pinned to, handed over by a switch or sign-out that is about to leave. +// The pin itself stays until the page is gone: the unload still runs as the account the page was built for +// (the players' resume beacons read the pin as they fire), and with the pin already moved it would save one +// account's position into the other. +const NEXT_ACCOUNT_KEY = "siftlode.nextAccount"; + +/** Pin the tab's next page to `id` — or to none, so it adopts the session's default account. */ +export function pinOnNextPage(id: number | null): void { + try { + sessionStorage.setItem(NEXT_ACCOUNT_KEY, id == null ? "" : String(id)); + } catch { + /* ignore */ + } +} + +// Taken over at import, before anything reads the pin. +try { + const next = sessionStorage.getItem(NEXT_ACCOUNT_KEY); + if (next != null) { + sessionStorage.removeItem(NEXT_ACCOUNT_KEY); + if (next) sessionStorage.setItem(ACTIVE_ACCOUNT_KEY, next); + else sessionStorage.removeItem(ACTIVE_ACCOUNT_KEY); + } +} catch { + /* ignore */ +} + export function activeAccountId(): number | null { try { const raw = sessionStorage.getItem(ACTIVE_ACCOUNT_KEY); diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index 372a9c9b..4f7eea38 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -11,19 +11,11 @@ import { getActiveAccount, HttpError, setActiveAccount, - clearActiveAccount, setUnauthorizedHandler, } from "./api/core"; // Re-export the shared HTTP machinery so consumers keep importing these from "./api" unchanged. -export { - alreadyReported, - HttpError, - getActiveAccount, - setActiveAccount, - clearActiveAccount, - setUnauthorizedHandler, -}; +export { alreadyReported, HttpError, getActiveAccount, setActiveAccount, setUnauthorizedHandler }; import { accountApi } from "./api/account"; export type * from "./api/account"; diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index e771ad56..d1323b89 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -6,7 +6,7 @@ import { getActiveToasts, notify, remove } from "../notifications"; import i18n from "../../i18n"; import { formatBytes } from "../format"; import { reportError } from "../errorDialog"; -import { activeAccountId, ACTIVE_ACCOUNT_KEY, pinActiveAccount } from "../activeAccount"; +import { activeAccountId, pinActiveAccount } from "../activeAccount"; /** The active UI language, as a bare code the server understands ("en"/"hu" — never "en-GB"). * Lives here so the request builders can stamp it themselves: a signal that has to be passed in by @@ -263,13 +263,6 @@ export function beacon(url: string, body: Record): void { } export const setActiveAccount = pinActiveAccount; -export function clearActiveAccount(): void { - try { - sessionStorage.removeItem(ACTIVE_ACCOUNT_KEY); - } catch { - /* ignore */ - } -} export async function req( url: string,