diff --git a/backend/app/models.py b/backend/app/models.py index 2544111b..d25911cd 100644 --- a/backend/app/models.py +++ b/backend/app/models.py @@ -47,7 +47,8 @@ class User(Base, TimestampMixin): email: Mapped[str] = mapped_column(String(320), unique=True, index=True) # argon2id hash for email+password login; NULL for Google-only / demo accounts. password_hash: Mapped[str | None] = mapped_column(String(255)) - # When SEC-10 dropped a preset password (see auth.revoke_preset_password_if_unverified). Without + # When SEC-10 dropped a preset password on Google adoption (see the single caller of + # auth.revoke_preset_password_if_unverified; admin approval no longer revokes — E9.5 S1). Without # this trace a revoked account was indistinguishable from a Google-only one, so it failed the # password-reset eligibility check and could never be recovered — it could neither sign in nor # reset. Set = "had a password, lost it to the guard" -> reset-eligible. Cleared whenever a diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index 70ee72f6..24f724fb 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -27,7 +27,6 @@ from app.models import ( Invite, User, ) -from app.routes.auth import revoke_preset_password_if_unverified from app.utils import iso, valid_email router = APIRouter(prefix="/api/admin", tags=["admin"]) @@ -88,13 +87,18 @@ def list_invites( def _activate_user_by_email(db: Session, email: str) -> None: """Approving access activates a matching (e.g. pending password-registration) account so - it can sign in. No-op if no such user exists yet (a Google user is created at first login).""" + it can sign in. No-op if no such user exists yet (a Google user is created at first login). + + E9.5 S1: this path deliberately does NOT revoke a preset password. Approval is not an ownership + attestation, and it does not need to be one — `password_login` refuses an unverified account + BEFORE it looks at `is_active`, so a pre-emptor who set a password on someone else's address + still cannot sign in with it, approved or not. Revoking here only ever destroyed the password a + legitimate registrant had just chosen, and made "your password was cleared for security" the + DEFAULT experience, because approve-before-verify is the normal ordering. The revoke stays on + the Google-adoption path (auth.revoke_preset_password_if_unverified), where the real owner does + attest ownership and the preset password must not survive.""" user = db.execute(select(User).where(User.email == email.lower())).scalar_one_or_none() if user is not None and not user.is_active: - # SEC-10: same guard as the Google-adoption path — approving an account whose email was never - # verified must not hand a working login to a password set on it by whoever registered the - # address (a pre-emption). Shared single-source helper so the two paths can't drift. - revoke_preset_password_if_unverified(user) user.is_active = True db.commit() diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index 8cc9c316..c6ff8a91 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -664,11 +664,13 @@ def verify_email_confirm(body: TokenIn, db: Session = Depends(get_db)) -> dict: """Confirm an email-verification token the SPA read from the URL fragment (SB3). Uniform 400 on any invalid/expired/used token. - `password_reset_required` tells the SPA to send the user straight to the reset flow: an admin who - approved this account BEFORE it was verified tripped SEC-10, so the password chosen at registration - is gone and sign-in would answer with the deliberately uniform "Invalid email or password" — true, - but unreadable to the person who just proved they own the mailbox. Saying so leaks nothing: holding - a valid single-use verify token for this account already proves that much.""" + `password_reset_required` tells the SPA to send the user straight to the reset flow: SEC-10 had + dropped the password this account registered with, so sign-in would answer with the deliberately + uniform "Invalid email or password" — true, but unreadable to the person who just proved they own + the mailbox. Saying so leaks nothing: holding a valid single-use verify token for this account + already proves that much. Since E9.5 S1 the common source of that state is gone (admin approval no + longer revokes), leaving accounts stripped before that change and the Google-adoption path, so this + flag is rare now — but it must stay, because those users have no other way back in.""" user = _consume_token(db, body.token, "verify") if user is None: raise HTTPException( @@ -859,10 +861,17 @@ def bump_session_epoch(user: User) -> None: def revoke_preset_password_if_unverified(user: User) -> None: """SEC-10 (account pre-emption): a password set on an account whose email was NEVER verified is untrusted — whoever registered the address (to pre-empt the real owner) could have chosen it. When - ownership is finally attested (Google adoption, or admin approval), drop that password and bump the - epoch to revoke any session it minted; the owner re-sets one via password-reset. No-op for an - already-verified account, so a legit no-SMTP registration is unaffected. The single source for this - rule — both the Google-callback and admin-activation paths call it so they can't drift. + the real owner attests ownership through Google, drop that password and bump the epoch to revoke any + session it minted; the owner re-sets one via password-reset. No-op for an already-verified account, + 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. `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 diff --git a/backend/tests/test_auth_session.py b/backend/tests/test_auth_session.py index fe63b04f..35ef8143 100644 --- a/backend/tests/test_auth_session.py +++ b/backend/tests/test_auth_session.py @@ -97,28 +97,73 @@ def test_reset_confirm_is_single_use_and_kills_existing_sessions(client, make_us assert replay.status_code == 400 -def test_activating_an_unverified_account_drops_its_preset_password(db, make_user): - # SEC-10 (account pre-emption): an attacker registers victim@x with a password before the approved - # owner arrives. The address was never verified, so admin approval must NOT hand that pre-set - # password a working login — it is dropped and any session it minted is revoked. +def test_approving_an_unverified_account_keeps_its_password(db, make_user): + # E9.5 S1, inverting an earlier rule. Approval used to drop a password set while the email was + # unverified (SEC-10, account pre-emption). That was redundant: password_login refuses an + # unverified account BEFORE it reads is_active, so approval hands a pre-emptor's password nothing + # — see the pair of tests below. All it did was destroy the password a legitimate registrant had + # just chosen, in what is the NORMAL ordering (the admin decides before the user reads their mail). from app.routes.admin import _activate_user_by_email - victim = make_user( - email="victim@example.com", - password="attacker-chosen", + user = make_user( + email="approved-first@example.com", + password="chosen at registration", email_verified=False, is_active=False, ) - epoch0 = victim.session_epoch or 0 - _activate_user_by_email(db, "victim@example.com") - assert victim.is_active is True - assert victim.password_hash is None - assert (victim.session_epoch or 0) == epoch0 + 1 + epoch0 = user.session_epoch or 0 + + _activate_user_by_email(db, "approved-first@example.com") + + assert user.is_active is True + assert user.password_hash is not None + assert user.password_revoked_at is None + assert (user.session_epoch or 0) == epoch0 # nothing to revoke, so no epoch bump either + + +def test_an_approved_but_unverified_account_still_cannot_sign_in(client, make_user): + # Why dropping the approval-path revoke is safe: this is the gate that actually stops a + # pre-emptor, and it runs whatever the admin decided. + make_user( + email="notyet@example.com", + password="attacker-chosen", + email_verified=False, + is_active=True, + ) + + r = _login(client, "notyet@example.com", "attacker-chosen") + + assert r.status_code == 403 + assert "verify" in r.json()["detail"].lower() + + +def test_verifying_afterwards_lets_the_original_password_in(client, db, make_user): + # The payoff for the legitimate registrant: the password chosen at registration still works once + # the mailbox is proven — no "your password was cleared for security" detour, no reset round-trip. + from app.routes.auth import _issue_token + + user = make_user( + email="ordering@example.com", + password="chosen at registration", + email_verified=False, + is_active=False, + ) + from app.routes.admin import _activate_user_by_email + + _activate_user_by_email(db, "ordering@example.com") # admin decides FIRST — the common ordering + raw = _issue_token(db, user, "verify", timedelta(hours=1)) + db.commit() + + verified = client.post("/auth/verify", json={"token": raw}) + + assert verified.status_code == 200 + assert verified.json() == {"ok": True, "password_reset_required": False} + assert _login(client, "ordering@example.com", "chosen at registration").status_code == 200 def test_activating_a_verified_account_keeps_its_password(db, make_user): - # A legit account whose email IS verified (e.g. a no-SMTP registration) keeps its password on - # approval — the guard must not break the normal flow. + # The no-SMTP shape: registration marks such an account verified up front, so it was never in + # scope for the SEC-10 guard even before E9.5 S1. Kept as the other half of the pair. from app.routes.admin import _activate_user_by_email user = make_user( @@ -212,7 +257,7 @@ def test_registration_with_an_already_pending_request_persists_the_account( def test_an_admin_can_activate_a_pre_approved_registrant(db, monkeypatch): # The recovery path for the account above, pinned so it cannot quietly break: re-entering the # address in the Access-requests whitelist box (POST /api/admin/invites) runs - # _activate_user_by_email, which is also what keeps the SEC-10 preset-password guard in play. + # _activate_user_by_email. from app.models import Invite from app.routes.admin import _activate_user_by_email @@ -225,7 +270,7 @@ def test_an_admin_can_activate_a_pre_approved_registrant(db, monkeypatch): db.expire_all() assert user.is_active is True - assert user.password_hash is not None # a verified account keeps its own password + assert user.password_hash is not None # approval never touches the password (E9.5 S1) def test_registration_of_an_unknown_email_still_persists_account_and_pending_invite( @@ -346,11 +391,24 @@ def test_register_route_is_refused_when_registration_is_closed(client, db, monke # --- SEC-10 lockout: a revoked preset password must stay RECOVERABLE --------------------------- # # Regression pack for the prod incident of 2026-09-02. An account registered with a password, then -# approved by an admin BEFORE its email was verified, ended up with password_hash = NULL. That is -# also what a Google-only account looks like, and reset eligibility keyed off `password_hash` alone — -# so the account could neither sign in (no password) nor obtain a reset link, and every response on -# the way was a deliberately uniform "ok"/401. `password_revoked_at` is the trace that tells the two -# apart; these pin each step of the way back in. +# stripped of it by the SEC-10 guard, ended up with password_hash = NULL. That is also what a +# Google-only account looks like, and reset eligibility keyed off `password_hash` alone — so the +# account could neither sign in (no password) nor obtain a reset link, and every response on the way +# was a deliberately uniform "ok"/401. `password_revoked_at` is the trace that tells the two apart; +# these pin each step of the way back in. +# +# E9.5 S1 removed the admin-approval caller, so Google adoption is the only path that still produces +# this state — plus every account stripped on prod before that change, which is why the whole recovery +# route below must keep working. These tests manufacture the state through the surviving producer. + + +def _revoke_as_google_adoption(db, user) -> None: + """Put an account into the SEC-10-revoked state the way the ONLY remaining producer does: the + Google callback adopting an account whose email was never verified.""" + from app.routes.auth import revoke_preset_password_if_unverified + + revoke_preset_password_if_unverified(user) + db.commit() def _capture_reset_links(monkeypatch) -> list[str]: @@ -362,9 +420,9 @@ def _capture_reset_links(monkeypatch) -> list[str]: return sent -def test_the_approval_revoke_stamps_password_revoked_at(db, make_user): - from app.routes.admin import _activate_user_by_email - +def test_the_google_adoption_revoke_stamps_password_revoked_at(db, make_user): + # The guard still fires where it earns its keep: the real owner has just proven ownership through + # Google, so a password set on the address while it was unverified must not survive. victim = make_user( email="stamped@example.com", password="attacker-chosen", @@ -372,27 +430,28 @@ def test_the_approval_revoke_stamps_password_revoked_at(db, make_user): is_active=False, ) assert victim.password_revoked_at is None + epoch0 = victim.session_epoch or 0 - _activate_user_by_email(db, "stamped@example.com") + _revoke_as_google_adoption(db, victim) assert victim.password_hash is None assert victim.password_revoked_at is not None # …so the reset flow can tell it HAD a password + assert (victim.session_epoch or 0) == epoch0 + 1 # any session that password minted is killed def test_an_account_whose_password_was_revoked_can_still_get_a_reset_link( db, make_user, monkeypatch ): # THE lockout bug: without the stamp this sent nothing, and the account was unrecoverable. - from app.routes.admin import _activate_user_by_email from app.routes.auth import _send_reset_if_eligible - make_user( + victim = make_user( email="lockedout@example.com", password="chosen at registration", email_verified=False, is_active=False, ) - _activate_user_by_email(db, "lockedout@example.com") + _revoke_as_google_adoption(db, victim) sent = _capture_reset_links(monkeypatch) _send_reset_if_eligible("lockedout@example.com") @@ -414,16 +473,15 @@ def test_a_google_only_account_is_still_not_reset_eligible(db, make_user, monkey def test_completing_a_reset_clears_the_revocation_stamp(client, db, make_user): - from app.routes.admin import _activate_user_by_email from app.routes.auth import _issue_token user = make_user( email="recovered@example.com", password="chosen at registration", email_verified=False, - is_active=False, + is_active=True, # already approved; reset-confirm verifies the email but never activates ) - _activate_user_by_email(db, "recovered@example.com") + _revoke_as_google_adoption(db, user) raw = _issue_token(db, user, "reset", timedelta(hours=1)) db.commit() @@ -442,7 +500,6 @@ def test_verify_tells_a_revoked_account_that_it_needs_a_new_password(client, db, # The banner's input. Saying so leaks nothing: the caller just proved mailbox control by holding a # valid single-use verify token. Without it the user meets only the uniform "Invalid email or # password" and has no way to learn why the password they chose stopped working. - from app.routes.admin import _activate_user_by_email from app.routes.auth import _issue_token user = make_user( @@ -451,7 +508,7 @@ def test_verify_tells_a_revoked_account_that_it_needs_a_new_password(client, db, email_verified=False, is_active=False, ) - _activate_user_by_email(db, "needspw@example.com") + _revoke_as_google_adoption(db, user) raw = _issue_token(db, user, "verify", timedelta(hours=1)) db.commit() diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index 563181db..a016d015 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -339,9 +339,10 @@ function AuthCard() { verifyFired.current = true; api .verifyEmail(verifyToken) - // "ok_reset": verified, but SEC-10 had dropped the password this account registered with (an - // admin approved it before it was verified). Sign-in would answer with the uniform credentials - // error, so point the user at the reset flow instead. Same value the legacy ?verify= redirect uses. + // "ok_reset": verified, but SEC-10 had dropped the password this account registered with. Sign-in + // would answer with the uniform credentials error, so point the user at the reset flow instead. + // Same value the legacy ?verify= redirect uses. Rare since E9.5 S1 (admin approval no longer + // revokes) — it still covers accounts stripped before that and the Google-adoption path. .then((r) => setVerify(r.password_reset_required ? "ok_reset" : "ok")) .catch(() => setVerify("invalid")); }, [verifyToken]); diff --git a/frontend/src/lib/api/auth.ts b/frontend/src/lib/api/auth.ts index 66cd7416..2f8ec006 100644 --- a/frontend/src/lib/api/auth.ts +++ b/frontend/src/lib/api/auth.ts @@ -37,9 +37,10 @@ export const authApi = { { quiet: true }, ), // Confirm an email-verification token the SPA read from the URL fragment (SB3). - // `password_reset_required` = SEC-10 dropped the password this account registered with (an admin - // approved it before it was verified), so signing in would fail with the uniform credentials error. - // The card sends the user to the reset flow instead of leaving them to guess. + // `password_reset_required` = SEC-10 dropped the password this account registered with, so signing + // in would fail with the uniform credentials error. The card sends the user to the reset flow instead + // of leaving them to guess. Rare since E9.5 S1: only accounts stripped before that change, and the + // Google-adoption path, still reach it. verifyEmail: (token: string): Promise<{ ok: boolean; password_reset_required?: boolean }> => req("/auth/verify", { method: "POST", body: JSON.stringify({ token }) }, { quiet: true }), // Re-issue a verification link (the first one expired, or was lost). Uniform response — never