fix(auth): check OAuth state before trading the code

CHANGELOG.md

@@ -70,6 +70,9 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   longer than 100 characters, 400 for text holding a NUL character and for an id above
   2,147,483,647. `/v1/stats/series` with an `end` in the first years of the calendar and no
   `start` now starts the range at 0001-01-01 instead of failing.
+- The OAuth sign-in and account-link callbacks check `state` before they send the code to
+  GitHub or Google. A refused or reused code now answers 400 instead of 500, and an unreachable
+  provider answers 502.
 - Restoring a backup whose database roles no longer exist (a dump taken before a role rename)
   now works: the restore skips the dump's owners and grants, then hands ownership to the
   migration role and restores every grant.

backend/app/api/auth/services/oauth/base.py

@@ -6,11 +6,15 @@
 from urllib.parse import parse_qsl, urlencode, urlparse, urlunparse
 
 import jwt
-from fastapi import Request, Response
+from fastapi import Request, Response, status
 from fastapi.responses import RedirectResponse
 from fastapi_users.jwt import decode_jwt
-from httpx_oauth.integrations.fastapi import OAuth2AuthorizeCallback
-from httpx_oauth.oauth2 import BaseOAuth2  # noqa: TC002 # Used at runtime for FastAPI validation
+from httpx_oauth.integrations.fastapi import OAuth2AuthorizeCallbackError
+from httpx_oauth.oauth2 import (  # Used at runtime for FastAPI validation
+    BaseOAuth2,
+    GetAccessTokenError,
+    OAuth2Token,
+)
 
 from app.api.auth.config import normalize_oauth_redirect_uri, settings
 from app.api.auth.exceptions import (
@@ -37,11 +41,15 @@
 )
 
 if TYPE_CHECKING:
+    from collections.abc import Awaitable, Callable
+
     from fastapi_users.jwt import SecretType
 
 OAuthRedirectStatus = Literal["success", "error", "mfa_required"]
 OAUTH_STATUS_ERROR = "error"
 OAUTH_STATUS_MFA_REQUIRED = "mfa_required"
+OAUTH_CODE_REJECTED = "OAUTH_CODE_REJECTED"
+OAUTH_PROVIDER_UNAVAILABLE = "OAUTH_PROVIDER_UNAVAILABLE"
 
 
 @dataclass(frozen=True, slots=True)
@@ -55,11 +63,46 @@ class OAuthFlowConfig:
     cookie_settings: OAuthCookieSettings = field(default_factory=OAuthCookieSettings)
 
 
-def authorize_callback_dependency(config: OAuthFlowConfig, callback_route_name: str) -> OAuth2AuthorizeCallback:
-    """Build the httpx-oauth callback dependency for this route."""
-    if config.redirect_url is not None:
-        return OAuth2AuthorizeCallback(config.oauth_client, redirect_url=config.redirect_url)
-    return OAuth2AuthorizeCallback(config.oauth_client, route_name=callback_route_name)
+def authorize_callback_dependency(
+    config: OAuthFlowConfig, callback_route_name: str
+) -> Callable[..., Awaitable[tuple[OAuth2Token, str]]]:
+    """Build the callback dependency: verify `state`, then trade `code` with the provider.
+
+    httpx-oauth's own dependency trades the code first, so a made-up code reached the
+    provider, and the provider's refusal came back as a 500.
+    """
+
+    async def authorize_callback(
+        request: Request,
+        code: str | None = None,
+        code_verifier: str | None = None,
+        state: str | None = None,
+        error: str | None = None,
+    ) -> tuple[OAuth2Token, str]:
+        if code is None or error is not None:
+            raise OAuth2AuthorizeCallbackError(status_code=status.HTTP_400_BAD_REQUEST, detail=error)
+        if state is None:
+            raise OAuthInvalidStateError
+        verify_oauth_state(config, request, state)
+
+        redirect_url = config.redirect_url or str(request.url_for(callback_route_name))
+        try:
+            token = await config.oauth_client.get_access_token(code, redirect_url, code_verifier)
+        except GetAccessTokenError as err:
+            # Only a 4xx from the provider means a bad code; a timeout or provider 5xx is not the caller's fault.
+            if err.response is None or not err.response.is_client_error:
+                raise OAuth2AuthorizeCallbackError(
+                    status_code=status.HTTP_502_BAD_GATEWAY, detail=OAUTH_PROVIDER_UNAVAILABLE
+                ) from err
+            raise OAuth2AuthorizeCallbackError(
+                status_code=status.HTTP_400_BAD_REQUEST, detail=OAUTH_CODE_REJECTED, response=err.response
+            ) from err
+        # GitHub refuses a code with a 200 and an `error` body.
+        if ACCESS_TOKEN_KEY not in token:
+            raise OAuth2AuthorizeCallbackError(status_code=status.HTTP_400_BAD_REQUEST, detail=OAUTH_CODE_REJECTED)
+        return token, state
+
+    return authorize_callback
 
 
 def build_state_data(

backend/tests/conftest.py

@@ -131,6 +131,9 @@ def _ensure_testcontainers_postgres() -> None:
         username="postgres",
         password="postgres",  # Test-password only
         dbname="postgres",
+        # docker-py gives up on an API call after 60 s, and a loaded Docker daemon can take
+        # longer than that to create a container with a published port.
+        docker_client_kw={"timeout": 300},
     )
     _postgres_container.start()
 

backend/tests/integration/api/test_schemathesis.py

@@ -45,14 +45,9 @@ def public_schema(test_app: FastAPI) -> BaseSchema:
     return schemathesis.openapi.from_dict(build_public_openapi(test_app))
 
 
-# TODO: the OAuth callbacks trade any `code` with the real provider before checking
-# `state`, and httpx-oauth answers 500 when the provider refuses it. Fuzz them once they
-# check `state` first; until then they would also call github.com and google.com.
-schema = (
-    schemathesis.pytest.from_fixture("public_schema")
-    .include(method="GET")
-    .exclude(path_regex=r"^/v1/oauth/[^/]+/(token|session|associate)/callback$")
-)
+# The OAuth callbacks are fuzzed too: a fuzzed `state` never verifies, so they answer 400
+# before any `code` reaches github.com or google.com.
+schema = schemathesis.pytest.from_fixture("public_schema").include(method="GET")
 
 
 # as_transport_kwargs serializes for requests; these are the keys httpx's request() shares.

backend/tests/unit/auth/test_oauth_callback.py

@@ -0,0 +1,111 @@
+"""Unit tests for the OAuth callback dependency."""
+
+from unittest.mock import AsyncMock, MagicMock
+
+import httpx
+import pytest
+from httpx_oauth.integrations.fastapi import OAuth2AuthorizeCallbackError
+from httpx_oauth.oauth2 import GetAccessTokenError
+
+from app.api.auth.exceptions import OAuthInvalidStateError, OAuthStateDecodeError
+from app.api.auth.services.oauth.base import OAuthFlowConfig, authorize_callback_dependency
+from app.api.auth.services.oauth.utils import (
+    CSRF_TOKEN_KEY,
+    OAUTH_FLOW_KEY,
+    OAUTH_PROVIDER_KEY,
+    generate_csrf_token,
+    generate_state_token,
+)
+
+STATE_SECRET = "test-state-secret-at-least-32-bytes-long-for-hmac-sha256"
+REDIRECT_URL = "https://api.example.com/v1/oauth/github/session/callback"
+
+
+def _config() -> tuple[OAuthFlowConfig, MagicMock]:
+    client = MagicMock()
+    client.name = "github"
+    client.get_access_token = AsyncMock(return_value={"access_token": "provider-token"})
+    config = OAuthFlowConfig(
+        oauth_client=client, state_secret=STATE_SECRET, oauth_flow="github:session", redirect_url=REDIRECT_URL
+    )
+    return config, client
+
+
+def _request_with_state(config: OAuthFlowConfig) -> tuple[MagicMock, str]:
+    csrf_token = generate_csrf_token()
+    state = generate_state_token(
+        {CSRF_TOKEN_KEY: csrf_token, OAUTH_PROVIDER_KEY: "github", OAUTH_FLOW_KEY: config.oauth_flow}, STATE_SECRET
+    )
+    request = MagicMock()
+    request.cookies = {config.cookie_settings.name: csrf_token}
+    return request, state
+
+
+async def test_trades_code_after_state_verifies() -> None:
+    """A verified state lets the code through to the provider."""
+    config, client = _config()
+    request, state = _request_with_state(config)
+
+    token, returned_state = await authorize_callback_dependency(config, "unused")(request, code="c", state=state)
+
+    assert token == {"access_token": "provider-token"}
+    assert returned_state == state
+    client.get_access_token.assert_awaited_once_with("c", REDIRECT_URL, None)
+
+
+@pytest.mark.parametrize(
+    ("state", "error"),
+    [(None, OAuthInvalidStateError), ("not-a-jwt", OAuthStateDecodeError)],
+)
+async def test_bad_state_never_reaches_provider(state: str | None, error: type[Exception]) -> None:
+    """A missing or forged state answers 400 without trading the code."""
+    config, client = _config()
+    request, _ = _request_with_state(config)
+
+    with pytest.raises(error):
+        await authorize_callback_dependency(config, "unused")(request, code="c", state=state)
+    client.get_access_token.assert_not_awaited()
+
+
+async def test_state_without_csrf_cookie_never_reaches_provider() -> None:
+    """A valid state from another browser answers 400 without trading the code."""
+    config, client = _config()
+    request, state = _request_with_state(config)
+    request.cookies = {}
+
+    with pytest.raises(OAuthInvalidStateError):
+        await authorize_callback_dependency(config, "unused")(request, code="c", state=state)
+    client.get_access_token.assert_not_awaited()
+
+
+@pytest.mark.parametrize(
+    ("exchange", "status_code"),
+    [
+        # Google refuses with a 400, GitHub with a 200 and an `error` body.
+        ({"side_effect": GetAccessTokenError("bad code", httpx.Response(400))}, 400),
+        ({"return_value": {"error": "bad_verification_code"}}, 400),
+        ({"side_effect": GetAccessTokenError("timed out")}, 502),
+        ({"side_effect": GetAccessTokenError("down", httpx.Response(503))}, 502),
+    ],
+)
+async def test_failed_exchange_is_no_500(exchange: dict[str, object], status_code: int) -> None:
+    """A refused code is the caller's fault (400); an unreachable provider is a 502."""
+    config, client = _config()
+    request, state = _request_with_state(config)
+    client.get_access_token.configure_mock(**exchange)
+
+    with pytest.raises(OAuth2AuthorizeCallbackError) as exc_info:
+        await authorize_callback_dependency(config, "unused")(request, code="c", state=state)
+    assert exc_info.value.status_code == status_code
+
+
+@pytest.mark.parametrize(("code", "error"), [(None, None), ("c", "access_denied")])
+async def test_missing_code_or_provider_error_answers_400(code: str | None, error: str | None) -> None:
+    """No code, or an error from the provider's consent screen, answers 400."""
+    config, client = _config()
+    request, state = _request_with_state(config)
+
+    with pytest.raises(OAuth2AuthorizeCallbackError) as exc_info:
+        await authorize_callback_dependency(config, "unused")(request, code=code, state=state, error=error)
+    assert exc_info.value.status_code == 400
+    client.get_access_token.assert_not_awaited()