fix(auth): close what the epic's own review found (E9.5 review round 1)

Eleven findings survived adversarial verification; these are the fixes.

**Address pre-emption.** S1 dropped the approval-time password revoke, arguing `password_login`
refuses an unverified account before it reads `is_active`. Two verifiers showed that holds only
while the address stays unverified — and S6's approval mail exists to get the real owner to end that
state, at which point the squatter's password works. While an address is unverified the NEWEST
registration now owns it: the password is replaced and every outstanding verify token deleted, so
the owner takes the row back simply by registering, and a link already in flight cannot confirm it
for anyone else. The residual (an owner who confirms and neither registers nor resets) is logged as
SEC-16 beside SEC-15, where the re-attestation path it needs already lives. The comments that
asserted the old, false invariant now say what actually guards this.

**The entry URL.** S2 moved `clearEntryUrl()` out of the signed-out auth card into a hook App mounts
unconditionally, and it blanket-stripped the whole query string, the fragment and `history.state` on
the FIRST commit. That broke first-run setup outright — the wizard mounts only after its query
resolves and then finds no `?token=` — and silently ate `?link=`/`?oauth=` (no toast, no refetch
after connecting Google), `?v=`/`?chan=` deep links, and scroll restoration. It now clears only the
fragment and the two one-time flags it owns, and preserves `history.state`.

**The env allow-list.** S3 canonicalised the Invite branch of `is_allowed` and left the
ALLOWED_EMAILS/ADMIN_EMAILS fallback matching literally — the 2026-09-03 lockout with a different
source of truth, plus a shadowing trick where an unauthenticated request under a sibling spelling
permanently masked an env grant. Canonical on both sides now, and the same for the delete dialog's
warning and the ADMIN_EMAILS role bootstrap.

**Honest states.** `send_invitation` stamped "I already wrote to them" before a fail-soft background
send; it sends inline and records only what left. The admin-side resend answered `{ok: true}` for a
dropped mail; it now says so. Both also activate/serialize consistently with the other decide paths.

**Correctness sweep.** The 7-day approval link said "expires in 168 hours" — durations render in
days once there is more than one. `_register_account`, `upsert_pending_invite` and `add_invite`
picked among canonical siblings with no ORDER BY and no exact-spelling tie-break, so which account
got the re-send, and whether a denied row was reopened over a pending one, was up to Postgres.
`purge_user` deleted every canonically-equal invite, revoking a surviving sibling account's access.
`ConfirmProvider` wrapped its ReactNode message in a `<p>`. Approving no longer leaves the
stuck-sign-ups list stale. The refusal panels announce and take focus.

**Tests.** Three mailers rewritten this epic never rendered in any test; every `send_*` now renders
in both languages. The orphan-key guard could not fail (it scanned the catalog that defines the
keys) and had no reverse direction. Newly pinned: canonical activation, the surviving sibling's
grant, a failed invitation and resend, an expired reset link, a throttled login's silence, the
per-language admin fan-out, and that the entry URL keeps what it does not own.
This commit is contained in:
2026-09-04 22:42:15 +02:00
parent 10c0ca4cf7
commit 563fa0513d
15 changed files with 613 additions and 75 deletions
+76 -23
View File
@@ -13,7 +13,7 @@ from authlib.integrations.starlette_client import OAuth, OAuthError
from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request
from fastapi.responses import JSONResponse, RedirectResponse
from pydantic import BaseModel
from sqlalchemy import select, update
from sqlalchemy import delete, select, update
from sqlalchemy.exc import IntegrityError
from sqlalchemy.orm import Session
@@ -28,6 +28,7 @@ from app.auth import (
READ_SCOPES,
WRITE_SCOPES,
current_user,
env_admin_canonicals,
find_user_by_email,
get_or_create_demo_user,
google_enabled,
@@ -229,9 +230,20 @@ def upsert_pending_invite(db: Session, email: str, lang: str | None = None) -> I
return InviteRequest(None, False)
lang = lang if lang in mailtext.SUPPORTED else None
canon = canonical_email(email)
rows = db.execute(select(Invite).where(Invite.email_canonical == canon)).scalars().all()
# Prefer a decided row over a pending one, so a denied duplicate can't mask an approval.
inv = next((r for r in rows if r.status == "approved"), None) or next(iter(rows), None)
rows = (
db.execute(select(Invite).where(Invite.email_canonical == canon).order_by(Invite.id))
.scalars()
.all()
)
# An approved row wins, so a denied duplicate can't mask an approval; then the exact spelling;
# then the oldest. Without the middle rule an unordered scan could hand back a DENIED sibling
# while a pending row already existed, and the denied branch below would reopen it — re-notifying
# every admin and putting one person in the queue twice.
inv = (
next((r for r in rows if r.status == "approved"), None)
or next((r for r in rows if r.email == email), None)
or next(iter(rows), None)
)
if inv is not None:
relearned = lang is not None and inv.lang != lang
if relearned:
@@ -467,7 +479,9 @@ async def callback(
# be locked out. Everyone else's role is managed in the admin UI and stored in the DB, so a
# routine login must NOT reset it (that would clobber a UI promotion/demotion). Only seed a
# default for a brand-new account (its role attribute is still unset before flush).
if email in settings.admin_email_set:
# Canonical, like every other read of the env lists: a dotted or `+tag`ged ADMIN_EMAILS entry
# would otherwise never bootstrap its admin, because Google answers with the plain primary.
if canonical_email(email) in env_admin_canonicals():
user.role = "admin"
elif not user.role:
user.role = "user"
@@ -479,8 +493,7 @@ async def callback(
# picked one yet); unsupported locales fall back to English.
prefs = dict(user.preferences or {})
if not prefs.get("language"):
loc = (userinfo.get("locale") or "").split("-")[0].lower()
prefs["language"] = loc if loc in {"en", "hu"} else "en"
prefs["language"] = mailtext.normalize((userinfo.get("locale") or "").split("-")[0].lower())
user.preferences = prefs
db.flush()
@@ -635,7 +648,7 @@ async def request_access(
db: Session = Depends(get_db),
) -> dict:
"""Public: ask for access. Idempotent — repeat requests for the same email don't
duplicate or re-spam admins (upsert returns None for an already-pending row).
duplicate or re-spam admins (upsert answers `is_new=False` for an already-pending row).
Answers a uniform ``"pending"`` for EVERY outcome — already-allowed, newly-pending,
already-pending, and rate-limited alike. The old ``"approved"`` reply for an allow-listed
@@ -799,9 +812,20 @@ def _register_account(email: str, password: str, lang: str | None = None) -> Non
# Canonical, so a second spelling of one Gmail mailbox cannot open a SECOND account. Prod
# already carries such a pair (user ids 13 and 14, both for npeter83temu@gmail.com); this is
# the end that stops new ones appearing, while the Google callback copes with the existing.
existing = db.execute(
select(User).where(User.email_canonical == canonical_email(email))
).first()
candidates = (
db.execute(
select(User).where(User.email_canonical == canonical_email(email)).order_by(User.id)
)
.scalars()
.all()
)
# Exact spelling wins when a mailbox carries two rows, same as every other canonical lookup
# in this epic (`find_user_by_email`, `_activate_user_by_email`, the Google callback). Without
# it `.first()` handed back whichever row Postgres felt like, so a re-registration of the
# dotted spelling could resolve to the undotted, already-confirmed twin and send nothing.
existing = next((u for u in candidates if u.email == email), None) or next(
iter(candidates), None
)
if existing is not None:
# Already registered. This used to return in silence, and that silence is the most
# likely explanation for the 2026-09-03 report that a confirmation mail "never arrived":
@@ -813,9 +837,29 @@ def _register_account(email: str, password: str, lang: str | None = None) -> Non
# ON THE ACCOUNT, and this is exactly what the public /auth/verify/resend already does
# for the same input — so it is not a new capability, just the end of a dead end. An
# account that is already confirmed still gets nothing: there is nothing to confirm.
user = existing[0]
if not user.email_verified and not user.is_demo and email_mod.email_enabled():
send_verification_link(db, user)
user = existing
if not user.email_verified and not user.is_demo:
# While the address is UNVERIFIED, the newest registration owns the account.
#
# Anyone can post any address here, so the first registration for a stranger's
# mailbox used to own it permanently: the password stayed, approval activated the
# account (E9.5 S1 stopped revoking it), and the confirmation link the approval mail
# carries goes to the real owner — whose click would open sign-in for the squatter's
# password. Letting the later registration replace the hash makes that pre-emption
# worthless the moment the real owner registers, and it costs a legitimate registrant
# nothing: re-registering an address you already registered is you, choosing a
# password again. Every verify token issued to the previous holder dies with it, so a
# link already in flight cannot confirm the account for them either.
user.password_hash = hash_password(password)
user.password_revoked_at = None
db.execute(
delete(AuthToken).where(
AuthToken.user_id == user.id, AuthToken.kind == "verify"
)
)
db.commit()
if email_mod.email_enabled():
send_verification_link(db, user)
return
# Without working SMTP we can't deliver a verification link, so email ownership can't gate
# sign-in. Admin approval stays the real gate (is_active=False); mark the account verified so
@@ -827,7 +871,7 @@ def _register_account(email: str, password: str, lang: str | None = None) -> Non
is_active=False,
email_verified=not email_ok,
# Only a supported code is stored, so a forged body can't put junk in preferences.
preferences={"language": lang} if lang in ("en", "hu") else {},
preferences={"language": lang} if lang in mailtext.SUPPORTED else {},
)
db.add(user)
try:
@@ -867,7 +911,7 @@ def issue_verification_link(db: Session, user: User, ttl: timedelta | None = Non
return f"{_app_base()}/#verify={raw}"
def send_verification_link(db: Session, user: User) -> None:
def send_verification_link(db: Session, user: User) -> bool:
"""Issue a fresh verification token for `user` and mail them the link.
Always to the address ON THE ACCOUNT, never a spelling typed into some form: the account's
@@ -877,8 +921,11 @@ def send_verification_link(db: Session, user: User) -> None:
One definition, three callers — registration, the self-service resend, and the admin-side resend
(E9.5 S5). The link's shape and TTL must not drift between them; a verification mail that differs
by who triggered it is a bug waiting to be discovered by exactly one user.
Returns whether the mail actually went out, so a caller with a human waiting on the answer (the
admin-side resend) can say so instead of reporting success for a message that was dropped.
"""
email_mod.send_verify_email(
return email_mod.send_verify_email(
user.email,
mailtext.lang_for_user(user),
issue_verification_link(db, user),
@@ -1125,12 +1172,18 @@ def revoke_preset_password_if_unverified(user: User) -> None:
so a legit no-SMTP registration is unaffected.
The Google-adoption callback is the ONLY caller (E9.5 S1). Admin approval used to call this too, and
that was redundant: `password_login` refuses an unverified account before it even reads `is_active`,
so approval hands a pre-emptor's password nothing it could use. All the admin-path revoke achieved
was destroying the password a legitimate registrant had just chosen — and since the admin normally
decides before the user reads their mail, that made the "your password was cleared" detour the
DEFAULT path rather than an edge case. Keep it that way: if a second caller ever looks justified,
check first whether the gate it is guarding already runs earlier.
dropping it was right for the reason S1 gave — the revoke destroyed the password a legitimate
registrant had just chosen, and since the admin normally decides before the user reads their mail,
that made the "your password was cleared" detour the DEFAULT path rather than an edge case.
S1 also claimed the revoke was REDUNDANT there, because `password_login` refuses an unverified
account before it reads `is_active`. That part was wrong, and the epic's own review caught it: the
gate holds only while the address is unverified, and S6's approval mail exists to get the real
owner to end that state. The guard that actually replaced it lives in `_register_account` — while
an address is unverified, the newest registration replaces the password and deletes the
outstanding verify tokens, so the owner reclaims the account by registering. Do not re-add a
caller here without re-reading that: two revokes racing would put "your password was cleared"
back in front of every legitimate registrant.
`password_revoked_at` is what makes "the owner re-sets one via password-reset" actually true. The
revoke leaves `password_hash IS NULL`, which is also what a Google-only account looks like — and