From 66508d7522015ab6582105130da691e2803eba0d Mon Sep 17 00:00:00 2001 From: Hyeonsang Cho Date: Thu, 22 Oct 2026 20:35:43 +0900 Subject: [PATCH] fix(auth): map malformed OIDC provider bodies to OIDCError (#7258) * fix(auth): map malformed OIDC provider bodies to OIDCError The SSO login route answers OIDCError with 502 and the callback with an sso_failed redirect, but oidc.py raised OIDCError only for transport and HTTP status errors. A provider answering 200 with an HTML page or any non-object JSON raised JSONDecodeError, KeyError or AttributeError instead, so both routes failed with a 500. A malformed UserInfo body also aborted the login, although UserInfo is best-effort. Decode discovery, token, JWKS and UserInfo bodies through one helper that requires a JSON object, require string endpoints and issuer, reject a non-list JWKS keys field, skip non-object JWK entries, and wrap a malformed ID token header as OIDCValidationError. Build discovery metadata before caching it so a rejected document is not served from the cache. * docs(changelog): reference #7258 for the OIDC malformed body fix * fix(auth): report a falsy non-string OIDC issuer as wrong-typed Check the issuer's type before its presence so 0, false and [] get the non-string message instead of "missing the issuer field"; an absent or empty issuer still reports missing. --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 10 ++ CHANGELOG_zh.md | 8 ++ backend/app/gateway/AGENTS.md | 1 + backend/app/gateway/auth/oidc.py | 60 +++++++-- backend/docs/SSO.md | 11 ++ backend/tests/test_oidc_auth.py | 219 +++++++++++++++++++++++++++++++ 6 files changed, 299 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2017c5dace..588d4578c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1397,6 +1397,15 @@ As of 2026-10-21, the milestone includes **829 merged pull requests**. ### Fixed +- **auth:** SSO login and callback no longer fail with a `500` when the OIDC + provider answers `200` with a body that is not the expected JSON object, such + as a maintenance HTML page. Discovery, token, JWKS and UserInfo bodies, a + malformed ID token header and a discovery document without a usable endpoint + now take the existing `502` / `/login?error=sso_failed` path, and a rejected + discovery document is no longer cached. A malformed UserInfo body is skipped + like a failed UserInfo request instead of aborting a login whose ID token is + valid. ([#7258]) + - **mcp:** MCP servers using the OAuth `refresh_token` grant keep working across Gateway restarts, tool-cache resets, repeated personal-connection runs and multiple Gateway instances when the provider rotates refresh tokens @@ -13004,3 +13013,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#7207]: https://github.com/bytedance/deer-flow/pull/7207 [#7217]: https://github.com/bytedance/deer-flow/pull/7217 [#7255]: https://github.com/bytedance/deer-flow/pull/7255 +[#7258]: https://github.com/bytedance/deer-flow/pull/7258 diff --git a/CHANGELOG_zh.md b/CHANGELOG_zh.md index 733bb1e555..d06035d15a 100644 --- a/CHANGELOG_zh.md +++ b/CHANGELOG_zh.md @@ -1112,6 +1112,13 @@ ### 修复 +- **认证：** OIDC 提供方以 `200` 返回非预期 JSON 对象的响应体（例如维护中的 HTML + 页面）时，SSO 登录与回调不再以 `500` 失败。discovery、token、JWKS 与 UserInfo + 响应体、格式错误的 ID token 头以及缺少可用端点的 discovery 文档，现在都走既有的 + `502` / `/login?error=sso_failed` 路径，被拒绝的 discovery 文档也不再被缓存。 + 格式错误的 UserInfo 响应体会像 UserInfo 请求失败一样被跳过，不再让 ID token + 有效的登录中断。([#7258]) + - **MCP：** 使用 OAuth `refresh_token` 授权的 MCP 服务器，在提供方轮换 refresh token （Auth0、Okta、Google）时，跨 Gateway 重启、工具缓存重置、个人连接的多次运行以及多个 Gateway 实例都能继续工作。此前令牌只保存在进程内存中，每个新进程或新 manager 都从配置里 @@ -10380,3 +10387,4 @@ DeerFlow 2.0 是围绕"超级智能体"框架的彻底重写，核心包含子 [#7207]: https://github.com/bytedance/deer-flow/pull/7207 [#7217]: https://github.com/bytedance/deer-flow/pull/7217 [#7255]: https://github.com/bytedance/deer-flow/pull/7255 +[#7258]: https://github.com/bytedance/deer-flow/pull/7258 diff --git a/backend/app/gateway/AGENTS.md b/backend/app/gateway/AGENTS.md index cf907a5bb1..2da931f5ac 100644 --- a/backend/app/gateway/AGENTS.md +++ b/backend/app/gateway/AGENTS.md @@ -69,6 +69,7 @@ and OIDC callbacks. A small `HttpOnly` preference cookie preserves the choice. Logout clears auth cookies and suppresses CSRF re-issue. OIDC redirects retain existing endpoint query bytes (RFC 6749 section 3.1). +Malformed IdP bodies raise `OIDCError` (502/`sso_failed`). `tests/test_oidc_auth.py` pins query, state-cookie, nonce and PKCE agreement. Personal Access Tokens (`app.gateway.auth.pat`, `Authorization: Bearer dfp_...`) run as their owning user: an invalid Bearer is a hard 401 with no cookie fallback, which keeps `CSRFMiddleware`'s Bearer skip safe (origin checks still run). Scopes narrow within the allowlisted threads/runs/projects routes (including `POST /api/threads/{id}/move`); every other authenticated route 403s PAT callers (admin included). PAT management and `/change-password` require session auth; only SHA-256 digests are stored (`0017`). diff --git a/backend/app/gateway/auth/oidc.py b/backend/app/gateway/auth/oidc.py index 626a2f3442..ae32f94609 100644 --- a/backend/app/gateway/auth/oidc.py +++ b/backend/app/gateway/auth/oidc.py @@ -66,6 +66,22 @@ class OIDCUserInfoMismatch(OIDCError): """UserInfo sub does not match ID token sub.""" +def _json_object(resp: httpx.Response, what: str) -> dict[str, Any]: + """Decode a provider response body that must be a JSON object. + + A provider in maintenance can answer 200 with an HTML page, and a broken + one with any JSON value. Both become ``OIDCError`` so routes keep their + 502 / ``sso_failed`` contract instead of failing with a raw 500. + """ + try: + data = resp.json() + except ValueError as exc: + raise OIDCError(f"{what} is not valid JSON") from exc + if not isinstance(data, dict): + raise OIDCError(f"{what} is not a JSON object") + return data + + # ── Service ──────────────────────────────────────────────────────────────── @@ -109,13 +125,17 @@ class OIDCService: try: resp = await self._http.get(discovery_url) resp.raise_for_status() - data: dict[str, Any] = resp.json() except httpx.HTTPStatusError as exc: raise OIDCError(f"OIDC discovery failed for issuer {issuer}: HTTP {exc.response.status_code}") from exc except httpx.RequestError as exc: raise OIDCError(f"OIDC discovery failed for issuer {issuer}: {exc}") from exc + data = _json_object(resp, f"OIDC discovery response from {issuer}") discovered_issuer = data.get("issuer") + # Type before presence: a falsy non-string (0, false, []) is a + # wrong-typed field, not a missing one. + if discovered_issuer is not None and not isinstance(discovered_issuer, str): + raise OIDCError(f"OIDC discovery response from {issuer} has a non-string issuer field") if not discovered_issuer: raise OIDCError(f"OIDC discovery response from {issuer} is missing the issuer field") @@ -126,18 +146,30 @@ class OIDCService: if discovered_issuer.rstrip("/") != issuer.rstrip("/"): raise OIDCError(f"OIDC discovered issuer '{discovered_issuer}' does not match configured issuer '{issuer}'") + # Build before caching so a document missing an endpoint is not + # served from the cache for the rest of the TTL. + metadata = self._metadata_from_dict(data, overrides) self._metadata_cache[issuer] = (now, data) - return self._metadata_from_dict(data, overrides) + return metadata def _metadata_from_dict(self, data: dict[str, Any], overrides: dict[str, str | None] | None) -> OIDCMetadata: """Build OIDCMetadata from a discovery dict, applying endpoint overrides.""" overrides = overrides or {} + + def endpoint(key: str, *, required: bool = True) -> str | None: + value = overrides.get(key) or data.get(key) + if not value and not required: + return None + if not isinstance(value, str) or not value: + raise OIDCError(f"OIDC discovery metadata has no usable {key}") + return value + return OIDCMetadata( issuer=data["issuer"], - authorization_endpoint=overrides.get("authorization_endpoint") or data["authorization_endpoint"], - token_endpoint=overrides.get("token_endpoint") or data["token_endpoint"], - userinfo_endpoint=overrides.get("userinfo_endpoint") or data.get("userinfo_endpoint"), - jwks_uri=overrides.get("jwks_uri") or data["jwks_uri"], + authorization_endpoint=endpoint("authorization_endpoint"), + token_endpoint=endpoint("token_endpoint"), + userinfo_endpoint=endpoint("userinfo_endpoint", required=False), + jwks_uri=endpoint("jwks_uri"), ) # ── Authorization URL ────────────────────────────────────────────────── @@ -209,7 +241,6 @@ class OIDCService: try: resp = await self._http.post(metadata.token_endpoint, data=data, headers=headers) resp.raise_for_status() - return resp.json() except httpx.HTTPStatusError as exc: body = "unknown" try: @@ -219,6 +250,7 @@ class OIDCService: raise OIDCError(f"Token exchange failed: HTTP {exc.response.status_code} — {body}") from exc except httpx.RequestError as exc: raise OIDCError(f"Token exchange failed: {exc}") from exc + return _json_object(resp, "Token response") # ── JWKS loading ─────────────────────────────────────────────────────── @@ -235,11 +267,13 @@ class OIDCService: try: resp = await self._http.get(jwks_uri) resp.raise_for_status() - data: dict[str, Any] = resp.json() except httpx.HTTPStatusError as exc: raise OIDCError(f"JWKS fetch failed: HTTP {exc.response.status_code}") from exc except httpx.RequestError as exc: raise OIDCError(f"JWKS fetch failed: {exc}") from exc + data = _json_object(resp, "JWKS response") + if not isinstance(data.get("keys", []), list): + raise OIDCError("JWKS response keys field is not a list") self._jwks_cache[jwks_uri] = (now, data) return data @@ -258,6 +292,9 @@ class OIDCService: logs a warning so a single bad entry does not crash validation. """ for jwk_dict in jwks_data.get("keys", []): + if not isinstance(jwk_dict, dict): + logger.warning("Skipping non-object JWK entry from %s", jwks_uri) + continue if kid and jwk_dict.get("kid") != kid: continue try: @@ -289,7 +326,10 @@ class OIDCService: jwks_data = await self._load_jwks(metadata.jwks_uri) # Resolve the signing key from the JWKS using the token's kid header - jwt_header = jwt.get_unverified_header(id_token) + try: + jwt_header = jwt.get_unverified_header(id_token) + except jwt.PyJWTError as exc: + raise OIDCValidationError(f"ID token is malformed: {exc}") from exc kid = jwt_header.get("kid") alg = jwt_header.get("alg", "RS256") @@ -361,11 +401,11 @@ class OIDCService: try: resp = await self._http.get(metadata.userinfo_endpoint, headers=headers) resp.raise_for_status() - userinfo: dict[str, Any] = resp.json() except httpx.HTTPStatusError as exc: raise OIDCError(f"UserInfo fetch failed: HTTP {exc.response.status_code}") from exc except httpx.RequestError as exc: raise OIDCError(f"UserInfo fetch failed: {exc}") from exc + userinfo = _json_object(resp, "UserInfo response") if userinfo.get("sub") and userinfo["sub"] != expected_sub: raise OIDCUserInfoMismatch("UserInfo sub does not match ID token sub") diff --git a/backend/docs/SSO.md b/backend/docs/SSO.md index 7040afb142..26f711e52f 100644 --- a/backend/docs/SSO.md +++ b/backend/docs/SSO.md @@ -271,6 +271,17 @@ The backend also rejects password change requests for OAuth users: | `GET /api/v1/auth/oauth/{provider}` | Initiates SSO login, redirects to the OIDC provider | | `GET /api/v1/auth/callback/{provider}` | OIDC callback — exchanges code, creates session, redirects to frontend | +### Provider failures + +An unreachable provider, an HTTP error, and a response body that is not the +expected JSON object (for example a `200` maintenance page) all fail the same +way. `GET /api/v1/auth/oauth/{provider}` answers `502`, and the callback +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. + ## Frontend Callback Flow The frontend handles the post-SSO flow at `/auth/callback`: diff --git a/backend/tests/test_oidc_auth.py b/backend/tests/test_oidc_auth.py index 4195a720d0..22282bd5d4 100644 --- a/backend/tests/test_oidc_auth.py +++ b/backend/tests/test_oidc_auth.py @@ -1,11 +1,14 @@ +import asyncio import time from unittest.mock import AsyncMock from urllib.parse import parse_qs, urlsplit +import httpx import jwt import pytest from cryptography.hazmat.primitives.asymmetric import rsa 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 @@ -605,3 +608,219 @@ async def test_id_token_nonce_claim_of_non_string_type_is_rejected_not_an_attrib with pytest.raises(OIDCValidationError, match="nonce claim is not a string"): await service.validate_id_token(metadata=_metadata(), client_id="deer-flow", id_token=_id_token(private_key, 12345), nonce="expected") + + +# ── Malformed provider bodies ───────────────────────────────────────────── +# +# A provider in maintenance can answer 200 with an HTML page, and a broken one +# with any JSON value. Every such body must surface as OIDCError so the login +# route keeps its 502 and the callback its sso_failed redirect; a raw +# JSONDecodeError / KeyError / AttributeError turned both into a 500. + +_ISSUER = "https://issuer.example.com" +_DISCOVERY = { + "issuer": _ISSUER, + "authorization_endpoint": f"{_ISSUER}/auth", + "token_endpoint": f"{_ISSUER}/token", + "userinfo_endpoint": f"{_ISSUER}/userinfo", + "jwks_uri": f"{_ISSUER}/jwks", +} +_HTML_PAGE = httpx.Response(200, text="Down for maintenance", headers={"content-type": "text/html"}) + + +def _service_answering(responses: dict[str, object]) -> OIDCService: + """OIDCService whose HTTP client answers each URL path from ``responses``. + + A value is either a ready ``httpx.Response`` or a JSON body served as 200. + """ + service = OIDCService() + + def handler(request: httpx.Request) -> httpx.Response: + body = responses[request.url.path] + return body if isinstance(body, httpx.Response) else httpx.Response(200, json=body) + + service._http = httpx.AsyncClient(transport=httpx.MockTransport(handler)) + return service + + +def _signed_provider() -> tuple[str, dict]: + """Return an ID token for ``_ISSUER`` and the JWKS that verifies it.""" + 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}, + private_key, + algorithm="RS256", + headers={"kid": "k1"}, + ) + jwk = RSAAlgorithm.to_jwk(private_key.public_key(), as_dict=True) + return id_token, {"keys": [{**jwk, "kid": "k1"}]} + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "document", + [ + _HTML_PAGE, + ["not", "an", "object"], + {key: value for key, value in _DISCOVERY.items() if key != "authorization_endpoint"}, + {**_DISCOVERY, "issuer": 42}, + {**_DISCOVERY, "jwks_uri": ["https://issuer.example.com/jwks"]}, + {**_DISCOVERY, "userinfo_endpoint": 42}, + ], + ids=["html", "json-list", "missing-endpoint", "non-string-issuer", "non-string-endpoint", "non-string-userinfo"], +) +async def test_oidc_discover_wraps_malformed_document_and_does_not_cache_it(document): + service = _service_answering({"/.well-known/openid-configuration": document}) + try: + with pytest.raises(OIDCError): + await service.discover(_ISSUER) + assert service._metadata_cache == {} + finally: + await service.close() + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("discovered_issuer", "message"), + [ + (42, "non-string issuer field"), + (0, "non-string issuer field"), + (False, "non-string issuer field"), + ([], "non-string issuer field"), + ("", "missing the issuer field"), + (None, "missing the issuer field"), + ], +) +async def test_oidc_discover_reports_wrong_typed_issuer_apart_from_missing_one(discovered_issuer, message): + document = {key: value for key, value in _DISCOVERY.items() if key != "issuer"} + if discovered_issuer is not None: + document["issuer"] = discovered_issuer + service = _service_answering({"/.well-known/openid-configuration": document}) + try: + with pytest.raises(OIDCError, match=message): + await service.discover(_ISSUER) + finally: + await service.close() + + +@pytest.mark.asyncio +async def test_oidc_discover_override_fills_an_endpoint_the_document_omits(): + service = _service_answering({"/.well-known/openid-configuration": {key: value for key, value in _DISCOVERY.items() if key != "authorization_endpoint"}}) + try: + metadata = await service.discover(_ISSUER, {"authorization_endpoint": f"{_ISSUER}/custom-auth"}) + finally: + await service.close() + + assert metadata.authorization_endpoint == f"{_ISSUER}/custom-auth" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("body", [_HTML_PAGE, ["id_token"]], ids=["html", "json-list"]) +async def test_oidc_exchange_code_wraps_malformed_token_response(body): + service = _service_answering({"/token": body}) + try: + with pytest.raises(OIDCError, match="^Token response is not"): + await service.exchange_code(_metadata(), "deer-flow", "secret", "code", "https://app.example.com/callback") + finally: + await service.close() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("body", [_HTML_PAGE, [{"kty": "RSA"}], {"keys": "k1"}], ids=["html", "json-list", "keys-not-a-list"]) +async def test_oidc_load_jwks_wraps_malformed_jwks_and_does_not_cache_it(body): + service = _service_answering({"/jwks": body}) + try: + with pytest.raises(OIDCError, match="^JWKS response"): + await service._load_jwks(f"{_ISSUER}/jwks") + assert service._jwks_cache == {} + finally: + await service.close() + + +@pytest.mark.asyncio +async def test_oidc_signing_key_lookup_skips_non_object_jwks_entries(): + id_token, jwks = _signed_provider() + service = _service_answering({"/jwks": {"keys": ["junk", 42, *jwks["keys"]]}}) + try: + claims = await service.validate_id_token(_metadata(), "deer-flow", id_token) + finally: + await service.close() + + assert claims["sub"] == "s1" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("id_token", ["not-a-jwt", 12345], ids=["garbage", "non-string"]) +async def test_oidc_validate_id_token_rejects_malformed_token_header(id_token): + _, jwks = _signed_provider() + service = _service_answering({"/jwks": jwks}) + try: + with pytest.raises(OIDCValidationError, match="^ID token is malformed"): + await service.validate_id_token(_metadata(), "deer-flow", id_token) + finally: + await service.close() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("body", [_HTML_PAGE, ["sub"]], ids=["html", "json-list"]) +async def test_oidc_callback_falls_back_to_id_token_when_userinfo_body_is_malformed(body): + """UserInfo is best-effort: a malformed body must not abort the login.""" + id_token, jwks = _signed_provider() + service = _service_answering({"/token": {"id_token": id_token, "access_token": "access"}, "/jwks": jwks, "/userinfo": body}) + metadata = OIDCMetadata(**_DISCOVERY) + try: + identity = await service.authenticate_callback("keycloak", metadata, "deer-flow", "secret", "code", "https://app.example.com/callback") + finally: + await service.close() + + assert identity.subject == "s1" + assert identity.email == "user@example.com" + + +def _oidc_router_client(monkeypatch, service: OIDCService): + from types import SimpleNamespace + + from fastapi import FastAPI + from fastapi.testclient import TestClient + + import deerflow.config.app_config as app_config_module + from app.gateway.auth import oidc_state + from app.gateway.auth.config import AuthConfig + from app.gateway.routers import auth as auth_router + + oidc_config = SimpleNamespace(enabled=True, providers={"keycloak": _provider_config()}, frontend_base_url="") + monkeypatch.setattr(app_config_module, "get_app_config", lambda: SimpleNamespace(auth=SimpleNamespace(oidc=oidc_config))) + monkeypatch.setattr(auth_router, "_get_oidc_service", lambda: service) + monkeypatch.setattr(oidc_state, "get_auth_config", lambda: AuthConfig(jwt_secret="test-secret-for-oidc-malformed-bodies-min-32")) + app = FastAPI() + app.include_router(auth_router.router) + return TestClient(app, follow_redirects=False) + + +def test_oidc_login_returns_502_when_discovery_is_an_html_page(monkeypatch): + service = _service_answering({"/.well-known/openid-configuration": _HTML_PAGE}) + client = _oidc_router_client(monkeypatch, service) + try: + response = client.get("/api/v1/auth/oauth/keycloak") + finally: + asyncio.run(service.close()) + + assert response.status_code == 502 + assert response.json() == {"detail": "Failed to connect to SSO provider"} + + +def test_oidc_callback_redirects_sso_failed_when_token_response_is_an_html_page(monkeypatch): + from app.gateway.auth.oidc_state import OIDCStatePayload, _sign_state_payload + + service = _service_answering({"/.well-known/openid-configuration": _DISCOVERY, "/token": _HTML_PAGE}) + client = _oidc_router_client(monkeypatch, service) + client.cookies.set("df_oidc_state_keycloak", _sign_state_payload(OIDCStatePayload(provider="keycloak", state="expected-state"))) + + try: + response = client.get("/api/v1/auth/callback/keycloak", params={"code": "code", "state": "expected-state"}) + finally: + asyncio.run(service.close()) + + assert response.status_code == 302 + assert response.headers["location"] == "/login?error=sso_failed"