From 05e77bf59064b2a0b736e69319a0cf6d16e8f959 Mon Sep 17 00:00:00 2001 From: devteamaegis Date: Sun, 24 May 2026 10:46:33 -0400 Subject: [PATCH] fix(auth): use CHAINLIT_ROOT_PATH value directly as cookie path Two bugs in `backend/chainlit/auth/cookie.py`: 1. **Cookie path computed incorrectly when CHAINLIT_ROOT_PATH is set** The module-level initialisation read: if _cookie_root_path := os.environ.get("CHAINLIT_ROOT_PATH", None): _cookie_path = os.environ.get(_cookie_root_path, "/") `os.environ.get(_cookie_root_path, "/")` used the *value* of `CHAINLIT_ROOT_PATH` (e.g. `"/myapp"`) as a *key* to look up yet another environment variable. Since `"/myapp"` is never a valid env var name the lookup always returned the default `"/"`. Cookie deletion in `set_auth_cookie` and `clear_auth_cookie` therefore used path `"/"` instead of the intended root path, leaving stale cookies on clients after logout or token rotation when the app is deployed under a sub-path. Fix: assign `_cookie_path = _cookie_root_path` directly. 2. **Non-constant-time OAuth state comparison** `validate_oauth_state_cookie` used `oauth_state != state` (regular string equality), which is not constant-time and can leak the expected state value via timing side-channels. Replace with `hmac.compare_digest`, which is the standard Python idiom for constant-time secret comparison. Also added an explicit `None` guard so a missing state cookie is rejected before the digest comparison. Tests added: - test_cookie_path_uses_chainlit_root_path_value - test_validate_oauth_state_cookie_rejects_missing_state - test_validate_oauth_state_cookie_accepts_correct_state --- backend/chainlit/auth/cookie.py | 11 ++++-- backend/tests/auth/test_cookie.py | 62 +++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/backend/chainlit/auth/cookie.py b/backend/chainlit/auth/cookie.py index 1cb4cc5ace..7ff72f3abc 100644 --- a/backend/chainlit/auth/cookie.py +++ b/backend/chainlit/auth/cookie.py @@ -1,3 +1,4 @@ +import hmac import os from typing import Literal, Optional, cast @@ -24,7 +25,7 @@ ) _cookie_secure = _cookie_samesite == "none" if _cookie_root_path := os.environ.get("CHAINLIT_ROOT_PATH", None): - _cookie_path = os.environ.get(_cookie_root_path, "/") + _cookie_path = _cookie_root_path else: _cookie_path = os.environ.get("CHAINLIT_AUTH_COOKIE_PATH", "/") _state_cookie_lifetime = int( @@ -186,11 +187,15 @@ def set_oauth_state_cookie(response: Response, token: str): def validate_oauth_state_cookie(request: Request, state: str): - """Check the state from the oauth provider against the browser cookie.""" + """Check the state from the oauth provider against the browser cookie. + + Uses ``hmac.compare_digest`` for constant-time comparison to prevent + timing-based side-channel leakage of the expected state value. + """ oauth_state = request.cookies.get(_state_cookie_name) - if oauth_state != state: + if oauth_state is None or not hmac.compare_digest(oauth_state, state): raise Exception("oauth state does not correspond") diff --git a/backend/tests/auth/test_cookie.py b/backend/tests/auth/test_cookie.py index 5f5c3848a5..a9482305a7 100644 --- a/backend/tests/auth/test_cookie.py +++ b/backend/tests/auth/test_cookie.py @@ -133,6 +133,68 @@ def test_overwrite_shorter_token_unchunked(client): assert len(chunk_cookies) == 0, f"Found {len(chunk_cookies)} residual cookies" +def test_cookie_path_uses_chainlit_root_path_value(monkeypatch): + """CHAINLIT_ROOT_PATH must be used as the cookie path directly. + + Before the fix, the code read:: + + _cookie_path = os.environ.get(_cookie_root_path, "/") + + where ``_cookie_root_path`` was the *value* of ``CHAINLIT_ROOT_PATH``, + not a key. This caused ``_cookie_path`` to always be ``"/"`` (the + default) whenever ``CHAINLIT_ROOT_PATH`` was set, because the value + (e.g. ``"/app"``) is not a valid environment variable name. Cookie + deletion then used the wrong path, leaving stale cookies on the client. + """ + monkeypatch.setenv("CHAINLIT_ROOT_PATH", "/myapp") + monkeypatch.delenv("CHAINLIT_AUTH_COOKIE_PATH", raising=False) + importlib.reload(cookie_module) + assert cookie_module._cookie_path == "/myapp", ( + f"Expected _cookie_path to be '/myapp' but got '{cookie_module._cookie_path}'. " + "CHAINLIT_ROOT_PATH value should be used directly as the cookie path." + ) + + +def test_validate_oauth_state_cookie_rejects_missing_state(monkeypatch): + """validate_oauth_state_cookie must raise when the state cookie is absent.""" + from chainlit.auth.cookie import validate_oauth_state_cookie + from starlette.requests import Request as StarletteRequest + + scope = { + "type": "http", + "headers": [], + "query_string": b"", + "method": "GET", + "path": "/", + } + request = StarletteRequest(scope) + # Cookies dict is empty — oauth_state cookie is absent + with pytest.raises(Exception, match="oauth state does not correspond"): + validate_oauth_state_cookie(request, "expected_state") + + +def test_validate_oauth_state_cookie_accepts_correct_state(): + """validate_oauth_state_cookie must not raise when states match.""" + from chainlit.auth.cookie import validate_oauth_state_cookie + from starlette.requests import Request as StarletteRequest + + state_value = "secret_state_token" + + # Build a minimal request with the oauth_state cookie set + scope = { + "type": "http", + "headers": [ + (b"cookie", f"oauth_state={state_value}".encode()), + ], + "query_string": b"", + "method": "GET", + "path": "/", + } + request = StarletteRequest(scope) + # Should not raise + validate_oauth_state_cookie(request, state_value) + + def test_state_cookie_lifetime_default(monkeypatch): """Test that _state_cookie_lifetime defaults to 180 seconds (3 minutes).""" monkeypatch.delenv("CHAINLIT_STATE_COOKIE_LIFETIME", raising=False)