From ac277739f9b0de17666fa8e77ce8cc8f468135cc Mon Sep 17 00:00:00 2001 From: Anchel135 Date: Sun, 30 Aug 2026 13:28:15 +0300 Subject: [PATCH 01/12] feat: verify email addresses before creating an account Email/password signup no longer creates a user straight away. The submitted details are parked on a PendingSignup node together with a hashed, single-use token, and a confirmation link is mailed out. Opening the link is what creates the User and establishes the browser session, so a freshly verified visitor is already logged in and never has to type their credentials a second time. Because an unconfirmed address is simply absent from the account graph there is no email_verified flag to check at each call site, and no way to hold a session for an unverified account. Mail delivery goes through a new api/mail.py transport seam that picks console, a file outbox or SMTP from the environment. MAIL_OUTBOX_DIR deliberately takes precedence over MAIL_SERVER so a test run cannot quietly mail real addresses; the Playwright suite reads the link back out of that outbox. Resends are throttled per address and capped, and the resend endpoint answers identically whether or not the address is pending. Closes #217 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .env.example | 29 +- .github/wordlist.txt | 4 + .github/workflows/playwright.yml | 4 + .gitignore | 1 + AGENTS.md | 2 + Makefile | 1 + README.md | 21 ++ api/auth/email_verification.py | 356 ++++++++++++++++++++ api/mail.py | 220 +++++++++++++ api/routes/auth.py | 261 ++++++++++++--- app/src/components/modals/LoginModal.tsx | 368 ++++++++++++++++++--- app/src/config/api.ts | 3 + app/src/contexts/AuthContext.tsx | 11 +- app/src/pages/Index.tsx | 64 +++- app/src/services/auth.ts | 73 ++++- app/src/types/api.ts | 19 ++ e2e/logic/api/apiResponses.ts | 9 + e2e/logic/api/mailbox.ts | 111 +++++++ e2e/tests/auth.setup.ts | 127 +++----- tests/test_auth_status.py | 10 +- tests/test_email_signup.py | 393 +++++++++++++++++++---- tests/test_email_verification.py | 252 +++++++++++++++ tests/test_mail.py | 164 ++++++++++ 23 files changed, 2261 insertions(+), 242 deletions(-) create mode 100644 api/auth/email_verification.py create mode 100644 api/mail.py create mode 100644 e2e/logic/api/mailbox.ts create mode 100644 tests/test_email_verification.py create mode 100644 tests/test_mail.py diff --git a/.env.example b/.env.example index 8bbc1919..e8d702da 100644 --- a/.env.example +++ b/.env.example @@ -161,16 +161,37 @@ FALKORDB_URL=redis://localhost:6379/0 # REQUIRED - change to your FalkorDB URL # OAUTH_BASE_URL=http://localhost:5000 # ----------------------------- -# Email Configuration (optional - for sending invitation emails) +# Email Configuration # ----------------------------- +# Signup with email/password sends a confirmation link, and the account is only +# created when that link is opened. Without MAIL_SERVER the message is written +# to the application log instead of being sent, which is enough for local +# development -- copy the link out of the log. +# +# Any provider works: Mailgun, SendGrid, Resend, SES and Postmark all expose an +# SMTP endpoint. # MAIL_SERVER=smtp.mailgun.org -# MAIL_PORT=587 -# MAIL_USE_TLS=True -# MAIL_USERNAME=your_mail_username +# MAIL_PORT=587 # 465 selects implicit TLS (SMTPS) +# MAIL_USE_TLS=True # STARTTLS on non-465 ports +# MAIL_USERNAME=your_mail_username # omit to send unauthenticated # MAIL_PASSWORD=your_mail_password # MAIL_DEFAULT_SENDER=noreply@yourdomain.com +# MAIL_TIMEOUT_SECONDS=10 + +# Write messages to this directory as .eml files instead of sending them. Used +# by the Playwright suite to read the verification link back. Takes precedence +# over MAIL_SERVER, so a machine with a real relay configured can still run the +# tests without mailing anyone. +# MAIL_OUTBOX_DIR=e2e/.mail + +# Email/password auth is on by default when no OAuth provider is configured. # EMAIL_AUTH_ENABLED=false +# Verification link lifetime and per-address send limits. +# EMAIL_VERIFICATION_TTL_HOURS=24 +# EMAIL_VERIFICATION_RESEND_SECONDS=60 +# EMAIL_VERIFICATION_MAX_SENDS=5 + # ----------------------------- # Frontend / analytics (optional) # ----------------------------- diff --git a/.github/wordlist.txt b/.github/wordlist.txt index 9dce194d..42c1d955 100644 --- a/.github/wordlist.txt +++ b/.github/wordlist.txt @@ -124,3 +124,7 @@ SDK Dependabot PyPI pypi +signup +SMTP +outbox +PendingSignup diff --git a/.github/workflows/playwright.yml b/.github/workflows/playwright.yml index db1f904b..f1acbadb 100644 --- a/.github/workflows/playwright.yml +++ b/.github/workflows/playwright.yml @@ -135,6 +135,7 @@ jobs: FASTAPI_DEBUG=False FALKORDB_URL=redis://localhost:6379 DISABLE_MCP=true + MAIL_OUTBOX_DIR=e2e/.mail EOF # Start the FastAPI application @@ -169,6 +170,9 @@ jobs: FASTAPI_DEBUG: False FALKORDB_URL: redis://localhost:6379 DISABLE_MCP: true + # Signup mails a confirmation link and the account is only created when + # it is opened, so the suite has to be able to read the message back. + MAIL_OUTBOX_DIR: e2e/.mail # Azure OpenAI API keys - required for database schema analysis AZURE_API_KEY: ${{ secrets.AZURE_API_KEY }} AZURE_API_BASE: ${{ secrets.AZURE_API_BASE }} diff --git a/.gitignore b/.gitignore index 2360cdc8..3ef769ff 100644 --- a/.gitignore +++ b/.gitignore @@ -39,6 +39,7 @@ demo_tokens.py /playwright/.cache/ /playwright/.auth/ e2e/.auth/ +e2e/.mail/ # Build artifacts clients/python/queryweaver_client.egg-info/ clients/ts/dist/ diff --git a/AGENTS.md b/AGENTS.md index e99d9abe..2ce774e9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -132,6 +132,8 @@ Optional overrides: `COMPLETION_MODEL`, `EMBEDDING_MODEL` (must match provider), Authentication is deliberately split three ways — a signed session cookie for the browser login (no FalkorDB dependency, see `api/auth/browser_session.py`), FalkorDB-backed API tokens for programmatic clients, and per-request data-source credentials. `validate_user` in `api/auth/user_management.py` owns the precedence between them. +Email/password signup is verified before the account exists: `POST /signup/email` parks the details on a `PendingSignup` node (`api/auth/email_verification.py`) and mails a link, and `GET /verify/email` is what creates the `User` and establishes the session. There is therefore no `email_verified` flag anywhere — an unconfirmed address is simply absent from the account graph. Mail goes through `api/mail.py`, which picks a transport from the environment: console (default), a file outbox (`MAIL_OUTBOX_DIR`, used by the Playwright suite to read the link back) or SMTP (`MAIL_SERVER`). + See `.env.example` for the full list. ## CI/CD diff --git a/Makefile b/Makefile index b79bc5ce..0bccaf65 100644 --- a/Makefile +++ b/Makefile @@ -76,6 +76,7 @@ clean: ## Clean up test artifacts rm -rf test-results/ rm -rf playwright-report/ rm -rf e2e/.auth/ + rm -rf e2e/.mail/ rm -rf __pycache__/ rm -rf dist/ rm -rf *.egg-info/ diff --git a/README.md b/README.md index cc407cc0..f528cf13 100644 --- a/README.md +++ b/README.md @@ -173,6 +173,27 @@ the browser, so there is none to revoke — tokens are created explicitly from t tokens API and revoked there. (A legacy `api_token` cookie left over from an older release is cleared and revoked on logout too.) +#### Email signup is verified before the account exists + +Signing up with an email address and password does not create an account. The +submitted details are parked, a confirmation link is mailed to the address, and +the account — and the session — come into being only when that link is opened. +So an address the registrant does not control never becomes an account at all, +and there is no half-real user for the rest of the system to reason about. + +The link is single-use and expires after 24 hours. Opening it signs the browser +in directly: the password was chosen minutes earlier, and asking for it again +would prove nothing. A signup can be re-sent from the same screen, subject to a +per-address rate limit. + +Without a mail server configured the message is written to the application log +instead of being sent, so local development can complete the flow by copying the +link out of the log. Set `MAIL_SERVER` (plus `MAIL_PORT`, `MAIL_USERNAME`, +`MAIL_PASSWORD`, `MAIL_DEFAULT_SENDER`) to send for real; any provider with an +SMTP endpoint works. `EMAIL_VERIFICATION_TTL_HOURS`, +`EMAIL_VERIFICATION_RESEND_SECONDS` and `EMAIL_VERIFICATION_MAX_SENDS` tune the +lifetime and the limits. See `.env.example` for the full list. + The trade-off of a signed session cookie is that it cannot be revoked from the server before it expires: the TTL bounds the damage, and rotating `FASTAPI_SECRET_KEY` invalidates every browser login at once. API tokens keep diff --git a/api/auth/email_verification.py b/api/auth/email_verification.py new file mode 100644 index 00000000..bad26942 --- /dev/null +++ b/api/auth/email_verification.py @@ -0,0 +1,356 @@ +"""Signup email verification -- the holding pen for accounts that do not exist yet. + +Signing up does not create a user. It records the submitted details on a +``PendingSignup`` node and mails a link; clicking that link is what creates the +``User`` and ``Identity`` and logs the browser in. An address that is never +confirmed therefore never becomes an account at all -- nothing to count, nothing +to log in as, nothing to clean up beyond an expiring node. + +That ordering is what makes the guarantee cheap. There is no ``email_verified`` +flag to check at every call site and no half-real user for the org graph, +analytics or quota logic to trip over, because an unverified address is simply +absent from the account graph. + +The link carries a 256-bit random token. Only its SHA-256 is stored, so a +snapshot of the graph does not yield a working link -- the same reasoning that +keeps passwords hashed. Plain SHA-256 rather than a slow KDF is deliberate and +sufficient here: the input is full-entropy random, so there is no dictionary to +grind and nothing for a work factor to buy. + +Tokens are single-use (consuming one deletes the node) and expiring, and sends +are rate-limited per address so the endpoint cannot be used to mail-bomb a third +party. +""" + +import hashlib +import html +import logging +import os +import secrets +import time +from dataclasses import dataclass +from typing import Optional, Tuple + +from api.config import ORGANIZATIONS_GRAPH +from api.extensions import db +from api.mail import send_mail + +# Long enough to survive a link sitting in an inbox overnight, short enough that +# an intercepted mail does not stay useful. +DEFAULT_TTL_HOURS = 24 + +# Minimum gap between two sends to one address. +DEFAULT_RESEND_INTERVAL_SECONDS = 60 + +# Total sends allowed for one pending signup, resend included. Bounds how much +# mail a single submitted address can generate. +DEFAULT_MAX_SENDS = 5 + +# Outcomes of redeeming a token, kept as constants so routes and tests agree on +# the spelling. +RESULT_OK = "ok" +RESULT_INVALID = "invalid" +RESULT_EXPIRED = "expired" + + +@dataclass(frozen=True) +class PendingSignup: + """The details captured at signup, replayed when the link is clicked.""" + + email: str + first_name: str + last_name: str + password_hash: str + + @property + def full_name(self) -> str: + """Display name, matching the format the signup route stored.""" + return f"{self.first_name} {self.last_name}".strip() + + +@dataclass(frozen=True) +class TokenIssue: + """The result of asking for a verification link. + + ``token`` is the only time the raw value exists outside the mail; the graph + keeps just its hash. ``throttled`` and ``exhausted`` are separated so the + caller can tell "come back in a minute" from "stop asking". + """ + + token: Optional[str] = None + first_name: Optional[str] = None + throttled: bool = False + exhausted: bool = False + missing: bool = False + + @property + def issued(self) -> bool: + """Whether a link was actually produced.""" + return self.token is not None + + +def _positive_int_env(name: str, default: int) -> int: + """Read a positive integer setting, falling back on anything unusable.""" + raw = os.getenv(name) + if raw: + try: + value = int(raw) + if value > 0: + return value + except ValueError: + pass + logging.warning("Invalid %s value %r, using %s", name, raw, default) + return default + + +def token_ttl_seconds() -> int: + """How long a verification link stays valid.""" + return _positive_int_env("EMAIL_VERIFICATION_TTL_HOURS", DEFAULT_TTL_HOURS) * 3600 + + +def resend_interval_seconds() -> int: + """Minimum gap between two sends to the same address.""" + return _positive_int_env( + "EMAIL_VERIFICATION_RESEND_SECONDS", DEFAULT_RESEND_INTERVAL_SECONDS + ) + + +def max_sends() -> int: + """Total sends allowed for a single pending signup.""" + return _positive_int_env("EMAIL_VERIFICATION_MAX_SENDS", DEFAULT_MAX_SENDS) + + +def _now_ms() -> int: + """Wall-clock milliseconds, matching the units Cypher's ``timestamp()`` uses.""" + return int(time.time() * 1000) + + +def hash_token(token: str) -> str: + """Hash a raw token for storage and lookup.""" + return hashlib.sha256(token.encode("utf-8")).hexdigest() + + +def _graph(): + """The Organizations graph, where identity records live.""" + return db.select_graph(ORGANIZATIONS_GRAPH) + + +async def _read_send_state(email: str) -> Tuple[Optional[int], int, Optional[str]]: + """Return ``(last_sent_at, send_count, first_name)`` for a pending signup.""" + result = await _graph().query( + """ + MATCH (p:PendingSignup {email: $email}) + RETURN p.last_sent_at AS last_sent_at, + p.send_count AS send_count, + p.first_name AS first_name + """, + {"email": email}, + ) + if not result.result_set: + return None, 0, None + last_sent_at, send_count, first_name = result.result_set[0] + return last_sent_at, int(send_count or 0), first_name + + +def _throttle(last_sent_at: Optional[int], send_count: int, now: int) -> Optional[TokenIssue]: + """Decide whether another send is allowed, or why it is not.""" + if send_count >= max_sends(): + return TokenIssue(exhausted=True) + if last_sent_at is not None and now - last_sent_at < resend_interval_seconds() * 1000: + return TokenIssue(throttled=True) + return None + + +async def start_pending_signup( + email: str, first_name: str, last_name: str, password_hash: str +) -> TokenIssue: + """Record a signup awaiting verification and return its link token. + + Re-submitting the form for an address that is already pending replaces the + stored details and invalidates the previous link, so the most recent attempt + is the one that works. The send counter deliberately survives that replace: + otherwise resubmitting would reset the rate limit and defeat it. + """ + now = _now_ms() + last_sent_at, send_count, _ = await _read_send_state(email) + + refusal = _throttle(last_sent_at, send_count, now) + if refusal is not None: + return refusal + + token = secrets.token_urlsafe(32) + await _graph().query( + """ + MERGE (p:PendingSignup {email: $email}) + ON CREATE SET p.created_at = $now + SET p.token_hash = $token_hash, + p.first_name = $first_name, + p.last_name = $last_name, + p.password_hash = $password_hash, + p.expires_at = $expires_at, + p.last_sent_at = $now, + p.send_count = $send_count + """, + { + "email": email, + "token_hash": hash_token(token), + "first_name": first_name, + "last_name": last_name, + "password_hash": password_hash, + "expires_at": now + token_ttl_seconds() * 1000, + "now": now, + "send_count": send_count + 1, + }, + ) + return TokenIssue(token=token, first_name=first_name) + + +async def refresh_pending_signup(email: str) -> TokenIssue: + """Issue a fresh link for an existing pending signup. + + Only ever refreshes; it will not create a pending signup, so the resend + endpoint cannot be used to send mail to an address nobody submitted. + """ + now = _now_ms() + last_sent_at, send_count, first_name = await _read_send_state(email) + if send_count == 0 and last_sent_at is None and first_name is None: + return TokenIssue(missing=True) + + refusal = _throttle(last_sent_at, send_count, now) + if refusal is not None: + return refusal + + token = secrets.token_urlsafe(32) + result = await _graph().query( + """ + MATCH (p:PendingSignup {email: $email}) + SET p.token_hash = $token_hash, + p.expires_at = $expires_at, + p.last_sent_at = $now, + p.send_count = $send_count + RETURN p.first_name AS first_name + """, + { + "email": email, + "token_hash": hash_token(token), + "expires_at": now + token_ttl_seconds() * 1000, + "now": now, + "send_count": send_count + 1, + }, + ) + if not result.result_set: + # Lost a race with a verification that just consumed the record. + return TokenIssue(missing=True) + + return TokenIssue(token=token, first_name=result.result_set[0][0]) + + +async def consume_pending_signup(token: str) -> Tuple[Optional[PendingSignup], str]: + """Redeem a verification token exactly once. + + Returns ``(pending, RESULT_OK)`` when the token was live. The node is + deleted in the same query that reads it, so a replayed link finds nothing -- + that, not a flag, is what makes the token single-use. + + Lookup is by token *hash*, an exact match on a stored value, so there is no + secret-dependent comparison here for timing to leak. + """ + if not token: + return None, RESULT_INVALID + + result = await _graph().query( + """ + MATCH (p:PendingSignup {token_hash: $token_hash}) + WITH p, + p.email AS email, + p.first_name AS first_name, + p.last_name AS last_name, + p.password_hash AS password_hash, + p.expires_at AS expires_at + DELETE p + RETURN email, first_name, last_name, password_hash, expires_at + """, + {"token_hash": hash_token(token)}, + ) + if not result.result_set: + return None, RESULT_INVALID + + email, first_name, last_name, password_hash, expires_at = result.result_set[0] + + # Expired tokens are consumed rather than left behind: the link is dead + # either way, and dropping the record keeps abandoned signups from + # accumulating. The user simply signs up again. + if not isinstance(expires_at, (int, float)) or _now_ms() >= expires_at: + return None, RESULT_EXPIRED + + if not email or not password_hash: + logging.error("Discarding a malformed pending signup record") + return None, RESULT_INVALID + + return ( + PendingSignup( + email=email, + first_name=first_name or "", + last_name=last_name or "", + password_hash=password_hash, + ), + RESULT_OK, + ) + + +async def discard_pending_signup(email: str) -> None: + """Drop any pending signup for an address. Best-effort; never fatal. + + Called once an account exists for the address by some other route, so a + stale link cannot later be redeemed against it. + """ + try: + await _graph().query( + "MATCH (p:PendingSignup {email: $email}) DELETE p", {"email": email} + ) + except Exception as e: # pylint: disable=broad-exception-caught + logging.warning("Could not discard pending signup: %s", e) + + +def _greeting(first_name: Optional[str]) -> str: + """``Hi Ada`` when a name is known, plain ``Hi`` when it is not.""" + name = (first_name or "").strip() + return f"Hi {name}," if name else "Hi," + + +async def send_verification_link( + email: str, first_name: Optional[str], verify_url: str +) -> bool: + """Mail the verification link. Returns whether it was handed to a transport.""" + hours = token_ttl_seconds() // 3600 + greeting = _greeting(first_name) + + text_body = ( + f"{greeting}\n\n" + "Confirm your email address to finish creating your QueryWeaver account:\n\n" + f"{verify_url}\n\n" + f"The link works once and expires in {hours} hours. Your account is not " + "created until you open it.\n\n" + "If you did not sign up for QueryWeaver, ignore this message -- no " + "account exists for this address and none will be created.\n" + ) + + html_body = ( + "" + f"

{html.escape(greeting)}

" + "

Confirm your email address to finish creating your QueryWeaver " + "account:

" + f'

Confirm my email address

' + f"

The link works once and expires in {hours} hours. Your account is " + "not created until you open it.

" + "

If you did not sign up for QueryWeaver, ignore this message — " + "no account exists for this address and none will be created.

" + "" + ) + + return await send_mail( + to=email, + subject="Confirm your QueryWeaver email address", + text_body=text_body, + html_body=html_body, + ) diff --git a/api/mail.py b/api/mail.py new file mode 100644 index 00000000..34e4b62a --- /dev/null +++ b/api/mail.py @@ -0,0 +1,220 @@ +"""Outbound mail. + +QueryWeaver sends one kind of message today -- the signup verification link -- +so this module stays small: pick a transport from the environment, hand it a +built message, and report whether it left the process. + +Three transports: + +* ``console`` (the default) writes the message to the log instead of sending + it, so local development completes the signup flow without a mail server. +* ``file`` writes each message to ``MAIL_OUTBOX_DIR`` as an ``.eml``. The + Playwright suite reads the verification link back out of it, which is what + keeps the end-to-end signup test exercising the real flow instead of a + test-only shortcut through the backend. It takes precedence over a configured + relay: nobody sets this variable by accident, and a test run that quietly + mails real addresses is a worse failure than one that quietly does not. +* ``smtp`` talks to any relay. Every hosted provider -- Mailgun, SendGrid, + Resend, SES, Postmark -- exposes an SMTP endpoint, so this covers them all + without binding the project to a vendor SDK. + +The relay is selected by whether ``MAIL_SERVER`` is set rather than by a +separate switch, so a half-configured relay cannot be selected by accident. + +``smtplib`` is synchronous, so sends run on a worker thread: a relay that takes +its time must not stall the event loop for every other request. +""" + +import asyncio +import logging +import os +import secrets +import smtplib +import ssl +import time +from email.message import EmailMessage +from pathlib import Path +from typing import Optional + +# Implicit-TLS port. Everything else is assumed to be plain SMTP that may be +# upgraded with STARTTLS. +SMTPS_PORT = 465 + +DEFAULT_SMTP_PORT = 587 +DEFAULT_TIMEOUT_SECONDS = 10.0 +FALLBACK_SENDER = "no-reply@queryweaver.local" + + +def _env_flag(name: str, default: bool) -> bool: + """Read a boolean environment variable, falling back on anything unrecognised.""" + raw = os.getenv(name) + if raw is None: + return default + value = raw.strip().lower() + if value in ("true", "1", "yes", "on"): + return True + if value in ("false", "0", "no", "off"): + return False + logging.warning("Invalid %s value %r, using %s", name, raw, default) + return default + + +def _smtp_timeout() -> float: + """Socket timeout for the relay, from ``MAIL_TIMEOUT_SECONDS``.""" + raw = os.getenv("MAIL_TIMEOUT_SECONDS") + if raw: + try: + timeout = float(raw) + if timeout > 0: + return timeout + except ValueError: + pass + logging.warning("Invalid MAIL_TIMEOUT_SECONDS value %r, ignoring", raw) + return DEFAULT_TIMEOUT_SECONDS + + +def _smtp_port() -> int: + """Relay port, from ``MAIL_PORT``.""" + raw = os.getenv("MAIL_PORT") + if raw: + try: + return int(raw) + except ValueError: + logging.warning("Invalid MAIL_PORT value %r, ignoring", raw) + return DEFAULT_SMTP_PORT + + +def is_smtp_configured() -> bool: + """Whether a relay is configured. When ``False``, mail is logged, not sent.""" + return bool(os.getenv("MAIL_SERVER", "").strip()) + + +def outbox_dir() -> str: + """Directory for the file transport, from ``MAIL_OUTBOX_DIR``. Empty when unset.""" + return os.getenv("MAIL_OUTBOX_DIR", "").strip() + + +def transport_name() -> str: + """Name of the active transport, for logs and diagnostics.""" + if outbox_dir(): + return "file" + return "smtp" if is_smtp_configured() else "console" + + +def default_sender() -> str: + """The ``From`` address.""" + return ( + os.getenv("MAIL_DEFAULT_SENDER", "").strip() + or os.getenv("MAIL_USERNAME", "").strip() + or FALLBACK_SENDER + ) + + +def _build_message(*, to: str, subject: str, text_body: str, html_body: Optional[str]) -> EmailMessage: + """Assemble the message. Raises ``ValueError`` for an unusable recipient.""" + # Header injection guard. Callers validate the address first, but a header + # split turns one verification mail into mail to anybody, so it is checked + # again at the point the header is actually written. + if not to or any(char in to for char in "\r\n"): + raise ValueError("Recipient address contains a line break") + + message = EmailMessage() + message["From"] = default_sender() + message["To"] = to + message["Subject"] = subject + message.set_content(text_body) + if html_body: + message.add_alternative(html_body, subtype="html") + return message + + +def _send_via_smtp(message: EmailMessage) -> None: + """Hand the message to the relay. Blocking; call it on a worker thread.""" + host = os.getenv("MAIL_SERVER", "").strip() + port = _smtp_port() + username = os.getenv("MAIL_USERNAME", "").strip() + password = os.getenv("MAIL_PASSWORD", "") + timeout = _smtp_timeout() + + if port == SMTPS_PORT: + context = ssl.create_default_context() + with smtplib.SMTP_SSL(host, port, timeout=timeout, context=context) as client: + if username: + client.login(username, password) + client.send_message(message) + return + + with smtplib.SMTP(host, port, timeout=timeout) as client: + if _env_flag("MAIL_USE_TLS", True): + client.starttls(context=ssl.create_default_context()) + # The pre-STARTTLS greeting is unauthenticated, so the capability + # list has to be re-read over the encrypted channel. + client.ehlo() + if username: + client.login(username, password) + client.send_message(message) + + +def _log_to_console(message: EmailMessage, text_body: str) -> None: + """Write the message to the log in place of sending it.""" + logging.info( + "[mail:console] No MAIL_SERVER configured, so this message was not sent.\n" + "To: %s\nSubject: %s\n\n%s", + message["To"], + message["Subject"], + text_body, + ) + + +def _write_to_outbox(message: EmailMessage, directory: str) -> bool: + """Drop the message into the outbox directory as an ``.eml`` file.""" + try: + path = Path(directory) + path.mkdir(parents=True, exist_ok=True) + # Timestamp first so readers can pick the newest by name; the random + # suffix keeps two sends in the same millisecond from colliding. + name = f"{int(time.time() * 1000)}-{secrets.token_hex(4)}.eml" + (path / name).write_bytes(message.as_bytes()) + return True + except OSError as e: + logging.error("Could not write mail to the outbox: %s", e) + return False + + +async def send_mail( + *, + to: str, + subject: str, + text_body: str, + html_body: Optional[str] = None, +) -> bool: + """Send one message. Returns ``False`` instead of raising when it could not go. + + Callers are signup paths, where a failed send must not lose the work already + done or leak relay detail to the client, so every failure is logged here and + reported as a plain boolean. + """ + try: + message = _build_message( + to=to, subject=subject, text_body=text_body, html_body=html_body + ) + except ValueError as e: + logging.error("Refusing to send mail: %s", e) + return False + + directory = outbox_dir() + if directory: + return _write_to_outbox(message, directory) + + if not is_smtp_configured(): + _log_to_console(message, text_body) + return True + + try: + await asyncio.to_thread(_send_via_smtp, message) + return True + except (smtplib.SMTPException, OSError, ssl.SSLError) as e: + # Deliberately does not log the message body: it carries the + # verification link, which is a credential. + logging.error("Could not send mail via SMTP: %s", e) + return False diff --git a/api/routes/auth.py b/api/routes/auth.py index e665fcf1..91888ccd 100644 --- a/api/routes/auth.py +++ b/api/routes/auth.py @@ -9,7 +9,7 @@ import re from pathlib import Path -from urllib.parse import urljoin +from urllib.parse import urlencode, urljoin from authlib.integrations.starlette_client import OAuth @@ -27,6 +27,15 @@ mark_provisioned, read_browser_session, ) +from api.auth.email_verification import ( + RESULT_EXPIRED, + consume_pending_signup, + discard_pending_signup, + refresh_pending_signup, + resend_interval_seconds, + send_verification_link, + start_pending_signup, +) from api.auth.user_management import delete_user_token, ensure_user_in_organizations, validate_user from api.config import ORGANIZATIONS_GRAPH from api.core.errors import AuthBackendUnavailableError, TRANSIENT_BACKEND_ERRORS @@ -106,6 +115,10 @@ class EmailSignupRequest(BaseModel): email: str password: str +class EmailResendRequest(BaseModel): + """Request to re-send a signup verification link.""" + email: str + # ---- Password utilities ---- def _hash_password(password: str) -> str: """Hash a password using PBKDF2 with a random salt.""" @@ -143,7 +156,10 @@ def _sanitize_for_log(value: str) -> str: def _validate_email(email: str) -> bool: """Basic email validation.""" - pattern = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$' + # ``\Z`` rather than ``$``: ``$`` also matches immediately before a trailing + # newline, which would let "victim@example.com\nBcc: ..." through and split + # the headers of the verification mail this address is about to receive. + pattern = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}\Z' return re.match(pattern, email) is not None async def _set_mail_hash(email: str, password_hash: str) -> bool: @@ -302,6 +318,44 @@ async def _complete_login(request: Request, provider: str, user_data: dict) -> N ) +# Values the SPA reads back off ``/?verified=`` after following a link. Kept as +# constants so the contract with the frontend is greppable from one place. +VERIFY_SUCCESS = "success" +VERIFY_INVALID = "invalid" +VERIFY_EXPIRED = "expired" +VERIFY_EXISTS = "exists" +VERIFY_FAILED = "failed" +VERIFY_UNAVAILABLE = "unavailable" + + +def _verification_redirect(result: str) -> RedirectResponse: + """Send the browser back into the app carrying the outcome.""" + return RedirectResponse( + url="/?" + urlencode({"verified": result}), + status_code=status.HTTP_303_SEE_OTHER, + ) + + +def _refuse_verification_send(issue) -> JSONResponse: + """Turn a refused link request into a response.""" + if issue.exhausted: + return JSONResponse( + {"success": False, + "error": "Too many verification emails have been sent to this address. " + "Please try again later."}, + status_code=status.HTTP_429_TOO_MANY_REQUESTS, + ) + wait = resend_interval_seconds() + return JSONResponse( + {"success": False, + "error": "A verification email was just sent. Please wait a moment before " + "requesting another.", + "retryAfterSeconds": wait}, + status_code=status.HTTP_429_TOO_MANY_REQUESTS, + headers={"Retry-After": str(wait)}, + ) + + # ---- Email Authentication Routes ---- @auth_router.post("/signup/email") async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSONResponse: @@ -347,56 +401,49 @@ async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSO # (CVE-2026-10130, authentication bypass via signup token issuance). if await _email_account_exists(email): logging.info("Signup attempt for existing account: %s", _sanitize_for_log(email)) + # An account exists, so any link still outstanding for this address + # must not stay redeemable against it. + await discard_pending_signup(email) return JSONResponse( {"success": False, "error": "An account with this email already exists"}, status_code=status.HTTP_409_CONFLICT ) - # ``api_token=None``: signup logs the browser in with the session cookie - # and never returns a token, so minting one here would only leave an - # orphan Token node behind. - is_new_identity, user_info = await ensure_user_in_organizations(email, email, - f"{first_name} {last_name}", "email", None) - - if not (is_new_identity and user_info and user_info.get("new_identity")): - # Creation failed (e.g. DB error) or raced with a concurrent signup. - logging.error("Failed to create new user during signup: %s", - _sanitize_for_log(email)) - return JSONResponse( - {"success": False, "error": "Registration failed"}, - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR - ) - - logging.info("New user created: %s", _sanitize_for_log(email)) - - # Hash password + # Nothing is created yet. The details are parked on a PendingSignup node + # and only become a User when the emailed link is opened, so an address + # the registrant does not control never turns into an account at all. password_hash = _hash_password(password) + issue = await start_pending_signup(email, first_name, last_name, password_hash) - # Set email hash - await _set_mail_hash(email, password_hash) - - logging.info("User registration successful: %s", _sanitize_for_log(email)) + if not issue.issued: + return _refuse_verification_send(issue) - if not establish_browser_session( - request, - email=email, - name=f"{first_name} {last_name}", - provider="email", - provider_user_id=email, - provisioned=True, - ): - # The account exists but the browser has no credential, so reporting - # success would leave the user staring at a logged-out page. - logging.error("Could not establish a browser session after email signup") + verify_url = _build_callback_url( + request, "verify/email?" + urlencode({"token": issue.token}) + ) + if not await send_verification_link(email, first_name, verify_url): + # The link never left the building, so the pending record is dead + # weight that would only burn the rate limit on the retry. + await discard_pending_signup(email) + logging.error("Could not send the verification email for %s", + _sanitize_for_log(email)) return JSONResponse( - {"success": False, "error": "Registration failed"}, - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR + {"success": False, + "error": "Could not send the verification email - please retry"}, + status_code=status.HTTP_503_SERVICE_UNAVAILABLE ) - response = JSONResponse({ + logging.info("Verification email sent for pending signup: %s", + _sanitize_for_log(email)) + + # 202, not 201: the account does not exist yet, and will not until the + # link is opened. No session is established here for the same reason. + return JSONResponse({ "success": True, - }, status_code=201) - return response + "pending": True, + "email": email, + "message": "Check your inbox for a link to confirm your email address.", + }, status_code=status.HTTP_202_ACCEPTED) except (AuthBackendUnavailableError, *TRANSIENT_BACKEND_ERRORS) as e: # Same reasoning as /login/email: an unreachable store is not a rejected @@ -415,6 +462,137 @@ async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSO status_code=status.HTTP_500_INTERNAL_SERVER_ERROR ) +@auth_router.get("/verify/email") +async def verify_email(request: Request, token: str = "") -> RedirectResponse: + """Redeem a signup verification link. + + This is where the account is actually created. Opening the link is the proof + that the registrant controls the address, so creating the ``User`` here -- + rather than at signup and flagging it afterwards -- is what keeps an + unconfirmed address from existing as an account at all. + + The browser is logged in on the way through. The person clicking chose their + password minutes ago; making them type it again would buy nothing. + """ + if not _is_email_auth_enabled(): + return _verification_redirect(VERIFY_FAILED) + + try: + pending, result = await consume_pending_signup(token) + + if result == RESULT_EXPIRED: + return _verification_redirect(VERIFY_EXPIRED) + if pending is None: + return _verification_redirect(VERIFY_INVALID) + + # The address may have acquired an account by another route (Google, + # GitHub) while the link sat unopened. The token is spent either way. + if await _email_account_exists(pending.email): + logging.info("Verification link for an address that now has an account: %s", + _sanitize_for_log(pending.email)) + return _verification_redirect(VERIFY_EXISTS) + + # ``api_token=None``: the browser is credentialed by the session cookie, + # so minting a token here would only leave an orphan Token node. + is_new_identity, user_info = await ensure_user_in_organizations( + pending.email, pending.email, pending.full_name, "email", None + ) + if not (is_new_identity and user_info and user_info.get("new_identity")): + logging.error("Could not create the verified account for %s", + _sanitize_for_log(pending.email)) + return _verification_redirect(VERIFY_FAILED) + + await _set_mail_hash(pending.email, pending.password_hash) + + if not establish_browser_session( + request, + email=pending.email, + name=pending.full_name, + provider="email", + provider_user_id=pending.email, + provisioned=True, + ): + # Unlike the old signup path this is recoverable: the account is + # real, so the user can simply log in. + logging.error("Verified %s but could not establish a browser session", + _sanitize_for_log(pending.email)) + return _verification_redirect(VERIFY_FAILED) + + logging.info("Email verified and account created: %s", + _sanitize_for_log(pending.email)) + return _verification_redirect(VERIFY_SUCCESS) + + except (AuthBackendUnavailableError, *TRANSIENT_BACKEND_ERRORS) as e: + logging.error("Auth store unreachable during email verification: %s", e) + return _verification_redirect(VERIFY_UNAVAILABLE) + except HTTPException: + # How _set_mail_hash reports failure. The identity exists but carries no + # password hash, so the login it enables is the one thing that will not + # work; surfacing a redirect beats a stack trace in the browser. + logging.error("Could not store the password for a freshly verified account") + return _verification_redirect(VERIFY_FAILED) + except Exception as e: + logging.error("Email verification error: %s", e) + return _verification_redirect(VERIFY_FAILED) + + +@auth_router.post("/signup/email/resend") +async def resend_verification_email( + request: Request, resend_data: EmailResendRequest +) -> JSONResponse: + """Re-send a signup verification link. + + Always answers the same way once the address is well-formed. Whether it has + a signup awaiting confirmation, an account already, or nothing at all is not + something an unauthenticated caller gets to learn from this endpoint -- and + that includes the rate-limit state, which would otherwise answer the same + question a beat later. + """ + if not _is_email_auth_enabled(): + return JSONResponse( + {"success": False, "error": "Email authentication is not enabled"}, + status_code=status.HTTP_403_FORBIDDEN + ) + + email = resend_data.email.strip().lower() if resend_data.email else "" + if not _validate_email(email): + return JSONResponse( + {"success": False, "error": "Invalid email format"}, + status_code=status.HTTP_400_BAD_REQUEST + ) + + accepted = JSONResponse( + {"success": True, + "message": "If that address is waiting to be confirmed, a new link is on its way."}, + status_code=status.HTTP_202_ACCEPTED + ) + + try: + issue = await refresh_pending_signup(email) + except (AuthBackendUnavailableError, *TRANSIENT_BACKEND_ERRORS) as e: + logging.error("Auth store unreachable during verification resend: %s", e) + return JSONResponse( + {"success": False, + "error": "Authentication service temporarily unavailable - please retry"}, + status_code=status.HTTP_503_SERVICE_UNAVAILABLE + ) + + if not issue.issued: + # Unknown address, throttled and exhausted are indistinguishable here by + # design; only the log tells them apart. + logging.info("Verification resend not issued for %s", _sanitize_for_log(email)) + return accepted + + verify_url = _build_callback_url( + request, "verify/email?" + urlencode({"token": issue.token}) + ) + if not await send_verification_link(email, issue.first_name, verify_url): + logging.error("Could not re-send the verification email for %s", + _sanitize_for_log(email)) + + return accepted + + @auth_router.post("/login/email") async def email_login(request: Request, login_data: EmailLoginRequest) -> JSONResponse: """Handle email/password user login.""" @@ -750,6 +928,7 @@ async def auth_status(request: Request) -> JSONResponse: response = JSONResponse( content={ "authenticated": True, + "providers": _get_auth_config(), "user": { # Falls back to the email so the id is always a usable string # for clients, even for database-backed API tokens. @@ -767,7 +946,7 @@ async def auth_status(request: Request) -> JSONResponse: # Not authenticated - return 200 with authenticated: false # This is NOT an error - unauthenticated users can still use the app return JSONResponse( - content={"authenticated": False}, + content={"authenticated": False, "providers": _get_auth_config()}, status_code=200 ) diff --git a/app/src/components/modals/LoginModal.tsx b/app/src/components/modals/LoginModal.tsx index 65c8a389..6daa01db 100644 --- a/app/src/components/modals/LoginModal.tsx +++ b/app/src/components/modals/LoginModal.tsx @@ -1,5 +1,11 @@ +import { useEffect, useState } from "react"; import { Dialog, DialogContent, DialogHeader, DialogTitle, DialogDescription } from "@/components/ui/dialog"; import { Button } from "@/components/ui/button"; +import { Input } from "@/components/ui/input"; +import { Label } from "@/components/ui/label"; +import { useToast } from "@/components/ui/use-toast"; +import { useAuth } from "@/contexts/AuthContext"; +import { AuthService } from "@/services/auth"; import { buildApiUrl, API_CONFIG } from "@/config/api"; interface LoginModalProps { @@ -8,7 +14,49 @@ interface LoginModalProps { canClose?: boolean; // Whether user can close the modal (false for required login) } +const MIN_PASSWORD_LENGTH = 8; + +// Mirrors the backend's own resend interval, so the button comes back at +// roughly the moment another request would actually be honoured. +const RESEND_COOLDOWN_SECONDS = 60; + +const emptyForm = { firstName: "", lastName: "", email: "", password: "" }; + const LoginModal = ({ open, onOpenChange, canClose = true }: LoginModalProps) => { + const { providers, refreshAuth } = useAuth(); + const { toast } = useToast(); + + const [mode, setMode] = useState<"login" | "signup">("login"); + const [form, setForm] = useState(emptyForm); + const [error, setError] = useState(null); + const [submitting, setSubmitting] = useState(false); + // Set once a signup has been accepted. While it holds an address the modal + // shows the "check your inbox" state instead of the form -- there is no + // account yet, so there is nothing else to offer. + const [awaitingEmail, setAwaitingEmail] = useState(null); + const [cooldown, setCooldown] = useState(0); + + const emailEnabled = providers?.email_auth_enabled ?? false; + // Default to showing the OAuth buttons: a backend that does not report + // providers at all would otherwise leave the user with no way in. + const googleEnabled = providers?.google_auth_enabled ?? true; + const githubEnabled = providers?.github_auth_enabled ?? true; + const showOAuth = googleEnabled || githubEnabled; + + useEffect(() => { + if (cooldown <= 0) return; + const timer = setTimeout(() => setCooldown((seconds) => seconds - 1), 1000); + return () => clearTimeout(timer); + }, [cooldown]); + + // Reopening should not drop the user back into a stale error. + useEffect(() => { + if (!open) { + setError(null); + setSubmitting(false); + } + }, [open]); + const handleGoogleLogin = () => { window.location.href = buildApiUrl(API_CONFIG.ENDPOINTS.LOGIN_GOOGLE); }; @@ -17,6 +65,215 @@ const LoginModal = ({ open, onOpenChange, canClose = true }: LoginModalProps) => window.location.href = buildApiUrl(API_CONFIG.ENDPOINTS.LOGIN_GITHUB); }; + const switchMode = (next: "login" | "signup") => { + setMode(next); + setError(null); + }; + + const update = + (field: keyof typeof emptyForm) => (event: React.ChangeEvent) => { + setForm((current) => ({ ...current, [field]: event.target.value })); + }; + + const handleSubmit = async (event: React.FormEvent) => { + event.preventDefault(); + setError(null); + + if (mode === "signup" && form.password.length < MIN_PASSWORD_LENGTH) { + setError(`Password must be at least ${MIN_PASSWORD_LENGTH} characters long`); + return; + } + + setSubmitting(true); + try { + if (mode === "login") { + const result = await AuthService.loginWithEmail(form.email.trim(), form.password); + if (!result.success) { + setError(result.error ?? "Could not sign you in."); + return; + } + await refreshAuth(); + setForm(emptyForm); + onOpenChange(false); + return; + } + + const result = await AuthService.signupWithEmail({ + firstName: form.firstName.trim(), + lastName: form.lastName.trim(), + email: form.email.trim(), + password: form.password, + }); + + if (!result.success) { + setError(result.error ?? "Could not create your account."); + return; + } + + // Deliberately no refreshAuth(): signing up does not sign you in. The + // account is created when the emailed link is opened, and opening it is + // what establishes the session. + setAwaitingEmail(result.email ?? form.email.trim()); + setCooldown(RESEND_COOLDOWN_SECONDS); + setForm(emptyForm); + } catch { + setError("Could not reach the server. Please try again."); + } finally { + setSubmitting(false); + } + }; + + const handleResend = async () => { + if (!awaitingEmail || cooldown > 0) return; + setCooldown(RESEND_COOLDOWN_SECONDS); + const result = await AuthService.resendVerification(awaitingEmail); + toast({ + title: result.success ? "Email sent" : "Could not send the email", + description: result.success + ? result.message ?? "Check your inbox for the confirmation link." + : result.error, + variant: result.success ? undefined : "destructive", + }); + }; + + const backToSignIn = () => { + setAwaitingEmail(null); + setMode("login"); + setError(null); + }; + + const renderAwaitingVerification = () => ( +
+

+ We sent a confirmation link to{" "} + {awaitingEmail}. Open it to + finish creating your account — you will be signed in straight away. +

+

+ The link works once and expires after 24 hours. Until you open it, no account exists. +

+ + +
+ ); + + const renderEmailForm = () => ( +
+ {mode === "signup" && ( +
+
+ + +
+
+ + +
+
+ )} + +
+ + +
+ +
+ + + {mode === "signup" && ( +

+ At least {MIN_PASSWORD_LENGTH} characters. +

+ )} +
+ + {error && ( +

+ {error} +

+ )} + + + +

+ {mode === "login" ? ( + <> + Don't have an account?{" "} + + + ) : ( + <> + Already have an account?{" "} + + + )} +

+
+ ); + return ( > - Welcome to QueryWeaver + {awaitingEmail ? "Check your inbox" : "Welcome to QueryWeaver"} - Sign in to access your databases and start querying + {awaitingEmail + ? "One more step before your account exists" + : "Sign in to access your databases and start querying"} -
- - - -
+ {awaitingEmail ? ( + renderAwaitingVerification() + ) : ( + <> + {showOAuth && ( +
+ {googleEnabled && ( + + )} + + {githubEnabled && ( + + )} +
+ )} + + {emailEnabled && showOAuth && ( +
+
+ +
+
+ or +
+
+ )} + + {emailEnabled &&
{renderEmailForm()}
} + + )} - {canClose && ( + {canClose && !awaitingEmail && (

By signing in, you agree to our Terms of Service and Privacy Policy

diff --git a/app/src/config/api.ts b/app/src/config/api.ts index cc5fc6ab..3ebef690 100644 --- a/app/src/config/api.ts +++ b/app/src/config/api.ts @@ -18,6 +18,9 @@ export const API_CONFIG = { AUTH_STATUS: '/auth-status', LOGIN_GOOGLE: '/login/google', LOGIN_GITHUB: '/login/github', + LOGIN_EMAIL: '/login/email', + SIGNUP_EMAIL: '/signup/email', + RESEND_VERIFICATION: '/signup/email/resend', LOGOUT: '/logout', // Graph/Database management diff --git a/app/src/contexts/AuthContext.tsx b/app/src/contexts/AuthContext.tsx index 81a52306..e0c962dd 100644 --- a/app/src/contexts/AuthContext.tsx +++ b/app/src/contexts/AuthContext.tsx @@ -1,6 +1,6 @@ import React, { createContext, useContext, useState, useEffect, useRef } from 'react'; import { AuthService } from '@/services/auth'; -import type { User } from '@/types/api'; +import type { AuthProviders, User } from '@/types/api'; // How long to wait before re-checking while the backend cannot answer. The // login survives the outage, so the session comes back on its own once the @@ -10,6 +10,8 @@ const UNAVAILABLE_RETRY_MS = 15000; interface AuthContextType { user: User | null; isAuthenticated: boolean; + /** Which sign-in methods the backend offers; null until the first check lands. */ + providers: AuthProviders | null; /** The check could not be made, as opposed to answering "not logged in". */ isUnavailable: boolean; isLoading: boolean; @@ -25,6 +27,7 @@ const AuthContext = createContext(undefined); export const AuthProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => { const [user, setUser] = useState(null); + const [providers, setProviders] = useState(null); const [isUnavailable, setIsUnavailable] = useState(false); const [isLoading, setIsLoading] = useState(true); const retryTimer = useRef | null>(null); @@ -38,6 +41,11 @@ export const AuthProvider: React.FC<{ children: React.ReactNode }> = ({ children setIsLoading(true); const status = await AuthService.checkAuthStatus(); setUser(status.user || null); + // Keep the last known providers when the backend cannot answer, so the + // sign-in form does not vanish mid-outage. + if (status.providers) { + setProviders(status.providers); + } setIsUnavailable(!!status.unavailable); if (status.unavailable) { retryTimer.current = setTimeout(checkAuth, UNAVAILABLE_RETRY_MS); @@ -73,6 +81,7 @@ export const AuthProvider: React.FC<{ children: React.ReactNode }> = ({ children const value: AuthContextType = { user, isAuthenticated: !!user, + providers, isUnavailable, isLoading, login: { diff --git a/app/src/pages/Index.tsx b/app/src/pages/Index.tsx index c9b33807..5c885749 100644 --- a/app/src/pages/Index.tsx +++ b/app/src/pages/Index.tsx @@ -26,7 +26,7 @@ import { } from "@/components/ui/dropdown-menu"; const Index = () => { - const { isAuthenticated, isUnavailable, isLoading: authLoading, logout, user } = useAuth(); + const { isAuthenticated, isUnavailable, isLoading: authLoading, logout, user, refreshAuth } = useAuth(); const { selectedGraph, graphs, selectGraph, uploadSchema } = useDatabase(); const { selectedQueryId, clearQueryHighlight } = useQueryHighlight(); const { toast } = useToast(); @@ -148,6 +148,68 @@ const Index = () => { clearQueryHighlight(); }, [clearQueryHighlight]); + // Report the outcome of an emailed signup confirmation link. + // + // Opening that link is what creates the account and starts the session, so a + // success also has to re-check auth: this page was loaded by the redirect + // that set the cookie, and nothing else would notice it. + useEffect(() => { + const params = new URLSearchParams(window.location.search); + const verified = params.get('verified'); + if (!verified) return; + + const outcomes: Record = { + success: { + title: "Email confirmed", + description: "Your account is ready and you are signed in.", + ok: true, + }, + invalid: { + title: "That link is not valid", + description: "It may have already been used. Sign up again to get a fresh one.", + ok: false, + }, + expired: { + title: "That link has expired", + description: "Sign up again to get a fresh confirmation link.", + ok: false, + }, + exists: { + title: "Account already confirmed", + description: "Sign in with your email and password.", + ok: false, + }, + unavailable: { + title: "We could not reach the database", + description: "Please open the link again in a moment.", + ok: false, + }, + failed: { + title: "We could not confirm your email", + description: "Please try signing up again.", + ok: false, + }, + }; + + const outcome = outcomes[verified] ?? outcomes.failed; + toast({ + title: outcome.title, + description: outcome.description, + variant: outcome.ok ? undefined : "destructive", + }); + + if (outcome.ok) { + void refreshAuth(); + } else { + setShowLoginModal(true); + } + + // Drop the parameter so a refresh does not replay the message. + params.delete('verified'); + const query = params.toString(); + window.history.replaceState({}, '', `${window.location.pathname}${query ? `?${query}` : ''}`); + }, [toast, refreshAuth]); + // Show login modal when not authenticated after loading completes useEffect(() => { // Only auto-open the login modal once per user/session to avoid locking diff --git a/app/src/services/auth.ts b/app/src/services/auth.ts index 2f9e490f..244f960d 100644 --- a/app/src/services/auth.ts +++ b/app/src/services/auth.ts @@ -1,6 +1,6 @@ import { API_CONFIG, buildApiUrl } from '@/config/api'; import { csrfHeaders } from '@/lib/csrf'; -import type { AuthStatus, User } from '@/types/api'; +import type { AuthStatus, SignupResult, User } from '@/types/api'; /** * Authentication Service @@ -94,6 +94,77 @@ export class AuthService { } } + /** + * Sign up with an email address and password. + * + * A successful call creates nothing: the backend holds the details and mails + * a confirmation link, and the account comes into existence when that link is + * opened. So there is no session to refresh here. + */ + static async signupWithEmail(details: { + firstName: string; + lastName: string; + email: string; + password: string; + }): Promise { + const response = await fetch(buildApiUrl(API_CONFIG.ENDPOINTS.SIGNUP_EMAIL), { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json', ...csrfHeaders() }, + body: JSON.stringify(details), + }); + + const data = await response.json().catch(() => ({})); + if (!response.ok) { + return { + success: false, + error: data.error || 'Could not create your account. Please try again.', + retryAfterSeconds: data.retryAfterSeconds, + }; + } + return data as SignupResult; + } + + /** + * Log in with an email address and password. + */ + static async loginWithEmail(email: string, password: string): Promise<{ success: boolean; error?: string }> { + const response = await fetch(buildApiUrl(API_CONFIG.ENDPOINTS.LOGIN_EMAIL), { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json', ...csrfHeaders() }, + body: JSON.stringify({ email, password }), + }); + + const data = await response.json().catch(() => ({})); + if (!response.ok) { + return { success: false, error: data.error || 'Could not sign you in. Please try again.' }; + } + return { success: true }; + } + + /** + * Ask for another copy of the signup confirmation link. + * + * The backend answers identically whether or not the address is waiting to be + * confirmed, so the caller cannot use this to probe for accounts -- and + * neither can the UI report anything more specific than "sent". + */ + static async resendVerification(email: string): Promise<{ success: boolean; message?: string; error?: string }> { + const response = await fetch(buildApiUrl(API_CONFIG.ENDPOINTS.RESEND_VERIFICATION), { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json', ...csrfHeaders() }, + body: JSON.stringify({ email }), + }); + + const data = await response.json().catch(() => ({})); + if (!response.ok) { + return { success: false, error: data.error || 'Could not send the email. Please try again.' }; + } + return { success: true, message: data.message }; + } + /** * Logout current user */ diff --git a/app/src/types/api.ts b/app/src/types/api.ts index 3798ae46..4716e33d 100644 --- a/app/src/types/api.ts +++ b/app/src/types/api.ts @@ -11,9 +11,18 @@ export interface User { } // Authentication types + +/** Which sign-in methods the backend has configured. */ +export interface AuthProviders { + email_auth_enabled: boolean; + google_auth_enabled: boolean; + github_auth_enabled: boolean; +} + export interface AuthStatus { authenticated: boolean; user?: User; + providers?: AuthProviders; /** * The backend could not check, rather than checking and saying no. Callers * should offer a retry instead of sending the user back to the login screen. @@ -21,6 +30,16 @@ export interface AuthStatus { unavailable?: boolean; } +/** Reply to a signup: no account exists yet, only a mailed link. */ +export interface SignupResult { + success: boolean; + pending?: boolean; + email?: string; + message?: string; + error?: string; + retryAfterSeconds?: number; +} + // Graph/Database types export interface Graph { id: string; diff --git a/e2e/logic/api/apiResponses.ts b/e2e/logic/api/apiResponses.ts index 611230c0..2b0fecf9 100644 --- a/e2e/logic/api/apiResponses.ts +++ b/e2e/logic/api/apiResponses.ts @@ -13,6 +13,11 @@ export interface User { export interface AuthStatusResponse { authenticated: boolean; user?: User; + providers?: { + email_auth_enabled: boolean; + google_auth_enabled: boolean; + github_auth_enabled: boolean; + }; } export interface LoginResponse { @@ -20,8 +25,12 @@ export interface LoginResponse { error?: string; } +/** Signup creates nothing: it mails a link and reports that it is waiting. */ export interface SignupResponse { success: boolean; + pending?: boolean; + email?: string; + message?: string; error?: string; } diff --git a/e2e/logic/api/mailbox.ts b/e2e/logic/api/mailbox.ts new file mode 100644 index 00000000..2cdb7430 --- /dev/null +++ b/e2e/logic/api/mailbox.ts @@ -0,0 +1,111 @@ +import fs from 'fs'; +import path from 'path'; +import { APIRequestContext } from '@playwright/test'; + +/** + * Reading the signup verification link out of the mail outbox. + * + * Signup no longer logs anybody in: it mails a link, and opening that link is + * what creates the account and starts the session. The suite therefore has to + * go through the mail. The backend's file transport (`MAIL_OUTBOX_DIR`) writes + * each message to disk, so the tests exercise the real flow rather than a + * test-only shortcut that would prove nothing about it. + */ + +const OUTBOX_DIR = process.env.MAIL_OUTBOX_DIR || 'e2e/.mail'; + +// The mail is written while the signup request is still in flight, so a short +// poll covers the gap without making the setup slow when it is already there. +const WAIT_TIMEOUT_MS = 10_000; +const POLL_INTERVAL_MS = 200; + +interface Message { + file: string; + contents: string; + mtimeMs: number; +} + +function readOutbox(): Message[] { + if (!fs.existsSync(OUTBOX_DIR)) return []; + return fs + .readdirSync(OUTBOX_DIR) + .filter((name) => name.endsWith('.eml')) + .map((name) => { + const file = path.join(OUTBOX_DIR, name); + return { + file, + contents: fs.readFileSync(file, 'utf8'), + mtimeMs: fs.statSync(file).mtimeMs, + }; + }); +} + +/** + * Undo quoted-printable encoding. + * + * The verification URL is longer than the 78-character line limit, so Python's + * email builder encodes the body: the line is split with a trailing `=` and, + * crucially, the `=` in `?token=` becomes `=3D`. A regex run over the raw file + * therefore matches a token with a spurious `3D` prefix, which the backend + * quite correctly rejects. + */ +function decodeQuotedPrintable(contents: string): string { + return contents + .replace(/=\r?\n/g, '') + .replace(/=([0-9A-Fa-f]{2})/g, (_, hex) => String.fromCharCode(parseInt(hex, 16))); +} + +function extractVerifyUrl(contents: string): string | null { + const match = decodeQuotedPrintable(contents).match( + /https?:\/\/[^\s"<>]*\/verify\/email\?token=[A-Za-z0-9_-]+/ + ); + return match ? match[0] : null; +} + +/** + * Wait for the verification link most recently mailed to `email`. + */ +export async function waitForVerificationUrl(email: string): Promise { + const deadline = Date.now() + WAIT_TIMEOUT_MS; + + while (Date.now() < deadline) { + const candidates = readOutbox() + .filter((message) => message.contents.includes(email)) + .sort((a, b) => b.mtimeMs - a.mtimeMs); + + for (const candidate of candidates) { + const url = extractVerifyUrl(candidate.contents); + // Consume it, so a later signup for the same address cannot match this + // one and follow a link that has already been spent. + if (url) { + fs.rmSync(candidate.file, { force: true }); + return url; + } + } + + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); + } + + throw new Error( + `No verification email for ${email} appeared in ${OUTBOX_DIR}. ` + + 'Is the server running with MAIL_OUTBOX_DIR set?' + ); +} + +/** + * Follow the verification link, which creates the account and signs the + * request context in. + */ +export async function completeVerification( + email: string, + requestContext: APIRequestContext +): Promise { + const url = await waitForVerificationUrl(email); + const response = await requestContext.get(url); + + // The link redirects into the SPA carrying the outcome. + const outcome = new URL(response.url()).searchParams.get('verified'); + if (outcome !== 'success') { + throw new Error(`Verification for ${email} returned "${outcome}"`); + } +} diff --git a/e2e/tests/auth.setup.ts b/e2e/tests/auth.setup.ts index fcbe1778..3a4a8f8d 100644 --- a/e2e/tests/auth.setup.ts +++ b/e2e/tests/auth.setup.ts @@ -1,116 +1,63 @@ -import { test as setup } from '@playwright/test'; +import { test as setup, Page } from '@playwright/test'; import apiCalls from '../logic/api/apiCalls'; +import { completeVerification } from '../logic/api/mailbox'; import { getTestUser, getTestUser2, getTestUser3 } from '../config/urls'; const authFile = 'e2e/.auth/user.json'; const authFile2 = 'e2e/.auth/user2.json'; const authFile3 = 'e2e/.auth/user3.json'; -setup('authenticate users', async ({ page }) => { - const api = new apiCalls(); - - // Authenticate user 1 - const { email, password } = getTestUser(); +/** + * Sign the context in as one test user, creating the account if it is missing. + * + * Signup does not log anybody in any more: it mails a confirmation link, and + * opening that link is what creates the account and starts the session. So the + * create path goes through the mail outbox rather than trusting the signup + * response, which is also what keeps this setup honest about the real flow. + */ +async function authenticateUser( + api: apiCalls, + page: Page, + user: { email: string; password: string }, + firstName: string, + lastName: string, + storagePath: string, + label: string +): Promise { + const { email, password } = user; try { - // Try to login first - let response = await api.loginWithEmail( - email, - password, - page.request - ); + const response = await api.loginWithEmail(email, password, page.request); - // If login fails, try to create the user - if (!response.success) { + if (!response.success) { const signupResponse = await api.signupWithEmail( - 'Test', - 'User', + firstName, + lastName, email, password, page.request ); if (!signupResponse.success) { - throw new Error(`Failed to create test user 1: ${signupResponse.error || 'Unknown error'}`); + throw new Error( + `Failed to create ${label}: ${signupResponse.error || 'Unknown error'}` + ); } - } - } catch (error) { - const errorMessage = (error as Error).message; - throw new Error( - `Authentication failed for user 1. \n Error: ${errorMessage}` - ); - } - - // Save authentication state for user 1 - await page.context().storageState({ path: authFile }); - // Authenticate user 2 - const user2 = getTestUser2(); - - try { - // Try to login first - let response = await api.loginWithEmail( - user2.email, - user2.password, - page.request - ); - - // If login fails, try to create the user - if (!response.success) { - const signupResponse = await api.signupWithEmail( - 'Test2', - 'User2', - user2.email, - user2.password, - page.request - ); - - if (!signupResponse.success) { - throw new Error(`Failed to create test user 2: ${signupResponse.error || 'Unknown error'}`); - } - } + await completeVerification(email, page.request); + } } catch (error) { const errorMessage = (error as Error).message; - throw new Error( - `Authentication failed for user 2. \n Error: ${errorMessage}` - ); + throw new Error(`Authentication failed for ${label}. \n Error: ${errorMessage}`); } - // Save authentication state for user 2 - await page.context().storageState({ path: authFile2 }); + await page.context().storageState({ path: storagePath }); +} - // Authenticate user 3 - const user3 = getTestUser3(); - - try { - // Try to login first - let response = await api.loginWithEmail( - user3.email, - user3.password, - page.request - ); - - // If login fails, try to create the user - if (!response.success) { - const signupResponse = await api.signupWithEmail( - 'Test3', - 'User3', - user3.email, - user3.password, - page.request - ); - - if (!signupResponse.success) { - throw new Error(`Failed to create test user 3: ${signupResponse.error || 'Unknown error'}`); - } - } - } catch (error) { - const errorMessage = (error as Error).message; - throw new Error( - `Authentication failed for user 3. \n Error: ${errorMessage}` - ); - } +setup('authenticate users', async ({ page }) => { + const api = new apiCalls(); - // Save authentication state for user 3 - await page.context().storageState({ path: authFile3 }); + await authenticateUser(api, page, getTestUser(), 'Test', 'User', authFile, 'test user 1'); + await authenticateUser(api, page, getTestUser2(), 'Test2', 'User2', authFile2, 'test user 2'); + await authenticateUser(api, page, getTestUser3(), 'Test3', 'User3', authFile3, 'test user 3'); }); diff --git a/tests/test_auth_status.py b/tests/test_auth_status.py index 2cb72d76..11f4329d 100644 --- a/tests/test_auth_status.py +++ b/tests/test_auth_status.py @@ -82,7 +82,15 @@ async def test_anonymous_visitor_is_not_an_error(self): response = await auth_status(FakeRequest()) assert response.status_code == 200 - assert _body(response) == {"authenticated": False} + body = _body(response) + assert body["authenticated"] is False + # The sign-in screen is rendered from this, so an anonymous visitor is + # exactly who needs to know which methods are on offer. + assert set(body["providers"]) == { + "email_auth_enabled", + "google_auth_enabled", + "github_auth_enabled", + } @pytest.mark.asyncio async def test_unreachable_auth_store_yields_503(self): diff --git a/tests/test_email_signup.py b/tests/test_email_signup.py index d461aad9..a7a6eedb 100644 --- a/tests/test_email_signup.py +++ b/tests/test_email_signup.py @@ -1,16 +1,33 @@ -"""Tests for the email signup endpoint, focused on the authentication-bypass fix. +"""Tests for the email signup, verification and resend endpoints. + +Two properties are pinned here. Regression coverage for CVE-2026-10130: signing up with an email that already belongs to an account (under any provider) must NOT issue a session token, which would otherwise allow taking over that account without knowing its password. + +And the property that replaces it: signup creates nothing at all. It parks the +details and mails a link, and only opening that link creates the account and +signs the browser in. So an address the registrant does not control never +becomes an account. """ from unittest.mock import AsyncMock, MagicMock, patch +from urllib.parse import parse_qs, urlparse import pytest +from fastapi import HTTPException +from api.auth.email_verification import RESULT_EXPIRED, RESULT_OK, PendingSignup, TokenIssue from api.core.errors import AuthBackendUnavailableError -from api.routes.auth import EmailSignupRequest, _email_account_exists, email_signup +from api.routes.auth import ( + EmailResendRequest, + EmailSignupRequest, + _email_account_exists, + email_signup, + resend_verification_email, + verify_email, +) pytestmark = [pytest.mark.unit, pytest.mark.auth] @@ -21,6 +38,7 @@ def _mock_request(): # The transport helpers read these; default to a plain http request. request.headers.get.return_value = None request.url.scheme = "http" + request.base_url = "http://testserver/" request.session = {} return request @@ -34,33 +52,49 @@ def _signup_data(email="victim@example.com"): ) +def _pending(email="new@example.com"): + return PendingSignup( + email=email, + first_name="Ada", + last_name="Lovelace", + password_hash="00" * 32, + ) + + def _set_cookie_header(response): return response.headers.get("set-cookie", "") or "" +def _verified_param(response): + """Read the ``verified`` outcome off a verification redirect.""" + query = parse_qs(urlparse(response.headers["location"]).query) + return (query.get("verified") or [None])[0] + + class TestEmailSignupExistingAccount: """An existing account must never be handed a session token via signup.""" @pytest.mark.asyncio - @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) + @patch("api.routes.auth.discard_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) async def test_existing_account_is_rejected_without_token( - self, _enabled, mock_exists, mock_set_hash, mock_ensure + self, _enabled, mock_exists, mock_start, mock_discard ): mock_exists.return_value = True response = await email_signup(_mock_request(), _signup_data()) assert response.status_code == 409 - body = response.body.decode() - assert "already exists" in body + assert "already exists" in response.body.decode() # No session token must be issued for an existing account. assert "api_token=" not in _set_cookie_header(response) - # The account/token graph mutation and password write must not run. - mock_ensure.assert_not_called() - mock_set_hash.assert_not_called() + # Nothing may be parked either: a link mailed now would be redeemable + # against an account that already exists. + mock_start.assert_not_called() + # Any link still outstanding for the address is revoked. + mock_discard.assert_awaited_once() @pytest.mark.asyncio @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @@ -75,16 +109,14 @@ async def test_existence_check_failure_fails_closed(self, _enabled, mock_exists) assert "api_token=" not in _set_cookie_header(response) @pytest.mark.asyncio - @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_an_outage_during_creation_is_retryable_not_a_bug( - self, _enabled, mock_exists, mock_ensure - ): + async def test_an_outage_is_retryable_not_a_bug(self, _enabled, mock_exists, mock_start): # 500 would tell the caller the registration is broken, when in fact # nothing was decided and retrying is the right move. mock_exists.return_value = False - mock_ensure.side_effect = AuthBackendUnavailableError("down") + mock_start.side_effect = AuthBackendUnavailableError("down") response = await email_signup(_mock_request(), _signup_data("new@example.com")) @@ -92,57 +124,317 @@ async def test_an_outage_during_creation_is_retryable_not_a_bug( assert "api_token=" not in _set_cookie_header(response) -class TestEmailSignupNewAccount: - """A genuinely new account should be created and logged in.""" +class TestEmailSignupPending: + """Signup mails a link and creates nothing.""" @pytest.mark.asyncio @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) + @patch("api.routes.auth.send_verification_link", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_new_account_is_created_and_logged_in( - self, _enabled, mock_exists, mock_set_hash, mock_ensure + async def test_signup_mails_a_link_and_creates_nothing( + self, _enabled, mock_exists, mock_start, mock_send, mock_set_hash, mock_ensure ): mock_exists.return_value = False - mock_ensure.return_value = (True, {"new_identity": True}) + mock_start.return_value = TokenIssue(token="raw-token", first_name="Mallory") + mock_send.return_value = True request = _mock_request() response = await email_signup(request, _signup_data("new@example.com")) - assert response.status_code == 201 - # The signed session is the credential; the token never reaches the browser. + # 202, not 201: nothing has been created. + assert response.status_code == 202 + body = response.body.decode() + assert '"pending":true' in body.replace(" ", "") assert "api_token=" not in _set_cookie_header(response) - assert request.session, "signup must establish the browser session" + # The whole point: no account and no session until the link is opened. + assert not request.session + mock_ensure.assert_not_called() + mock_set_hash.assert_not_called() + + # The mailed link must carry the raw token, which exists nowhere else. + _, kwargs = mock_send.await_args + args = mock_send.await_args.args + verify_url = kwargs.get("verify_url") or args[2] + assert parse_qs(urlparse(verify_url).query)["token"] == ["raw-token"] + + @pytest.mark.asyncio + @patch("api.routes.auth.discard_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth.send_verification_link", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_undeliverable_mail_is_reported_and_rolled_back( + self, _enabled, mock_exists, mock_start, mock_send, mock_discard + ): + # Answering 202 here would leave the user waiting for a mail that was + # never sent, and the dead record would burn the rate limit on retry. + mock_exists.return_value = False + mock_start.return_value = TokenIssue(token="raw-token", first_name="Mallory") + mock_send.return_value = False + + response = await email_signup(_mock_request(), _signup_data("new@example.com")) + + assert response.status_code == 503 + mock_discard.assert_awaited_once() + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_link", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_throttled_signup_is_refused_with_retry_after( + self, _enabled, mock_exists, mock_start, mock_send + ): + mock_exists.return_value = False + mock_start.return_value = TokenIssue(throttled=True) + + response = await email_signup(_mock_request(), _signup_data("new@example.com")) + + assert response.status_code == 429 + assert int(response.headers["retry-after"]) > 0 + mock_send.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_link", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_exhausted_signup_is_refused_without_inviting_a_retry( + self, _enabled, mock_exists, mock_start, mock_send + ): + mock_exists.return_value = False + mock_start.return_value = TokenIssue(exhausted=True) + + response = await email_signup(_mock_request(), _signup_data("new@example.com")) + + assert response.status_code == 429 + assert "Too many" in response.body.decode() + mock_send.assert_not_called() + + +class TestVerifyEmail: + """Opening the link is what creates the account and the session.""" + + @pytest.mark.asyncio + @patch("api.routes.auth.establish_browser_session", return_value=True) + @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_valid_link_creates_the_account_and_logs_in( + self, _enabled, mock_consume, mock_exists, mock_ensure, mock_set_hash, mock_session + ): + pending = _pending() + mock_consume.return_value = (pending, RESULT_OK) + mock_exists.return_value = False + mock_ensure.return_value = (True, {"new_identity": True}) + + response = await verify_email(_mock_request(), token="raw-token") + + assert response.status_code == 303 + assert _verified_param(response) == "success" mock_ensure.assert_awaited_once() - mock_set_hash.assert_awaited_once() + # No API token is minted: the session cookie is the browser credential. + assert mock_ensure.await_args.args[-1] is None + mock_set_hash.assert_awaited_once_with(pending.email, pending.password_hash) + assert mock_session.call_args.kwargs["provisioned"] is True + + @pytest.mark.asyncio + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_unknown_or_replayed_token_creates_nothing( + self, _enabled, mock_consume, mock_ensure + ): + # A second click finds nothing, because the first one deleted the node. + mock_consume.return_value = (None, "invalid") + + response = await verify_email(_mock_request(), token="already-used") + + assert _verified_param(response) == "invalid" + mock_ensure.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_expired_token_is_distinguished_from_an_invalid_one( + self, _enabled, mock_consume, mock_ensure + ): + # The user can act on "expired" (sign up again); "invalid" reads like a + # broken link, so conflating them would send them to support instead. + mock_consume.return_value = (None, RESULT_EXPIRED) + + response = await verify_email(_mock_request(), token="stale") + + assert _verified_param(response) == "expired" + mock_ensure.assert_not_called() + @pytest.mark.asyncio + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_link_for_an_address_that_gained_an_account_is_refused( + self, _enabled, mock_consume, mock_exists, mock_ensure + ): + # Signed up by email, then logged in with Google before clicking. The + # link must not log anyone into that account. + mock_consume.return_value = (_pending(), RESULT_OK) + mock_exists.return_value = True + + response = await verify_email(_mock_request(), token="raw-token") -class TestEmailSignupCreationFailure: - """If account creation does not yield a new identity, no token is issued.""" + assert _verified_param(response) == "exists" + mock_ensure.assert_not_called() @pytest.mark.asyncio - @patch("api.routes.auth.delete_user_token", new_callable=AsyncMock) + @patch("api.routes.auth.establish_browser_session", return_value=False) + @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_unset_session_is_reported_as_a_failure( + self, _enabled, mock_consume, mock_exists, mock_ensure, _set_hash, _session + ): + # Landing on a logged-out page with no explanation is worse than being + # told it did not work; the account is real, so logging in still works. + mock_consume.return_value = (_pending(), RESULT_OK) + mock_exists.return_value = False + mock_ensure.return_value = (True, {"new_identity": True}) + + response = await verify_email(_mock_request(), token="raw-token") + + assert _verified_param(response) == "failed" + + @pytest.mark.asyncio + @patch("api.routes.auth.establish_browser_session", return_value=True) @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_non_new_identity_is_rejected_and_no_token_minted( - self, _enabled, mock_exists, mock_set_hash, mock_ensure, mock_delete + async def test_password_write_failure_does_not_log_anyone_in( + self, _enabled, mock_consume, mock_exists, mock_ensure, mock_set_hash, mock_session ): - # Passes the pre-check, but creation reports the identity already existed - # (e.g. a concurrent signup race). No token must leak. + mock_consume.return_value = (_pending(), RESULT_OK) mock_exists.return_value = False - mock_ensure.return_value = (False, {"new_identity": False}) + mock_ensure.return_value = (True, {"new_identity": True}) + mock_set_hash.side_effect = HTTPException(status_code=500) - response = await email_signup(_mock_request(), _signup_data("race@example.com")) + response = await verify_email(_mock_request(), token="raw-token") - assert response.status_code == 500 - assert "api_token=" not in _set_cookie_header(response) - mock_set_hash.assert_not_called() - # Signup asks for the records without a token, so there is nothing to - # revoke on the failure path. - assert mock_ensure.await_args.args[-1] is None - mock_delete.assert_not_awaited() + assert _verified_param(response) == "failed" + mock_session.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_backend_outage_asks_the_user_to_try_the_link_again( + self, _enabled, mock_consume + ): + mock_consume.side_effect = AuthBackendUnavailableError("down") + + response = await verify_email(_mock_request(), token="raw-token") + + assert _verified_param(response) == "unavailable" + + @pytest.mark.asyncio + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=False) + async def test_disabled_email_auth_redeems_nothing(self, _enabled, mock_consume): + response = await verify_email(_mock_request(), token="raw-token") + + assert _verified_param(response) == "failed" + mock_consume.assert_not_called() + + +class TestResendVerification: + """The resend endpoint must not become an account-existence oracle.""" + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_link", new_callable=AsyncMock) + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_pending_address_gets_a_fresh_link(self, _enabled, mock_refresh, mock_send): + mock_refresh.return_value = TokenIssue(token="fresh-token", first_name="Ada") + mock_send.return_value = True + + response = await resend_verification_email( + _mock_request(), EmailResendRequest(email="pending@example.com") + ) + + assert response.status_code == 202 + mock_send.assert_awaited_once() + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_link", new_callable=AsyncMock) + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_unknown_and_throttled_answer_exactly_like_a_success( + self, _enabled, mock_refresh, mock_send + ): + # Same status and same body for all three, or the endpoint would tell an + # anonymous caller which addresses are mid-signup. + mock_refresh.return_value = TokenIssue(token="fresh-token", first_name="Ada") + mock_send.return_value = True + issued = await resend_verification_email( + _mock_request(), EmailResendRequest(email="pending@example.com") + ) + + mock_refresh.return_value = TokenIssue(missing=True) + unknown = await resend_verification_email( + _mock_request(), EmailResendRequest(email="nobody@example.com") + ) + + mock_refresh.return_value = TokenIssue(throttled=True) + throttled = await resend_verification_email( + _mock_request(), EmailResendRequest(email="pending@example.com") + ) + + assert issued.status_code == unknown.status_code == throttled.status_code == 202 + assert issued.body == unknown.body == throttled.body + + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_link", new_callable=AsyncMock) + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_unknown_address_is_never_mailed(self, _enabled, mock_refresh, mock_send): + mock_refresh.return_value = TokenIssue(missing=True) + + await resend_verification_email( + _mock_request(), EmailResendRequest(email="nobody@example.com") + ) + + mock_send.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_malformed_address_is_rejected(self, _enabled, mock_refresh): + response = await resend_verification_email( + _mock_request(), EmailResendRequest(email="not-an-email") + ) + + assert response.status_code == 400 + mock_refresh.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_backend_outage_is_retryable(self, _enabled, mock_refresh): + mock_refresh.side_effect = AuthBackendUnavailableError("down") + + response = await resend_verification_email( + _mock_request(), EmailResendRequest(email="pending@example.com") + ) + + assert response.status_code == 503 class TestEmailAccountExistsResultHandling: @@ -184,24 +476,3 @@ async def test_query_error_propagates_to_fail_closed(self): with patch("api.routes.auth.db.select_graph", return_value=graph): with pytest.raises(RuntimeError): await _email_account_exists("err@example.com") - - -class TestEmailSignupSessionFailure: - """A created account with no browser credential is not a successful signup.""" - - @pytest.mark.asyncio - @patch("api.routes.auth.establish_browser_session", return_value=False) - @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) - @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) - @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_unset_session_is_reported_as_a_failure( - self, _enabled, mock_exists, _set_hash, mock_ensure, _establish - ): - # Reporting 201 here would leave the user staring at a logged-out page. - mock_exists.return_value = False - mock_ensure.return_value = (True, {"new_identity": True}) - - response = await email_signup(_mock_request(), _signup_data("new@example.com")) - - assert response.status_code == 500 diff --git a/tests/test_email_verification.py b/tests/test_email_verification.py new file mode 100644 index 00000000..e157c25c --- /dev/null +++ b/tests/test_email_verification.py @@ -0,0 +1,252 @@ +"""Tests for the pending-signup store that backs email verification. + +The store is what makes "no account until the link is opened" true, so the +properties pinned here are the ones the guarantee rests on: the raw token is +never stored, a token is redeemable exactly once, and the send limits cannot be +reset by asking again. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from api.auth import email_verification as ev + +pytestmark = [pytest.mark.unit, pytest.mark.auth] + + +def _result(rows): + return MagicMock(result_set=rows) + + +class _FakeGraph: + """Records the queries a helper runs and replays canned result sets.""" + + def __init__(self, results): + self._results = list(results) + self.calls = [] + self.query = AsyncMock(side_effect=self._query) + + async def _query(self, cypher, params=None): + self.calls.append((cypher, params or {})) + return self._results.pop(0) if self._results else _result([]) + + +def _patch_graph(graph): + return patch("api.auth.email_verification._graph", return_value=graph) + + +class TestStartPendingSignup: + """Parking a signup, and the limits on how much mail it can generate.""" + + @pytest.mark.asyncio + async def test_only_the_token_hash_is_stored(self): + graph = _FakeGraph([_result([])]) # no existing pending record + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert issue.issued + _, params = graph.calls[-1] + assert params["token_hash"] == ev.hash_token(issue.token) + # A graph snapshot must not yield a working link. + assert issue.token not in params.values() + + @pytest.mark.asyncio + async def test_resubmitting_cannot_reset_the_send_limit(self): + # Otherwise the rate limit is decorative: re-post the form and the + # counter starts over. + graph = _FakeGraph([_result([[None, 3, "Ada"]])]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert issue.issued + _, params = graph.calls[-1] + assert params["send_count"] == 4 + + @pytest.mark.asyncio + async def test_a_recent_send_is_throttled_rather_than_repeated(self, monkeypatch): + monkeypatch.setenv("EMAIL_VERIFICATION_RESEND_SECONDS", "60") + graph = _FakeGraph([_result([[ev._now_ms(), 1, "Ada"]])]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert not issue.issued + assert issue.throttled + # Only the read ran; nothing was written. + assert len(graph.calls) == 1 + + @pytest.mark.asyncio + async def test_send_budget_is_finite(self, monkeypatch): + # Bounds how much mail one submitted address can aim at a third party. + monkeypatch.setenv("EMAIL_VERIFICATION_MAX_SENDS", "2") + graph = _FakeGraph([_result([[None, 2, "Ada"]])]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert issue.exhausted + assert not issue.issued + + +class TestRefreshPendingSignup: + """Resending refreshes an existing signup and never invents one.""" + + @pytest.mark.asyncio + async def test_unknown_address_is_not_created(self): + # A MERGE here would turn the resend endpoint into a way to mail an + # address nobody ever submitted. + graph = _FakeGraph([_result([])]) + with _patch_graph(graph): + issue = await ev.refresh_pending_signup("nobody@example.com") + + assert issue.missing + assert not issue.issued + assert len(graph.calls) == 1 + assert "MERGE" not in graph.calls[0][0] + + @pytest.mark.asyncio + async def test_refresh_replaces_the_previous_link(self): + graph = _FakeGraph([_result([[None, 1, "Ada"]]), _result([["Ada"]])]) + with _patch_graph(graph): + issue = await ev.refresh_pending_signup("pending@example.com") + + assert issue.issued + assert issue.first_name == "Ada" + write_cypher, params = graph.calls[-1] + assert "MATCH" in write_cypher and "MERGE" not in write_cypher + assert params["token_hash"] == ev.hash_token(issue.token) + assert params["send_count"] == 2 + + @pytest.mark.asyncio + async def test_losing_a_race_with_verification_is_not_an_error(self): + # The record was consumed between the read and the write. + graph = _FakeGraph([_result([[None, 1, "Ada"]]), _result([])]) + with _patch_graph(graph): + issue = await ev.refresh_pending_signup("pending@example.com") + + assert issue.missing + assert not issue.issued + + +class TestConsumePendingSignup: + """Redeeming a link.""" + + @staticmethod + def _row(expires_at): + return [["new@example.com", "Ada", "Lovelace", "hash", expires_at]] + + @pytest.mark.asyncio + async def test_empty_token_never_reaches_the_database(self): + graph = _FakeGraph([]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup("") + + assert pending is None + assert result == ev.RESULT_INVALID + assert not graph.calls + + @pytest.mark.asyncio + async def test_live_token_returns_the_details_and_deletes_the_record(self): + future = ev._now_ms() + 60_000 + graph = _FakeGraph([_result(self._row(future))]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup("raw-token") + + assert result == ev.RESULT_OK + assert pending.email == "new@example.com" + assert pending.full_name == "Ada Lovelace" + cypher, params = graph.calls[0] + # Single-use is structural: the read and the delete are one query, so a + # replay cannot find the node no matter how the caller behaves. + assert "DELETE" in cypher + assert params["token_hash"] == ev.hash_token("raw-token") + + @pytest.mark.asyncio + async def test_replayed_token_finds_nothing(self): + graph = _FakeGraph([_result([])]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup("already-used") + + assert pending is None + assert result == ev.RESULT_INVALID + + @pytest.mark.asyncio + async def test_expired_token_is_reported_and_consumed(self): + past = ev._now_ms() - 1 + graph = _FakeGraph([_result(self._row(past))]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup("stale") + + assert pending is None + assert result == ev.RESULT_EXPIRED + + @pytest.mark.asyncio + async def test_record_without_an_expiry_is_not_treated_as_eternal(self): + # A missing expiry must fail closed, not read as "never expires". + graph = _FakeGraph([_result(self._row(None))]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup("malformed") + + assert pending is None + assert result == ev.RESULT_EXPIRED + + @pytest.mark.asyncio + async def test_record_missing_a_password_is_rejected(self): + future = ev._now_ms() + 60_000 + graph = _FakeGraph([_result([["new@example.com", "Ada", "Lovelace", None, future]])]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup("malformed") + + assert pending is None + assert result == ev.RESULT_INVALID + + +class TestDiscardPendingSignup: + """Discarding is best-effort: it must never be the thing that fails a request.""" + + @pytest.mark.asyncio + async def test_a_failing_delete_is_swallowed(self): + graph = MagicMock() + graph.query = AsyncMock(side_effect=RuntimeError("db down")) + with _patch_graph(graph): + await ev.discard_pending_signup("new@example.com") + + +class TestSendVerificationLink: + """The mail itself.""" + + @pytest.mark.asyncio + async def test_link_is_included_in_both_bodies(self): + with patch("api.auth.email_verification.send_mail", + new_callable=AsyncMock) as mock_send: + mock_send.return_value = True + sent = await ev.send_verification_link( + "new@example.com", "Ada", "http://testserver/verify/email?token=raw" + ) + + assert sent is True + kwargs = mock_send.await_args.kwargs + assert kwargs["to"] == "new@example.com" + assert "token=raw" in kwargs["text_body"] + assert "token=raw" in kwargs["html_body"] + + @pytest.mark.asyncio + async def test_a_name_from_the_form_cannot_inject_markup(self): + # The first name is attacker-controlled and lands in an HTML body. + with patch("api.auth.email_verification.send_mail", + new_callable=AsyncMock) as mock_send: + mock_send.return_value = True + await ev.send_verification_link( + "new@example.com", "", "http://testserver/v" + ) + + html_body = mock_send.await_args.kwargs["html_body"] + assert "", "http://testserver/v" + await ev.send_verification_code( + "new@example.com", "", "123456" ) html_body = mock_send.await_args.kwargs["html_body"] From 9f29749d01e023476cac12d471f611d0ce1fce1f Mon Sep 17 00:00:00 2001 From: Anchel135 Date: Sun, 30 Aug 2026 15:38:43 +0300 Subject: [PATCH 05/12] docs: describe the confirmation code where comments still said link The mail module, the signup route, the .env example and the e2e response type were all still describing the flow the previous commit replaced. These comments are the only place the security property is written down next to the code that implements it, so a stale one is worse than none. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .env.example | 9 +++++---- api/mail.py | 8 ++++---- api/routes/auth.py | 6 +++--- e2e/logic/api/apiResponses.ts | 2 +- tests/test_mail.py | 20 ++++++++++---------- 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/.env.example b/.env.example index 93de05d1..0bea6cf3 100644 --- a/.env.example +++ b/.env.example @@ -163,10 +163,11 @@ FALKORDB_URL=redis://localhost:6379/0 # REQUIRED - change to your FalkorDB URL # ----------------------------- # Email Configuration # ----------------------------- -# Signup with email/password sends a confirmation link, and the account is only -# created when that link is opened. Without MAIL_SERVER the message is written -# to the application log instead of being sent, which is enough for local -# development -- copy the link out of the log. +# Signup with email/password sends a six-digit confirmation code, and the +# account is only created when that code is typed back into the signup form. +# Without MAIL_SERVER the message is written to the application log instead of +# being sent, which is enough for local development -- copy the code out of the +# log. # # Any provider works: Mailgun, SendGrid, Resend, SES and Postmark all expose an # SMTP endpoint. diff --git a/api/mail.py b/api/mail.py index dea6bb04..afcca6af 100644 --- a/api/mail.py +++ b/api/mail.py @@ -1,6 +1,6 @@ """Outbound mail. -QueryWeaver sends one kind of message today -- the signup verification link -- +QueryWeaver sends one kind of message today -- the signup confirmation code -- so this module stays small: pick a transport from the environment, hand it a built message, and report whether it left the process. @@ -9,7 +9,7 @@ * ``console`` (the default) writes the message to the log instead of sending it, so local development completes the signup flow without a mail server. * ``file`` writes each message to ``MAIL_OUTBOX_DIR`` as an ``.eml``. The - Playwright suite reads the verification link back out of it, which is what + Playwright suite reads the confirmation code back out of it, which is what keeps the end-to-end signup test exercising the real flow instead of a test-only shortcut through the backend. It takes precedence over a configured relay: nobody sets this variable by accident, and a test run that quietly @@ -161,7 +161,7 @@ def _sanitize_for_log(value: str) -> str: # ``.replace('\n', ...)`` must be the outermost call for CodeQL's # log-injection sanitizer to recognise it. The body is deliberately # rendered on one line rather than dropped: the console transport exists so - # a developer can copy the verification link out of the log. + # a developer can copy the confirmation code out of the log. return str(value).replace("\r", " ").replace("\n", " ") @@ -225,6 +225,6 @@ async def send_mail( return True except (smtplib.SMTPException, OSError, ssl.SSLError) as e: # Deliberately does not log the message body: it carries the - # verification link, which is a credential. + # confirmation code, which is a credential. logging.error("Could not send mail via SMTP: %s", e) return False diff --git a/api/routes/auth.py b/api/routes/auth.py index 0d724781..6bd32b10 100644 --- a/api/routes/auth.py +++ b/api/routes/auth.py @@ -390,7 +390,7 @@ async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSO # (CVE-2026-10130, authentication bypass via signup token issuance). if await _email_account_exists(email): logging.info("Signup attempt for existing account: %s", _sanitize_for_log(email)) - # An account exists, so any link still outstanding for this address + # An account exists, so any code still outstanding for this address # must not stay redeemable against it. await discard_pending_signup(email) return JSONResponse( @@ -399,8 +399,8 @@ async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSO ) # Nothing is created yet. The details are parked on a PendingSignup node - # and only become a User when the emailed link is opened, so an address - # the registrant does not control never turns into an account at all. + # and only become a User when the mailed code is typed back in, so an + # address the registrant does not control never turns into an account. password_hash = _hash_password(password) issue = await start_pending_signup(email, first_name, last_name, password_hash) diff --git a/e2e/logic/api/apiResponses.ts b/e2e/logic/api/apiResponses.ts index 2b0fecf9..364bac16 100644 --- a/e2e/logic/api/apiResponses.ts +++ b/e2e/logic/api/apiResponses.ts @@ -25,7 +25,7 @@ export interface LoginResponse { error?: string; } -/** Signup creates nothing: it mails a link and reports that it is waiting. */ +/** Signup creates nothing: it mails a code and reports that it is waiting. */ export interface SignupResponse { success: boolean; pending?: boolean; diff --git a/tests/test_mail.py b/tests/test_mail.py index 582dd3d5..3e31b441 100644 --- a/tests/test_mail.py +++ b/tests/test_mail.py @@ -2,7 +2,7 @@ The transport is deliberately thin, so the only things worth pinning are the ones that would be silently wrong: that a failed send is reported rather than -raised, that a failure does not leak the verification link into the logs, and +raised, that a failure does not leak the confirmation code into the logs, and that a recipient address cannot smuggle extra headers into the message. """ @@ -58,18 +58,18 @@ async def test_an_outbox_keeps_a_configured_relay_unused(self, monkeypatch, tmp_ @pytest.mark.asyncio async def test_console_transport_logs_the_body(self, caplog): # Local development has no mail server, so the log is where the - # verification link has to be readable from. + # confirmation code has to be readable from. with caplog.at_level("INFO"): sent = await mail.send_mail( - to="new@example.com", subject="Confirm", text_body="http://link" + to="new@example.com", subject="Confirm", text_body="code 123456" ) assert sent is True - assert "http://link" in caplog.text + assert "code 123456" in caplog.text class TestFileTransport: - """The outbox the end-to-end suite reads the verification link out of.""" + """The outbox the end-to-end suite reads the confirmation code out of.""" @pytest.mark.asyncio async def test_message_is_written_as_a_readable_file(self, monkeypatch, tmp_path): @@ -77,7 +77,7 @@ async def test_message_is_written_as_a_readable_file(self, monkeypatch, tmp_path monkeypatch.setenv("MAIL_OUTBOX_DIR", str(outbox)) sent = await mail.send_mail( - to="new@example.com", subject="Confirm", text_body="http://link" + to="new@example.com", subject="Confirm", text_body="code 123456" ) assert sent is True @@ -85,7 +85,7 @@ async def test_message_is_written_as_a_readable_file(self, monkeypatch, tmp_path assert len(files) == 1 contents = files[0].read_text() assert "new@example.com" in contents - assert "http://link" in contents + assert "code 123456" in contents @pytest.mark.asyncio async def test_an_unwritable_outbox_is_reported_not_raised(self, monkeypatch, tmp_path): @@ -112,13 +112,13 @@ async def test_a_broken_server_is_reported_not_raised(self, monkeypatch, caplog) sent = await mail.send_mail( to="new@example.com", subject="Confirm", - text_body="http://link-that-must-not-leak", + text_body="code-that-must-not-leak", ) assert sent is False - # The body carries a live verification link; logging it on failure would + # The body carries a live confirmation code; logging it on failure would # put a credential in the log file. - assert "link-that-must-not-leak" not in caplog.text + assert "code-that-must-not-leak" not in caplog.text @pytest.mark.asyncio async def test_an_smtp_error_is_reported_not_raised(self, monkeypatch): From 259ef411ec170962bac8ad248db723fb29d78b93 Mon Sep 17 00:00:00 2001 From: Anchel135 Date: Sun, 30 Aug 2026 15:58:34 +0300 Subject: [PATCH 06/12] fix: create the verified account and its password in one write Verification spent the code, created the User and Identity, and only then stored the password in a second query. A failure in that second query left an account that could not be logged into -- no password to check -- and could not be signed up for again, because the address now belongs to an account and signup answers 409. The code was already spent, so there was no way forward for that address at all. The password now goes in with the identity: ensure_user_in_organizations takes an optional password_hash and sets it in the ON CREATE branch of the same MERGE. Only ON CREATE, so this can never overwrite the password of an identity that already exists. A failed write now leaves nothing behind and the address is still free. _set_mail_hash went with it. It was the only caller, and its MERGE keyed identities on {provider_user_id, email} while every other write keys them on {provider, provider_user_id} -- a second pattern for the same node that no longer has a reason to exist. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/playwright.yml | 4 +-- api/auth/user_management.py | 26 ++++++++++++--- api/routes/auth.py | 57 ++++---------------------------- tests/test_email_signup.py | 47 ++++++++++++++++++-------- 4 files changed, 62 insertions(+), 72 deletions(-) diff --git a/.github/workflows/playwright.yml b/.github/workflows/playwright.yml index f1acbadb..bca72d3c 100644 --- a/.github/workflows/playwright.yml +++ b/.github/workflows/playwright.yml @@ -170,8 +170,8 @@ jobs: FASTAPI_DEBUG: False FALKORDB_URL: redis://localhost:6379 DISABLE_MCP: true - # Signup mails a confirmation link and the account is only created when - # it is opened, so the suite has to be able to read the message back. + # Signup mails a confirmation code and the account is only created when + # it is typed back, so the suite has to be able to read the message. MAIL_OUTBOX_DIR: e2e/.mail # Azure OpenAI API keys - required for database schema analysis AZURE_API_KEY: ${{ secrets.AZURE_API_KEY }} diff --git a/api/auth/user_management.py b/api/auth/user_management.py index c63763e9..97d3633d 100644 --- a/api/auth/user_management.py +++ b/api/auth/user_management.py @@ -132,6 +132,8 @@ async def ensure_user_in_organizations( # pylint: disable=too-many-arguments, d provider: str, api_token: Optional[str], picture: str | None = None, + *, + password_hash: Optional[str] = None, ) -> tuple[bool, Optional[IdentityInfo]]: """ Check if identity exists in Organizations graph, create if not. @@ -142,6 +144,10 @@ async def ensure_user_in_organizations( # pylint: disable=too-many-arguments, d minting a programmatic token — used when repairing an existing browser login that was established while the graph was unreachable. + ``password_hash`` is stored on the identity as it is created, in the same + write. A second write would be a window in which the account exists but + cannot be logged into, and the address is already taken for signup. + Returns (is_new_identity, user_info). ``user_info`` is ``None`` when the records could not be persisted, so callers should test that, not the flag. Raises :class:`AuthBackendUnavailableError` when the graph is unreachable, @@ -162,7 +168,10 @@ async def ensure_user_in_organizations( # pylint: disable=too-many-arguments, d organizations_graph = db.select_graph(ORGANIZATIONS_GRAPH) first_name, last_name = _extract_name_parts(name) - merge_query = _build_user_merge_query(include_token=api_token is not None) + merge_query = _build_user_merge_query( + include_token=api_token is not None, + include_password=password_hash is not None, + ) query_params = _build_query_params( provider, provider_user_id, @@ -172,6 +181,7 @@ async def ensure_user_in_organizations( # pylint: disable=too-many-arguments, d first_name=first_name, last_name=last_name, api_token=api_token, + password_hash=password_hash, ) result = await organizations_graph.query(merge_query, query_params) @@ -458,11 +468,14 @@ def _extract_name_parts(name: str) -> tuple: return first_name, last_name -def _build_user_merge_query(include_token: bool = True) -> str: +def _build_user_merge_query(include_token: bool = True, include_password: bool = False) -> str: """Build the Cypher query for user/identity merge operations. ``include_token`` drops the Token MERGE so an identity can be persisted - without issuing a programmatic credential. + without issuing a programmatic credential. ``include_password`` stores a + password on the identity as it is created; it is deliberately absent from + the ON MATCH branch, so this can never overwrite the password of an + identity that already exists. """ token_clause = ( """ @@ -476,6 +489,7 @@ def _build_user_merge_query(include_token: bool = True) -> str: if include_token else "" ) + password_clause = ",\n identity.password_hash = $password_hash" if include_password else "" return """ // First, ensure user exists (merge by email) MERGE (user:User {email: $email}) @@ -491,7 +505,7 @@ def _build_user_merge_query(include_token: bool = True) -> str: identity.name = $name, identity.picture = $picture, identity.created_at = timestamp(), - identity.last_login = timestamp() + identity.last_login = timestamp()""" + password_clause + """ ON MATCH SET identity.email = $email, identity.name = $name, @@ -518,7 +532,8 @@ def _build_query_params( # pylint: disable=too-many-arguments picture: str | None = None, first_name: str, last_name: str, - api_token: Optional[str] + api_token: Optional[str], + password_hash: Optional[str] = None, ) -> dict: """Build query parameters for the database operation.""" return { @@ -530,6 +545,7 @@ def _build_query_params( # pylint: disable=too-many-arguments "first_name": first_name, "last_name": last_name, "api_token": api_token, + "password_hash": password_hash, } diff --git a/api/routes/auth.py b/api/routes/auth.py index 6bd32b10..41666e37 100644 --- a/api/routes/auth.py +++ b/api/routes/auth.py @@ -169,44 +169,6 @@ def _validate_email(email: str) -> bool: pattern = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}\Z' return re.match(pattern, email) is not None -async def _set_mail_hash(email: str, password_hash: str) -> bool: - """Set email hash for the user in the database.""" - # Sanitized up front so the error path below can log it too. - safe_email = _sanitize_for_log(email) - try: - organizations_graph = db.select_graph(ORGANIZATIONS_GRAPH) - - # Create new email identity and user - create_query = """ - MERGE (i:Identity { - provider_user_id: $email, - email: $email - }) - SET i.password_hash = $password_hash - RETURN i - """ - - result = await organizations_graph.query(create_query, { - "email": email, - "password_hash": password_hash, - }) - - if result.result_set: - return True - else: - logging.error("Failed to set email hash for user: %s", safe_email) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error" - ) - - except Exception as e: - logging.error("Error setting email hash for user %s: %s", safe_email, e) - raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="Internal server error" - ) - async def _email_account_exists(email: str) -> bool: """Return True if an account already exists for the given email (any provider). @@ -509,9 +471,13 @@ async def verify_email(request: Request, verify_data: EmailVerifyRequest) -> JSO ) # ``api_token=None``: the browser is credentialed by the session cookie, - # so minting a token here would only leave an orphan Token node. + # so minting a token here would only leave an orphan Token node. The + # password goes in with the identity: a second write could fail and + # leave an account that cannot be logged into and cannot be signed up + # for again. is_new_identity, user_info = await ensure_user_in_organizations( - pending.email, pending.email, pending.full_name, "email", None + pending.email, pending.email, pending.full_name, "email", None, + password_hash=pending.password_hash, ) if not (is_new_identity and user_info and user_info.get("new_identity")): logging.error("Could not create the verified account for %s", @@ -521,8 +487,6 @@ async def verify_email(request: Request, verify_data: EmailVerifyRequest) -> JSO status_code=status.HTTP_500_INTERNAL_SERVER_ERROR ) - await _set_mail_hash(pending.email, pending.password_hash) - if not establish_browser_session( request, email=pending.email, @@ -557,15 +521,6 @@ async def verify_email(request: Request, verify_data: EmailVerifyRequest) -> JSO "error": "Authentication service temporarily unavailable - please retry"}, status_code=status.HTTP_503_SERVICE_UNAVAILABLE ) - except HTTPException: - # How _set_mail_hash reports failure. The identity exists but carries no - # password hash, so the login it enables is the one thing that will not - # work. - logging.error("Could not store the password for a freshly verified account") - return JSONResponse( - {"success": False, "error": "Could not finish creating your account"}, - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR - ) except Exception as e: logging.error("Email verification error: %s", e) return JSONResponse( diff --git a/tests/test_email_signup.py b/tests/test_email_signup.py index 0804a681..ba7c6f10 100644 --- a/tests/test_email_signup.py +++ b/tests/test_email_signup.py @@ -17,7 +17,6 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest -from fastapi import HTTPException from api.auth.email_verification import ( RESULT_EXPIRED, @@ -26,6 +25,7 @@ CodeIssue, PendingSignup, ) +from api.auth.user_management import _build_user_merge_query from api.core.errors import AuthBackendUnavailableError from api.routes.auth import ( EmailResendRequest, @@ -135,13 +135,12 @@ class TestEmailSignupPending: @pytest.mark.asyncio @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) async def test_signup_mails_a_code_and_creates_nothing( - self, _enabled, mock_exists, mock_start, mock_send, mock_set_hash, mock_ensure + self, _enabled, mock_exists, mock_start, mock_send, mock_ensure ): mock_exists.return_value = False mock_start.return_value = CodeIssue(code="123456", first_name="Mallory") @@ -158,7 +157,6 @@ async def test_signup_mails_a_code_and_creates_nothing( # The whole point: no account and no session until the code comes back. assert not request.session mock_ensure.assert_not_called() - mock_set_hash.assert_not_called() # The mail carries the raw code, which exists nowhere else. assert mock_send.await_args.args[2] == "123456" @@ -226,13 +224,12 @@ class TestVerifyEmail: @pytest.mark.asyncio @patch("api.routes.auth.establish_browser_session", return_value=True) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) async def test_valid_code_creates_the_account_and_logs_in( - self, _enabled, mock_consume, mock_exists, mock_ensure, mock_set_hash, mock_session + self, _enabled, mock_consume, mock_exists, mock_ensure, mock_session ): pending = _pending() mock_consume.return_value = (pending, RESULT_OK) @@ -245,7 +242,9 @@ async def test_valid_code_creates_the_account_and_logs_in( mock_ensure.assert_awaited_once() # No API token is minted: the session cookie is the browser credential. assert mock_ensure.await_args.args[-1] is None - mock_set_hash.assert_awaited_once_with(pending.email, pending.password_hash) + # The password is written with the identity, not after it: a separate + # write could fail and leave an account nobody can log into. + assert mock_ensure.await_args.kwargs["password_hash"] == pending.password_hash assert mock_session.call_args.kwargs["provisioned"] is True @pytest.mark.asyncio @@ -314,13 +313,12 @@ async def test_code_for_an_address_that_gained_an_account_is_refused( @pytest.mark.asyncio @patch("api.routes.auth.establish_browser_session", return_value=False) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) async def test_unset_session_is_reported_as_a_failure( - self, _enabled, mock_consume, mock_exists, mock_ensure, _set_hash, _session + self, _enabled, mock_consume, mock_exists, mock_ensure, _session ): # Silently reporting success would leave the user logged out with no # explanation; the account is real, so logging in still works. @@ -334,18 +332,18 @@ async def test_unset_session_is_reported_as_a_failure( @pytest.mark.asyncio @patch("api.routes.auth.establish_browser_session", return_value=True) - @patch("api.routes.auth._set_mail_hash", new_callable=AsyncMock) @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_password_write_failure_does_not_log_anyone_in( - self, _enabled, mock_consume, mock_exists, mock_ensure, mock_set_hash, mock_session + async def test_a_failed_account_write_does_not_log_anyone_in( + self, _enabled, mock_consume, mock_exists, mock_ensure, mock_session ): + # The account and its password are one write, so a failure leaves + # nothing behind and the address is still free to sign up again. mock_consume.return_value = (_pending(), RESULT_OK) mock_exists.return_value = False - mock_ensure.return_value = (True, {"new_identity": True}) - mock_set_hash.side_effect = HTTPException(status_code=500) + mock_ensure.return_value = (False, None) response = await verify_email(_mock_request(), _verify_data()) @@ -456,6 +454,27 @@ async def test_backend_outage_is_retryable(self, _enabled, mock_refresh): assert response.status_code == 503 +class TestPasswordIsWrittenWithTheIdentity: + """The password clause on the identity merge. + + Verification consumes the code before it creates the account, so there is + no second chance: an account written without its password could neither be + logged into nor signed up for again. + """ + + def test_no_password_is_written_unless_one_is_given(self): + assert "password_hash" not in _build_user_merge_query() + + def test_the_password_is_set_as_the_identity_is_created(self): + query = _build_user_merge_query(include_token=False, include_password=True) + on_create, on_match = query.split("ON MATCH SET", 1) + + assert "identity.password_hash = $password_hash" in on_create + # Never on the ON MATCH branch: an identity that already exists keeps + # the password it already has. + assert "password_hash" not in on_match + + class TestEmailAccountExistsResultHandling: """Coverage for how `_email_account_exists` interprets query results. From 875782f066f6f13864096d9c73ae0fe0bcfea8a8 Mon Sep 17 00:00:00 2001 From: Anchel135 Date: Sun, 30 Aug 2026 16:32:35 +0300 Subject: [PATCH 07/12] fix: bind a pending signup to the browser that submitted it Holding the code was enough to finish a signup, so a stranger could submit someone else's address with a password of their own and the code that landed in the victim's inbox would create an account under that password. Each submission now mints a ticket that stays in the submitting browser's session and is matched, hashed, inside the same query that spends the code. Alongside it: an expired pending signup starts a fresh send budget, so exhausting the budget can no longer lock an address out for good; the console mail transport is confined to APP_ENV=development, so a deployment that forgets MAIL_SERVER fails the send instead of logging every code; a failed re-send puts back the code it displaced rather than discarding one already delivered; and every branch of POST /signup/email answers the same 202, so the endpoint no longer discloses which addresses are mid-signup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .env.example | 8 +- AGENTS.md | 2 +- README.md | 22 ++-- api/auth/browser_session.py | 42 +++++++ api/auth/email_verification.py | 158 +++++++++++++++--------- api/mail.py | 37 +++++- api/routes/auth.py | 100 +++++++++------ tests/test_email_signup.py | 203 ++++++++++++++++++++++++++----- tests/test_email_verification.py | 187 +++++++++++++++++++++++----- tests/test_mail.py | 35 +++++- 10 files changed, 625 insertions(+), 169 deletions(-) diff --git a/.env.example b/.env.example index 0bea6cf3..a016c471 100644 --- a/.env.example +++ b/.env.example @@ -165,9 +165,11 @@ FALKORDB_URL=redis://localhost:6379/0 # REQUIRED - change to your FalkorDB URL # ----------------------------- # Signup with email/password sends a six-digit confirmation code, and the # account is only created when that code is typed back into the signup form. -# Without MAIL_SERVER the message is written to the application log instead of -# being sent, which is enough for local development -- copy the code out of the -# log. +# With APP_ENV=development and no MAIL_SERVER the message is written to the +# application log instead of being sent, which is enough for local development +# -- copy the code out of the log. Anywhere else an unconfigured process +# refuses the send rather than logging the code, so signup fails loudly instead +# of leaving users waiting for mail nobody sent. # # Any provider works: Mailgun, SendGrid, Resend, SES and Postmark all expose an # SMTP endpoint. diff --git a/AGENTS.md b/AGENTS.md index 0fb82deb..29e4856e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -132,7 +132,7 @@ Optional overrides: `COMPLETION_MODEL`, `EMBEDDING_MODEL` (must match provider), Authentication is deliberately split three ways — a signed session cookie for the browser login (no FalkorDB dependency, see `api/auth/browser_session.py`), FalkorDB-backed API tokens for programmatic clients, and per-request data-source credentials. `validate_user` in `api/auth/user_management.py` owns the precedence between them. -Email/password signup is verified before the account exists: `POST /signup/email` parks the details on a `PendingSignup` node (`api/auth/email_verification.py`) and mails a six-digit code, and `POST /signup/email/verify` is what creates the `User` and establishes the session. A code rather than a link, so that confirmation has to come back through the session that submitted the form — an emailed link can be opened by a victim who never signed up, which would create an account under a password the sender chose. Wrong guesses are charged against a per-record attempt budget that destroys the pending signup once it runs out; that budget, not the six digits, is what makes a short code safe. There is therefore no `email_verified` flag anywhere — an unconfirmed address is simply absent from the account graph. Mail goes through `api/mail.py`, which picks a transport from the environment: console (default), a file outbox (`MAIL_OUTBOX_DIR`, used by the Playwright suite to read the code back) or SMTP (`MAIL_SERVER`). +Email/password signup is verified before the account exists: `POST /signup/email` parks the details on a `PendingSignup` node (`api/auth/email_verification.py`) and mails a six-digit code, and `POST /signup/email/verify` is what creates the `User` and establishes the session. A code rather than a link, so that confirmation has to come back through the session that submitted the form — an emailed link can be opened by a victim who never signed up, which would create an account under a password the sender chose. Holding the code is not on its own enough: every submission also mints a ticket that stays in the submitting browser's session (`remember_signup_ticket` in `api/auth/browser_session.py`) and is matched, hashed, inside the same query that spends the code, so a code read out of someone else's inbox cannot be redeemed. Wrong guesses are charged against a per-record attempt budget that destroys the pending signup once it runs out; that budget, not the six digits, is what makes a short code safe. The send budget is likewise per record and resets when the record expires, so exhausting it cannot lock an address out permanently. There is therefore no `email_verified` flag anywhere — an unconfirmed address is simply absent from the account graph. Every branch of `POST /signup/email` answers the same 202, so nothing about a pending signup is observable from outside. Mail goes through `api/mail.py`, which picks a transport from the environment: a file outbox (`MAIL_OUTBOX_DIR`, used by the Playwright suite to read the code back), SMTP (`MAIL_SERVER`), or — only when `APP_ENV=development` — the console; an unconfigured process anywhere else fails the send rather than logging the code. See `.env.example` for the full list. diff --git a/README.md b/README.md index 14a8c001..8f2ebc61 100644 --- a/README.md +++ b/README.md @@ -191,14 +191,20 @@ only one who ever holds both halves. The code is single-use, expires after 15 minutes and tolerates only a handful of wrong guesses before the pending signup is discarded — a short code is only safe -while the number of attempts is small. Entering it signs the browser in -directly: the password was chosen minutes earlier, and asking for it again would -prove nothing. A code can be re-sent from the same screen, subject to a -per-address rate limit. - -Without a mail server configured the message is written to the application log -instead of being sent, so local development can complete the flow by copying the -code out of the log. Set `MAIL_SERVER` (plus `MAIL_PORT`, `MAIL_USERNAME`, +while the number of attempts is small. It is also only redeemable in the browser +that submitted the form: each submission mints a ticket that stays in that +browser's session, and a code presented without its ticket is refused. Entering +it signs the browser in directly: the password was chosen minutes earlier, and +asking for it again would prove nothing. A code can be re-sent from the same +screen, subject to a per-address rate limit; the send budget is per pending +signup, so it starts over once the pending signup expires and an address can +always be signed up again later. + +In development, a message with no mail server configured is written to the +application log instead of being sent, so the flow can be completed by copying +the code out of the log. This needs `APP_ENV=development` — anywhere else an +unconfigured process refuses the send rather than logging the code and +reporting success. Set `MAIL_SERVER` (plus `MAIL_PORT`, `MAIL_USERNAME`, `MAIL_PASSWORD`, `MAIL_DEFAULT_SENDER`) to send for real; any provider with an SMTP endpoint works. `EMAIL_VERIFICATION_TTL_MINUTES`, `EMAIL_VERIFICATION_MAX_ATTEMPTS`, `EMAIL_VERIFICATION_RESEND_SECONDS` and diff --git a/api/auth/browser_session.py b/api/auth/browser_session.py index 7ae8a81b..03a4a1eb 100644 --- a/api/auth/browser_session.py +++ b/api/auth/browser_session.py @@ -30,6 +30,11 @@ # Key under which the login payload lives inside the Starlette session dict. SESSION_KEY = "browser_login" +# Key under which a signup awaiting its mailed code parks its ticket. Separate +# from the login: it is held before any account exists, and clearing one must +# not clear the other. +SIGNUP_TICKET_KEY = "signup_ticket" + # Bumped whenever the payload shape changes, so old cookies are ignored rather # than misread. SESSION_VERSION = 1 @@ -176,3 +181,40 @@ def mark_provisioned(request: Request) -> None: payload["provisioned"] = True # Reassign so Starlette re-serialises the mutated payload. store[SESSION_KEY] = payload + + +def remember_signup_ticket(request: Request, *, email: str, ticket: str) -> None: + """Hold the ticket for a signup this browser just started. + + Not a login -- there is no account yet. It is the browser's half of the + pending signup, and it lives here rather than in the graph because that is + exactly what it has to prove: that the caller redeeming the mailed code is + the same browser that submitted the password being redeemed. One at a time + is enough; a second signup in the same browser replaces the first. + + Storing it in a signed-but-readable cookie is fine. It is a capability of + the browser it was handed to, so its owner reading it learns nothing they + did not already have, and it is worthless without the code that was mailed. + """ + store = _session_store(request) + if store is None: + logging.error("Cannot hold a signup ticket: SessionMiddleware is not installed") + return + store[SIGNUP_TICKET_KEY] = {"email": email, "ticket": ticket} + + +def read_signup_ticket(request: Request, *, email: str) -> Optional[str]: + """Return this browser's ticket for ``email``, or ``None``.""" + store = _session_store(request) + payload = store.get(SIGNUP_TICKET_KEY) if store else None + if not isinstance(payload, dict) or payload.get("email") != email: + return None + ticket = payload.get("ticket") + return ticket if isinstance(ticket, str) and ticket else None + + +def forget_signup_ticket(request: Request) -> None: + """Drop any held signup ticket. The code it belonged to is spent.""" + store = _session_store(request) + if store is not None: + store.pop(SIGNUP_TICKET_KEY, None) diff --git a/api/auth/email_verification.py b/api/auth/email_verification.py index a367a095..24d864b0 100644 --- a/api/auth/email_verification.py +++ b/api/auth/email_verification.py @@ -19,6 +19,16 @@ the session that submitted the form, and the person who submitted it never sees the mail. It also means no URL for a mail scanner to fetch and silently burn. +The code alone is not quite enough, because two people can submit the same +address. Whoever writes last owns the stored password, and the code that goes +out is mailed to the address, not to the submitter -- so a stranger could +re-submit someone else's pending signup with a password of their own and let the +owner redeem it for them. Each submission therefore also mints a ticket that +stays in the submitting browser's session, and a code is only redeemable by the +browser holding the ticket that was issued with it. Re-submission then costs the +first submitter their pending signup, which is a nuisance they can undo by +signing up again, rather than an account under someone else's password. + The code is short, so its secrecy cannot rest on entropy -- a million possibilities is nothing to a script. What bounds it is the attempt limit: a handful of wrong guesses destroys the pending signup outright, and the code @@ -86,22 +96,24 @@ def full_name(self) -> str: @dataclass(frozen=True) -class CodeIssue: # pylint: disable=too-many-instance-attributes +class CodeIssue: """The result of asking for a verification code. - ``code`` is the only time the raw value exists outside the mail; the graph - keeps just its hash. ``throttled`` and ``exhausted`` are separated so the - caller can tell "come back in a minute" from "stop asking". + ``code`` is the only time the raw value exists outside the mail, and + ``ticket`` the only time that value exists outside the caller's session; the + graph keeps just their hashes. Nothing here says *why* a code was refused: + the routes answer refusals and successes identically, so the reason would + only be a way to ask the graph questions about other people's addresses. The ``previous_*`` fields are the code this one displaced, so ``revert_verification_send`` can put it back if the mail never goes out. + They also say whether there was a pending signup here at all: a code hash + means the record pre-dated this call. """ code: Optional[str] = None first_name: Optional[str] = None - throttled: bool = False - exhausted: bool = False - missing: bool = False + ticket: Optional[str] = None previous_code_hash: Optional[str] = None previous_expires_at: Optional[int] = None previous_attempts: Optional[int] = None @@ -159,6 +171,16 @@ def generate_code() -> str: return f"{secrets.randbelow(10 ** CODE_DIGITS):0{CODE_DIGITS}d}" +def generate_ticket() -> str: + """A fresh signup ticket. + + Unlike the code this one is never typed by anybody, so it is as long as it + wants to be -- and it has to be, because it is the half of the pair that + never leaves the browser and therefore has no attempt limit behind it. + """ + return secrets.token_urlsafe(32) + + def hash_code(code: str) -> str: """Hash a raw code for storage and lookup.""" return hashlib.sha256(code.encode("utf-8")).hexdigest() @@ -169,64 +191,70 @@ def _graph(): return db.select_graph(ORGANIZATIONS_GRAPH) -async def _read_send_state(email: str) -> Tuple[Optional[int], int, Optional[str]]: - """Return ``(last_sent_at, send_count, first_name)`` for a pending signup.""" - result = await _graph().query( - """ - MATCH (p:PendingSignup {email: $email}) - RETURN p.last_sent_at AS last_sent_at, - p.send_count AS send_count, - p.first_name AS first_name - """, - {"email": email}, - ) - if not result.result_set: - return None, 0, None - last_sent_at, send_count, first_name = result.result_set[0] - return last_sent_at, int(send_count or 0), first_name - - # The guard the two issuing queries share. It rides along in the write itself # rather than being checked first: a single Cypher query is atomic and writes to # one graph are serialised, so checking separately would let two concurrent # requests both pass the check and both send while the counter advanced once. -_SEND_ALLOWED = """ - WHERE p.send_count < $max_sends +# +# ``stale`` is what keeps the send budget from turning into a permanent lock on +# an address. Nothing deletes a pending signup that is simply never confirmed, +# so without it a stranger could spend an address's five sends, leave the +# counter pinned at its limit for good, and the real owner would never be able +# to sign up. Once the code the counter was rationing has expired the record is +# spent anyway, and the next submission starts a fresh budget on it. +_SEND_GUARD = """ + WITH p, (p.expires_at IS NULL OR p.expires_at < $now) AS stale + WHERE (stale OR p.send_count < $max_sends) AND (p.last_sent_at IS NULL OR $now - p.last_sent_at >= $interval_ms) """ +# Counts this send, starting over when the record it lands on had expired. The +# interval still applies on that path, so an expired record is not a way to send +# faster -- only a way to keep sending at all. +_COUNT_SEND = "p.send_count = CASE WHEN stale THEN 1 ELSE p.send_count + 1 END" + # Creates the pending signup if it is new, then replaces its details and issues # a code -- but only for a node the guard lets through. A node created by this -# very query starts at zero sends, so it always passes. +# very query has no code and no expiry, so it counts as stale and always passes. _START_SIGNUP = ( """ MERGE (p:PendingSignup {email: $email}) ON CREATE SET p.created_at = $now, p.send_count = 0 - WITH p """ - + _SEND_ALLOWED + + _SEND_GUARD + """ + WITH p, stale, + p.code_hash AS previous_code_hash, + p.expires_at AS previous_expires_at, + p.attempts AS previous_attempts SET p.code_hash = $code_hash, + p.ticket_hash = $ticket_hash, p.first_name = $first_name, p.last_name = $last_name, p.password_hash = $password_hash, p.expires_at = $expires_at, p.attempts = 0, p.last_sent_at = $now, - p.send_count = p.send_count + 1 - RETURN p.send_count AS send_count + """ + + _COUNT_SEND + + """ + RETURN previous_code_hash, + previous_expires_at, + previous_attempts """ ) # Refresh only: never MERGE, so the resend endpoint cannot conjure a pending -# signup for an address nobody submitted. +# signup for an address nobody submitted. The ticket is deliberately left alone +# -- a resend is another copy of the same signup, not a new one, and rotating it +# would lock out the browser that is waiting for the code. _REFRESH_SIGNUP = ( """ MATCH (p:PendingSignup {email: $email}) """ - + _SEND_ALLOWED + + _SEND_GUARD + """ - WITH p, + WITH p, stale, p.code_hash AS previous_code_hash, p.expires_at AS previous_expires_at, p.attempts AS previous_attempts @@ -234,7 +262,9 @@ async def _read_send_state(email: str) -> Tuple[Optional[int], int, Optional[str p.expires_at = $expires_at, p.attempts = 0, p.last_sent_at = $now, - p.send_count = p.send_count + 1 + """ + + _COUNT_SEND + + """ RETURN p.first_name AS first_name, previous_code_hash, previous_expires_at, @@ -255,7 +285,7 @@ async def _read_send_state(email: str) -> Tuple[Optional[int], int, Optional[str def _throttle_params(now: int) -> dict: - """The parameters ``_SEND_ALLOWED`` reads.""" + """The parameters ``_SEND_GUARD`` reads.""" return { "now": now, "max_sends": max_sends(), @@ -263,33 +293,26 @@ def _throttle_params(now: int) -> dict: } -async def _classify_refusal(email: str) -> CodeIssue: - """Say why the guard rejected a send. Only ever reports, never decides.""" - last_sent_at, send_count, first_name = await _read_send_state(email) - if last_sent_at is None and send_count == 0 and first_name is None: - return CodeIssue(missing=True) - if send_count >= max_sends(): - return CodeIssue(exhausted=True) - return CodeIssue(throttled=True) - - async def start_pending_signup( email: str, first_name: str, last_name: str, password_hash: str ) -> CodeIssue: - """Record a signup awaiting verification and return its code. + """Record a signup awaiting verification and return its code and ticket. Re-submitting the form for an address that is already pending replaces the - stored details and invalidates the previous code, so the most recent attempt - is the one that works. The send counter deliberately survives that replace: - otherwise resubmitting would reset the rate limit and defeat it. + stored details and invalidates the previous code *and* ticket, so only the + browser behind the most recent attempt can complete it. The send counter + deliberately survives that replace: otherwise resubmitting would reset the + rate limit and defeat it. """ now = _now_ms() code = generate_code() + ticket = generate_ticket() result = await _graph().query( _START_SIGNUP, { "email": email, "code_hash": hash_code(code), + "ticket_hash": hash_code(ticket), "first_name": first_name, "last_name": last_name, "password_hash": password_hash, @@ -298,9 +321,21 @@ async def start_pending_signup( }, ) if not result.result_set: - return await _classify_refusal(email) + # Sent to a moment ago, or out of sends while the current code is still + # live. Which one is not reported: the route answers a refusal exactly + # like a success, so telling them apart here would only be a way to ask + # whether a stranger's address has a signup in flight. + return CodeIssue() - return CodeIssue(code=code, first_name=first_name) + previous_code_hash, previous_expires_at, previous_attempts = result.result_set[0] + return CodeIssue( + code=code, + first_name=first_name, + ticket=ticket, + previous_code_hash=previous_code_hash, + previous_expires_at=previous_expires_at, + previous_attempts=previous_attempts, + ) async def refresh_pending_signup(email: str) -> CodeIssue: @@ -325,8 +360,8 @@ async def refresh_pending_signup(email: str) -> CodeIssue: if not result.result_set: # No pending signup, one that has used up its sends, or one that was # sent to a moment ago. Also covers losing a race with a verification - # that just consumed the record. - return await _classify_refusal(email) + # that just consumed the record. Indistinguishable on purpose. + return CodeIssue() first_name, previous_code_hash, previous_expires_at, previous_attempts = ( result.result_set[0] @@ -371,7 +406,9 @@ async def revert_verification_send(email: str, issue: CodeIssue) -> None: # check that only one increment ever answered for. _CONSUME_CODE = """ MATCH (p:PendingSignup {email: $email}) - WHERE p.code_hash = $code_hash AND p.attempts < $max_attempts + WHERE p.code_hash = $code_hash + AND p.ticket_hash = $ticket_hash + AND p.attempts < $max_attempts WITH p, p.first_name AS first_name, p.last_name AS last_name, @@ -394,7 +431,7 @@ async def revert_verification_send(email: str, issue: CodeIssue) -> None: async def consume_pending_signup( - email: str, code: str + email: str, code: str, ticket: str ) -> Tuple[Optional[PendingSignup], str]: """Redeem a verification code exactly once. @@ -402,6 +439,12 @@ async def consume_pending_signup( in the same query that reads it, so a replayed code finds nothing -- that, not a flag, is what makes it single-use. + Both halves are required. The code proves the caller reads the address's + mail; the ticket proves they are the browser that submitted the password + being redeemed. Without the ticket, anyone could re-submit a stranger's + pending signup under a password of their own and let the address's owner + confirm it into an account the submitter can log into. + A wrong guess is charged against the record's attempt budget, and running that budget out deletes the pending signup. That, rather than the length of the code, is what makes six digits enough. @@ -409,7 +452,7 @@ async def consume_pending_signup( Lookup is by code *hash*, an exact match on a stored value, so there is no secret-dependent comparison here for timing to leak. """ - if not email or not code: + if not email or not code or not ticket: return None, RESULT_INVALID result = await _graph().query( @@ -417,6 +460,7 @@ async def consume_pending_signup( { "email": email, "code_hash": hash_code(code), + "ticket_hash": hash_code(ticket), "max_attempts": max_attempts(), }, ) diff --git a/api/mail.py b/api/mail.py index afcca6af..ad12eee8 100644 --- a/api/mail.py +++ b/api/mail.py @@ -6,8 +6,13 @@ Three transports: -* ``console`` (the default) writes the message to the log instead of sending - it, so local development completes the signup flow without a mail server. +* ``console`` writes the message to the log instead of sending it, so local + development completes the signup flow without a mail server. It is what an + unconfigured process falls back to, which is why it is confined to + ``APP_ENV=development``: reached by omission anywhere else, it would turn a + deployment that forgot ``MAIL_SERVER`` into one that logs every confirmation + code and tells the user it sent them. Outside development the send simply + fails, which the signup routes surface and roll back. * ``file`` writes each message to ``MAIL_OUTBOX_DIR`` as an ``.eml``. The Playwright suite reads the confirmation code back out of it, which is what keeps the end-to-end signup test exercising the real flow instead of a @@ -95,11 +100,28 @@ def outbox_dir() -> str: return os.getenv("MAIL_OUTBOX_DIR", "").strip() +def console_transport_allowed() -> bool: + """Whether mail may be logged instead of sent. + + Fail secure, the same way the session cookie's ``Secure`` flag does: the + console transport is what a laptop with no relay falls back to, and it + writes the confirmation code -- a credential -- into the log. Since it is + reached by *omitting* configuration rather than choosing it, a deployment + that forgets ``MAIL_SERVER`` would otherwise swallow every code silently and + tell the user their mail was on its way. Only an explicit + ``APP_ENV=development`` opts in. + """ + app_env = os.getenv("APP_ENV") + return app_env is not None and app_env.strip().lower() == "development" + + def transport_name() -> str: """Name of the active transport, for logs and diagnostics.""" if outbox_dir(): return "file" - return "smtp" if is_smtp_configured() else "console" + if is_smtp_configured(): + return "smtp" + return "console" if console_transport_allowed() else "none" def default_sender() -> str: @@ -217,6 +239,15 @@ async def send_mail( return _write_to_outbox(message, directory) if not is_smtp_configured(): + if not console_transport_allowed(): + # No relay, and not a development run. Refusing is the honest + # answer: the caller rolls the signup back and tells the user to + # retry, instead of the code being logged and never delivered. + logging.error( + "No MAIL_SERVER is configured, so this message cannot be sent. " + "Set MAIL_SERVER, or APP_ENV=development to log mail instead." + ) + return False _log_to_console(message, text_body) return True diff --git a/api/routes/auth.py b/api/routes/auth.py index 41666e37..117d56dd 100644 --- a/api/routes/auth.py +++ b/api/routes/auth.py @@ -23,9 +23,12 @@ from api.auth.browser_session import ( clear_browser_session, establish_browser_session, + forget_signup_ticket, is_provisioned, mark_provisioned, read_browser_session, + read_signup_ticket, + remember_signup_ticket, ) from api.auth.email_verification import ( RESULT_OK, @@ -287,24 +290,25 @@ async def _complete_login(request: Request, provider: str, user_data: dict) -> N ) -def _refuse_verification_send(issue) -> JSONResponse: - """Turn a refused code request into a response.""" - if issue.exhausted: - return JSONResponse( - {"success": False, - "error": "Too many verification emails have been sent to this address. " - "Please try again later."}, - status_code=status.HTTP_429_TOO_MANY_REQUESTS, - ) - wait = resend_interval_seconds() - return JSONResponse( - {"success": False, - "error": "A verification email was just sent. Please wait a moment before " - "requesting another.", - "retryAfterSeconds": wait}, - status_code=status.HTTP_429_TOO_MANY_REQUESTS, - headers={"Retry-After": str(wait)}, - ) +def _signup_accepted(email: str) -> JSONResponse: + """The one answer ``/signup/email`` gives once the form itself is valid. + + 202, not 201: the account does not exist yet, and will not until the code + comes back. It reads the same whether a code went out or the address was + refused one for being asked too often -- a 429 here would say that a signup + for this address is already in flight, which is the same question + ``/signup/email/resend`` deliberately refuses to answer. + """ + return JSONResponse({ + "success": True, + "pending": True, + "email": email, + "message": "Check your inbox for the confirmation code.", + "codeTtlSeconds": code_ttl_seconds(), + # So the resend button comes back exactly when another request + # would be honoured, whatever the deployment configured. + "retryAfterSeconds": resend_interval_seconds(), + }, status_code=status.HTTP_202_ACCEPTED) # ---- Email Authentication Routes ---- @@ -367,12 +371,22 @@ async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSO issue = await start_pending_signup(email, first_name, last_name, password_hash) if not issue.issued: - return _refuse_verification_send(issue) + # Refused for asking too often. Answered exactly like a success so + # the state of a stranger's signup stays private; the caller can + # still use the code they were sent a moment ago. + logging.info("Verification code not issued for %s", _sanitize_for_log(email)) + return _signup_accepted(email) if not await send_verification_code(email, first_name, issue.code): - # The code never left the building, so the pending record is dead - # weight that would only burn the rate limit on the retry. - await discard_pending_signup(email) + if issue.previous_code_hash: + # This address already had a live code before the failed send. + # Put it back rather than deleting the record: the user may be + # holding that code, and it is the only one that ever arrived. + await revert_verification_send(email, issue) + else: + # Nothing was displaced, so the record is dead weight that would + # only burn the rate limit on the retry. + await discard_pending_signup(email) logging.error("Could not send the verification email for %s", _sanitize_for_log(email)) return JSONResponse( @@ -381,21 +395,15 @@ async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSO status_code=status.HTTP_503_SERVICE_UNAVAILABLE ) + # The other half of the code. It stays in this browser's session, so the + # code is only redeemable here -- someone else re-submitting this + # address cannot have their password confirmed by its owner. + remember_signup_ticket(request, email=email, ticket=issue.ticket) + logging.info("Verification code sent for pending signup: %s", _sanitize_for_log(email)) - # 202, not 201: the account does not exist yet, and will not until the - # code comes back. No session is established here for the same reason. - return JSONResponse({ - "success": True, - "pending": True, - "email": email, - "message": "Check your inbox for the confirmation code.", - "codeTtlSeconds": code_ttl_seconds(), - # So the resend button comes back exactly when another request - # would be honoured, whatever the deployment configured. - "retryAfterSeconds": resend_interval_seconds(), - }, status_code=status.HTTP_202_ACCEPTED) + return _signup_accepted(email) except (AuthBackendUnavailableError, *TRANSIENT_BACKEND_ERRORS) as e: # Same reasoning as /login/email: an unreachable store is not a rejected @@ -429,6 +437,12 @@ async def verify_email(request: Request, verify_data: EmailVerifyRequest) -> JSO create the account, and the stranger would know the password to it. The person who fills in the form is the only one who sees both halves. + Both halves are checked. The ticket held in this browser's session is the + other one, and without it the same attack survives the change to codes: + re-submit a stranger's pending signup with a password of your own, and the + code that reaches them redeems *your* password. A code is only good in the + browser it was issued to. + Every refusal reads the same. Whether the address has a signup pending, a wrong code, an expired one or nothing at all is not something an unauthenticated caller gets to learn by guessing. @@ -452,13 +466,26 @@ async def verify_email(request: Request, verify_data: EmailVerifyRequest) -> JSO if not _validate_email(email) or not code: return rejected + ticket = read_signup_ticket(request, email=email) + if not ticket: + # No signup was started in this browser for this address, so there is + # nothing here to confirm -- whatever code the caller has, it belongs to + # somebody else's session. + logging.info("Verification without a signup ticket for %s", + _sanitize_for_log(email)) + return rejected + try: - pending, result = await consume_pending_signup(email, code) + pending, result = await consume_pending_signup(email, code, ticket) if result != RESULT_OK or pending is None: logging.info("Verification refused for %s: %s", _sanitize_for_log(email), result) return rejected + # The code is spent from here on, whatever happens next, so the ticket + # that went with it has nothing left to unlock. + forget_signup_ticket(request) + # The address may have acquired an account by another route (Google, # GitHub) while the code sat unused. The code is spent either way. if await _email_account_exists(pending.email): @@ -573,7 +600,8 @@ async def resend_verification_email( if not issue.issued: # Unknown address, throttled and exhausted are indistinguishable here by - # design; only the log tells them apart. + # design, and are not told apart anywhere else either: knowing which one + # it was is the answer this endpoint exists to withhold. logging.info("Verification resend not issued for %s", _sanitize_for_log(email)) return accepted diff --git a/tests/test_email_signup.py b/tests/test_email_signup.py index ba7c6f10..c06406f8 100644 --- a/tests/test_email_signup.py +++ b/tests/test_email_signup.py @@ -18,6 +18,7 @@ import pytest +from api.auth.browser_session import SESSION_KEY, SIGNUP_TICKET_KEY from api.auth.email_verification import ( RESULT_EXPIRED, RESULT_INVALID, @@ -40,14 +41,25 @@ pytestmark = [pytest.mark.unit, pytest.mark.auth] -def _mock_request(): - """Build a minimal mock Request for the signup handler.""" +def _mock_request(signup_ticket="ticket", ticket_email="new@example.com"): + """Build a minimal mock Request for the signup handler. + + Redeeming a code takes the ticket the browser was handed when it submitted + the form, so the default request carries one for the address the + verification tests use. Pass ``signup_ticket=None`` for a browser that never + started a signup. + """ request = MagicMock() # The transport helpers read these; default to a plain http request. request.headers.get.return_value = None request.url.scheme = "http" request.base_url = "http://testserver/" request.session = {} + if signup_ticket: + request.session[SIGNUP_TICKET_KEY] = { + "email": ticket_email, + "ticket": signup_ticket, + } return request @@ -143,10 +155,12 @@ async def test_signup_mails_a_code_and_creates_nothing( self, _enabled, mock_exists, mock_start, mock_send, mock_ensure ): mock_exists.return_value = False - mock_start.return_value = CodeIssue(code="123456", first_name="Mallory") + mock_start.return_value = CodeIssue( + code="123456", first_name="Mallory", ticket="ticket-abc" + ) mock_send.return_value = True - request = _mock_request() + request = _mock_request(signup_ticket=None) response = await email_signup(request, _signup_data("new@example.com")) # 202, not 201: nothing has been created. @@ -154,8 +168,9 @@ async def test_signup_mails_a_code_and_creates_nothing( body = response.body.decode() assert '"pending":true' in body.replace(" ", "") assert "api_token=" not in _set_cookie_header(response) - # The whole point: no account and no session until the code comes back. - assert not request.session + # The whole point: no account and no login until the code comes back. + # The session holds the signup's ticket and nothing else. + assert SESSION_KEY not in request.session mock_ensure.assert_not_called() # The mail carries the raw code, which exists nowhere else. @@ -164,6 +179,33 @@ async def test_signup_mails_a_code_and_creates_nothing( # is that only whoever reads the inbox learns it. assert "123456" not in body + @pytest.mark.asyncio + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_the_ticket_stays_in_the_browser_that_signed_up( + self, _enabled, mock_exists, mock_start, mock_send + ): + # It is the half of the pair that never goes in the mail: whoever else + # submits this address, only this browser can redeem a code for it. + mock_exists.return_value = False + mock_start.return_value = CodeIssue( + code="123456", first_name="Mallory", ticket="ticket-abc" + ) + mock_send.return_value = True + + request = _mock_request(signup_ticket=None) + response = await email_signup(request, _signup_data("new@example.com")) + + assert request.session[SIGNUP_TICKET_KEY] == { + "email": "new@example.com", + "ticket": "ticket-abc", + } + # Not in the reply, and not in the mail either: it is the cookie. + assert "ticket-abc" not in response.body.decode() + assert "ticket-abc" not in mock_send.await_args.args + @pytest.mark.asyncio @patch("api.routes.auth.discard_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) @@ -176,46 +218,96 @@ async def test_undeliverable_mail_is_reported_and_rolled_back( # Answering 202 here would leave the user waiting for a mail that was # never sent, and the dead record would burn the rate limit on retry. mock_exists.return_value = False - mock_start.return_value = CodeIssue(code="123456", first_name="Mallory") + mock_start.return_value = CodeIssue( + code="123456", first_name="Mallory", ticket="ticket-abc" + ) mock_send.return_value = False - response = await email_signup(_mock_request(), _signup_data("new@example.com")) + response = await email_signup( + _mock_request(signup_ticket=None), _signup_data("new@example.com") + ) assert response.status_code == 503 mock_discard.assert_awaited_once() + @pytest.mark.asyncio + @patch("api.routes.auth.revert_verification_send", new_callable=AsyncMock) + @patch("api.routes.auth.discard_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) + @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_a_failed_send_does_not_destroy_a_code_already_delivered( + self, _enabled, mock_exists, mock_start, mock_send, mock_discard, mock_revert + ): + # Re-submitting the form displaces a code that did arrive. If the new + # mail then fails, deleting the record would take the working code with + # it and leave the user with nothing to type. + mock_exists.return_value = False + mock_start.return_value = CodeIssue( + code="123456", + first_name="Mallory", + ticket="ticket-abc", + previous_code_hash="old-hash", + previous_expires_at=4242, + previous_attempts=1, + ) + mock_send.return_value = False + + response = await email_signup( + _mock_request(signup_ticket=None), _signup_data("new@example.com") + ) + + assert response.status_code == 503 + mock_revert.assert_awaited_once() + mock_discard.assert_not_called() + @pytest.mark.asyncio @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_throttled_signup_is_refused_with_retry_after( + async def test_a_refused_send_reads_exactly_like_a_sent_one( self, _enabled, mock_exists, mock_start, mock_send ): + # A 429 here would say "this address has a signup in flight", which is + # the question /signup/email/resend refuses to answer. Answering it on + # the way in would give it away just the same. mock_exists.return_value = False - mock_start.return_value = CodeIssue(throttled=True) + mock_start.return_value = CodeIssue( + code="123456", first_name="Mallory", ticket="ticket-abc" + ) + mock_send.return_value = True + sent = await email_signup( + _mock_request(signup_ticket=None), _signup_data("new@example.com") + ) - response = await email_signup(_mock_request(), _signup_data("new@example.com")) + mock_start.return_value = CodeIssue() + refused = await email_signup( + _mock_request(signup_ticket=None), _signup_data("new@example.com") + ) - assert response.status_code == 429 - assert int(response.headers["retry-after"]) > 0 - mock_send.assert_not_called() + assert sent.status_code == refused.status_code == 202 + assert sent.body == refused.body + assert mock_send.await_count == 1 @pytest.mark.asyncio @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) @patch("api.routes.auth.start_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) - async def test_exhausted_signup_is_refused_without_inviting_a_retry( + async def test_a_refused_send_hands_out_no_ticket( self, _enabled, mock_exists, mock_start, mock_send ): + # There is no code to go with one, and overwriting the ticket already + # held would lock this browser out of the signup it did start. mock_exists.return_value = False - mock_start.return_value = CodeIssue(exhausted=True) + mock_start.return_value = CodeIssue() - response = await email_signup(_mock_request(), _signup_data("new@example.com")) + request = _mock_request(signup_ticket="held", ticket_email="new@example.com") + await email_signup(request, _signup_data("new@example.com")) - assert response.status_code == 429 - assert "Too many" in response.body.decode() + assert request.session[SIGNUP_TICKET_KEY]["ticket"] == "held" mock_send.assert_not_called() @@ -247,6 +339,56 @@ async def test_valid_code_creates_the_account_and_logs_in( assert mock_ensure.await_args.kwargs["password_hash"] == pending.password_hash assert mock_session.call_args.kwargs["provisioned"] is True + @pytest.mark.asyncio + @patch("api.routes.auth.establish_browser_session", return_value=True) + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth._email_account_exists", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_the_spent_ticket_is_dropped( + self, _enabled, mock_consume, mock_exists, mock_ensure, _session + ): + mock_consume.return_value = (_pending(), RESULT_OK) + mock_exists.return_value = False + mock_ensure.return_value = (True, {"new_identity": True}) + + request = _mock_request() + await verify_email(request, _verify_data()) + + assert SIGNUP_TICKET_KEY not in request.session + + @pytest.mark.asyncio + @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_a_code_alone_confirms_nothing( + self, _enabled, mock_consume, mock_ensure + ): + # The attack the ticket exists for: submit someone else's address with a + # password of your own, and the code that lands in their inbox would + # otherwise create an account you know the password to. A browser that + # did not start this signup cannot finish it, whatever it read. + response = await verify_email(_mock_request(signup_ticket=None), _verify_data()) + + assert response.status_code == 400 + mock_consume.assert_not_called() + mock_ensure.assert_not_called() + + @pytest.mark.asyncio + @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) + @patch("api.routes.auth._is_email_auth_enabled", return_value=True) + async def test_a_ticket_for_another_address_does_not_travel( + self, _enabled, mock_consume + ): + # Signing up for one address must not confer the right to confirm a + # pending signup for a different one. + response = await verify_email( + _mock_request(ticket_email="other@example.com"), _verify_data() + ) + + assert response.status_code == 400 + mock_consume.assert_not_called() + @pytest.mark.asyncio @patch("api.routes.auth.consume_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) @@ -257,9 +399,12 @@ async def test_the_code_is_matched_against_the_address_that_was_submitted( # guessing against every one of them at once. mock_consume.return_value = (None, RESULT_INVALID) - await verify_email(_mock_request(), _verify_data(email="Ada@Example.com ")) + await verify_email( + _mock_request(ticket_email="ada@example.com"), + _verify_data(email="Ada@Example.com "), + ) - assert mock_consume.await_args.args == ("ada@example.com", "123456") + assert mock_consume.await_args.args == ("ada@example.com", "123456", "ticket") @pytest.mark.asyncio @patch("api.routes.auth.ensure_user_in_organizations", new_callable=AsyncMock) @@ -396,33 +541,33 @@ async def test_pending_address_gets_a_fresh_code(self, _enabled, mock_refresh, m async def test_unknown_and_throttled_answer_exactly_like_a_success( self, _enabled, mock_refresh, mock_send ): - # Same status and same body for all three, or the endpoint would tell an - # anonymous caller which addresses are mid-signup. + # Same status and same body either way, or the endpoint would tell an + # anonymous caller which addresses are mid-signup. The store does not + # even report which refusal it was, so there is nothing to leak. mock_refresh.return_value = CodeIssue(code="654321", first_name="Ada") mock_send.return_value = True issued = await resend_verification_email( _mock_request(), EmailResendRequest(email="pending@example.com") ) - mock_refresh.return_value = CodeIssue(missing=True) + mock_refresh.return_value = CodeIssue() unknown = await resend_verification_email( _mock_request(), EmailResendRequest(email="nobody@example.com") ) - mock_refresh.return_value = CodeIssue(throttled=True) - throttled = await resend_verification_email( + refused = await resend_verification_email( _mock_request(), EmailResendRequest(email="pending@example.com") ) - assert issued.status_code == unknown.status_code == throttled.status_code == 202 - assert issued.body == unknown.body == throttled.body + assert issued.status_code == unknown.status_code == refused.status_code == 202 + assert issued.body == unknown.body == refused.body @pytest.mark.asyncio @patch("api.routes.auth.send_verification_code", new_callable=AsyncMock) @patch("api.routes.auth.refresh_pending_signup", new_callable=AsyncMock) @patch("api.routes.auth._is_email_auth_enabled", return_value=True) async def test_unknown_address_is_never_mailed(self, _enabled, mock_refresh, mock_send): - mock_refresh.return_value = CodeIssue(missing=True) + mock_refresh.return_value = CodeIssue() await resend_verification_email( _mock_request(), EmailResendRequest(email="nobody@example.com") diff --git a/tests/test_email_verification.py b/tests/test_email_verification.py index dd5050ff..2c3c5ffc 100644 --- a/tests/test_email_verification.py +++ b/tests/test_email_verification.py @@ -39,9 +39,14 @@ def _patch_graph(graph): class TestStartPendingSignup: """Parking a signup, and the limits on how much mail it can generate.""" + @staticmethod + def _issued(previous_code_hash=None, previous_expires_at=None, previous_attempts=None): + """The row the issuing query returns: the code this one displaced.""" + return _result([[previous_code_hash, previous_expires_at, previous_attempts]]) + @pytest.mark.asyncio async def test_only_the_code_hash_is_stored(self): - graph = _FakeGraph([_result([[1]])]) + graph = _FakeGraph([self._issued()]) with _patch_graph(graph): issue = await ev.start_pending_signup( "new@example.com", "Ada", "Lovelace", "hash" @@ -53,9 +58,39 @@ async def test_only_the_code_hash_is_stored(self): # A graph snapshot must not yield a usable code. assert issue.code not in params.values() + @pytest.mark.asyncio + async def test_only_the_ticket_hash_is_stored(self): + # Same reasoning as the code: the ticket is the other half of the pair, + # so a reader of the graph must not be able to lift a usable one. + graph = _FakeGraph([self._issued()]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + _, params = graph.calls[-1] + assert params["ticket_hash"] == ev.hash_code(issue.ticket) + assert issue.ticket not in params.values() + + @pytest.mark.asyncio + async def test_every_signup_gets_its_own_ticket(self): + # The ticket is what stops a second submission for the same address + # from having its password confirmed by the address's owner. + graph = _FakeGraph([self._issued(), self._issued("old-hash", 4242, 0)]) + with _patch_graph(graph): + first = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + second = await ev.start_pending_signup( + "new@example.com", "Mal", "Lory", "other-hash" + ) + + assert first.ticket and second.ticket + assert first.ticket != second.ticket + @pytest.mark.asyncio async def test_the_code_is_six_digits(self): - graph = _FakeGraph([_result([[1]])]) + graph = _FakeGraph([self._issued()]) with _patch_graph(graph): issue = await ev.start_pending_signup( "new@example.com", "Ada", "Lovelace", "hash" @@ -70,7 +105,7 @@ async def test_the_code_is_six_digits(self): async def test_the_limit_is_enforced_inside_the_write(self): # Checking first and writing after would let two concurrent requests # both pass the check and both send while the counter advanced once. - graph = _FakeGraph([_result([[1]])]) + graph = _FakeGraph([self._issued()]) with _patch_graph(graph): await ev.start_pending_signup("new@example.com", "Ada", "Lovelace", "hash") @@ -83,7 +118,7 @@ async def test_the_limit_is_enforced_inside_the_write(self): async def test_resubmitting_cannot_reset_the_send_limit(self): # Otherwise the rate limit is decorative: re-post the form and the # counter starts over. Only a record this query creates starts at zero. - graph = _FakeGraph([_result([[4]])]) + graph = _FakeGraph([self._issued("old-hash", 4242, 1)]) with _patch_graph(graph): issue = await ev.start_pending_signup( "new@example.com", "Ada", "Lovelace", "hash" @@ -91,35 +126,78 @@ async def test_resubmitting_cannot_reset_the_send_limit(self): assert issue.issued cypher, params = graph.calls[-1] - assert "p.send_count = p.send_count + 1" in cypher + assert "ELSE p.send_count + 1" in cypher assert "ON CREATE SET p.created_at = $now, p.send_count = 0" in cypher assert "send_count" not in params @pytest.mark.asyncio - async def test_a_recent_send_is_throttled_rather_than_repeated(self, monkeypatch): + async def test_an_expired_record_starts_a_fresh_budget(self): + # Nothing deletes a signup that is never confirmed, so a spent send + # budget would otherwise lock an address out of the product for good -- + # five submissions by a stranger and the real owner can never sign up. + graph = _FakeGraph([self._issued("old-hash", 1, 0)]) + with _patch_graph(graph): + await ev.start_pending_signup("new@example.com", "Ada", "Lovelace", "hash") + + cypher, _ = graph.calls[-1] + assert "(p.expires_at IS NULL OR p.expires_at < $now) AS stale" in cypher + assert "WHERE (stale OR p.send_count < $max_sends)" in cypher + assert "p.send_count = CASE WHEN stale THEN 1" in cypher + + @pytest.mark.asyncio + async def test_expiry_does_not_lift_the_interval(self): + # An expired record is a way to keep sending, not a way to send faster. + graph = _FakeGraph([self._issued()]) + with _patch_graph(graph): + await ev.start_pending_signup("new@example.com", "Ada", "Lovelace", "hash") + + cypher, _ = graph.calls[-1] + interval = "AND (p.last_sent_at IS NULL OR $now - p.last_sent_at >= $interval_ms)" + assert interval in cypher + assert "stale OR p.last_sent_at" not in cypher + + @pytest.mark.asyncio + async def test_the_displaced_code_comes_back_for_reverting(self): + # The caller needs it to tell "this address already had a live code" + # from "this record is one I just created", and to put it back. + graph = _FakeGraph([self._issued("old-hash", 4242, 3)]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert issue.previous_code_hash == "old-hash" + assert issue.previous_expires_at == 4242 + assert issue.previous_attempts == 3 + + @pytest.mark.asyncio + async def test_a_refused_send_says_nothing_about_why(self, monkeypatch): + # The route answers a refusal exactly like a success, so asking the + # graph why would only be a way to probe for a stranger's signup. monkeypatch.setenv("EMAIL_VERIFICATION_RESEND_SECONDS", "60") - # The guard rejects the write; the follow-up read only explains why. - graph = _FakeGraph([_result([]), _result([[ev._now_ms(), 1, "Ada"]])]) + graph = _FakeGraph([_result([])]) with _patch_graph(graph): issue = await ev.start_pending_signup( "new@example.com", "Ada", "Lovelace", "hash" ) assert not issue.issued - assert issue.throttled + assert issue == ev.CodeIssue() + assert len(graph.calls) == 1 @pytest.mark.asyncio async def test_send_budget_is_finite(self, monkeypatch): # Bounds how much mail one submitted address can aim at a third party. monkeypatch.setenv("EMAIL_VERIFICATION_MAX_SENDS", "2") - graph = _FakeGraph([_result([]), _result([[None, 2, "Ada"]])]) + graph = _FakeGraph([_result([])]) with _patch_graph(graph): issue = await ev.start_pending_signup( "new@example.com", "Ada", "Lovelace", "hash" ) - assert issue.exhausted assert not issue.issued + _, params = graph.calls[-1] + assert params["max_sends"] == 2 class TestRefreshPendingSignup: @@ -129,14 +207,26 @@ class TestRefreshPendingSignup: async def test_unknown_address_is_not_created(self): # A MERGE here would turn the resend endpoint into a way to mail an # address nobody ever submitted. - graph = _FakeGraph([_result([]), _result([])]) + graph = _FakeGraph([_result([])]) with _patch_graph(graph): issue = await ev.refresh_pending_signup("nobody@example.com") - assert issue.missing assert not issue.issued assert all("MERGE" not in cypher for cypher, _ in graph.calls) + @pytest.mark.asyncio + async def test_a_resend_keeps_the_ticket(self): + # A resend is another copy of the same signup. Minting a new ticket + # would lock out the browser that is sitting on the code entry screen. + graph = _FakeGraph([_result([["Ada", "old-hash", 4242, 3]])]) + with _patch_graph(graph): + issue = await ev.refresh_pending_signup("pending@example.com") + + cypher, params = graph.calls[-1] + assert "ticket_hash" not in cypher + assert "ticket_hash" not in params + assert issue.ticket is None + @pytest.mark.asyncio async def test_refresh_replaces_the_previous_code(self): graph = _FakeGraph([_result([["Ada", "old-hash", 4242, 3]])]) @@ -148,7 +238,7 @@ async def test_refresh_replaces_the_previous_code(self): write_cypher, params = graph.calls[-1] assert "MATCH" in write_cypher and "MERGE" not in write_cypher assert params["code_hash"] == ev.hash_code(issue.code) - assert "p.send_count = p.send_count + 1" in write_cypher + assert "ELSE p.send_count + 1" in write_cypher # A fresh code deserves a fresh budget of guesses. assert "p.attempts = 0" in write_cypher # Kept so a send that never reaches a transport can be undone. @@ -158,13 +248,11 @@ async def test_refresh_replaces_the_previous_code(self): @pytest.mark.asyncio async def test_losing_a_race_with_verification_is_not_an_error(self): - # The record was consumed between the write and the read that explains - # why the write matched nothing. - graph = _FakeGraph([_result([]), _result([])]) + # The record was consumed between the write and this call. + graph = _FakeGraph([_result([])]) with _patch_graph(graph): issue = await ev.refresh_pending_signup("pending@example.com") - assert issue.missing assert not issue.issued @@ -232,7 +320,7 @@ async def test_nothing_to_undo_when_nothing_was_issued(self): graph = _FakeGraph([]) with _patch_graph(graph): await ev.revert_verification_send( - "pending@example.com", ev.CodeIssue(throttled=True) + "pending@example.com", ev.CodeIssue() ) assert graph.calls == [] @@ -257,7 +345,23 @@ def _row(expires_at): async def test_empty_code_never_reaches_the_database(self): graph = _FakeGraph([]) with _patch_graph(graph): - pending, result = await ev.consume_pending_signup("new@example.com", "") + pending, result = await ev.consume_pending_signup( + "new@example.com", "", "ticket" + ) + + assert pending is None + assert result == ev.RESULT_INVALID + assert not graph.calls + + @pytest.mark.asyncio + async def test_a_code_without_a_ticket_never_reaches_the_database(self): + # The pair is the credential. Half of it is not a partial answer, it is + # a request from a browser that never submitted this signup. + graph = _FakeGraph([]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "123456", "" + ) assert pending is None assert result == ev.RESULT_INVALID @@ -269,7 +373,7 @@ async def test_live_code_returns_the_details_and_deletes_the_record(self): graph = _FakeGraph([_result(self._row(future))]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( - "new@example.com", "123456" + "new@example.com", "123456", "ticket" ) assert result == ev.RESULT_OK @@ -281,6 +385,31 @@ async def test_live_code_returns_the_details_and_deletes_the_record(self): assert "DELETE" in cypher assert params["code_hash"] == ev.hash_code("123456") + @pytest.mark.asyncio + async def test_the_ticket_is_matched_in_the_same_query(self): + # Otherwise re-submitting a stranger's pending signup with a password + # of your own gets it confirmed by the address's owner. + future = ev._now_ms() + 60_000 + graph = _FakeGraph([_result(self._row(future))]) + with _patch_graph(graph): + await ev.consume_pending_signup("new@example.com", "123456", "ticket") + + cypher, params = graph.calls[0] + assert "p.ticket_hash = $ticket_hash" in cypher + assert params["ticket_hash"] == ev.hash_code("ticket") + + @pytest.mark.asyncio + async def test_the_right_code_with_the_wrong_ticket_is_refused(self): + # The graph matches nothing, exactly as it would for a wrong code. + graph = _FakeGraph([_result([]), _result([])]) + with _patch_graph(graph): + pending, result = await ev.consume_pending_signup( + "new@example.com", "123456", "someone-elses-ticket" + ) + + assert pending is None + assert result == ev.RESULT_INVALID + @pytest.mark.asyncio async def test_the_attempt_limit_is_enforced_inside_the_write(self): # Reading the counter first would let a burst of concurrent guesses all @@ -288,7 +417,7 @@ async def test_the_attempt_limit_is_enforced_inside_the_write(self): future = ev._now_ms() + 60_000 graph = _FakeGraph([_result(self._row(future))]) with _patch_graph(graph): - await ev.consume_pending_signup("new@example.com", "123456") + await ev.consume_pending_signup("new@example.com", "123456", "ticket") cypher, params = graph.calls[0] assert "p.attempts < $max_attempts" in cypher @@ -300,7 +429,7 @@ async def test_a_wrong_code_is_charged_for(self): graph = _FakeGraph([_result([]), _result([])]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( - "new@example.com", "000000" + "new@example.com", "000000", "ticket" ) assert pending is None @@ -314,7 +443,7 @@ async def test_running_out_of_guesses_destroys_the_signup(self): # costs the attacker their target while the user just signs up again. graph = _FakeGraph([_result([]), _result([[5]])]) with _patch_graph(graph): - await ev.consume_pending_signup("new@example.com", "000000") + await ev.consume_pending_signup("new@example.com", "000000", "ticket") charge_cypher, params = graph.calls[-1] assert "attempts >= $max_attempts" in charge_cypher @@ -330,7 +459,7 @@ async def test_a_failing_charge_is_not_a_free_retry_signal(self): ) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( - "new@example.com", "000000" + "new@example.com", "000000", "ticket" ) assert pending is None @@ -341,7 +470,7 @@ async def test_replayed_code_finds_nothing(self): graph = _FakeGraph([_result([]), _result([])]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( - "new@example.com", "123456" + "new@example.com", "123456", "ticket" ) assert pending is None @@ -353,7 +482,7 @@ async def test_expired_code_is_reported_and_consumed(self): graph = _FakeGraph([_result(self._row(past))]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( - "new@example.com", "123456" + "new@example.com", "123456", "ticket" ) assert pending is None @@ -365,7 +494,7 @@ async def test_record_without_an_expiry_is_not_treated_as_eternal(self): graph = _FakeGraph([_result(self._row(None))]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( - "new@example.com", "123456" + "new@example.com", "123456", "ticket" ) assert pending is None @@ -377,7 +506,7 @@ async def test_record_missing_a_password_is_rejected(self): graph = _FakeGraph([_result([["Ada", "Lovelace", None, future]])]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( - "new@example.com", "123456" + "new@example.com", "123456", "ticket" ) assert pending is None diff --git a/tests/test_mail.py b/tests/test_mail.py index 3e31b441..792b48ea 100644 --- a/tests/test_mail.py +++ b/tests/test_mail.py @@ -18,18 +18,47 @@ @pytest.fixture(autouse=True) def _clean_mail_env(monkeypatch): - """Start every test from the default (console) transport.""" + """Start every test from an unconfigured development process.""" monkeypatch.delenv("MAIL_SERVER", raising=False) monkeypatch.delenv("MAIL_OUTBOX_DIR", raising=False) + monkeypatch.setenv("APP_ENV", "development") class TestTransportSelection: """Which transport is used, and how that is reported.""" - def test_console_is_the_default(self): + def test_console_is_the_development_fallback(self): assert mail.is_smtp_configured() is False assert mail.transport_name() == "console" + def test_there_is_no_fallback_outside_development(self, monkeypatch): + # Console is reached by omitting configuration, so a deployment that + # forgets MAIL_SERVER would otherwise log every confirmation code and + # report the mail as sent. Fail secure, like the session cookie. + for app_env in ("production", "staging", "Development ", ""): + monkeypatch.setenv("APP_ENV", app_env) + assert mail.console_transport_allowed() is ( + app_env.strip().lower() == "development" + ) + + monkeypatch.delenv("APP_ENV", raising=False) + assert mail.console_transport_allowed() is False + assert mail.transport_name() == "none" + + @pytest.mark.asyncio + async def test_an_unconfigured_deployment_fails_the_send(self, monkeypatch, caplog): + # Reporting success would leave the user waiting for a mail nobody sent + # while its code sat in the log. The signup route rolls back on False. + monkeypatch.setenv("APP_ENV", "production") + + with caplog.at_level("INFO"): + sent = await mail.send_mail( + to="new@example.com", subject="Confirm", text_body="code 123456" + ) + + assert sent is False + assert "123456" not in caplog.text + def test_configuring_a_server_switches_to_smtp(self, monkeypatch): monkeypatch.setenv("MAIL_SERVER", "smtp.example.com") assert mail.is_smtp_configured() is True @@ -57,7 +86,7 @@ async def test_an_outbox_keeps_a_configured_relay_unused(self, monkeypatch, tmp_ @pytest.mark.asyncio async def test_console_transport_logs_the_body(self, caplog): - # Local development has no mail server, so the log is where the + # A development run has no mail server, so the log is where the # confirmation code has to be readable from. with caplog.at_level("INFO"): sent = await mail.send_mail( From f13a45c1535dbe4e3b009352538dd8b056807297 Mon Sep 17 00:00:00 2001 From: Anchel135 Date: Sun, 30 Aug 2026 16:47:49 +0300 Subject: [PATCH 08/12] docs: keep the new mail wording spellcheck-clean The spellchecker has no possessive for "else" and no entry for "unconfigured": the first is reworded, the second is a real word the wordlist was missing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/wordlist.txt | 1 + AGENTS.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/wordlist.txt b/.github/wordlist.txt index 42c1d955..393bfb34 100644 --- a/.github/wordlist.txt +++ b/.github/wordlist.txt @@ -128,3 +128,4 @@ signup SMTP outbox PendingSignup +unconfigured diff --git a/AGENTS.md b/AGENTS.md index 29e4856e..a5d7e41a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -132,7 +132,7 @@ Optional overrides: `COMPLETION_MODEL`, `EMBEDDING_MODEL` (must match provider), Authentication is deliberately split three ways — a signed session cookie for the browser login (no FalkorDB dependency, see `api/auth/browser_session.py`), FalkorDB-backed API tokens for programmatic clients, and per-request data-source credentials. `validate_user` in `api/auth/user_management.py` owns the precedence between them. -Email/password signup is verified before the account exists: `POST /signup/email` parks the details on a `PendingSignup` node (`api/auth/email_verification.py`) and mails a six-digit code, and `POST /signup/email/verify` is what creates the `User` and establishes the session. A code rather than a link, so that confirmation has to come back through the session that submitted the form — an emailed link can be opened by a victim who never signed up, which would create an account under a password the sender chose. Holding the code is not on its own enough: every submission also mints a ticket that stays in the submitting browser's session (`remember_signup_ticket` in `api/auth/browser_session.py`) and is matched, hashed, inside the same query that spends the code, so a code read out of someone else's inbox cannot be redeemed. Wrong guesses are charged against a per-record attempt budget that destroys the pending signup once it runs out; that budget, not the six digits, is what makes a short code safe. The send budget is likewise per record and resets when the record expires, so exhausting it cannot lock an address out permanently. There is therefore no `email_verified` flag anywhere — an unconfirmed address is simply absent from the account graph. Every branch of `POST /signup/email` answers the same 202, so nothing about a pending signup is observable from outside. Mail goes through `api/mail.py`, which picks a transport from the environment: a file outbox (`MAIL_OUTBOX_DIR`, used by the Playwright suite to read the code back), SMTP (`MAIL_SERVER`), or — only when `APP_ENV=development` — the console; an unconfigured process anywhere else fails the send rather than logging the code. +Email/password signup is verified before the account exists: `POST /signup/email` parks the details on a `PendingSignup` node (`api/auth/email_verification.py`) and mails a six-digit code, and `POST /signup/email/verify` is what creates the `User` and establishes the session. A code rather than a link, so that confirmation has to come back through the session that submitted the form — an emailed link can be opened by a victim who never signed up, which would create an account under a password the sender chose. Holding the code is not on its own enough: every submission also mints a ticket that stays in the submitting browser's session (`remember_signup_ticket` in `api/auth/browser_session.py`) and is matched, hashed, inside the same query that spends the code, so a code read out of another person's inbox cannot be redeemed. Wrong guesses are charged against a per-record attempt budget that destroys the pending signup once it runs out; that budget, not the six digits, is what makes a short code safe. The send budget is likewise per record and resets when the record expires, so exhausting it cannot lock an address out permanently. There is therefore no `email_verified` flag anywhere — an unconfirmed address is simply absent from the account graph. Every branch of `POST /signup/email` answers the same 202, so nothing about a pending signup is observable from outside. Mail goes through `api/mail.py`, which picks a transport from the environment: a file outbox (`MAIL_OUTBOX_DIR`, used by the Playwright suite to read the code back), SMTP (`MAIL_SERVER`), or — only when `APP_ENV=development` — the console; an unconfigured process anywhere else fails the send rather than logging the code. See `.env.example` for the full list. From 7b0bca819b5a545dd3308cf40eebc9bd2e223a9a Mon Sep 17 00:00:00 2001 From: Anchel135 Date: Sun, 30 Aug 2026 16:50:34 +0300 Subject: [PATCH 09/12] docs: correct the is_smtp_configured docstring Without a relay, mail is only logged in development; anywhere else the send is refused. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/mail.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/mail.py b/api/mail.py index ad12eee8..80eca43d 100644 --- a/api/mail.py +++ b/api/mail.py @@ -91,7 +91,7 @@ def _smtp_port() -> int: def is_smtp_configured() -> bool: - """Whether a relay is configured. When ``False``, mail is logged, not sent.""" + """Whether a relay is configured, from ``MAIL_SERVER``.""" return bool(os.getenv("MAIL_SERVER", "").strip()) From 7457192a048af2160f299d798bc0dd0c39c1c416 Mon Sep 17 00:00:00 2001 From: Anchel135 Date: Sun, 30 Aug 2026 17:04:24 +0300 Subject: [PATCH 10/12] fix: undo a failed send in full, not field by field The revert restored the code, expiry and attempts, but issuing a code also rewrites the ticket, the name and the password hash. Restoring a subset left a record nobody submitted: the ticket stayed rotated, so the delivered code was unusable in the browser holding it, and restoring only the ticket would have been worse still -- that browser's code would then have confirmed an account under the password of whoever made the send that failed. The issuing queries now return properties(p) as it stood before the write, and the revert is SET p = $previous, which also drops properties the send added and cannot fall behind a change to what issuing a code writes. last_sent_at is then pushed back to now, so the budget is refunded without letting a broken transport be retried faster than the interval. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/auth/email_verification.py | 89 ++++++++++++--------------- api/routes/auth.py | 2 +- tests/test_email_signup.py | 4 +- tests/test_email_verification.py | 100 ++++++++++++++++++++----------- 4 files changed, 105 insertions(+), 90 deletions(-) diff --git a/api/auth/email_verification.py b/api/auth/email_verification.py index 24d864b0..4e31d169 100644 --- a/api/auth/email_verification.py +++ b/api/auth/email_verification.py @@ -105,24 +105,30 @@ class CodeIssue: the routes answer refusals and successes identically, so the reason would only be a way to ask the graph questions about other people's addresses. - The ``previous_*`` fields are the code this one displaced, so - ``revert_verification_send`` can put it back if the mail never goes out. - They also say whether there was a pending signup here at all: a code hash - means the record pre-dated this call. + ``previous`` is the record exactly as it stood before this send, so + ``revert_verification_send`` can put it back if the mail never goes out. It + is the whole property map rather than the handful of fields that seemed + interesting: issuing a code rewrites the name, the password hash and the + ticket as well, and an undo that restores only some of those leaves a + record nobody submitted -- someone else's code against this caller's + password. An empty map means the record did not exist until this call. """ code: Optional[str] = None first_name: Optional[str] = None ticket: Optional[str] = None - previous_code_hash: Optional[str] = None - previous_expires_at: Optional[int] = None - previous_attempts: Optional[int] = None + previous: Optional[dict] = None @property def issued(self) -> bool: """Whether a code was actually produced.""" return self.code is not None + @property + def displaced(self) -> bool: + """Whether a live code was overwritten, and so is there to put back.""" + return bool(self.previous and self.previous.get("code_hash")) + def _positive_int_env(name: str, default: int) -> int: """Read a positive integer setting, falling back on anything unusable.""" @@ -223,10 +229,7 @@ def _graph(): """ + _SEND_GUARD + """ - WITH p, stale, - p.code_hash AS previous_code_hash, - p.expires_at AS previous_expires_at, - p.attempts AS previous_attempts + WITH p, stale, properties(p) AS previous SET p.code_hash = $code_hash, p.ticket_hash = $ticket_hash, p.first_name = $first_name, @@ -238,9 +241,7 @@ def _graph(): """ + _COUNT_SEND + """ - RETURN previous_code_hash, - previous_expires_at, - previous_attempts + RETURN previous """ ) @@ -254,10 +255,7 @@ def _graph(): """ + _SEND_GUARD + """ - WITH p, stale, - p.code_hash AS previous_code_hash, - p.expires_at AS previous_expires_at, - p.attempts AS previous_attempts + WITH p, stale, properties(p) AS previous SET p.code_hash = $code_hash, p.expires_at = $expires_at, p.attempts = 0, @@ -265,22 +263,23 @@ def _graph(): """ + _COUNT_SEND + """ - RETURN p.first_name AS first_name, - previous_code_hash, - previous_expires_at, - previous_attempts + RETURN p.first_name AS first_name, previous """ ) -# Undoes one send. Matching on the hash this send wrote makes it a no-op if -# another request has since issued a code of its own. +# Undoes one send, by putting the record back exactly as it was rather than +# by naming the fields to restore -- ``SET p = $previous`` also drops properties +# the send added, and cannot fall behind a change to what issuing a code writes. +# ``last_sent_at`` is then pushed back to now: the send budget is refunded with +# the rest of the snapshot, but retries still stay one per interval, which is +# what stops a broken transport from being hammered. Matching on the hash this +# send wrote makes the whole thing a no-op if another request has since issued a +# code of its own. _REVERT_SEND = """ MATCH (p:PendingSignup {email: $email}) WHERE p.code_hash = $code_hash - SET p.code_hash = $previous_code_hash, - p.expires_at = $previous_expires_at, - p.attempts = $previous_attempts, - p.send_count = p.send_count - 1 + SET p = $previous + SET p.last_sent_at = $now """ @@ -327,14 +326,11 @@ async def start_pending_signup( # whether a stranger's address has a signup in flight. return CodeIssue() - previous_code_hash, previous_expires_at, previous_attempts = result.result_set[0] return CodeIssue( code=code, first_name=first_name, ticket=ticket, - previous_code_hash=previous_code_hash, - previous_expires_at=previous_expires_at, - previous_attempts=previous_attempts, + previous=result.result_set[0][0], ) @@ -363,28 +359,20 @@ async def refresh_pending_signup(email: str) -> CodeIssue: # that just consumed the record. Indistinguishable on purpose. return CodeIssue() - first_name, previous_code_hash, previous_expires_at, previous_attempts = ( - result.result_set[0] - ) - return CodeIssue( - code=code, - first_name=first_name, - previous_code_hash=previous_code_hash, - previous_expires_at=previous_expires_at, - previous_attempts=previous_attempts, - ) + first_name, previous = result.result_set[0] + return CodeIssue(code=code, first_name=first_name, previous=previous) async def revert_verification_send(email: str, issue: CodeIssue) -> None: """Give back a send whose mail never left. Best-effort; never fatal. - Refunds the counter and puts the displaced code back in force, so a - transport failure costs the user neither their send budget nor the code - they may already be holding. ``last_sent_at`` is deliberately left where - the failed attempt put it: retries stay one per interval even when they - fail, which is what stops a broken transport from being hammered. + Puts the record back exactly as it stood, so a transport failure costs the + user neither their send budget nor the code they may already be holding. + Only for a send that displaced a live code -- there is nothing to restore + a record to if this call is what created it, and ``discard_pending_signup`` + is the right way to undo that. """ - if not issue.issued: + if not issue.issued or not issue.displaced: return try: await _graph().query( @@ -392,9 +380,8 @@ async def revert_verification_send(email: str, issue: CodeIssue) -> None: { "email": email, "code_hash": hash_code(issue.code), - "previous_code_hash": issue.previous_code_hash, - "previous_expires_at": issue.previous_expires_at, - "previous_attempts": issue.previous_attempts, + "previous": issue.previous, + "now": _now_ms(), }, ) except Exception as e: # pylint: disable=broad-exception-caught diff --git a/api/routes/auth.py b/api/routes/auth.py index 117d56dd..6c6f0d0c 100644 --- a/api/routes/auth.py +++ b/api/routes/auth.py @@ -378,7 +378,7 @@ async def email_signup(request: Request, signup_data: EmailSignupRequest) -> JSO return _signup_accepted(email) if not await send_verification_code(email, first_name, issue.code): - if issue.previous_code_hash: + if issue.displaced: # This address already had a live code before the failed send. # Put it back rather than deleting the record: the user may be # holding that code, and it is the only one that ever arrived. diff --git a/tests/test_email_signup.py b/tests/test_email_signup.py index c06406f8..eca3501d 100644 --- a/tests/test_email_signup.py +++ b/tests/test_email_signup.py @@ -248,9 +248,7 @@ async def test_a_failed_send_does_not_destroy_a_code_already_delivered( code="123456", first_name="Mallory", ticket="ticket-abc", - previous_code_hash="old-hash", - previous_expires_at=4242, - previous_attempts=1, + previous={"code_hash": "old-hash", "expires_at": 4242, "attempts": 1}, ) mock_send.return_value = False diff --git a/tests/test_email_verification.py b/tests/test_email_verification.py index 2c3c5ffc..a15a6c6e 100644 --- a/tests/test_email_verification.py +++ b/tests/test_email_verification.py @@ -40,9 +40,9 @@ class TestStartPendingSignup: """Parking a signup, and the limits on how much mail it can generate.""" @staticmethod - def _issued(previous_code_hash=None, previous_expires_at=None, previous_attempts=None): - """The row the issuing query returns: the code this one displaced.""" - return _result([[previous_code_hash, previous_expires_at, previous_attempts]]) + def _issued(**previous): + """The row the issuing query returns: the record as it stood before.""" + return _result([[previous]]) @pytest.mark.asyncio async def test_only_the_code_hash_is_stored(self): @@ -76,7 +76,7 @@ async def test_only_the_ticket_hash_is_stored(self): async def test_every_signup_gets_its_own_ticket(self): # The ticket is what stops a second submission for the same address # from having its password confirmed by the address's owner. - graph = _FakeGraph([self._issued(), self._issued("old-hash", 4242, 0)]) + graph = _FakeGraph([self._issued(), self._issued(code_hash="old-hash")]) with _patch_graph(graph): first = await ev.start_pending_signup( "new@example.com", "Ada", "Lovelace", "hash" @@ -118,7 +118,7 @@ async def test_the_limit_is_enforced_inside_the_write(self): async def test_resubmitting_cannot_reset_the_send_limit(self): # Otherwise the rate limit is decorative: re-post the form and the # counter starts over. Only a record this query creates starts at zero. - graph = _FakeGraph([self._issued("old-hash", 4242, 1)]) + graph = _FakeGraph([self._issued(code_hash="old-hash", send_count=1)]) with _patch_graph(graph): issue = await ev.start_pending_signup( "new@example.com", "Ada", "Lovelace", "hash" @@ -135,7 +135,7 @@ async def test_an_expired_record_starts_a_fresh_budget(self): # Nothing deletes a signup that is never confirmed, so a spent send # budget would otherwise lock an address out of the product for good -- # five submissions by a stranger and the real owner can never sign up. - graph = _FakeGraph([self._issued("old-hash", 1, 0)]) + graph = _FakeGraph([self._issued(code_hash="old-hash", expires_at=1)]) with _patch_graph(graph): await ev.start_pending_signup("new@example.com", "Ada", "Lovelace", "hash") @@ -157,18 +157,38 @@ async def test_expiry_does_not_lift_the_interval(self): assert "stale OR p.last_sent_at" not in cypher @pytest.mark.asyncio - async def test_the_displaced_code_comes_back_for_reverting(self): - # The caller needs it to tell "this address already had a live code" - # from "this record is one I just created", and to put it back. - graph = _FakeGraph([self._issued("old-hash", 4242, 3)]) + async def test_the_whole_displaced_record_comes_back_for_reverting(self): + # Not just the code: issuing rewrites the name, the password hash and + # the ticket too, and an undo that restores a subset would leave a + # record nobody submitted -- one person's code against another's + # password. The snapshot is also how the caller tells "this address + # already had a live code" from "this record is one I just created". + previous = { + "code_hash": "old-hash", + "ticket_hash": "old-ticket", + "password_hash": "someone-elses-password", + "expires_at": 4242, + "attempts": 3, + } + graph = _FakeGraph([self._issued(**previous)]) with _patch_graph(graph): issue = await ev.start_pending_signup( "new@example.com", "Ada", "Lovelace", "hash" ) - assert issue.previous_code_hash == "old-hash" - assert issue.previous_expires_at == 4242 - assert issue.previous_attempts == 3 + assert issue.previous == previous + assert issue.displaced + + @pytest.mark.asyncio + async def test_a_record_this_call_created_displaced_nothing(self): + graph = _FakeGraph([self._issued(email="new@example.com", send_count=0)]) + with _patch_graph(graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert issue.issued + assert not issue.displaced @pytest.mark.asyncio async def test_a_refused_send_says_nothing_about_why(self, monkeypatch): @@ -218,7 +238,7 @@ async def test_unknown_address_is_not_created(self): async def test_a_resend_keeps_the_ticket(self): # A resend is another copy of the same signup. Minting a new ticket # would lock out the browser that is sitting on the code entry screen. - graph = _FakeGraph([_result([["Ada", "old-hash", 4242, 3]])]) + graph = _FakeGraph([_result([["Ada", {"code_hash": "old-hash"}]])]) with _patch_graph(graph): issue = await ev.refresh_pending_signup("pending@example.com") @@ -229,7 +249,8 @@ async def test_a_resend_keeps_the_ticket(self): @pytest.mark.asyncio async def test_refresh_replaces_the_previous_code(self): - graph = _FakeGraph([_result([["Ada", "old-hash", 4242, 3]])]) + previous = {"code_hash": "old-hash", "ticket_hash": "kept", "attempts": 3} + graph = _FakeGraph([_result([["Ada", previous]])]) with _patch_graph(graph): issue = await ev.refresh_pending_signup("pending@example.com") @@ -242,9 +263,7 @@ async def test_refresh_replaces_the_previous_code(self): # A fresh code deserves a fresh budget of guesses. assert "p.attempts = 0" in write_cypher # Kept so a send that never reaches a transport can be undone. - assert issue.previous_code_hash == "old-hash" - assert issue.previous_expires_at == 4242 - assert issue.previous_attempts == 3 + assert issue.previous == previous @pytest.mark.asyncio async def test_losing_a_race_with_verification_is_not_an_error(self): @@ -264,35 +283,43 @@ def _issued(): return ev.CodeIssue( code="123456", first_name="Ada", - previous_code_hash="old-hash", - previous_expires_at=4242, - previous_attempts=3, + previous={ + "code_hash": "old-hash", + "ticket_hash": "old-ticket", + "password_hash": "someone-elses-password", + "expires_at": 4242, + "attempts": 3, + "send_count": 1, + }, ) @pytest.mark.asyncio - async def test_the_send_is_refunded_and_the_old_code_restored(self): + async def test_the_record_is_put_back_exactly_as_it_was(self): # A transport failure must cost the user neither their send budget nor - # the code they may already be holding. + # the code they may already be holding. Restoring the whole map rather + # than named fields is also what keeps the undo honest: the send + # rewrote the password hash and the ticket too. graph = _FakeGraph([]) with _patch_graph(graph): await ev.revert_verification_send("pending@example.com", self._issued()) cypher, params = graph.calls[-1] - assert "p.send_count = p.send_count - 1" in cypher - assert params["previous_code_hash"] == "old-hash" - assert params["previous_expires_at"] == 4242 + assert "SET p = $previous" in cypher + assert params["previous"] == self._issued().previous @pytest.mark.asyncio - async def test_the_spent_guesses_come_back_too(self): - # Otherwise a failed send would hand out a free reset of the attempt - # budget, which is the thing keeping six digits honest. + async def test_a_send_that_displaced_nothing_is_not_reverted(self): + # There is no record to restore this one to. Reverting anyway would + # write back the bare node the MERGE created, leaving a husk behind; + # discarding it is the caller's job. graph = _FakeGraph([]) with _patch_graph(graph): - await ev.revert_verification_send("pending@example.com", self._issued()) + await ev.revert_verification_send( + "new@example.com", + ev.CodeIssue(code="123456", previous={"email": "new@example.com"}), + ) - cypher, params = graph.calls[-1] - assert "p.attempts = $previous_attempts" in cypher - assert params["previous_attempts"] == 3 + assert graph.calls == [] @pytest.mark.asyncio async def test_a_code_issued_since_is_left_alone(self): @@ -308,12 +335,15 @@ async def test_a_code_issued_since_is_left_alone(self): @pytest.mark.asyncio async def test_the_clock_is_not_rolled_back(self): - # Otherwise a broken transport could be retried without limit. + # The snapshot would restore the old last_sent_at along with everything + # else, and a broken transport could then be retried without limit. graph = _FakeGraph([]) with _patch_graph(graph): await ev.revert_verification_send("pending@example.com", self._issued()) - assert "last_sent_at" not in graph.calls[-1][0] + cypher, params = graph.calls[-1] + assert "SET p.last_sent_at = $now" in cypher + assert params["now"] >= params["previous"].get("last_sent_at", 0) @pytest.mark.asyncio async def test_nothing_to_undo_when_nothing_was_issued(self): From 1fc8807c2c58510dad8795d196f69dabd906fb3d Mon Sep 17 00:00:00 2001 From: Anchel135 Date: Mon, 31 Aug 2026 11:23:34 +0300 Subject: [PATCH 11/12] fix: keep an expired pending signup so a resend can revive it Redeeming deleted the record before it looked at the expiry, so a code typed a minute too late took the pending signup with it. That undid the recovery path the send budget relies on: the budget only resets because an expired record is still there to be refreshed, and a user whose code had just lapsed was pushed back to the start of signup instead of the resend button that was in front of them. The delete is now conditional on the record being live, inside the same write that reads it, so an expired code reports itself as expired and leaves the record alone. A late but correct code no longer spends one of the small number of wrong guesses either. A record with no expiry at all fails the comparison and so is not deleted, which is the safe direction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 4 +++- api/auth/email_verification.py | 26 ++++++++++++++++-------- tests/test_email_verification.py | 35 ++++++++++++++++++-------------- 3 files changed, 40 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 8f2ebc61..3e7e7e46 100644 --- a/README.md +++ b/README.md @@ -198,7 +198,9 @@ it signs the browser in directly: the password was chosen minutes earlier, and asking for it again would prove nothing. A code can be re-sent from the same screen, subject to a per-address rate limit; the send budget is per pending signup, so it starts over once the pending signup expires and an address can -always be signed up again later. +always be signed up again later. Typing a code that has expired is not one of +the wrong guesses and does not discard anything — the pending signup is left +where it is so the same screen can send a fresh code. In development, a message with no mail server configured is written to the application log instead of being sent, so the flow can be completed by copying diff --git a/api/auth/email_verification.py b/api/auth/email_verification.py index 4e31d169..c0de85c0 100644 --- a/api/auth/email_verification.py +++ b/api/auth/email_verification.py @@ -391,18 +391,24 @@ async def revert_verification_send(email: str, issue: CodeIssue) -> None: # Redeems a code. The attempt guard rides inside the write for the same reason # the send guard does: checking first would let concurrent guesses all pass a # check that only one increment ever answered for. +# +# The delete is conditional because an expired record is still worth something: +# the send guard lets a resend revive it, and deleting it here would strand a +# user who typed the right code a minute late -- their resend would quietly do +# nothing, because refreshing only ever MATCHes. A NULL ``expires_at`` fails the +# comparison and so is treated as not live, which is the safe way round. _CONSUME_CODE = """ MATCH (p:PendingSignup {email: $email}) WHERE p.code_hash = $code_hash AND p.ticket_hash = $ticket_hash AND p.attempts < $max_attempts WITH p, + p.expires_at >= $now AS live, p.first_name AS first_name, p.last_name AS last_name, - p.password_hash AS password_hash, - p.expires_at AS expires_at - DELETE p - RETURN first_name, last_name, password_hash, expires_at + p.password_hash AS password_hash + FOREACH (_ IN CASE WHEN live THEN [1] ELSE [] END | DELETE p) + RETURN live, first_name, last_name, password_hash """ # Charges a wrong guess, and destroys the signup once the budget is gone. A @@ -449,18 +455,20 @@ async def consume_pending_signup( "code_hash": hash_code(code), "ticket_hash": hash_code(ticket), "max_attempts": max_attempts(), + "now": _now_ms(), }, ) if not result.result_set: await _charge_failed_attempt(email) return None, RESULT_INVALID - first_name, last_name, password_hash, expires_at = result.result_set[0] + live, first_name, last_name, password_hash = result.result_set[0] - # Expired codes are consumed rather than left behind: the code is dead - # either way, and dropping the record keeps abandoned signups from - # accumulating. The user simply signs up again. - if not isinstance(expires_at, (int, float)) or _now_ms() >= expires_at: + # The record survives an expired code, so the user can ask for a fresh one + # from the screen they are already on. No attempt is charged either: the + # code was right, and running the budget out here would delete the very + # record the resend needs. + if not live: return None, RESULT_EXPIRED if not password_hash: diff --git a/tests/test_email_verification.py b/tests/test_email_verification.py index a15a6c6e..cb47066a 100644 --- a/tests/test_email_verification.py +++ b/tests/test_email_verification.py @@ -368,8 +368,8 @@ class TestConsumePendingSignup: """Redeeming a code, and the budget of wrong guesses.""" @staticmethod - def _row(expires_at): - return [["Ada", "Lovelace", "hash", expires_at]] + def _row(live): + return [[live, "Ada", "Lovelace", "hash"]] @pytest.mark.asyncio async def test_empty_code_never_reaches_the_database(self): @@ -399,8 +399,7 @@ async def test_a_code_without_a_ticket_never_reaches_the_database(self): @pytest.mark.asyncio async def test_live_code_returns_the_details_and_deletes_the_record(self): - future = ev._now_ms() + 60_000 - graph = _FakeGraph([_result(self._row(future))]) + graph = _FakeGraph([_result(self._row(True))]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( "new@example.com", "123456", "ticket" @@ -412,15 +411,14 @@ async def test_live_code_returns_the_details_and_deletes_the_record(self): cypher, params = graph.calls[0] # Single-use is structural: the read and the delete are one query, so a # replay cannot find the node no matter how the caller behaves. - assert "DELETE" in cypher + assert "DELETE p" in cypher assert params["code_hash"] == ev.hash_code("123456") @pytest.mark.asyncio async def test_the_ticket_is_matched_in_the_same_query(self): # Otherwise re-submitting a stranger's pending signup with a password # of your own gets it confirmed by the address's owner. - future = ev._now_ms() + 60_000 - graph = _FakeGraph([_result(self._row(future))]) + graph = _FakeGraph([_result(self._row(True))]) with _patch_graph(graph): await ev.consume_pending_signup("new@example.com", "123456", "ticket") @@ -444,8 +442,7 @@ async def test_the_right_code_with_the_wrong_ticket_is_refused(self): async def test_the_attempt_limit_is_enforced_inside_the_write(self): # Reading the counter first would let a burst of concurrent guesses all # pass a check that only one increment ever answered for. - future = ev._now_ms() + 60_000 - graph = _FakeGraph([_result(self._row(future))]) + graph = _FakeGraph([_result(self._row(True))]) with _patch_graph(graph): await ev.consume_pending_signup("new@example.com", "123456", "ticket") @@ -507,9 +504,11 @@ async def test_replayed_code_finds_nothing(self): assert result == ev.RESULT_INVALID @pytest.mark.asyncio - async def test_expired_code_is_reported_and_consumed(self): - past = ev._now_ms() - 1 - graph = _FakeGraph([_result(self._row(past))]) + async def test_an_expired_code_leaves_the_record_to_be_resent(self): + # Deleting it would strand a user who typed the right code a minute + # late: refreshing only ever MATCHes, so the resend button on the screen + # they are looking at would quietly do nothing. + graph = _FakeGraph([_result(self._row(False))]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( "new@example.com", "123456", "ticket" @@ -517,10 +516,17 @@ async def test_expired_code_is_reported_and_consumed(self): assert pending is None assert result == ev.RESULT_EXPIRED + cypher, params = graph.calls[0] + assert "FOREACH (_ IN CASE WHEN live THEN [1] ELSE [] END | DELETE p)" in cypher + assert params["now"] > 0 + # The code was right, so no guess is charged -- and charging one could + # run the budget out and delete the record the resend needs. + assert len(graph.calls) == 1 @pytest.mark.asyncio async def test_record_without_an_expiry_is_not_treated_as_eternal(self): - # A missing expiry must fail closed, not read as "never expires". + # A missing expiry must fail closed, not read as "never expires". The + # comparison in the query yields NULL, which is not true. graph = _FakeGraph([_result(self._row(None))]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( @@ -532,8 +538,7 @@ async def test_record_without_an_expiry_is_not_treated_as_eternal(self): @pytest.mark.asyncio async def test_record_missing_a_password_is_rejected(self): - future = ev._now_ms() + 60_000 - graph = _FakeGraph([_result([["Ada", "Lovelace", None, future]])]) + graph = _FakeGraph([_result([[True, "Ada", "Lovelace", None]])]) with _patch_graph(graph): pending, result = await ev.consume_pending_signup( "new@example.com", "123456", "ticket" From 8db4fb32b1a33b401d05dff20651d420840679af Mon Sep 17 00:00:00 2001 From: Anchel135 Date: Mon, 31 Aug 2026 11:23:34 +0300 Subject: [PATCH 12/12] test: run the verification queries against a real FalkorDB The unit tests assert on the Cypher as text, which cannot tell whether FalkorDB agrees with it. Every subtle bug in this flow so far has been in what a query does rather than in what it says: SET p = $map replacing the map, a FOREACH deleting on only one branch, a NULL comparison. These tests run the real queries against a throwaway graph and read the record back, so the next one of those fails here. Each test gets its own client. The one in api.extensions is built at import and pools connections against whichever event loop first used it, which the per-test loop then closes underneath it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/test_email_verification_graph.py | 279 +++++++++++++++++++++++++ 1 file changed, 279 insertions(+) create mode 100644 tests/test_email_verification_graph.py diff --git a/tests/test_email_verification_graph.py b/tests/test_email_verification_graph.py new file mode 100644 index 00000000..597c7177 --- /dev/null +++ b/tests/test_email_verification_graph.py @@ -0,0 +1,279 @@ +"""The verification queries, run against a real FalkorDB. + +The unit tests hand ``email_verification`` a fake graph, so they pin what the +module *does* with a result set but never execute a line of Cypher: a query with +a typo in it passes them all. That matters most for +``revert_verification_send``, which swallows every exception by design and would +fail silently in production while the suite stayed green. + +So this file runs the real queries against a real graph and asserts on the +records they leave behind. It is one round trip per behaviour, not a second copy +of the unit tests -- what is being checked is that the Cypher is valid and means +what the module thinks it means. +""" + +import os +import uuid +from unittest.mock import patch + +import pytest +from falkordb.asyncio import FalkorDB + +from api.auth import email_verification as ev + +pytestmark = [pytest.mark.integration] + + +@pytest.fixture(name="graph") +async def _graph(monkeypatch): + """A throwaway graph, so a failing test cannot poison the next one. + + Its own client, too: the one in ``api.extensions`` is built at import and + pools connections against whichever event loop first used it, which the + per-test loop then closes underneath it. + + The resend interval is switched off, because most of what is being checked + here needs two sends in a row and none of it is about the clock. The test + that *is* about the clock puts an interval back. + """ + monkeypatch.setattr(ev, "resend_interval_seconds", lambda: 0) + url = os.getenv("FALKORDB_URL") + client = FalkorDB.from_url(url) if url else FalkorDB(host="localhost", port=6379) + name = f"test_pending_signup_{uuid.uuid4().hex}" + handle = client.select_graph(name) + with patch("api.auth.email_verification._graph", return_value=handle): + yield handle + try: + await handle.delete() + finally: + await client.connection.aclose() + + +async def _record(graph, email="new@example.com"): + """Every property of the pending signup, or ``None`` if there is none.""" + result = await graph.query( + "MATCH (p:PendingSignup {email: $email}) RETURN properties(p)", + {"email": email}, + ) + return dict(result.result_set[0][0]) if result.result_set else None + + +class TestIssuingAndReverting: + """The queries that write a code, and the one that takes it back.""" + + @pytest.mark.asyncio + async def test_a_signup_is_parked_with_only_hashes(self, graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "password-hash" + ) + + stored = await _record(graph) + assert stored["code_hash"] == ev.hash_code(issue.code) + assert stored["ticket_hash"] == ev.hash_code(issue.ticket) + assert issue.code not in stored.values() + assert issue.ticket not in stored.values() + assert stored["send_count"] == 1 + + @pytest.mark.asyncio + async def test_a_failed_send_puts_the_record_back_exactly(self, graph): + first = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "her-password" + ) + before = await _record(graph) + + # Somebody re-submits the address with a password of their own, and + # this time the mail does not go out. + second = await ev.start_pending_signup( + "new@example.com", "Mal", "Lory", "his-password" + ) + assert second.displaced + await ev.revert_verification_send("new@example.com", second) + + after = await _record(graph) + assert after == {**before, "last_sent_at": after["last_sent_at"]} + assert after["last_sent_at"] >= before["last_sent_at"] + + # The point of restoring the whole record rather than the code alone: + # the ticket and the password are hers again, so her code still works + # and still creates *her* account. + pending, result = await ev.consume_pending_signup( + "new@example.com", first.code, first.ticket + ) + assert result == ev.RESULT_OK + assert pending.password_hash == "her-password" + + @pytest.mark.asyncio + async def test_reverting_a_code_that_was_replaced_does_nothing(self, graph): + stale = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + current = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + await ev.revert_verification_send("new@example.com", stale) + + # The revert matches the hash it wrote, which is no longer the one + # stored, so the code that did go out is untouched. + assert (await _record(graph))["code_hash"] == ev.hash_code(current.code) + + @pytest.mark.asyncio + async def test_a_resend_keeps_the_ticket_and_replaces_the_code(self, graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + before = await _record(graph) + + resent = await ev.refresh_pending_signup("new@example.com") + + after = await _record(graph) + assert resent.first_name == "Ada" + assert after["code_hash"] == ev.hash_code(resent.code) + assert after["ticket_hash"] == before["ticket_hash"] + assert after["send_count"] == 2 + + # The browser waiting on the code screen holds the original ticket, and + # the code that just arrived. Both must still work together. + _, result = await ev.consume_pending_signup( + "new@example.com", resent.code, issue.ticket + ) + assert result == ev.RESULT_OK + + @pytest.mark.asyncio + async def test_a_resend_will_not_invent_a_pending_signup(self, graph): + issue = await ev.refresh_pending_signup("nobody@example.com") + + assert not issue.issued + assert await _record(graph, "nobody@example.com") is None + + +class TestTheSendBudget: + """The guard that rides inside the write.""" + + @pytest.mark.asyncio + async def test_sends_are_spaced_out(self, graph, monkeypatch): + monkeypatch.setattr(ev, "resend_interval_seconds", lambda: 600) + issued = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + refused = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + assert not refused.issued + # Refused inside the write, so nothing moved: the live code is intact. + assert (await _record(graph))["code_hash"] == ev.hash_code(issued.code) + + @pytest.mark.asyncio + async def test_the_budget_runs_out(self, graph, monkeypatch): + monkeypatch.setenv("EMAIL_VERIFICATION_MAX_SENDS", "2") + + assert (await ev.start_pending_signup("new@example.com", "A", "B", "h")).issued + assert (await ev.start_pending_signup("new@example.com", "A", "B", "h")).issued + assert not ( + await ev.start_pending_signup("new@example.com", "A", "B", "h") + ).issued + assert (await _record(graph))["send_count"] == 2 + + @pytest.mark.asyncio + async def test_an_expired_record_starts_a_fresh_budget(self, graph, monkeypatch): + # Nothing deletes an abandoned pending signup, so without this a + # stranger could spend an address's budget and lock it out for good. + monkeypatch.setenv("EMAIL_VERIFICATION_MAX_SENDS", "1") + await ev.start_pending_signup("new@example.com", "A", "B", "h") + assert not ( + await ev.start_pending_signup("new@example.com", "A", "B", "h") + ).issued + + await graph.query( + "MATCH (p:PendingSignup {email: $email}) SET p.expires_at = 1", + {"email": "new@example.com"}, + ) + + assert (await ev.start_pending_signup("new@example.com", "A", "B", "h")).issued + assert (await _record(graph))["send_count"] == 1 + + +class TestRedeeming: + """Spending a code, and what a wrong or late one costs.""" + + @pytest.mark.asyncio + async def test_a_code_is_single_use(self, graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + _, first = await ev.consume_pending_signup( + "new@example.com", issue.code, issue.ticket + ) + _, replay = await ev.consume_pending_signup( + "new@example.com", issue.code, issue.ticket + ) + + assert first == ev.RESULT_OK + assert replay == ev.RESULT_INVALID + assert await _record(graph) is None + + @pytest.mark.asyncio + async def test_the_right_code_in_the_wrong_browser_is_refused(self, graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + + pending, result = await ev.consume_pending_signup( + "new@example.com", issue.code, ev.generate_ticket() + ) + + assert pending is None + assert result == ev.RESULT_INVALID + assert await _record(graph) is not None + + @pytest.mark.asyncio + async def test_wrong_guesses_run_out(self, graph, monkeypatch): + monkeypatch.setenv("EMAIL_VERIFICATION_MAX_ATTEMPTS", "3") + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + wrong = "000000" if issue.code != "000000" else "111111" + + for _ in range(3): + await ev.consume_pending_signup("new@example.com", wrong, issue.ticket) + + assert await _record(graph) is None + + @pytest.mark.asyncio + async def test_a_late_code_leaves_the_record_for_a_resend(self, graph): + issue = await ev.start_pending_signup( + "new@example.com", "Ada", "Lovelace", "hash" + ) + await graph.query( + "MATCH (p:PendingSignup {email: $email}) SET p.expires_at = 1", + {"email": "new@example.com"}, + ) + + pending, result = await ev.consume_pending_signup( + "new@example.com", issue.code, issue.ticket + ) + + assert pending is None + assert result == ev.RESULT_EXPIRED + # Still there, and still costing no guesses, so the resend button on + # the screen the user is looking at can get them out of this. + stored = await _record(graph) + assert stored is not None + assert stored["attempts"] == 0 + + resent = await ev.refresh_pending_signup("new@example.com") + _, result = await ev.consume_pending_signup( + "new@example.com", resent.code, issue.ticket + ) + assert result == ev.RESULT_OK + + @pytest.mark.asyncio + async def test_discarding_removes_the_record(self, graph): + await ev.start_pending_signup("new@example.com", "Ada", "Lovelace", "hash") + + await ev.discard_pending_signup("new@example.com") + + assert await _record(graph) is None