From 445f44c8d96d34a0a2876bc19242813d149b2871 Mon Sep 17 00:00:00 2001 From: Koman Rudden <1030623+KomanRudden@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:03:52 +0200 Subject: [PATCH 1/3] fix(auth): require OAuth callback state and validate ID token nonce handle_redirect only checked `state` when the callback carried one, so a callback with the state parameter stripped skipped validation entirely and went straight to token exchange (login CSRF / code injection). The nonce stored at login was also never compared with the ID token. - Always require state: reject the callback unless a state was stored for a login started by the SDK in this session and the request presents the same value. Callbacks with no login in progress (nothing stored) are rejected before token exchange, with or without a state parameter. - After token exchange, when a nonce was stored and the response has an ID token, reject a missing or mismatched `nonce` claim. - Consume stored state and nonce as soon as state validates, so they are single-use even if exchange or nonce validation fails. A rejected callback leaves the pending state in place, so a forged request cannot clear it to make a later callback pass. - Stop logging raw state values at warning level. Adds testv2/testv2_auth/test_oauth_callback_security.py covering the standalone, Flask and FastAPI callback paths. --- kinde_sdk/auth/oauth.py | 45 ++- .../test_oauth_callback_security.py | 335 ++++++++++++++++++ 2 files changed, 365 insertions(+), 15 deletions(-) create mode 100644 testv2/testv2_auth/test_oauth_callback_security.py diff --git a/kinde_sdk/auth/oauth.py b/kinde_sdk/auth/oauth.py index 0c2d19d1..97109ac7 100644 --- a/kinde_sdk/auth/oauth.py +++ b/kinde_sdk/auth/oauth.py @@ -2,6 +2,7 @@ import requests import logging import time +import jwt from typing import Any, Dict, Optional from urllib.parse import urlencode @@ -515,14 +516,24 @@ async def handle_redirect(self, code: str, user_id: str, state: Optional[str] = Returns: Dict with user and token information """ - # Verify state if provided - if state: - stored_state = self._session_manager.storage_manager.get("user:state") - self._logger.warning(f"stored_state: {stored_state}, state: {state}") - if not stored_state or state != stored_state.get("value"): - self._logger.error(f"State mismatch: received {state}, stored {stored_state}") - raise KindeLoginException("Invalid state parameter") - + storage = self._session_manager.storage_manager + + # Verify state. A callback is only accepted for a login started by this + # SDK in this session: a state must have been stored, and the callback + # must present the same value. Missing state is rejected, not skipped. + stored_state = storage.get("user:state") + expected_state = stored_state.get("value") if stored_state else None + if not expected_state or state != expected_state: + self._logger.error("State mismatch in OAuth callback") + raise KindeLoginException("Invalid state parameter") + + # State and nonce are single-use: consume them once state has been + # validated, so they can't be replayed even if the rest of the flow fails. + stored_nonce = storage.get("user:nonce") + expected_nonce = stored_nonce.get("value") if stored_nonce else None + storage.delete("user:state") + storage.delete("user:nonce") + # Get code verifier for PKCE code_verifier = None stored_code_verifier = self._session_manager.storage_manager.get("user:code_verifier") @@ -538,6 +549,17 @@ async def handle_redirect(self, code: str, user_id: str, state: Optional[str] = except Exception as e: self._logger.error(f"Token exchange failed: {str(e)}") raise KindeTokenException(f"Failed to exchange code for tokens: {str(e)}") from e + + # Verify the ID token nonce matches the one sent in the authorization request + id_token = token_data.get("id_token") + if expected_nonce and id_token: + try: + id_token_claims = jwt.decode(id_token, options={"verify_signature": False}) + except jwt.PyJWTError as e: + raise KindeLoginException("Invalid ID token") from e + if id_token_claims.get("nonce") != expected_nonce: + self._logger.error("Nonce mismatch in ID token") + raise KindeLoginException("Invalid nonce in ID token") # Store tokens user_info = { @@ -565,13 +587,6 @@ async def handle_redirect(self, code: str, user_id: str, state: Optional[str] = logger=self._logger ) - # Clean up state - if state: - self._session_manager.storage_manager.delete("user:state") - - # Clean up nonce - self._session_manager.storage_manager.delete("user:nonce") - return { "tokens": token_data, "user": user_details diff --git a/testv2/testv2_auth/test_oauth_callback_security.py b/testv2/testv2_auth/test_oauth_callback_security.py new file mode 100644 index 00000000..4db929b8 --- /dev/null +++ b/testv2/testv2_auth/test_oauth_callback_security.py @@ -0,0 +1,335 @@ +""" +Security regression tests for the OAuth callback (``OAuth.handle_redirect``). + +Covers: + * state is always required: a callback is rejected unless a login was + started by the SDK in this session (state stored) and the callback + presents the same state (login CSRF / authorization code injection), + * the ID token ``nonce`` claim must match the nonce sent in the + authorization request, + * stored state / nonce are single-use and cleared after the callback. + +The standalone tests use the real in-memory storage path (null framework); +the Flask / FastAPI tests drive the real ``/login`` and ``/callback`` routes. +Only the network calls (token endpoint, userinfo) are mocked. +""" +import asyncio +import tempfile +from unittest.mock import patch, MagicMock, AsyncMock +from urllib.parse import urlparse, parse_qs + +import jwt +import pytest + +from kinde_sdk.auth.oauth import OAuth +from kinde_sdk.core.exceptions import KindeLoginException +from kinde_sdk.core.framework.framework_factory import FrameworkFactory +from kinde_sdk.core.storage.storage_manager import StorageManager + +CLIENT_ID = "test_client_id" +REDIRECT_URI = "http://localhost/callback" + + +def _id_token(**claims): + payload = {"sub": "kp_user", "aud": CLIENT_ID, "iss": "https://example.kinde.com"} + payload.update(claims) + # Signature is irrelevant here: the SDK decodes the ID token without + # verifying the signature (token comes straight from the token endpoint). + return jwt.encode(payload, "test-secret-key-of-sufficient-length-123", algorithm="HS256") + + +def _token_response(id_token=None, include_id_token=True): + resp = MagicMock() + resp.status_code = 200 + body = { + "access_token": jwt.encode({"sub": "kp_user"}, "test-secret-key-of-sufficient-length-123", algorithm="HS256"), + "token_type": "bearer", + "expires_in": 3600, + } + if include_id_token: + body["id_token"] = id_token + resp.json.return_value = body + resp.text = "ok" + return resp + + +def _query(url): + return {k: v[0] for k, v in parse_qs(urlparse(url).query).items()} + + +@pytest.fixture +def isolated_singletons(): + """Isolate the process-wide StorageManager / FrameworkFactory singletons.""" + sm = StorageManager() + saved = (sm._storage, sm._device_id, sm._storage_type) + with patch.object(FrameworkFactory, "_framework_instance", None): + yield + sm._storage, sm._device_id, sm._storage_type = saved + + +@pytest.fixture +def standalone_oauth(isolated_singletons): + oauth = OAuth( + client_id=CLIENT_ID, + client_secret="test_client_secret", + redirect_uri=REDIRECT_URI, + host="https://example.kinde.com", + ) + return oauth + + +@pytest.fixture +def network(): + """Mock the token endpoint and userinfo lookup.""" + with patch("kinde_sdk.auth.oauth.requests.post") as post, \ + patch("kinde_sdk.auth.oauth.helper_get_user_details", new=AsyncMock(return_value={"id": "kp_user"})): + yield post + + +def _storage(): + return StorageManager() + + +# --------------------------------------------------------------------------- +# Standalone (null framework, real memory storage) +# --------------------------------------------------------------------------- + +class TestCallbackStateStandalone: + + def test_callback_without_state_rejected_when_state_stored(self, standalone_oauth, network): + """(a) A callback with no `state` param must not reach token exchange.""" + asyncio.run(standalone_oauth.login()) + assert _storage().get("user:state") is not None + + network.return_value = _token_response(_id_token(nonce="whatever")) + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("attacker_code", "victim", None)) + network.assert_not_called() + + def test_callback_with_empty_state_rejected_when_state_stored(self, standalone_oauth, network): + asyncio.run(standalone_oauth.login()) + network.return_value = _token_response(_id_token(nonce="whatever")) + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("attacker_code", "victim", "")) + network.assert_not_called() + + def test_callback_with_wrong_state_rejected(self, standalone_oauth, network): + asyncio.run(standalone_oauth.login()) + network.return_value = _token_response(_id_token(nonce="whatever")) + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("code", "victim", "not-the-state")) + network.assert_not_called() + + def test_state_is_single_use(self, standalone_oauth, network): + q = _query(asyncio.run(standalone_oauth.login())) + network.return_value = _token_response(_id_token(nonce=q["nonce"])) + asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) + assert _storage().get("user:state") is None + assert _storage().get("user:nonce") is None + assert _storage().get("user:code_verifier") is None + + network.reset_mock() + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) + network.assert_not_called() + + def test_forged_callback_cannot_clear_pending_state(self, standalone_oauth, network): + """A rejected callback must not consume the pending state; otherwise an + attacker could send one forged callback to clear it and a second, + state-less one that would then be accepted.""" + q = _query(asyncio.run(standalone_oauth.login())) + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("code", "victim", "not-the-state")) + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("attacker_code", "victim", None)) + network.assert_not_called() + + # The victim's genuine callback still completes. + network.return_value = _token_response(_id_token(nonce=q["nonce"])) + asyncio.run(standalone_oauth.handle_redirect("code", "victim", q["state"])) + assert network.call_count == 1 + assert _storage().get("user:state") is None + assert _storage().get("user:nonce") is None + + +class TestCallbackNoLoginInProgressStandalone: + """Nothing stored (no SDK-started login in this session / already logged in).""" + + @pytest.mark.parametrize("state", [None, "", "attacker-state"]) + def test_callback_rejected_when_no_login_in_progress(self, standalone_oauth, network, state): + assert _storage().get("user:state") is None + network.return_value = _token_response(_id_token(nonce="x")) + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("attacker_code", "victim", state)) + network.assert_not_called() + assert standalone_oauth._session_manager.get_token_manager("victim") is None + + def test_callback_rejected_after_completed_login(self, standalone_oauth, network): + """Once a login has completed, a further (forged) callback is rejected + and does not replace the established session.""" + q = _query(asyncio.run(standalone_oauth.login())) + network.return_value = _token_response(_id_token(nonce=q["nonce"])) + asyncio.run(standalone_oauth.handle_redirect("code", "victim", q["state"])) + network.reset_mock() + + for state in (None, q["state"]): + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("attacker_code", "victim", state)) + network.assert_not_called() + + +class TestCallbackNonceStandalone: + + def test_valid_state_and_nonce_accepted(self, standalone_oauth, network): + q = _query(asyncio.run(standalone_oauth.login())) + verifier = _storage().get("user:code_verifier")["value"] + network.return_value = _token_response(_id_token(nonce=q["nonce"])) + + result = asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) + + assert result["user"] == {"id": "kp_user"} + sent = network.call_args.kwargs["data"] + assert sent["code"] == "code" + assert sent["code_verifier"] == verifier + + def test_id_token_with_wrong_nonce_rejected(self, standalone_oauth, network): + """(b) An ID token whose nonce doesn't match the stored nonce is rejected.""" + q = _query(asyncio.run(standalone_oauth.login())) + network.return_value = _token_response(_id_token(nonce="attacker-nonce")) + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) + # No session must have been established for the user + assert standalone_oauth._session_manager.get_token_manager("u1") is None + assert _storage().get("user:nonce") is None + + def test_id_token_without_nonce_rejected(self, standalone_oauth, network): + """(b) An ID token with no nonce claim is rejected when a nonce was sent.""" + q = _query(asyncio.run(standalone_oauth.login())) + network.return_value = _token_response(_id_token()) + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) + assert standalone_oauth._session_manager.get_token_manager("u1") is None + + def test_undecodable_id_token_rejected(self, standalone_oauth, network): + q = _query(asyncio.run(standalone_oauth.login())) + network.return_value = _token_response("not-a-jwt") + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) + + def test_token_response_without_id_token_still_accepted(self, standalone_oauth, network): + """Nonce binds the ID token only; non-OIDC responses (no id_token) are unaffected.""" + q = _query(asyncio.run(standalone_oauth.login())) + network.return_value = _token_response(include_id_token=False) + result = asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) + assert "id_token" not in result["tokens"] + + +# --------------------------------------------------------------------------- +# Flask routes +# --------------------------------------------------------------------------- + +@pytest.fixture +def flask_client(isolated_singletons, monkeypatch): + from flask import Flask + monkeypatch.setenv("SECRET_KEY", "test-secret") + monkeypatch.setenv("SESSION_FILE_DIR", tempfile.mkdtemp(prefix="kinde_cb_test_")) + app = Flask(__name__) + OAuth( + client_id=CLIENT_ID, + client_secret="test_client_secret", + redirect_uri=REDIRECT_URI, + host="https://example.kinde.com", + framework="flask", + app=app, + ) + app.config["TESTING"] = True + return app.test_client() + + +class TestFlaskCallback: + + def test_callback_without_state_rejected(self, flask_client, network): + r = flask_client.get("/login") + assert r.status_code == 302 + network.return_value = _token_response(_id_token(nonce="x")) + + r = flask_client.get("/callback?code=attacker_code") + + assert r.status_code == 400 + network.assert_not_called() + + @pytest.mark.parametrize("qs", ["code=attacker_code", "code=attacker_code&state=attacker-state"]) + def test_callback_rejected_when_no_login_in_progress(self, flask_client, network, qs): + network.return_value = _token_response(_id_token(nonce="x")) + r = flask_client.get(f"/callback?{qs}") + assert r.status_code == 400 + network.assert_not_called() + + def test_callback_wrong_nonce_rejected(self, flask_client, network): + q = _query(flask_client.get("/login").headers["Location"]) + network.return_value = _token_response(_id_token(nonce="attacker-nonce")) + r = flask_client.get(f"/callback?code=c&state={q['state']}") + assert r.status_code == 400 + + def test_callback_happy_path(self, flask_client, network): + q = _query(flask_client.get("/login").headers["Location"]) + network.return_value = _token_response(_id_token(nonce=q["nonce"])) + r = flask_client.get(f"/callback?code=c&state={q['state']}") + assert r.status_code == 302 + assert network.call_count == 1 + + +# --------------------------------------------------------------------------- +# FastAPI routes +# --------------------------------------------------------------------------- + +@pytest.fixture +def fastapi_client(isolated_singletons): + from fastapi import FastAPI + from fastapi.testclient import TestClient + from starlette.middleware.sessions import SessionMiddleware + app = FastAPI() + OAuth( + client_id=CLIENT_ID, + client_secret="test_client_secret", + redirect_uri=REDIRECT_URI, + host="https://example.kinde.com", + framework="fastapi", + app=app, + ) + app.add_middleware(SessionMiddleware, secret_key="test-secret") + return TestClient(app, raise_server_exceptions=False, follow_redirects=False) + + +class TestFastAPICallback: + + def test_callback_without_state_rejected(self, fastapi_client, network): + r = fastapi_client.get("/login") + assert r.status_code in (302, 307) + network.return_value = _token_response(_id_token(nonce="x")) + + r = fastapi_client.get("/callback?code=attacker_code") + + # FastAPI route re-raises SDK exceptions (pre-existing behaviour) -> 500 + assert r.status_code >= 400 + network.assert_not_called() + + @pytest.mark.parametrize("qs", ["code=attacker_code", "code=attacker_code&state=attacker-state"]) + def test_callback_rejected_when_no_login_in_progress(self, fastapi_client, network, qs): + network.return_value = _token_response(_id_token(nonce="x")) + r = fastapi_client.get(f"/callback?{qs}") + assert r.status_code >= 400 + network.assert_not_called() + + def test_callback_wrong_nonce_rejected(self, fastapi_client, network): + q = _query(fastapi_client.get("/login").headers["location"]) + network.return_value = _token_response(_id_token(nonce="attacker-nonce")) + r = fastapi_client.get(f"/callback?code=c&state={q['state']}") + assert r.status_code >= 400 + + def test_callback_happy_path(self, fastapi_client, network): + q = _query(fastapi_client.get("/login").headers["location"]) + network.return_value = _token_response(_id_token(nonce=q["nonce"])) + r = fastapi_client.get(f"/callback?code=c&state={q['state']}") + assert r.status_code in (302, 307) + assert network.call_count == 1 From ad0c2eee3db9e6e9f7af8ad5f02263e1bc3b7f27 Mon Sep 17 00:00:00 2001 From: Koman Rudden <1030623+KomanRudden@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:39:35 +0200 Subject: [PATCH 2/3] test: disable GC around SmartOAuth timing loops The TestSmartOAuthPerformance tests measure wall time over a few milliseconds. If an automatic full (gen-2) garbage collection is triggered inside the timed region it adds ~50-110 ms for the current test-suite heap, failing test_method_delegation_performance (< 10 ms) regardless of SmartOAuth's delegation cost. Whether that happens depends only on the allocation history of earlier tests, so adding unrelated tests can flip it (seen on Python 3.10 CI). Pause automatic GC only around each timed region and restore it in a finally block (re-enabling only if it was enabled). Limits are unchanged. --- .../test_smart_oauth_integration.py | 50 ++++++++++++++----- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/testv2/testv2_auth/test_smart_oauth_integration.py b/testv2/testv2_auth/test_smart_oauth_integration.py index 3302f2a9..5bcf3f80 100644 --- a/testv2/testv2_auth/test_smart_oauth_integration.py +++ b/testv2/testv2_auth/test_smart_oauth_integration.py @@ -202,27 +202,43 @@ class TestSmartOAuthPerformance: def test_initialization_performance(self): """Test that SmartOAuth initialization is fast.""" + import gc import time with patch('kinde_sdk.auth.smart_oauth.OAuth'), \ patch('kinde_sdk.auth.smart_oauth.AsyncOAuth'): - start_time = time.time() - smart_oauth = SmartOAuth(client_id="test") - end_time = time.time() + # Keep automatic (full) GC pauses out of the timed region + gc_was_enabled = gc.isenabled() + gc.disable() + try: + start_time = time.time() + smart_oauth = SmartOAuth(client_id="test") + end_time = time.time() + finally: + if gc_was_enabled: + gc.enable() # Initialization should be fast (< 100ms) assert (end_time - start_time) < 0.1 def test_method_delegation_performance(self, smart_oauth, mock_sync_oauth): """Test that method delegation is fast.""" + import gc import time - # Test sync method performance - start_time = time.time() - for _ in range(100): - smart_oauth.is_authenticated() - end_time = time.time() + # Test sync method performance (automatic GC paused so a full + # collection can't land in the timed region) + gc_was_enabled = gc.isenabled() + gc.disable() + try: + start_time = time.time() + for _ in range(100): + smart_oauth.is_authenticated() + end_time = time.time() + finally: + if gc_was_enabled: + gc.enable() # 100 calls should be fast (< 10ms) assert (end_time - start_time) < 0.01 @@ -230,13 +246,21 @@ def test_method_delegation_performance(self, smart_oauth, mock_sync_oauth): @pytest.mark.asyncio async def test_async_method_performance(self, smart_oauth, mock_async_oauth): """Test that async method delegation is fast.""" + import gc import time - # Test async method performance - start_time = time.time() - for _ in range(10): - await smart_oauth.get_user_info_async() - end_time = time.time() + # Test async method performance (automatic GC paused so a full + # collection can't land in the timed region) + gc_was_enabled = gc.isenabled() + gc.disable() + try: + start_time = time.time() + for _ in range(10): + await smart_oauth.get_user_info_async() + end_time = time.time() + finally: + if gc_was_enabled: + gc.enable() # 10 async calls should be fast (< 100ms) assert (end_time - start_time) < 0.1 From 514f84072e3d62f9c0185a29501256f41b838289 Mon Sep 17 00:00:00 2001 From: Koman Rudden Date: Thu, 1 Oct 2026 06:18:38 +0200 Subject: [PATCH 3/3] fix(auth): always validate the ID token nonce; return 400 from the FastAPI callback Review follow-up for #209. - Reject the callback when no nonce was stored for the login (before the code is redeemed) and when the token response has no ID token. Both previously skipped the nonce check, so a swapped code from an authorization request without `openid` could still establish a session. - FastAPI /callback: reject a missing `code` with 400 before handle_redirect, which would otherwise consume the pending state, nonce and code verifier; return 400 for KindeLoginException instead of 500, matching Flask. Removes the "State not found" branch, which nothing raises. - Tests: the no-ID-token test now expects a rejection; add tests for a missing stored nonce and a missing code (Flask and FastAPI); FastAPI route tests assert 400 exactly; test_session_secrets returns an ID token with the login's nonce. Co-Authored-By: Claude Opus 5.5 --- kinde_fastapi/framework/fastapi_framework.py | 12 ++-- kinde_sdk/auth/oauth.py | 34 ++++++++---- .../test_oauth_callback_security.py | 55 +++++++++++++++---- testv2/testv2_auth/test_session_secrets.py | 13 +++-- 4 files changed, 85 insertions(+), 29 deletions(-) diff --git a/kinde_fastapi/framework/fastapi_framework.py b/kinde_fastapi/framework/fastapi_framework.py index 3d45c165..8f0a078a 100644 --- a/kinde_fastapi/framework/fastapi_framework.py +++ b/kinde_fastapi/framework/fastapi_framework.py @@ -3,6 +3,7 @@ from fastapi.responses import RedirectResponse, HTMLResponse from kinde_sdk.core.framework.framework_interface import FrameworkInterface from kinde_sdk.auth.oauth import OAuth +from kinde_sdk.core.exceptions import KindeLoginException from ..middleware.framework_middleware import FrameworkMiddleware import os import uuid @@ -183,15 +184,18 @@ async def callback(request: Request, code: Optional[str] = None, state: Optional except Exception as ex: return HTMLResponse(f"Error parsing reauth state: {str(ex)}", status_code=400) + # Reject before handle_redirect, which would consume the pending state + if not code: + return HTMLResponse("Authentication failed: Missing authorization code", status_code=400) + user_id = request.session.get('user_id') or str(uuid.uuid4()) try: assert self._oauth is not None await self._oauth.handle_redirect(code, user_id, state) - except Exception as e: - if "State not found" in str(e): - return HTMLResponse("Error: State not found. Please check Kinde Python SDK documentation.\n" + str(e), status_code=500) - raise e + except KindeLoginException as e: + # Missing, forged or stale callbacks are client errors, as in the Flask route + return HTMLResponse(f"Authentication failed: {e}", status_code=400) request.session['user_id'] = user_id diff --git a/kinde_sdk/auth/oauth.py b/kinde_sdk/auth/oauth.py index 12f1496a..e87a6e9c 100644 --- a/kinde_sdk/auth/oauth.py +++ b/kinde_sdk/auth/oauth.py @@ -526,10 +526,16 @@ async def handle_redirect(self, code: str, user_id: str, state: Optional[str] = self._logger.error("State mismatch in OAuth callback") raise KindeLoginException("Invalid state parameter") - # State and nonce are single-use: consume them once state has been - # validated, so they can't be replayed even if the rest of the flow fails. + # Every SDK-started login stores a nonce. Without it the ID token can't be + # bound to this login, so reject before the code is redeemed. stored_nonce = storage.get("user:nonce") expected_nonce = stored_nonce.get("value") if stored_nonce else None + if not expected_nonce: + self._logger.error("No nonce stored for OAuth callback") + raise KindeLoginException("Missing nonce for this login") + + # State and nonce are single-use: consume them once they have been + # validated, so they can't be replayed even if the rest of the flow fails. storage.delete("user:state") storage.delete("user:nonce") @@ -549,16 +555,22 @@ async def handle_redirect(self, code: str, user_id: str, state: Optional[str] = self._logger.error(f"Token exchange failed: {str(e)}") raise KindeTokenException(f"Failed to exchange code for tokens: {str(e)}") from e - # Verify the ID token nonce matches the one sent in the authorization request + # Verify the ID token nonce matches the one sent in the authorization request. + # A response without an ID token is rejected too: otherwise a swapped code + # from an authorization request without `openid` would skip this check. id_token = token_data.get("id_token") - if expected_nonce and id_token: - try: - id_token_claims = jwt.decode(id_token, options={"verify_signature": False}) - except jwt.PyJWTError as e: - raise KindeLoginException("Invalid ID token") from e - if id_token_claims.get("nonce") != expected_nonce: - self._logger.error("Nonce mismatch in ID token") - raise KindeLoginException("Invalid nonce in ID token") + if not id_token: + self._logger.error("Token response has no ID token") + raise KindeLoginException("Missing ID token") + try: + # Signature not verified, as elsewhere in the SDK: the token comes + # directly from the token endpoint over TLS + id_token_claims = jwt.decode(id_token, options={"verify_signature": False}) + except jwt.PyJWTError as e: + raise KindeLoginException("Invalid ID token") from e + if id_token_claims.get("nonce") != expected_nonce: + self._logger.error("Nonce mismatch in ID token") + raise KindeLoginException("Invalid nonce in ID token") # Store tokens user_info = { diff --git a/testv2/testv2_auth/test_oauth_callback_security.py b/testv2/testv2_auth/test_oauth_callback_security.py index 4db929b8..7f339f69 100644 --- a/testv2/testv2_auth/test_oauth_callback_security.py +++ b/testv2/testv2_auth/test_oauth_callback_security.py @@ -5,8 +5,9 @@ * state is always required: a callback is rejected unless a login was started by the SDK in this session (state stored) and the callback presents the same state (login CSRF / authorization code injection), - * the ID token ``nonce`` claim must match the nonce sent in the - authorization request, + * a nonce must have been stored for the login, the token response must + contain an ID token, and its ``nonce`` claim must match the nonce sent + in the authorization request, * stored state / nonce are single-use and cleared after the callback. The standalone tests use the real in-memory storage path (null framework); @@ -216,12 +217,24 @@ def test_undecodable_id_token_rejected(self, standalone_oauth, network): with pytest.raises(KindeLoginException): asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) - def test_token_response_without_id_token_still_accepted(self, standalone_oauth, network): - """Nonce binds the ID token only; non-OIDC responses (no id_token) are unaffected.""" + def test_token_response_without_id_token_rejected(self, standalone_oauth, network): + """Without an ID token the nonce can't be checked, so a swapped code from an + authorization request without `openid` must not establish a session.""" q = _query(asyncio.run(standalone_oauth.login())) network.return_value = _token_response(include_id_token=False) - result = asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) - assert "id_token" not in result["tokens"] + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) + assert standalone_oauth._session_manager.get_token_manager("u1") is None + + def test_missing_stored_nonce_rejected_before_token_exchange(self, standalone_oauth, network): + """Losing the stored nonce must not turn nonce validation off.""" + q = _query(asyncio.run(standalone_oauth.login())) + _storage().delete("user:nonce") + network.return_value = _token_response(_id_token(nonce=q["nonce"])) + with pytest.raises(KindeLoginException): + asyncio.run(standalone_oauth.handle_redirect("code", "u1", q["state"])) + network.assert_not_called() + assert standalone_oauth._session_manager.get_token_manager("u1") is None # --------------------------------------------------------------------------- @@ -271,6 +284,17 @@ def test_callback_wrong_nonce_rejected(self, flask_client, network): r = flask_client.get(f"/callback?code=c&state={q['state']}") assert r.status_code == 400 + def test_callback_without_code_rejected_and_keeps_pending_login(self, flask_client, network): + q = _query(flask_client.get("/login").headers["Location"]) + r = flask_client.get(f"/callback?state={q['state']}") + assert r.status_code == 400 + network.assert_not_called() + + network.return_value = _token_response(_id_token(nonce=q["nonce"])) + r = flask_client.get(f"/callback?code=c&state={q['state']}") + assert r.status_code == 302 + assert network.call_args.kwargs["data"].get("code_verifier") + def test_callback_happy_path(self, flask_client, network): q = _query(flask_client.get("/login").headers["Location"]) network.return_value = _token_response(_id_token(nonce=q["nonce"])) @@ -310,22 +334,33 @@ def test_callback_without_state_rejected(self, fastapi_client, network): r = fastapi_client.get("/callback?code=attacker_code") - # FastAPI route re-raises SDK exceptions (pre-existing behaviour) -> 500 - assert r.status_code >= 400 + assert r.status_code == 400 network.assert_not_called() @pytest.mark.parametrize("qs", ["code=attacker_code", "code=attacker_code&state=attacker-state"]) def test_callback_rejected_when_no_login_in_progress(self, fastapi_client, network, qs): network.return_value = _token_response(_id_token(nonce="x")) r = fastapi_client.get(f"/callback?{qs}") - assert r.status_code >= 400 + assert r.status_code == 400 network.assert_not_called() def test_callback_wrong_nonce_rejected(self, fastapi_client, network): q = _query(fastapi_client.get("/login").headers["location"]) network.return_value = _token_response(_id_token(nonce="attacker-nonce")) r = fastapi_client.get(f"/callback?code=c&state={q['state']}") - assert r.status_code >= 400 + assert r.status_code == 400 + + def test_callback_without_code_rejected_and_keeps_pending_login(self, fastapi_client, network): + q = _query(fastapi_client.get("/login").headers["location"]) + r = fastapi_client.get(f"/callback?state={q['state']}") + assert r.status_code == 400 + network.assert_not_called() + + # State, nonce and code verifier weren't consumed: the genuine callback still completes + network.return_value = _token_response(_id_token(nonce=q["nonce"])) + r = fastapi_client.get(f"/callback?code=c&state={q['state']}") + assert r.status_code in (302, 307) + assert network.call_args.kwargs["data"].get("code_verifier") def test_callback_happy_path(self, fastapi_client, network): q = _query(fastapi_client.get("/login").headers["location"]) diff --git a/testv2/testv2_auth/test_session_secrets.py b/testv2/testv2_auth/test_session_secrets.py index b464f59c..528d80a6 100644 --- a/testv2/testv2_auth/test_session_secrets.py +++ b/testv2/testv2_auth/test_session_secrets.py @@ -40,11 +40,16 @@ def _token_data(self): } def _complete_login(self, user_id="user_1"): - # Start the login through the SDK so a state is stored, then return it - # on the callback like Kinde would. + # Start the login through the SDK so a state and nonce are stored, then + # return them on the callback and in the ID token like Kinde would. login_url = asyncio.run(self.oauth.login()) - state = parse_qs(urlparse(login_url).query)["state"][0] - with patch.object(OAuth, "exchange_code_for_tokens", AsyncMock(return_value=self._token_data())), \ + query = parse_qs(urlparse(login_url).query) + state, nonce = query["state"][0], query["nonce"][0] + token_data = { + **self._token_data(), + "id_token": jwt.encode({"sub": "user_1", "nonce": nonce}, "key", algorithm="HS256"), + } + with patch.object(OAuth, "exchange_code_for_tokens", AsyncMock(return_value=token_data)), \ patch("kinde_sdk.auth.oauth.helper_get_user_details", AsyncMock(return_value={})): asyncio.run(self.oauth.handle_redirect("code", user_id, state))