From 4d879abf867ebce2f3eb7303b5e02b55d194c3f3 Mon Sep 17 00:00:00 2001 From: Amr Gaber Date: Wed, 27 May 2026 12:40:56 -0500 Subject: [PATCH] fix(auth): cascade DELETE /auth/me to oauth_account + CORS on 500s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DELETE /auth/me` 500s for any user with linked OAuth accounts. SQLAlchemy's default ORM cascade ("save-update, merge") tries to disassociate the children with `UPDATE oauth_account SET user_id = NULL` before deleting the parent, which the `NOT NULL` on `oauth_account.user_id` rejects. Adding `cascade="all, delete"` to the `User.oauth_accounts` relationship makes the ORM emit DELETE statements for the children first, so the parent delete succeeds. Not using `passive_deletes=True` deliberately: it would skip the ORM-side DELETEs and rely on the DB cascade, which is true in prod (Postgres) but false in the SQLite test harness without `PRAGMA foreign_keys=ON`, and would mask regressions. The bug surfaced as a CORS error in the browser ("No 'Access-Control-Allow-Origin' header is present on the requested resource") rather than a server error — because Starlette routes `Exception` and `500` handlers to `ServerErrorMiddleware`, which is hardcoded outside all user middleware and emits the 500 via the outer ASGI `send`, bypassing `CORSMiddleware`. Registering an `Exception` handler that re-derives the CORS headers and attaches them to the response itself keeps the diagnostic info intact for any future 500. Both issues covered by regression tests in `test_delete_account.py`. --- app/main.py | 44 ++++++++++++ app/models/user.py | 14 +++- tests/test_delete_account.py | 136 +++++++++++++++++++++++++++++++++++ 3 files changed, 193 insertions(+), 1 deletion(-) create mode 100644 tests/test_delete_account.py diff --git a/app/main.py b/app/main.py index a72a382..8880665 100644 --- a/app/main.py +++ b/app/main.py @@ -1,3 +1,4 @@ +import re import time import uuid @@ -55,6 +56,49 @@ app.state.limiter = limiter app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler) + +def _cors_response_headers(origin: str | None) -> dict[str, str]: + """Compute the CORS headers we'd attach to a normal response. + + ``CORSMiddleware`` adds these automatically on the response path — + but unhandled exceptions are caught by Starlette's + ``ServerErrorMiddleware`` (hardcoded outside of all user middleware), + so the 500 it emits travels via the outer ASGI ``send`` and bypasses + ``CORSMiddleware`` entirely. The result: a browser sees a server + error as "No 'Access-Control-Allow-Origin' header" instead of the + actual failure, and the real bug stays hidden until someone reads + the server logs. Re-deriving the same headers here keeps the + diagnostic information accurate when something goes wrong. + """ + if not origin: + return {} + allowed = origin in settings.cors_origin_list + if not allowed and settings.cors_origin_regex: + allowed = bool(re.fullmatch(settings.cors_origin_regex, origin)) + if not allowed: + return {} + return { + "Access-Control-Allow-Origin": origin, + "Access-Control-Allow-Credentials": "true", + "Vary": "Origin", + } + + +@app.exception_handler(Exception) +async def unhandled_exception_handler(request: Request, exc: Exception) -> JSONResponse: + logger.exception( + "unhandled_exception", + path=request.url.path, + method=request.method, + exc_type=type(exc).__name__, + ) + return JSONResponse( + status_code=500, + content={"detail": "Internal Server Error"}, + headers=_cors_response_headers(request.headers.get("origin")), + ) + + # --- Auth routes --- # Custom refresh/logout routes (included before FastAPI-Users so /auth/jwt/logout is shadowed) app.include_router(auth_refresh_router) diff --git a/app/models/user.py b/app/models/user.py index 2580689..28b8a52 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -46,6 +46,18 @@ class User(SQLAlchemyBaseUserTableUUID, Base): Boolean, default=False, server_default="false", nullable=False ) + # ``cascade="all, delete"`` makes the ORM emit DELETEs for the linked + # oauth_account rows when the user is deleted. Without it, SQLAlchemy + # defaults to "save-update, merge" and tries to disassociate the + # children by issuing ``UPDATE oauth_account SET user_id = NULL`` + # first — which the NOT NULL constraint on ``oauth_account.user_id`` + # rejects, 500ing ``DELETE /auth/me``. We deliberately don't add + # ``passive_deletes=True`` here: the optimization would skip the ORM + # DELETEs and rely on the DB's ``ON DELETE CASCADE`` instead, which + # is true in prod (Postgres) but false in the SQLite test harness + # without ``PRAGMA foreign_keys=ON``, masking regressions. oauth_accounts: Mapped[list["OAuthAccount"]] = relationship( # noqa: F821 - "OAuthAccount", lazy="joined" + "OAuthAccount", + lazy="joined", + cascade="all, delete", ) diff --git a/tests/test_delete_account.py b/tests/test_delete_account.py new file mode 100644 index 0000000..0975f74 --- /dev/null +++ b/tests/test_delete_account.py @@ -0,0 +1,136 @@ +"""Regression tests for account deletion and CORS-on-500. + +Two stories covered here: + +1. ``DELETE /auth/me`` for a user with linked OAuth accounts. + ``User.oauth_accounts`` needs ``cascade="all, delete"`` on the + relationship — without it, SQLAlchemy's default cascade tries to + disassociate the children via ``UPDATE oauth_account SET user_id + = NULL`` before the parent delete, which the NOT NULL constraint + rejects and the endpoint 500s. + +2. Unhandled-exception 500s carry ``Access-Control-Allow-Origin``. + Starlette routes ``Exception`` handlers to ``ServerErrorMiddleware`` + (hardcoded outside all user middleware), so the 500 it emits + travels via the outer ASGI ``send`` and never passes through + ``CORSMiddleware``. The browser then reports any server error as + "No 'Access-Control-Allow-Origin' header is present" instead of + the actual failure. ``app/main.py``'s exception handler attaches + the CORS headers to the response itself so they survive the + middleware-bypass path. +""" + +from __future__ import annotations + +from uuid import UUID, uuid4 + +from fastapi import Depends +from httpx import ASGITransport, AsyncClient +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from app.auth import current_active_user +from app.database import get_async_session +from app.main import app +from app.models.oauth_account import OAuthAccount +from app.models.user import User + + +def _override_current_user_by_id(user_id: UUID) -> None: + """Make ``current_active_user`` re-fetch the user from the same + session the endpoint uses. Mirrors prod, where both come out of + the same per-request session via the dependency cache — without + this, ``session.delete(user)`` raises "object attached to another + session" because the seeded user was created in the test session + while the route resolves its session afresh.""" + + async def _fetch(s: AsyncSession = Depends(get_async_session)) -> User: + return (await s.execute(select(User).where(User.id == user_id))).unique().scalar_one() + + app.dependency_overrides[current_active_user] = _fetch + + +async def test_delete_me_succeeds_for_user_with_linked_oauth( + client: AsyncClient, + session: AsyncSession, +) -> None: + """Regression baseline: without the relationship cascade, this 500s + on flush with a NotNullViolation on ``oauth_account.user_id``.""" + user_id = uuid4() + user = User( + id=user_id, + email=None, + hashed_password="!steam-oauth-no-password", + is_active=True, + is_verified=False, + has_usable_password=False, + ) + session.add(user) + session.add( + OAuthAccount( + user_id=user_id, + oauth_name="steam", + access_token="fake-steam-token", + account_id="76561198000000000", + account_email="", + ) + ) + await session.commit() + + _override_current_user_by_id(user_id) + try: + resp = await client.delete("/auth/me") + finally: + app.dependency_overrides.pop(current_active_user, None) + + assert resp.status_code == 204, resp.text + + remaining = ( + (await session.execute(select(User).where(User.id == user_id))) + .unique() + .scalar_one_or_none() + ) + assert remaining is None + + # And the cascade reached the linked oauth_account row, not just + # the parent. The ORM is what does the deletion under + # ``cascade="all, delete"``; the DB's ``ON DELETE CASCADE`` FK would + # also catch this in prod but doesn't fire in SQLite tests without + # ``PRAGMA foreign_keys=ON``, so the ORM-level work is load-bearing + # for testability. + remaining_link = ( + await session.execute(select(OAuthAccount).where(OAuthAccount.user_id == user_id)) + ).scalar_one_or_none() + assert remaining_link is None + + +async def test_unhandled_exception_response_includes_cors_headers() -> None: + """A 500 from an uncaught route-level exception must still carry + ``Access-Control-Allow-Origin`` — otherwise the browser surfaces + the failure as a CORS violation and obscures the real bug. + + ``raise_app_exceptions=False`` on the transport is required because + Starlette's ``ServerErrorMiddleware`` re-raises after sending the + 500 (for the benefit of debug tooling), and httpx's ASGITransport + propagates that re-raise into the test by default. Production + (uvicorn) just logs the re-raise; the response has already been + delivered to the client at that point. + """ + + async def _boom() -> None: + raise RuntimeError("regression-test exception for CORS-on-500") + + app.dependency_overrides[current_active_user] = _boom + transport = ASGITransport(app=app, raise_app_exceptions=False) + try: + async with AsyncClient(transport=transport, base_url="http://test") as client: + resp = await client.get( + "/auth/me", + headers={"Origin": "http://localhost:5173"}, + ) + finally: + app.dependency_overrides.pop(current_active_user, None) + + assert resp.status_code == 500 + assert resp.headers.get("access-control-allow-origin") == "http://localhost:5173" + assert resp.headers.get("access-control-allow-credentials") == "true"