diff --git a/backend/app/gateway/auth/oidc.py b/backend/app/gateway/auth/oidc.py index ae32f94609..1c04e88462 100644 --- a/backend/app/gateway/auth/oidc.py +++ b/backend/app/gateway/auth/oidc.py @@ -63,7 +63,7 @@ class OIDCValidationError(OIDCError): class OIDCUserInfoMismatch(OIDCError): - """UserInfo sub does not match ID token sub.""" + """UserInfo sub is missing or does not match ID token sub.""" def _json_object(resp: httpx.Response, what: str) -> dict[str, Any]: @@ -391,8 +391,10 @@ class OIDCService: async def fetch_userinfo(self, metadata: OIDCMetadata, access_token: str, expected_sub: str) -> dict[str, Any]: """Fetch userinfo from the UserInfo endpoint. - Validates that the ``sub`` claim matches ``expected_sub`` - (from the ID token) to prevent userinfo injection. + The response is returned only when its ``sub`` equals ``expected_sub`` + (from the ID token). OIDC Core 5.3.2 requires that check, and a + response without a ``sub`` cannot be tied to this login, so it raises + ``OIDCUserInfoMismatch`` too instead of overriding the ID token claims. """ if not metadata.userinfo_endpoint: return {} @@ -407,7 +409,12 @@ class OIDCService: raise OIDCError(f"UserInfo fetch failed: {exc}") from exc userinfo = _json_object(resp, "UserInfo response") - if userinfo.get("sub") and userinfo["sub"] != expected_sub: + sub = userinfo.get("sub") + if sub is None or sub == "": + raise OIDCUserInfoMismatch("UserInfo response has no sub claim") + if not isinstance(sub, str): + raise OIDCUserInfoMismatch(f"UserInfo sub claim is not a string ({type(sub).__name__})") + if sub != expected_sub: raise OIDCUserInfoMismatch("UserInfo sub does not match ID token sub") return userinfo diff --git a/backend/docs/SSO.md b/backend/docs/SSO.md index 26f711e52f..4a056feb64 100644 --- a/backend/docs/SSO.md +++ b/backend/docs/SSO.md @@ -280,7 +280,11 @@ redirects to `/login?error=sso_failed`. A discovery document without a usable authorization, token or JWKS endpoint counts as failed unless the provider config overrides that endpoint, and it is not cached. A malformed UserInfo body is skipped like a failed UserInfo request: the login continues with the ID -token claims. The cause stays in the Gateway log. +token claims. The cause stays in the Gateway log. A UserInfo response whose +`sub` is missing or differs from the ID token's is skipped the same way, since +OIDC Core requires the two to match. With a provider that omits `sub` from +UserInfo and sends the email only there, a first login gets `403` because no +email reaches provisioning. Users already linked by subject still sign in. ## Frontend Callback Flow @@ -296,7 +300,7 @@ The frontend handles the post-SSO flow at `/auth/callback`: - **State cookies** are HttpOnly, SameSite=Lax, Max-Age=300 seconds, and signed with the JWT secret - **PKCE** prevents authorization code interception attacks - **Nonce** prevents ID token replay attacks -- **UserInfo sub check** ensures the UserInfo response matches the ID token subject +- **UserInfo sub check** — UserInfo claims are used only when the response's `sub` equals the ID token subject; a response without a `sub` counts as a mismatch - **Reject alg=none** — ID tokens with algorithm "none" are always rejected - **No email auto-linking** — SSO accounts are always separate from email/password accounts. An email collision with an existing local account blocks the SSO login (409) rather than merging the two. - **Verified email requirement** — SSO users must have verified emails by default diff --git a/backend/tests/test_oidc_auth.py b/backend/tests/test_oidc_auth.py index ba64029362..eedbcdda54 100644 --- a/backend/tests/test_oidc_auth.py +++ b/backend/tests/test_oidc_auth.py @@ -1,4 +1,5 @@ import asyncio +import re import time from unittest.mock import AsyncMock from urllib.parse import parse_qs, urlsplit @@ -11,7 +12,7 @@ from fastapi import HTTPException from jwt.algorithms import RSAAlgorithm from app.gateway.auth.models import User -from app.gateway.auth.oidc import OIDCError, OIDCIdentity, OIDCMetadata, OIDCService, OIDCValidationError +from app.gateway.auth.oidc import OIDCError, OIDCIdentity, OIDCMetadata, OIDCService, OIDCUserInfoMismatch, OIDCValidationError from app.gateway.auth.user_provisioning import get_or_provision_oidc_user from deerflow.config.auth_config import OIDCProviderConfig @@ -672,12 +673,15 @@ def _service_answering(responses: dict[str, object]) -> OIDCService: return service -def _signed_provider() -> tuple[str, dict]: - """Return an ID token for ``_ISSUER`` and the JWKS that verifies it.""" +def _signed_provider(**claims) -> tuple[str, dict]: + """Return an ID token for ``_ISSUER`` and the JWKS that verifies it. + + ``claims`` override the default ID token claims. + """ private_key = rsa.generate_private_key(public_exponent=65537, key_size=2048) now = int(time.time()) id_token = jwt.encode( - {"iss": _ISSUER, "aud": "deer-flow", "sub": "s1", "exp": now + 300, "iat": now, "email": "user@example.com", "email_verified": True}, + {"iss": _ISSUER, "aud": "deer-flow", "sub": "s1", "exp": now + 300, "iat": now, "email": "user@example.com", "email_verified": True, **claims}, private_key, algorithm="RS256", headers={"kid": "k1"}, @@ -807,6 +811,105 @@ async def test_oidc_callback_falls_back_to_id_token_when_userinfo_body_is_malfor assert identity.email == "user@example.com" +# ── UserInfo subject check ──────────────────────────────────────────────── +# +# OIDC Core 5.3.2: the UserInfo ``sub`` must exactly match the ID token's +# before any UserInfo claim is used. A response without a ``sub`` cannot be +# tied to this login, so it is rejected like a mismatching one; otherwise its +# email and email_verified replace the signed ID token's. + +# Each case carries the reason the Gateway logs, so an operator can tell an +# absent ``sub`` from a malformed one. +_UNUSABLE_SUB_CASES = [ + pytest.param({}, "UserInfo response has no sub claim", id="missing"), + pytest.param({"sub": ""}, "UserInfo response has no sub claim", id="empty"), + pytest.param({"sub": None}, "UserInfo response has no sub claim", id="null"), + pytest.param({"sub": ["s1"]}, "UserInfo sub claim is not a string (list)", id="non-string"), + pytest.param({"sub": 1}, "UserInfo sub claim is not a string (int)", id="number"), +] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("sub_field", "message"), + [ + *_UNUSABLE_SUB_CASES, + pytest.param({"sub": "s2"}, "does not match", id="other-subject"), + pytest.param({"sub": "S1"}, "does not match", id="other-case"), + ], +) +async def test_oidc_fetch_userinfo_rejects_a_response_not_bound_to_the_id_token(sub_field, message): + service = _service_answering({"/userinfo": {**sub_field, "email": "other@example.com", "email_verified": True}}) + try: + with pytest.raises(OIDCUserInfoMismatch, match=re.escape(message)): + await service.fetch_userinfo(OIDCMetadata(**_DISCOVERY), "access", expected_sub="s1") + finally: + await service.close() + + +@pytest.mark.asyncio +async def test_oidc_fetch_userinfo_returns_a_response_for_the_same_subject(): + body = {"sub": "s1", "email": "user@example.com", "name": "User"} + service = _service_answering({"/userinfo": body}) + try: + assert await service.fetch_userinfo(OIDCMetadata(**_DISCOVERY), "access", expected_sub="s1") == body + finally: + await service.close() + + +@pytest.mark.asyncio +@pytest.mark.parametrize(("sub_field", "reason"), _UNUSABLE_SUB_CASES) +async def test_oidc_callback_keeps_id_token_claims_when_userinfo_has_no_usable_sub(sub_field, reason, caplog): + id_token, jwks = _signed_provider(email="user@example.com", email_verified=False, name="ID Token User") + userinfo = {**sub_field, "email": "admin@example.com", "email_verified": True, "name": "Someone Else"} + service = _service_answering({"/token": {"id_token": id_token, "access_token": "access"}, "/jwks": jwks, "/userinfo": userinfo}) + try: + with caplog.at_level("WARNING", logger="app.gateway.auth.oidc"): + identity = await service.authenticate_callback("keycloak", OIDCMetadata(**_DISCOVERY), "deer-flow", "secret", "code", "https://app.example.com/callback") + finally: + await service.close() + + assert (identity.subject, identity.email, identity.email_verified, identity.name) == ("s1", "user@example.com", False, "ID Token User") + assert "admin@example.com" not in identity.claims.values() + assert reason in caplog.text + + +@pytest.mark.asyncio +async def test_oidc_callback_uses_userinfo_claims_for_the_same_subject(): + id_token, jwks = _signed_provider(email="user@example.com", email_verified=False) + userinfo = {"sub": "s1", "email": "user@corp.example", "email_verified": True, "name": "UserInfo User"} + service = _service_answering({"/token": {"id_token": id_token, "access_token": "access"}, "/jwks": jwks, "/userinfo": userinfo}) + try: + identity = await service.authenticate_callback("keycloak", OIDCMetadata(**_DISCOVERY), "deer-flow", "secret", "code", "https://app.example.com/callback") + finally: + await service.close() + + assert (identity.subject, identity.email, identity.email_verified, identity.name) == ("s1", "user@corp.example", True, "UserInfo User") + + +@pytest.mark.asyncio +async def test_oidc_userinfo_without_sub_cannot_make_a_first_login_an_admin(): + """End to end: a sub-less UserInfo email must not reach admin_emails.""" + id_token, jwks = _signed_provider(email="user@example.com", email_verified=True) + service = _service_answering({"/token": {"id_token": id_token, "access_token": "access"}, "/jwks": jwks, "/userinfo": {"email": "admin@example.com", "email_verified": True}}) + try: + identity = await service.authenticate_callback("keycloak", OIDCMetadata(**_DISCOVERY), "deer-flow", "secret", "code", "https://app.example.com/callback") + finally: + await service.close() + + local_provider = AsyncMock() + local_provider.get_user_by_oauth.return_value = None + local_provider.get_user_by_email.return_value = None + await get_or_provision_oidc_user( + provider_id="keycloak", + provider_config=_provider_config(admin_emails=["admin@example.com"]), + identity=identity, + local_provider=local_provider, + ) + + local_provider.create_oauth_user.assert_awaited_once_with(email="user@example.com", oauth_provider="keycloak", oauth_id="s1", system_role="user") + + def _oidc_router_client(monkeypatch, service: OIDCService): from types import SimpleNamespace