From c66a6279e95ac7b2adc74eeb950fb09868fca690 Mon Sep 17 00:00:00 2001 From: Amr Gaber Date: Wed, 27 May 2026 13:03:55 -0500 Subject: [PATCH] fix(oauth): redirect on associate-callback errors instead of rendering JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the provider redirects the browser directly to /auth//associate/callback (prod) and the handler raises HTTPException, FastAPI surfaces the raw JSON to the user — most visibly oauth_account_already_linked, which a tester hit when re-linking a Steam account already attached to another user. Wrap the handler so any HTTPException whose detail carries a `code` becomes a 302 to {frontend_url}/profile?associate_error=&associate_provider=. The SPA renders the message as a proper alert from there (separate PR). Exceptions without a structured code re-raise so we don't accidentally swallow unrelated errors. Updated the state-validation and conflict tests to assert the redirect shape instead of JSON. --- app/routers/auth_providers.py | 50 ++++++++++++++++++++++++++++ tests/test_provider_associate.py | 57 ++++++++++++++++++++++---------- 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/app/routers/auth_providers.py b/app/routers/auth_providers.py index 3c49fdc..4fb2aa5 100644 --- a/app/routers/auth_providers.py +++ b/app/routers/auth_providers.py @@ -88,6 +88,28 @@ def _frontend_complete_url(provider_name: str, *, associated: bool = False) -> s return f"{settings.frontend_url}/callback/{provider_name}-{suffix}" +def _frontend_associate_error_url(provider_name: str, code: str) -> str: + """Where to drop the user when the associate flow fails. + + The browser lands on the profile page (where they started), with the + error code in the query string. Otherwise FastAPI would render raw + JSON to the user in prod, where Steam/Google redirect straight to + this API rather than through the SPA's callback page. + """ + return ( + f"{settings.frontend_url}/profile?associate_error={code}&associate_provider={provider_name}" + ) + + +def _oauth_error_code(detail: Any) -> str | None: + """Pull the ``code`` string out of an HTTPException's structured detail.""" + if isinstance(detail, dict): + code = detail.get("code") + if isinstance(code, str): + return code + return None + + # --- State (CSRF + purpose binding) --------------------------------------- @@ -390,6 +412,34 @@ async def associate_callback( request: Request, user: User = Depends(current_active_user), session: AsyncSession = Depends(get_async_session), +): + # In prod the provider redirects the browser directly here, so an + # HTTPException would render as raw JSON to the user. Convert known + # error codes into a 302 back to /profile with the code in the query + # string — the SPA renders a proper alert from there. + try: + return await _associate_callback_impl( + provider_name=provider_name, + request=request, + user=user, + session=session, + ) + except HTTPException as exc: + code = _oauth_error_code(exc.detail) + if code is None: + raise + return RedirectResponse( + url=_frontend_associate_error_url(provider_name, code), + status_code=302, + ) + + +async def _associate_callback_impl( + *, + provider_name: str, + request: Request, + user: User, + session: AsyncSession, ): provider = _resolve_provider(provider_name) state = request.query_params.get("state") diff --git a/tests/test_provider_associate.py b/tests/test_provider_associate.py index 3fceb2f..8872653 100644 --- a/tests/test_provider_associate.py +++ b/tests/test_provider_associate.py @@ -6,16 +6,17 @@ Steam-first user can link Google. The OAuthAccount table doesn't care which provider came first. 2. Conflict detection. Linking a provider identity that's already - attached to another user returns 409. + attached to another user redirects with ``oauth_account_already_linked``. 3. Idempotency. Linking the same provider identity to the same user twice doesn't error or duplicate. 4. State validation. Wrong purpose, wrong user, or a missing CSRF - cookie all yield 400. + cookie all redirect to /profile with the error code in the URL. 5. ``GET /auth/me/connections`` returns the linked-provider list. """ from __future__ import annotations +from urllib.parse import parse_qs, urlparse from uuid import uuid4 import pytest @@ -24,6 +25,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.auth import current_active_user +from app.config import settings from app.main import app from app.models.oauth_account import OAuthAccount from app.models.user import User @@ -37,6 +39,21 @@ _generate_state, ) + +def _assert_associate_error(resp, *, expected_code: str, expected_provider: str) -> None: + """Errors in the associate callback now 302 to /profile with the + failure code in the query string so the SPA can render an alert.""" + assert resp.status_code == 302, resp.text + parsed = urlparse(resp.headers["location"]) + assert parsed.path == "/profile" + expected_base = urlparse(settings.frontend_url) + if expected_base.netloc: + assert parsed.netloc == expected_base.netloc + params = parse_qs(parsed.query) + assert params.get("associate_error") == [expected_code] + assert params.get("associate_provider") == [expected_provider] + + # --- helpers -------------------------------------------------------------- @@ -262,9 +279,11 @@ async def test_link_steam_already_on_other_user_returns_409( finally: _clear_user_override() - assert resp.status_code == 409, resp.text - body = resp.json() - assert body["detail"]["code"] == "oauth_account_already_linked" + _assert_associate_error( + resp, + expected_code="oauth_account_already_linked", + expected_provider="steam", + ) # No new row was created; the Steam ID remains tied to the original user. rows = ( @@ -333,7 +352,7 @@ async def test_relinking_same_provider_identity_is_noop( class TestStateValidation: - async def test_missing_state_returns_400( + async def test_missing_state_redirects_to_profile_with_error( self, client: AsyncClient, linked_user: User, @@ -345,10 +364,11 @@ async def test_missing_state_returns_400( resp = await client.get("/auth/steam/associate/callback", follow_redirects=False) finally: _clear_user_override() - assert resp.status_code == 400, resp.text - assert resp.json()["detail"]["code"] == "oauth_state_missing" + _assert_associate_error( + resp, expected_code="oauth_state_missing", expected_provider="steam" + ) - async def test_purpose_mismatch_returns_400( + async def test_purpose_mismatch_redirects_to_profile_with_error( self, client: AsyncClient, linked_user: User, @@ -367,10 +387,11 @@ async def test_purpose_mismatch_returns_400( ) finally: _clear_user_override() - assert resp.status_code == 400, resp.text - assert resp.json()["detail"]["code"] == "oauth_state_wrong_purpose" + _assert_associate_error( + resp, expected_code="oauth_state_wrong_purpose", expected_provider="steam" + ) - async def test_user_mismatch_returns_400( + async def test_user_mismatch_redirects_to_profile_with_error( self, client: AsyncClient, linked_user: User, @@ -392,10 +413,11 @@ async def test_user_mismatch_returns_400( ) finally: _clear_user_override() - assert resp.status_code == 400, resp.text - assert resp.json()["detail"]["code"] == "oauth_state_user_mismatch" + _assert_associate_error( + resp, expected_code="oauth_state_user_mismatch", expected_provider="steam" + ) - async def test_missing_csrf_cookie_returns_400( + async def test_missing_csrf_cookie_redirects_to_profile_with_error( self, client: AsyncClient, linked_user: User, @@ -413,8 +435,9 @@ async def test_missing_csrf_cookie_returns_400( ) finally: _clear_user_override() - assert resp.status_code == 400, resp.text - assert resp.json()["detail"]["code"] == "oauth_csrf_mismatch" + _assert_associate_error( + resp, expected_code="oauth_csrf_mismatch", expected_provider="steam" + ) # --- /auth/me/connections -------------------------------------------------