revert(auth): the newest registration does not own an unverified account (E9.5 review round 2)
Round 1 fixed the address pre-emption chain by letting a later registration replace the password on an unverified account and delete its outstanding verify tokens. Round 2 read that code and showed it was worse than what it fixed, so it goes back out. It closes nothing. The rule is symmetric and an attacker can simply register last — on a timer, inside the five-per-five-minutes budget — while the owner registers once. What it does add is two primitives an unauthenticated caller did not have: overwrite a real registrant's chosen password, and delete the confirmation link already sitting in their inbox (the approval mail's 7-day token is an ordinary verify token, so the blanket delete took that too). Worst of all it fired on the rows SEC-10 left stripped — password_hash NULL, password_revoked_at set, active — installing an attacker's password where the entire state means "there is none", and clearing the stamp that is the only reason password-reset still accepts them. That is a straight takeover which did not exist before, in the code written to prevent one. The owner's recourse needed no new primitive and already worked: password-reset accepts a pre-empted account, proves the mailbox, rewrites the hash and bumps the session epoch. What remains is the bounded window between a confirmation click and that reset, which is SEC-16 — logged with this attempt written down, so the next person does not reach for it again. Two smaller round-2 findings go with it. `upsert_pending_invite`'s new exact-spelling tie-break ran before any status preference, so it could deterministically pick a DENIED sibling over a pending one and reopen it — the very outcome its comment claimed to prevent; the status preference now comes first. And `send_invitation` activated the account before the send it might have to report as failed, so a request the UI called a failure had quietly let someone in; activation now follows a mail that actually left. The verify token minted to render that mail still commits either way — it is single-use, expires on its own and was mailed nowhere, and the note says so rather than leaving the next reader to wonder whether it was missed.
This commit is contained in:
+17
-10
@@ -143,12 +143,12 @@ def _activate_user_by_email(db: Session, email: str) -> None:
|
||||
S1 justified that by saying `password_login` refuses an unverified account BEFORE it looks at
|
||||
`is_active`, so a pre-emptor could not sign in "approved or not". A review of the finished epic
|
||||
showed that argument only holds while the address stays unverified — and S6's approval mail
|
||||
hands the real owner a confirmation link precisely so they will end that state. What actually
|
||||
guards this now is `_register_account`: while an address is unverified the NEWEST registration
|
||||
replaces the password and kills the outstanding verify tokens, so the real owner takes the
|
||||
account back by registering, and a pre-emptor's password is only ever as good as the owner's
|
||||
silence. The residual (an owner who confirms without ever registering or resetting) is logged
|
||||
for E12 beside SEC-15. The revoke also stays on the Google-adoption path
|
||||
hands the real owner a confirmation link precisely so they will end that state. Nothing replaced
|
||||
the revoke, and that is deliberate: round 2 of the same review tried "the newest registration
|
||||
owns an unverified account" and reverted it, because the rule is symmetric — an attacker can
|
||||
always register last — and it handed anonymous callers a write onto a real account's credential.
|
||||
The owner's recourse is password-reset, which proves the mailbox, rewrites the hash and bumps
|
||||
the session epoch. The residual is logged as SEC-16. The revoke also stays on the Google-adoption path
|
||||
(auth.revoke_preset_password_if_unverified), where ownership is actually attested."""
|
||||
# Canonical, with the literal spelling preferred when both exist: approving
|
||||
# `npeter83.temu@gmail.com` must activate the account that registered as `npeter83temu@gmail.com`,
|
||||
@@ -299,10 +299,6 @@ def send_invitation(
|
||||
# but a button the admin pressed must not answer with a success toast for a mail that was
|
||||
# never sent.
|
||||
raise HTTPException(status_code=409, detail={"code": "invite_send", "reason": "email_off"})
|
||||
# Same as the other two paths that decide for an address (`approve_invite`, `add_invite`): an
|
||||
# approved row means this mailbox may come in, so finish it. Without this the mail can promise
|
||||
# "your account is ready — just sign in" to someone `password_login` will refuse for `is_active`.
|
||||
_activate_user_by_email(db, inv.email)
|
||||
link, has_account, hours = _welcome_mail_parts(db, inv.email)
|
||||
# An explicit pick from the dialog is recorded, so a later mail to this address matches the one
|
||||
# the admin already sent. Only the explicit pick — writing the fallback back would turn a column
|
||||
@@ -317,10 +313,21 @@ def send_invitation(
|
||||
# exists to answer "did I already write to them?", and the UI dims the button and warns about a
|
||||
# second mail on the strength of it. Stamping it before a send that then failed makes the one
|
||||
# question it answers a lie, and talks the admin out of the retry that would fix it.
|
||||
#
|
||||
# One thing a failed send does leave behind: the confirmation token `_welcome_mail_parts` minted
|
||||
# to render the link, which commits inside `_issue_token`. Accepted rather than chased — the
|
||||
# token is single-use, expires on its own, and was mailed nowhere; deferring it would mean
|
||||
# rewriting a helper three callers share so the handler owns its commit.
|
||||
if not email_mod.send_access_invitation(inv.email, lang, link, has_account, hours):
|
||||
raise HTTPException(
|
||||
status_code=422, detail={"code": "invite_send", "reason": "send_failed"}
|
||||
)
|
||||
# Only after the mail is actually out. Same as the other two paths that decide for an address
|
||||
# (`approve_invite`, `add_invite`): an approved row means this mailbox may come in, so finish it,
|
||||
# or the mail can promise "your account is ready — just sign in" to someone `password_login` will
|
||||
# refuse for `is_active`. Doing it BEFORE the send would mean a request the UI reports as failed
|
||||
# had quietly activated an account behind the admin's back.
|
||||
_activate_user_by_email(db, inv.email)
|
||||
inv.invited_at = datetime.now(timezone.utc)
|
||||
audit.record(
|
||||
db,
|
||||
|
||||
+29
-32
@@ -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 delete, select, update
|
||||
from sqlalchemy import select, update
|
||||
from sqlalchemy.exc import IntegrityError
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
@@ -235,12 +235,15 @@ def upsert_pending_invite(db: Session, email: str, lang: str | None = None) -> I
|
||||
.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.
|
||||
# An approved row wins, so a denied duplicate can't mask an approval. Then a row that is not
|
||||
# DENIED, preferring the exact spelling — a denied sibling must never be picked while a pending
|
||||
# row exists, or the reopen branch below would fire and put one person in the admin's queue
|
||||
# twice. Only when every row is denied does the spelling decide which one reopens.
|
||||
undecided = [r for r in rows if r.status != "denied"]
|
||||
inv = (
|
||||
next((r for r in rows if r.status == "approved"), None)
|
||||
or next((r for r in undecided if r.email == email), None)
|
||||
or next(iter(undecided), None)
|
||||
or next((r for r in rows if r.email == email), None)
|
||||
or next(iter(rows), None)
|
||||
)
|
||||
@@ -837,29 +840,21 @@ 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.
|
||||
#
|
||||
# And a re-send is ALL it does: the submitted password is discarded. E9.5's closing
|
||||
# review tried "while the address is unverified, the newest registration owns it"
|
||||
# (replace the hash, kill the outstanding verify tokens) as a fix for address
|
||||
# pre-emption, and round 2 showed it was worse than the problem — DO NOT REPEAT IT.
|
||||
# The rule is symmetric, and an attacker can always be last: it hands an unauthenticated
|
||||
# caller the power to overwrite a real registrant's chosen password and delete the
|
||||
# confirmation link already in their inbox, and on a SEC-10-stripped row (password_hash
|
||||
# NULL, password_revoked_at set) it would install a password where the whole point was
|
||||
# that there is none. The owner's real recourse already exists and needs no new
|
||||
# primitive: password-reset accepts this account, proves the mailbox, rewrites the hash
|
||||
# and bumps the session epoch. The residual is logged as SEC-16.
|
||||
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)
|
||||
if not user.email_verified and not user.is_demo and 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
|
||||
@@ -1179,11 +1174,13 @@ def revoke_preset_password_if_unverified(user: User) -> None:
|
||||
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.
|
||||
owner to end that state — at which point a pre-emptor's password works. Nothing replaced the
|
||||
revoke, and that is deliberate. The review tried "the newest registration owns an unverified
|
||||
account" and reverted it: the rule is symmetric, so an attacker can simply register last, and it
|
||||
would have handed anonymous callers a write onto a real account's credential. What the owner
|
||||
actually has is password-reset, which proves the mailbox, rewrites the hash and bumps the epoch.
|
||||
The residual is logged as SEC-16; re-adding a caller here would put "your password was cleared"
|
||||
back in front of every legitimate registrant, which is the whole reason S1 removed it.
|
||||
|
||||
`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
|
||||
|
||||
@@ -87,68 +87,92 @@ def test_a_weeks_link_is_described_in_days_not_hours(wire):
|
||||
assert "24 óra" in wire[1][2]
|
||||
|
||||
|
||||
# --- the pre-emption guard: while unverified, the newest registration owns the account -------------
|
||||
# --- what a second registration may and may not do -------------------------------------------------
|
||||
|
||||
|
||||
def test_registering_again_takes_the_account_back_from_a_pre_emptor(db, client, smtp_on, wire):
|
||||
"""The hole E9.5 S1's argument left open, and the guard that replaces it.
|
||||
def test_a_later_registration_never_touches_an_existing_password(db, client, smtp_on, wire):
|
||||
"""The rule E9.5's review round 2 put back, having tried the opposite and found it worse.
|
||||
|
||||
S1 stopped admin approval revoking a preset password, reasoning that `password_login` refuses an
|
||||
unverified account before it reads `is_active`. True — but S6's approval mail exists to get the
|
||||
real owner to confirm the address, and that click opens the gate for whatever password is on the
|
||||
row. So the owner must be able to take the row back, and re-registering is how.
|
||||
"While the address is unverified, the newest registration owns it" looked like a fix for address
|
||||
pre-emption. It is symmetric — an attacker can always be last — so it closed nothing, and it
|
||||
handed an unauthenticated caller the power to overwrite a real registrant's chosen password and
|
||||
delete the confirmation link already sitting in their inbox. On a SEC-10-stripped row it would
|
||||
have installed a password where the whole point is that there is none. Reverted; the owner's
|
||||
recourse is password-reset, which proves the mailbox. Residual logged as SEC-16.
|
||||
"""
|
||||
client.post("/auth/register", json={"email": "victim@example.com", "password": "pre-emptor pw"})
|
||||
db.expire_all()
|
||||
user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one()
|
||||
squatted = user.password_hash
|
||||
assert squatted is not None
|
||||
|
||||
client.post(
|
||||
"/auth/register", json={"email": "victim@example.com", "password": "the owner's pw"}
|
||||
"/auth/register", json={"email": "victim@example.com", "password": "the first password"}
|
||||
)
|
||||
|
||||
db.expire_all()
|
||||
user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one()
|
||||
assert user.password_hash != squatted
|
||||
# One account, not two — the canonical pre-check still holds.
|
||||
assert (
|
||||
len(db.execute(select(User).where(User.email_canonical == "victim@example.com")).all()) == 1
|
||||
)
|
||||
|
||||
|
||||
def test_the_pre_emptors_verify_link_dies_with_their_password(db, client, smtp_on, wire):
|
||||
client.post("/auth/register", json={"email": "victim@example.com", "password": "pre-emptor pw"})
|
||||
db.expire_all()
|
||||
user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one()
|
||||
first = db.execute(select(AuthToken.id).where(AuthToken.user_id == user.id)).scalars().all()
|
||||
assert len(first) == 1
|
||||
|
||||
client.post(
|
||||
"/auth/register", json={"email": "victim@example.com", "password": "the owner's pw"}
|
||||
)
|
||||
|
||||
db.expire_all()
|
||||
live = db.execute(select(AuthToken).where(AuthToken.user_id == user.id)).scalars().all()
|
||||
# Exactly one token, and not the one already in the pre-emptor's hands: otherwise they could
|
||||
# confirm the address themselves and re-open the gate for a password that is no longer theirs.
|
||||
assert len(live) == 1 and live[0].id != first[0]
|
||||
|
||||
|
||||
def test_a_confirmed_account_is_not_re_opened_by_a_later_registration(
|
||||
db, client, smtp_on, make_user
|
||||
):
|
||||
# The guard is scoped to the UNVERIFIED window. Once the address is confirmed the account is its
|
||||
# owner's, and a stranger posting the form must not touch the password.
|
||||
user = make_user(email="done@example.com", password=_PW) # already verified
|
||||
before = user.password_hash
|
||||
|
||||
client.post("/auth/register", json={"email": "done@example.com", "password": "not yours"})
|
||||
client.post(
|
||||
"/auth/register", json={"email": "victim@example.com", "password": "the second password"}
|
||||
)
|
||||
|
||||
db.expire_all()
|
||||
assert db.get(User, user.id).password_hash == before
|
||||
|
||||
|
||||
def test_a_later_registration_leaves_a_link_already_in_flight_alive(db, client, smtp_on, wire):
|
||||
client.post(
|
||||
"/auth/register", json={"email": "victim@example.com", "password": "the first password"}
|
||||
)
|
||||
db.expire_all()
|
||||
user = db.execute(select(User).where(User.email == "victim@example.com")).scalar_one()
|
||||
first = db.execute(select(AuthToken.id).where(AuthToken.user_id == user.id)).scalars().all()
|
||||
|
||||
client.post(
|
||||
"/auth/register", json={"email": "victim@example.com", "password": "the second password"}
|
||||
)
|
||||
|
||||
db.expire_all()
|
||||
live = db.execute(select(AuthToken.id).where(AuthToken.user_id == user.id)).scalars().all()
|
||||
# The re-send adds a link; it must not invalidate the one the owner may already be holding.
|
||||
assert set(first) <= set(live)
|
||||
|
||||
|
||||
def test_a_stripped_account_keeps_its_recovery_stamp(db, client, smtp_on, make_user):
|
||||
"""Prod carries accounts SEC-10 left with no password and a `password_revoked_at` stamp. That
|
||||
stamp is the only reason password-reset still accepts them; an anonymous POST must not be able to
|
||||
clear it, nor to put a password on a row whose whole state is "there is none"."""
|
||||
stripped = make_user(
|
||||
email="stripped@example.com",
|
||||
password=None,
|
||||
email_verified=False,
|
||||
is_active=True,
|
||||
password_revoked_at=datetime.now(timezone.utc),
|
||||
)
|
||||
|
||||
client.post(
|
||||
"/auth/register", json={"email": "stripped@example.com", "password": "an attacker password"}
|
||||
)
|
||||
|
||||
db.expire_all()
|
||||
row = db.get(User, stripped.id)
|
||||
assert row.password_hash is None and row.password_revoked_at is not None
|
||||
|
||||
|
||||
def test_a_denied_sibling_is_not_reopened_while_a_pending_row_exists(db, client):
|
||||
# Canonical siblings exist on prod. Picking the denied one reopens it and re-notifies every
|
||||
# admin, putting one person in the queue twice — and the exact-spelling tie-break made that
|
||||
# deterministic rather than merely possible.
|
||||
db.add_all(
|
||||
[
|
||||
Invite(email="pair.one@gmail.com", status="pending"),
|
||||
Invite(email="pairone@gmail.com", status="denied"),
|
||||
]
|
||||
)
|
||||
db.commit()
|
||||
|
||||
client.post("/auth/request-access", json={"email": "pairone@gmail.com"})
|
||||
|
||||
db.expire_all()
|
||||
rows = {i.email: i.status for i in db.execute(select(Invite)).scalars().all()}
|
||||
assert rows == {"pair.one@gmail.com": "pending", "pairone@gmail.com": "denied"}
|
||||
|
||||
|
||||
# --- the env bootstrap answers the same question as the gate --------------------------------------
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user