Repository navigation
fix(auth): validate OAuth state and verify id_token in azure-ad-hybrid callback - #3070
failsafesecurity wants to merge 1 commit into
Conversation
…d callback The azure-ad-hybrid login callback accepted an OAuth code and an id_token straight from the browser, never validated the OAuth state cookie, and never verified the id_token before forwarding it to the application oauth_callback. An unauthenticated attacker could forge an alg=none id_token for any identity and be issued a session cookie for that principal. This change: - adds a required state form field and calls validate_oauth_state_cookie, matching the generic OAuth callback; - verifies the id_token signature (tenant JWKS), aud, iss/exp and nonce before it is trusted or forwarded; - generates a per-login OIDC nonce bound to the browser and verifies it against the returned id_token, instead of a nonce frozen on a provider instance constructed once at module import. Finding: azure-ad-hybrid OAuth callback skips state validation and trusts a client-supplied id_token (CVSS 9.3, CWE-347).
There was a problem hiding this comment.
5 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/chainlit/oauth_providers.py">
<violation number="1" location="backend/chainlit/oauth_providers.py:291">
P2: Removing this call leaves `random_secret` unused, and the repository's Ruff check fails on the import. Remove the stale import.</violation>
<violation number="2" location="backend/chainlit/oauth_providers.py:300">
P2: The existing Azure hybrid initialization test still requires a nonce in `authorize_params`, so it fails after this removal. Update the test to check the per-login nonce behavior instead.</violation>
</file>
<file name="backend/chainlit/auth/cookie.py">
<violation number="1" location="backend/chainlit/auth/cookie.py:217">
P1: The default `SameSite=Lax` cookie is omitted on Azure's cross-site `form_post`, so the callback receives no nonce and rejects the login. Configure both OAuth state and nonce cookies as `SameSite=None; Secure` for this flow; the state cookie uses the same default.</violation>
</file>
<file name="backend/chainlit/server.py">
<violation number="1" location="backend/chainlit/server.py:634">
P3: A new `PyJWKClient` is constructed on every callback, and PyJWKClient's JWK-set cache is per-instance (default lifespan 300s). Each hybrid login therefore performs a fresh HTTPS fetch of the tenant JWKS before `decode`, adding a network round-trip and making every login depend on the Microsoft discovery endpoint being reachable. Hold one module-level (lazily initialized) client and reuse it across callbacks.</violation>
<violation number="2" location="backend/chainlit/server.py:640">
P2: `jwt.decode` here never verifies the token's `iss` claim. PyJWT's `_validate_iss` returns early when `issuer` is None, so passing only `audience` skips issuer validation entirely — despite the docstring and PR description claiming `iss` is checked. Compute the expected issuer from the same authority/tenant used for the JWKS URL and pass it to `decode`.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
Turn on auto-fix | Re-trigger cubic
| _nonce_cookie_name, | ||
| token, | ||
| httponly=True, | ||
| samesite=_cookie_samesite, |
There was a problem hiding this comment.
P1: The default SameSite=Lax cookie is omitted on Azure's cross-site form_post, so the callback receives no nonce and rejects the login. Configure both OAuth state and nonce cookies as SameSite=None; Secure for this flow; the state cookie uses the same default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At backend/chainlit/auth/cookie.py, line 217:
<comment>The default `SameSite=Lax` cookie is omitted on Azure's cross-site `form_post`, so the callback receives no nonce and rejects the login. Configure both OAuth state and nonce cookies as `SameSite=None; Secure` for this flow; the state cookie uses the same default.</comment>
<file context>
@@ -207,6 +208,28 @@ def validate_oauth_state_cookie(request: Request, state: str):
+ _nonce_cookie_name,
+ token,
+ httponly=True,
+ samesite=_cookie_samesite,
+ secure=_cookie_secure,
+ max_age=_state_cookie_lifetime,
</file context>
| def __init__(self): | ||
| self.client_id = os.environ.get("OAUTH_AZURE_AD_HYBRID_CLIENT_ID") | ||
| self.client_secret = os.environ.get("OAUTH_AZURE_AD_HYBRID_CLIENT_SECRET") | ||
| nonce = random_secret(16) |
There was a problem hiding this comment.
P2: Removing this call leaves random_secret unused, and the repository's Ruff check fails on the import. Remove the stale import.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At backend/chainlit/oauth_providers.py, line 291:
<comment>Removing this call leaves `random_secret` unused, and the repository's Ruff check fails on the import. Remove the stale import.</comment>
<file context>
@@ -21,6 +21,10 @@ class OAuthProvider:
authorize_params: Dict[str, str]
default_prompt: Optional[str] = None
+ # Set to True by providers whose authorization request carries an OIDC
+ # ``nonce`` that must be generated per login and verified on return.
+ uses_per_login_nonce: bool = False
+
def is_configured(self):
return all([os.environ.get(env) for env in self.env])
</file context>
| "https://graph.microsoft.com/User.Read https://graph.microsoft.com/openid offline_access", | ||
| ), | ||
| "response_mode": "form_post", | ||
| "nonce": nonce, |
There was a problem hiding this comment.
P2: The existing Azure hybrid initialization test still requires a nonce in authorize_params, so it fails after this removal. Update the test to check the per-login nonce behavior instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At backend/chainlit/oauth_providers.py, line 300:
<comment>The existing Azure hybrid initialization test still requires a nonce in `authorize_params`, so it fails after this removal. Update the test to check the per-login nonce behavior instead.</comment>
<file context>
@@ -288,7 +293,12 @@ class AzureADHybridOAuthProvider(OAuthProvider):
+ # instances are constructed once at module import, so a nonce stored on
+ # ``self.authorize_params`` would be identical for every login for the
+ # lifetime of the process and could never provide replay protection.
+ # The nonce is generated per authorization request in ``oauth_login``
+ # and verified against the returned ``id_token`` in the callback.
self.authorize_params = {
</file context>
| id_token, | ||
| signing_key.key, | ||
| algorithms=["RS256"], | ||
| audience=provider.client_id, |
There was a problem hiding this comment.
P2: jwt.decode here never verifies the token's iss claim. PyJWT's _validate_iss returns early when issuer is None, so passing only audience skips issuer validation entirely — despite the docstring and PR description claiming iss is checked. Compute the expected issuer from the same authority/tenant used for the JWKS URL and pass it to decode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At backend/chainlit/server.py, line 640:
<comment>`jwt.decode` here never verifies the token's `iss` claim. PyJWT's `_validate_iss` returns early when `issuer` is None, so passing only `audience` skips issuer validation entirely — despite the docstring and PR description claiming `iss` is checked. Compute the expected issuer from the same authority/tenant used for the JWKS URL and pass it to `decode`.</comment>
<file context>
@@ -611,6 +615,40 @@ async def header_auth(request: Request):
+ id_token,
+ signing_key.key,
+ algorithms=["RS256"],
+ audience=provider.client_id,
+ )
+
</file context>
| """ | ||
| import jwt | ||
|
|
||
| jwks_client = jwt.PyJWKClient(_azure_hf_jwks_url()) |
There was a problem hiding this comment.
P3: A new PyJWKClient is constructed on every callback, and PyJWKClient's JWK-set cache is per-instance (default lifespan 300s). Each hybrid login therefore performs a fresh HTTPS fetch of the tenant JWKS before decode, adding a network round-trip and making every login depend on the Microsoft discovery endpoint being reachable. Hold one module-level (lazily initialized) client and reuse it across callbacks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At backend/chainlit/server.py, line 634:
<comment>A new `PyJWKClient` is constructed on every callback, and PyJWKClient's JWK-set cache is per-instance (default lifespan 300s). Each hybrid login therefore performs a fresh HTTPS fetch of the tenant JWKS before `decode`, adding a network round-trip and making every login depend on the Microsoft discovery endpoint being reachable. Hold one module-level (lazily initialized) client and reuse it across callbacks.</comment>
<file context>
@@ -611,6 +615,40 @@ async def header_auth(request: Request):
+ """
+ import jwt
+
+ jwks_client = jwt.PyJWKClient(_azure_hf_jwks_url())
+ signing_key = jwks_client.get_signing_key_from_jwt(id_token)
+ decoded = jwt.decode(
</file context>
Security fix: azure-ad-hybrid OAuth callback skips state validation and trusts a client-supplied
id_tokenSeverity: CRITICAL · CVSS: 9.3 (
CVSS:4.0/AV:N/AC:L/AT:N/PR:N/UI:N/VC:H/VI:H/VA:N/SC:N/SI:N/SA:N) · CWE-347 · OWASP A04:2025 Cryptographic FailuresSummary
The
azure-ad-hybridlogin callback accepts an OAuthcodeand anid_tokenstraight from the browser, never validates the OAuthstatecookie, and never verifies theid_tokenbefore handing it to the application'soauth_callback. Because the framework passes the token through untouched, the identity used to mint the session is whatever the caller wrote into that form field.oauth_azure_hf_callbackinbackend/chainlit/server.pymirrored the generic OAuth callback except that it declared nostateargument and never calledvalidate_oauth_state_cookie; the generic route did. It readcodeandid_tokenas client form fields and forwarded the rawid_tokenintoconfig.code.oauth_callback(...). No signature, issuer, audience, expiry or nonce check existed anywhere along that path.Separately, the nonce afforded no protection even where it was generated:
AzureADHybridOAuthProvider.__init__drew a nonce and stored it onself.authorize_params, but provider instances are built once at module import, so the same nonce was sent on every authorization request for the process lifetime, and it was never compared against the returned token.Impact
An unauthenticated attacker can obtain a valid session cookie for any identity on the deployment, including administrative users, and then read or act through the authenticated API as that principal without holding any Azure AD credential.
PoC summary
Live poisoning of the callback with an
alg=noneforged token, against the real app with Azure calls stubbed:A nonce was observed to be reused across independent logins and never verified. (See attached scan report for the full reproduction harness.)
Fix
oauth_azure_hf_callbacknow declares a requiredstateform field and callsvalidate_oauth_state_cookie(request, state)before processing the callback, matching the generic route.id_tokenis verified before it is trusted or forwarded: signature against the tenant JWKS, plusaud,issandexp.nonceis now generated per authorization request inoauth_login, bound to the browser via an httponly cookie, and compared against the returnedid_token'snonceclaim (hmac.compare_digest). The nonce is no longer derived from a provider instance constructed once at module import.Verification
python -m py_compileon the three changed modules.id_tokenwithout state →303 /login?error=oauthSignin, noaccess_tokencookie; nonce differs across independent logins (nonce_reused: false).Scan
Scan ID:
cmuwxf0uj0073lf01nna76pt1· Finding:cmux90q9j00cvlf01x8qc9wnaSummary by cubic
Hardens the
azure-ad-hybridOAuth callback so it validates the OAuth state and verifies theid_tokenbefore issuing a session. Previously the callback accepted a client-suppliedid_tokenwithout state or token validation, letting an unauthenticated attacker mint a session for any identity.Security fix
stateform field against the OAuth state cookie, matching the generic OAuth callback.id_tokensignature against the tenant JWKS and checksaud,iss, andexpbefore trusting it.nonceper login, bound to the browser via an httponly cookie, and compares it against the returned token'snonceclaim.noncethat was set once at module import on the provider instance.Written for commit 9d5d4bb. Summary will update on new commits.