diff --git a/CHANGELOG.md b/CHANGELOG.md index f04137ec7..2420b4a71 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/backend/app/api/auth/services/oauth/base.py b/backend/app/api/auth/services/oauth/base.py index e28e17cfe..256223a84 100644 --- a/backend/app/api/auth/services/oauth/base.py +++ b/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( diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 7c61eac32..84d7e5c14 100644 --- a/backend/tests/conftest.py +++ b/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() diff --git a/backend/tests/integration/api/test_schemathesis.py b/backend/tests/integration/api/test_schemathesis.py index a943d5010..0b7f20e11 100644 --- a/backend/tests/integration/api/test_schemathesis.py +++ b/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. diff --git a/backend/tests/unit/auth/test_oauth_callback.py b/backend/tests/unit/auth/test_oauth_callback.py new file mode 100644 index 000000000..d8884e042 --- /dev/null +++ b/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()