From b5cc3a81c3a1605f96a8a2963ce38efec7be3f18 Mon Sep 17 00:00:00 2001 From: Daoyuan Li <94409450+DaoyuanLi2816@users.noreply.github.com> Date: Sat, 25 Jul 2026 18:52:16 -0700 Subject: [PATCH] fix(auth): resolve email accounts case-insensitively (#4101) * fix(auth): handle email addresses case-insensitively Normalize new and changed addresses, use case-insensitive lookup, and keep legacy case-variant rows stable during unrelated updates. * fix(auth): reject legacy case-variant registrations --------- Co-authored-by: Willem Jiang --- backend/app/gateway/auth/repositories/base.py | 9 +- .../app/gateway/auth/repositories/sqlite.py | 62 +++- backend/tests/test_auth.py | 311 ++++++++++++++++++ 3 files changed, 376 insertions(+), 6 deletions(-) diff --git a/backend/app/gateway/auth/repositories/base.py b/backend/app/gateway/auth/repositories/base.py index b5baa02c7..3ec18f75c 100644 --- a/backend/app/gateway/auth/repositories/base.py +++ b/backend/app/gateway/auth/repositories/base.py @@ -30,7 +30,10 @@ class UserRepository(ABC): user: User object to create Returns: - Created User with ID assigned + Created User with ID assigned. ``email`` is the canonical + (lowercase) stored form, which may differ in case from what was + passed in -- implementations mutate the input ``user`` in place + to reflect this rather than returning a fresh object. Raises: ValueError: If email already exists @@ -69,7 +72,9 @@ class UserRepository(ABC): user: User object with updated fields Returns: - Updated User + Updated User. ``email`` is the canonical (lowercase) stored + form -- implementations mutate the input ``user`` in place to + reflect this rather than returning a fresh object. Raises: UserNotFoundError: If no row exists for ``user.id``. This is diff --git a/backend/app/gateway/auth/repositories/sqlite.py b/backend/app/gateway/auth/repositories/sqlite.py index 3ee3978e3..c9adeac8e 100644 --- a/backend/app/gateway/auth/repositories/sqlite.py +++ b/backend/app/gateway/auth/repositories/sqlite.py @@ -24,6 +24,25 @@ from app.gateway.auth.repositories.base import UserNotFoundError, UserRepository from deerflow.persistence.user.model import UserRow +def _normalize_email(email: str) -> str: + """Canonicalise an email address for storage and lookup. + + An email identifies exactly one account regardless of the case the client + sends. The two write paths would otherwise disagree: local registration + normalises through ``EmailStr``, which lowercases only the *domain* and + keeps the local-part case (``Victim@X.COM`` -> ``Victim@x.com``), while OIDC + provisioning lowercases the whole address (``-> victim@x.com``). Combined + with the previous case-sensitive lookup, ``Victim@x.com`` and + ``victim@x.com`` resolved to two separate rows, defeating the invariant + that a local account blocks an SSO login on the same email. + + Canonicalising to lowercase at every write site and matching + case-insensitively on read closes that gap for new accounts while letting + existing mixed-case rows keep resolving, without a destructive bulk rewrite. + """ + return email.lower() + + class SQLiteUserRepository(UserRepository): """Async user repository backed by the shared SQLAlchemy engine.""" @@ -65,9 +84,20 @@ class SQLiteUserRepository(UserRepository): # ── CRUD ────────────────────────────────────────────────────────── async def create_user(self, user: User) -> User: - """Insert a new user. Raises ``ValueError`` on duplicate email.""" + """Insert a new user. Raises ``ValueError`` on duplicate email. + + The email is canonicalised to lowercase before insert so the existing + unique constraint enforces case-insensitive uniqueness for new rows and + the returned ``User`` reflects the stored form. + """ + user.email = _normalize_email(user.email) row = self._user_to_row(user) async with self._sf() as session: + # The unique constraint is case-sensitive, so it cannot catch a + # canonical address colliding with a mixed-case legacy row. + existing = select(UserRow.id).where(func.lower(UserRow.email) == user.email).limit(1) + if await session.scalar(existing) is not None: + raise ValueError(f"Email already registered: {user.email}") session.add(row) try: await session.commit() @@ -82,10 +112,17 @@ class SQLiteUserRepository(UserRepository): return self._row_to_user(row) if row is not None else None async def get_user_by_email(self, email: str) -> User | None: - stmt = select(UserRow).where(UserRow.email == email) + # Case-insensitive match: an account is keyed by its email regardless of + # the case the caller supplies (see ``_normalize_email``). ``.first()`` + # with a deterministic ``created_at`` ordering resolves to the oldest + # account instead of raising if a pre-fix database already holds two + # rows differing only in case, so the fix never turns a legacy duplicate + # pair into a 500. ``id`` is a secondary tiebreaker so the choice stays + # deterministic even if two legacy rows share the same ``created_at``. + stmt = select(UserRow).where(func.lower(UserRow.email) == _normalize_email(email)).order_by(UserRow.created_at, UserRow.id).limit(1) async with self._sf() as session: result = await session.execute(stmt) - row = result.scalar_one_or_none() + row = result.scalars().first() return self._row_to_user(row) if row is not None else None async def update_user(self, user: User) -> User: @@ -99,7 +136,24 @@ class SQLiteUserRepository(UserRepository): # success would let the caller log "password reset" for # a row that no longer exists. raise UserNotFoundError(f"User {user.id} no longer exists") - row.email = user.email + # Canonicalise the email only when it actually changes, comparing + # case-insensitively against the stored value, then mirror the + # persisted value back onto the returned object. Re-lowercasing an + # *unchanged* legacy mixed-case email — e.g. a password-only update + # on a pre-fix ``Victim@x.com`` row while a canonical ``victim@x.com`` + # row also exists — would rewrite it onto the other row's unique + # email and raise IntegrityError, surfacing as a 500 on the + # change-password / reset-admin paths that do not catch it. Guarding + # against the *raw* stored value (``canonical != row.email``) is not + # enough: the mixed-case row's canonical form still differs from its + # own stored casing, so it would rewrite and collide anyway. A genuine + # change still normalises, so the unique constraint keeps enforcing + # case-insensitive uniqueness for updated rows; get_user_by_email + # already resolves legacy mixed-case rows case-insensitively on read. + canonical_email = _normalize_email(user.email) + if canonical_email != _normalize_email(row.email): + row.email = canonical_email + user.email = row.email row.password_hash = user.password_hash row.system_role = user.system_role row.oauth_provider = user.oauth_provider diff --git a/backend/tests/test_auth.py b/backend/tests/test_auth.py index 4491bb8c5..b2b6aa1a4 100644 --- a/backend/tests/test_auth.py +++ b/backend/tests/test_auth.py @@ -478,6 +478,317 @@ def test_update_user_raises_when_row_concurrently_deleted(tmp_path): asyncio.run(_run()) +# ── Email case-insensitivity (account collision invariant) ────────────────── +# +# Regression coverage for the case-collision gap: local registration normalises +# email through ``EmailStr`` (lowercases only the domain) while OIDC lowercases +# the whole address, and the repo lookup used to be case-sensitive, so +# ``Victim@x.com`` and ``victim@x.com`` became two separate accounts — defeating +# the invariant that a local account blocks an SSO login on the same email +# (flagged on PR #3506, fixed there only OIDC-side). The repo now canonicalises +# to lowercase on write and matches case-insensitively on read. + + +def test_email_lookup_is_case_insensitive(tmp_path): + """A user registered with mixed case resolves for any-case lookup.""" + import asyncio + + from app.gateway.auth.repositories.sqlite import SQLiteUserRepository + + async def _run() -> None: + from deerflow.persistence.engine import close_engine, get_session_factory, init_engine + + url = f"sqlite+aiosqlite:///{tmp_path}/scratch.db" + await init_engine("sqlite", url=url, sqlite_dir=str(tmp_path)) + try: + repo = SQLiteUserRepository(get_session_factory()) + created = await repo.create_user(User(email="Victim@x.com", password_hash="h", system_role="user")) + # Stored canonical (lowercase) and reflected back on the returned object. + assert created.email == "victim@x.com" + + for variant in ("victim@x.com", "VICTIM@X.COM", "Victim@x.com"): + found = await repo.get_user_by_email(variant) + assert found is not None, f"lookup missed {variant!r}" + assert str(found.id) == str(created.id) + finally: + await close_engine() + + asyncio.run(_run()) + + +def test_create_user_rejects_email_differing_only_in_case(tmp_path): + """The second case-variant registration collides on the canonical email.""" + import asyncio + + from app.gateway.auth.repositories.sqlite import SQLiteUserRepository + + async def _run() -> None: + from deerflow.persistence.engine import close_engine, get_session_factory, init_engine + + url = f"sqlite+aiosqlite:///{tmp_path}/scratch.db" + await init_engine("sqlite", url=url, sqlite_dir=str(tmp_path)) + try: + repo = SQLiteUserRepository(get_session_factory()) + await repo.create_user(User(email="Victim@x.com", password_hash="h", system_role="user")) + with pytest.raises(ValueError): + await repo.create_user(User(email="victim@x.com", password_hash="h", system_role="user")) + assert await repo.count_users() == 1 + finally: + await close_engine() + + asyncio.run(_run()) + + +def test_create_user_rejects_legacy_mixed_case_email(tmp_path): + """Registration must not duplicate a mixed-case row created before normalization.""" + import asyncio + from uuid import uuid4 + + from app.gateway.auth.repositories.sqlite import SQLiteUserRepository + + async def _run() -> None: + from deerflow.persistence.engine import close_engine, get_session_factory, init_engine + from deerflow.persistence.user.model import UserRow + + url = f"sqlite+aiosqlite:///{tmp_path}/scratch.db" + await init_engine("sqlite", url=url, sqlite_dir=str(tmp_path)) + try: + sf = get_session_factory() + repo = SQLiteUserRepository(sf) + async with sf() as session: + session.add( + UserRow( + id=str(uuid4()), + email="Victim@x.com", + password_hash="h", + system_role="user", + needs_setup=False, + token_version=0, + ) + ) + await session.commit() + + with pytest.raises(ValueError, match="Email already registered"): + await repo.create_user(User(email="victim@x.com", password_hash="h", system_role="user")) + assert await repo.count_users() == 1 + finally: + await close_engine() + + asyncio.run(_run()) + + +def test_update_user_normalizes_email(tmp_path): + """Changing an email through update_user stores the canonical lowercase form.""" + import asyncio + + from app.gateway.auth.repositories.sqlite import SQLiteUserRepository + + async def _run() -> None: + from deerflow.persistence.engine import close_engine, get_session_factory, init_engine + + url = f"sqlite+aiosqlite:///{tmp_path}/scratch.db" + await init_engine("sqlite", url=url, sqlite_dir=str(tmp_path)) + try: + repo = SQLiteUserRepository(get_session_factory()) + user = await repo.create_user(User(email="user@x.com", password_hash="h", system_role="user")) + user.email = "New@Mixed.COM" + await repo.update_user(user) + refetched = await repo.get_user_by_id(str(user.id)) + assert refetched is not None + assert refetched.email == "new@mixed.com" + assert await repo.get_user_by_email("NEW@MIXED.com") is not None + finally: + await close_engine() + + asyncio.run(_run()) + + +def test_distinct_emails_remain_distinct(tmp_path): + """Case-folding must not collapse genuinely different addresses.""" + import asyncio + + from app.gateway.auth.repositories.sqlite import SQLiteUserRepository + + async def _run() -> None: + from deerflow.persistence.engine import close_engine, get_session_factory, init_engine + + url = f"sqlite+aiosqlite:///{tmp_path}/scratch.db" + await init_engine("sqlite", url=url, sqlite_dir=str(tmp_path)) + try: + repo = SQLiteUserRepository(get_session_factory()) + alice = await repo.create_user(User(email="alice@x.com", password_hash="h", system_role="user")) + bob = await repo.create_user(User(email="bob@x.com", password_hash="h", system_role="user")) + assert await repo.count_users() == 2 + fa = await repo.get_user_by_email("Alice@x.com") + fb = await repo.get_user_by_email("BOB@x.com") + assert fa is not None and str(fa.id) == str(alice.id) + assert fb is not None and str(fb.id) == str(bob.id) + assert str(fa.id) != str(fb.id) + finally: + await close_engine() + + asyncio.run(_run()) + + +def test_legacy_mixed_case_duplicate_rows_resolve_without_error(tmp_path): + """A pre-fix DB with two case-variant rows resolves to the oldest, never 500s. + + Migration-safety: existing installations may already hold ``Victim@x.com`` + and ``victim@x.com`` as separate rows. The case-insensitive lookup must not + raise ``MultipleResultsFound``; it deterministically returns the oldest + (most-established) account. + """ + import asyncio + from datetime import UTC, datetime, timedelta + from uuid import uuid4 + + from app.gateway.auth.repositories.sqlite import SQLiteUserRepository + + async def _run() -> None: + from deerflow.persistence.engine import close_engine, get_session_factory, init_engine + from deerflow.persistence.user.model import UserRow + + url = f"sqlite+aiosqlite:///{tmp_path}/scratch.db" + await init_engine("sqlite", url=url, sqlite_dir=str(tmp_path)) + try: + sf = get_session_factory() + repo = SQLiteUserRepository(sf) + older = datetime.now(UTC) - timedelta(days=5) + newer = datetime.now(UTC) + # Insert raw rows (bypassing create_user's normalisation) to mimic + # data written before this fix. + async with sf() as session: + session.add(UserRow(id=str(uuid4()), email="Victim@x.com", password_hash="h", system_role="user", created_at=older, needs_setup=False, token_version=0)) + session.add(UserRow(id=str(uuid4()), email="victim@x.com", password_hash="h", system_role="user", created_at=newer, needs_setup=False, token_version=0)) + await session.commit() + + found = await repo.get_user_by_email("VICTIM@X.COM") + assert found is not None + assert found.email == "Victim@x.com" # oldest wins, deterministically + finally: + await close_engine() + + asyncio.run(_run()) + + +def test_update_user_on_legacy_mixed_case_row_does_not_collide(tmp_path): + """A password-only update on a legacy mixed-case row must not 500. + + Migration-safety, write side. A pre-fix DB may already hold two rows + differing only in case (``Victim@x.com`` + ``victim@x.com``). A password + change or admin reset reloads the row and calls ``update_user`` with the + email unchanged. ``update_user`` must not opportunistically re-lowercase the + mixed-case email, because that collides with the already-canonical row's + unique email and raises ``IntegrityError`` — which surfaces as a 500 on the + change-password / reset-admin paths that don't catch it. The read path was + hardened for this legacy state; the write path must match, so only a genuine + email change (differing case-insensitively from the stored value) rewrites + the column. + """ + import asyncio + from datetime import UTC, datetime, timedelta + from uuid import uuid4 + + from app.gateway.auth.repositories.sqlite import SQLiteUserRepository + + async def _run() -> None: + from deerflow.persistence.engine import close_engine, get_session_factory, init_engine + from deerflow.persistence.user.model import UserRow + + url = f"sqlite+aiosqlite:///{tmp_path}/scratch.db" + await init_engine("sqlite", url=url, sqlite_dir=str(tmp_path)) + try: + sf = get_session_factory() + repo = SQLiteUserRepository(sf) + mixed_id = str(uuid4()) + canonical_id = str(uuid4()) + older = datetime.now(UTC) - timedelta(days=5) + newer = datetime.now(UTC) + # Raw rows written before the fix (the mixed-case row is the oldest). + async with sf() as session: + session.add(UserRow(id=mixed_id, email="Victim@x.com", password_hash="old-hash", system_role="user", created_at=older, needs_setup=False, token_version=0)) + session.add(UserRow(id=canonical_id, email="victim@x.com", password_hash="canonical-hash", system_role="user", created_at=newer, needs_setup=False, token_version=0)) + await session.commit() + + # Simulate a password change on the mixed-case row: reload it, keep + # the email as-stored, set a new hash + bump the token version. + mixed = await repo.get_user_by_id(mixed_id) + assert mixed is not None + assert mixed.email == "Victim@x.com" + mixed.password_hash = "new-hash-after-change" + mixed.token_version += 1 + + # Must not raise IntegrityError even though lowercasing the email + # would collide with the canonical row's unique email. + await repo.update_user(mixed) + + # The mixed-case row kept its stored casing and took the new password. + refetched = await repo.get_user_by_id(mixed_id) + assert refetched is not None + assert refetched.email == "Victim@x.com" + assert refetched.password_hash == "new-hash-after-change" + assert refetched.token_version == 1 + + # The canonical row is untouched, and no row was lost or merged. + canonical = await repo.get_user_by_id(canonical_id) + assert canonical is not None + assert canonical.email == "victim@x.com" + assert canonical.password_hash == "canonical-hash" + assert await repo.count_users() == 2 + + # Case-insensitive lookup still resolves the mixed-case row (oldest wins). + found = await repo.get_user_by_email("VICTIM@X.COM") + assert found is not None + assert str(found.id) == mixed_id + finally: + await close_engine() + + asyncio.run(_run()) + + +def test_oidc_login_blocked_by_existing_local_account_across_case(tmp_path): + """End-to-end invariant: an SSO login cannot create a duplicate of a local + account whose email differs only in case. + + Uses the real repository + provider + provisioning (no mocks), so it covers + the cross-path gap the mock-based OIDC tests could not: local registration + keeps the local-part case (``Victim@x.com``) while OIDC lowercases the whole + address (``victim@x.com``). + """ + import asyncio + + from app.gateway.auth.local_provider import LocalAuthProvider + from app.gateway.auth.repositories.sqlite import SQLiteUserRepository + from app.gateway.auth.user_provisioning import get_or_provision_oidc_user + from deerflow.config.auth_config import OIDCProviderConfig + + async def _run() -> None: + from fastapi import HTTPException + + from app.gateway.auth.oidc import OIDCIdentity + from deerflow.persistence.engine import close_engine, get_session_factory, init_engine + + url = f"sqlite+aiosqlite:///{tmp_path}/scratch.db" + await init_engine("sqlite", url=url, sqlite_dir=str(tmp_path)) + try: + provider = LocalAuthProvider(SQLiteUserRepository(get_session_factory())) + await provider.create_user(email="Victim@x.com", password="pw-abc-123!", system_role="user") + + cfg = OIDCProviderConfig(display_name="Test SSO", issuer="https://issuer.example.com", client_id="deer-flow", auto_create_users=True) + identity = OIDCIdentity(provider="keycloak", subject="sub-1", email="Victim@x.com", email_verified=True, name="Victim", claims={}) + + with pytest.raises(HTTPException) as exc_info: + await get_or_provision_oidc_user(provider_id="keycloak", provider_config=cfg, identity=identity, local_provider=provider) + + assert exc_info.value.status_code == 409 + # No duplicate auto-created — the local account still owns the email. + assert await provider.count_users() == 1 + finally: + await close_engine() + + asyncio.run(_run()) + + # ── Token Versioning ───────────────────────────────────────────────────────