-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: scope cookies to CHAINLIT_ROOT_PATH for multi-app deployments #2825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
5a6b4a0
19139a3
9f63b7f
7247fe4
14c30fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,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 = os.environ.get("CHAINLIT_AUTH_COOKIE_PATH", _cookie_root_path) | ||
| else: | ||
| _cookie_path = os.environ.get("CHAINLIT_AUTH_COOKIE_PATH", "/") | ||
| _state_cookie_lifetime = int( | ||
|
|
@@ -34,6 +34,20 @@ | |
| _state_cookie_name = "oauth_state" | ||
|
|
||
|
|
||
| def _delete_legacy_cookies(response: Response, *names: str): | ||
| """Delete cookies at path='/' left over from pre-scoped versions. | ||
|
|
||
| Only acts when _cookie_path != '/' to avoid no-op deletes in | ||
| single-app deployments. | ||
| """ | ||
| if _cookie_path == "/": | ||
| return | ||
| for name in names: | ||
| response.delete_cookie( | ||
| key=name, path="/", secure=_cookie_secure, samesite=_cookie_samesite | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Unconditional root-path legacy cookie deletion can remove same-named cookies from other apps on the same host, causing cross-app session/logout disruption in multi-app deployments. Prompt for AI agents |
||
| ) | ||
|
|
||
|
|
||
| class OAuth2PasswordBearerWithCookie(SecurityBase): | ||
| """ | ||
| OAuth2 password flow with cookie support with fallback to bearer token. | ||
|
|
@@ -132,23 +146,27 @@ def set_auth_cookie(request: Request, response: Response, token: str): | |
| response.set_cookie( | ||
| key=k, | ||
| value=chunk, | ||
| path=_cookie_path, | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| httponly=True, | ||
| secure=_cookie_secure, | ||
| samesite=_cookie_samesite, | ||
| max_age=config.project.user_session_timeout, | ||
| ) | ||
| _delete_legacy_cookies(response, k) | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
|
|
||
| existing_cookies.discard(k) | ||
| else: | ||
| # Default (shorter cookies) | ||
| response.set_cookie( | ||
| key=_auth_cookie_name, | ||
| value=token, | ||
| path=_cookie_path, | ||
| httponly=True, | ||
| secure=_cookie_secure, | ||
| samesite=_cookie_samesite, | ||
| max_age=config.project.user_session_timeout, | ||
| ) | ||
| _delete_legacy_cookies(response, _auth_cookie_name) | ||
|
|
||
| existing_cookies.discard(_auth_cookie_name) | ||
|
|
||
|
|
@@ -172,17 +190,20 @@ def clear_auth_cookie(request: Request, response: Response): | |
| response.delete_cookie( | ||
| key=k, path=_cookie_path, secure=_cookie_secure, samesite=_cookie_samesite | ||
| ) | ||
| _delete_legacy_cookies(response, k) | ||
|
|
||
|
|
||
| def set_oauth_state_cookie(response: Response, token: str): | ||
| response.set_cookie( | ||
| _state_cookie_name, | ||
| token, | ||
| path=_cookie_path, | ||
| httponly=True, | ||
| samesite=_cookie_samesite, | ||
| secure=_cookie_secure, | ||
| max_age=_state_cookie_lifetime, | ||
| ) | ||
| _delete_legacy_cookies(response, _state_cookie_name) | ||
|
|
||
|
|
||
| def validate_oauth_state_cookie(request: Request, state: str): | ||
|
|
@@ -196,4 +217,5 @@ def validate_oauth_state_cookie(request: Request, state: str): | |
|
|
||
| def clear_oauth_state_cookie(response: Response): | ||
| """Oauth complete, delete state token.""" | ||
| response.delete_cookie(_state_cookie_name) # Do we set path here? | ||
| response.delete_cookie(_state_cookie_name, path=_cookie_path) | ||
| _delete_legacy_cookies(response, _state_cookie_name) | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -147,6 +147,56 @@ def test_state_cookie_lifetime_custom(monkeypatch): | |||||||
| assert cookie_module._state_cookie_lifetime == 600 | ||||||||
|
|
||||||||
|
|
||||||||
| def test_cookie_path_defaults_to_root(monkeypatch): | ||||||||
| """When neither CHAINLIT_ROOT_PATH nor CHAINLIT_AUTH_COOKIE_PATH is set, _cookie_path defaults to '/'.""" | ||||||||
| monkeypatch.delenv("CHAINLIT_ROOT_PATH", raising=False) | ||||||||
| monkeypatch.delenv("CHAINLIT_AUTH_COOKIE_PATH", raising=False) | ||||||||
| importlib.reload(cookie_module) | ||||||||
| assert cookie_module._cookie_path == "/" | ||||||||
|
|
||||||||
|
|
||||||||
| def test_cookie_path_uses_root_path(monkeypatch): | ||||||||
| """When CHAINLIT_ROOT_PATH is set, _cookie_path uses its value.""" | ||||||||
| monkeypatch.setenv("CHAINLIT_ROOT_PATH", "/app1") | ||||||||
| monkeypatch.delenv("CHAINLIT_AUTH_COOKIE_PATH", raising=False) | ||||||||
| importlib.reload(cookie_module) | ||||||||
| assert cookie_module._cookie_path == "/app1" | ||||||||
|
|
||||||||
|
|
||||||||
| def test_cookie_path_explicit_overrides_root_path(monkeypatch): | ||||||||
| """CHAINLIT_AUTH_COOKIE_PATH takes precedence over CHAINLIT_ROOT_PATH.""" | ||||||||
| monkeypatch.setenv("CHAINLIT_ROOT_PATH", "/app1") | ||||||||
| monkeypatch.setenv("CHAINLIT_AUTH_COOKIE_PATH", "/custom") | ||||||||
| importlib.reload(cookie_module) | ||||||||
| assert cookie_module._cookie_path == "/custom" | ||||||||
|
|
||||||||
|
|
||||||||
| def test_delete_legacy_cookies_skips_when_path_is_root(monkeypatch): | ||||||||
| """_delete_legacy_cookies is a no-op when _cookie_path == '/'.""" | ||||||||
| monkeypatch.delenv("CHAINLIT_ROOT_PATH", raising=False) | ||||||||
| monkeypatch.delenv("CHAINLIT_AUTH_COOKIE_PATH", raising=False) | ||||||||
| importlib.reload(cookie_module) | ||||||||
|
|
||||||||
| response = Response() | ||||||||
| cookie_module._delete_legacy_cookies(response, "access_token") | ||||||||
| # Response should have no Set-Cookie headers (no delete issued) | ||||||||
| assert "set-cookie" not in response.headers | ||||||||
|
|
||||||||
|
|
||||||||
| def test_delete_legacy_cookies_deletes_at_root_when_scoped(monkeypatch): | ||||||||
| """_delete_legacy_cookies issues a delete at path='/' when _cookie_path != '/'.""" | ||||||||
| monkeypatch.setenv("CHAINLIT_ROOT_PATH", "/app1") | ||||||||
| monkeypatch.delenv("CHAINLIT_AUTH_COOKIE_PATH", raising=False) | ||||||||
| importlib.reload(cookie_module) | ||||||||
|
|
||||||||
| response = Response() | ||||||||
| cookie_module._delete_legacy_cookies(response, "access_token") | ||||||||
| set_cookie_header = response.headers.get("set-cookie", "") | ||||||||
| assert "access_token" in set_cookie_header | ||||||||
| assert 'Path=/' in set_cookie_header | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Substring check for Prompt for AI agents
Suggested change
|
||||||||
| assert 'Max-Age=0' in set_cookie_header | ||||||||
|
|
||||||||
|
|
||||||||
| def test_clear_auth_cookie(client): | ||||||||
| """Test cookie clearing removes all chunks.""" | ||||||||
| # Set initial token | ||||||||
|
|
||||||||
Uh oh!
There was an error while loading. Please reload this page.