codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3608483987
########## superset/utils/auth_session_stamp.py: ########## @@ -0,0 +1,390 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Session stamp: invalidate all browser sessions when a user's password changes.""" + +from __future__ import annotations + +import logging +import time +from typing import Any +from uuid import uuid4 + +from flask import Flask, has_request_context, session +from flask_login import current_user, logout_user +from sqlalchemy.exc import IntegrityError, SQLAlchemyError + +from superset.extensions import db +from superset.utils.decorators import transaction + +logger: logging.Logger = logging.getLogger(__name__) + +SESSION_AUTH_STAMP_SESSION_KEY = "_auth_session_stamp" +SESSION_AUTH_STAMP_VALIDATED_AT_KEY = "_auth_session_stamp_validated_at" +SESSION_AUTH_STAMP_VALIDATED_DB_STAMP_KEY = "_auth_session_stamp_validated_db_stamp" + +_SAFE_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) +_STAMP_CACHE_KEY_PREFIX = "auth_session_stamp:" +_DEFAULT_STAMP_CACHE_TIMEOUT_SECONDS = 300 + + +def _stamp_cache_key(user_id: int) -> str: + return f"{_STAMP_CACHE_KEY_PREFIX}{user_id}" + + +def _get_revalidation_seconds() -> int: + """Return ``SESSION_AUTH_STAMP_REVALIDATION_SECONDS``, defaulting on bad config.""" + from flask import current_app + + raw = current_app.config.get("SESSION_AUTH_STAMP_REVALIDATION_SECONDS", 0) + try: + return int(raw) + except (TypeError, ValueError): + logger.warning( + "Invalid SESSION_AUTH_STAMP_REVALIDATION_SECONDS=%r; " + "defaulting to 0 (check on every request)", + raw, + ) + return 0 + + +def _stamp_cache_timeout_seconds() -> int: + if (revalidation_seconds := _get_revalidation_seconds()) > 0: + return revalidation_seconds + return _DEFAULT_STAMP_CACHE_TIMEOUT_SECONDS + + +def _get_cached_user_session_stamp(user_id: int) -> str | None: + try: + from superset.extensions import cache_manager + + cached = cache_manager.cache.get(_stamp_cache_key(user_id)) + except Exception: # noqa: BLE001 + logger.debug( + "Unable to read session auth stamp cache for user_id=%s", + user_id, + exc_info=True, + ) + return None + return cached if isinstance(cached, str) else None + + +def _cache_user_session_stamp(user_id: int, stamp: str) -> None: + try: + from superset.extensions import cache_manager + + cache_manager.cache.set( + _stamp_cache_key(user_id), + stamp, + timeout=_stamp_cache_timeout_seconds(), + ) + except Exception: # noqa: BLE001 + logger.debug( + "Unable to write session auth stamp cache for user_id=%s", + user_id, + exc_info=True, + ) + + +def register_session_auth_stamp_hook(app: Flask) -> None: + """Register request hooks that manage the per-user session stamp.""" + if getattr(app, "superset_session_auth_stamp_hook_registered", False): + return + app.superset_session_auth_stamp_hook_registered = True + + @app.before_request + def _validate_user_session_auth_stamp() -> None: # noqa: WPS430 + """Log out requests whose session cookie carries an outdated auth stamp.""" + validate_session_auth_stamp_for_request() + + +@transaction() +def ensure_user_session_stamp_value(user_id: int) -> str: + """Return the stamp for ``user_id``, inserting a stable row if missing.""" + from superset.models.user_session_auth_stamp import UserSessionAuthStamp + + row = db.session.get(UserSessionAuthStamp, user_id) + if row is not None: + return row.stamp + stamp = str(uuid4()) + try: + with db.session.begin_nested(): + db.session.add(UserSessionAuthStamp(user_id=user_id, stamp=stamp)) + return stamp + except IntegrityError: + row = db.session.get(UserSessionAuthStamp, user_id) + if row is None: + logger.exception( + "Failed to resolve session auth stamp after IntegrityError " + "for user_id=%s", + user_id, + ) + raise + return row.stamp + + +def clear_flask_login_remember_cookie() -> None: + """ + Schedule deletion of any Flask-Login remember-me cookie on the HTTP response. + + Superset does not expose remember-me in the React login flow, but Flask-Login + and FAB still support persistent cookies when ``remember=True``. After a + password change, clear any existing remember token so it cannot + re-establish a session without re-authentication. + """ + if not has_request_context(): + return + session["_remember"] = "clear" + + +def _invalidate_stale_auth_session() -> None: + """Log out, clear remember-me, and raise 401 for a stamp mismatch.""" + from werkzeug.exceptions import Unauthorized + + logout_user() + session.clear() + # ``session.clear()`` drops the remember marker; set it after so Flask-Login + # still deletes any remember-me cookie on the response. + session["_remember"] = "clear" + raise Unauthorized("Session invalidated") + + +def sync_session_auth_stamp_on_login(user: Any) -> None: + """Copy the DB stamp into the signed session cookie after a successful login.""" + if not has_request_context(): + return + uid = getattr(user, "id", None) + if uid is None: + return + user_id = int(uid) + stamp = ensure_user_session_stamp_value(user_id) + session[SESSION_AUTH_STAMP_SESSION_KEY] = stamp + _mark_session_stamp_validated(stamp) + _cache_user_session_stamp(user_id, stamp) + + +def cache_user_session_auth_stamp(user_id: int, stamp: str) -> None: + """Write a validated session auth stamp into the shared cache.""" + _cache_user_session_stamp(user_id, stamp) + + +@transaction() +def bump_user_session_auth_stamp(user_id: int) -> str: + """Assign a new stamp so every other session for this user becomes invalid.""" + from superset.models.user_session_auth_stamp import UserSessionAuthStamp + + new_stamp = str(uuid4()) + row = db.session.get(UserSessionAuthStamp, user_id) + if row is None: + try: + with db.session.begin_nested(): + db.session.add(UserSessionAuthStamp(user_id=user_id, stamp=new_stamp)) + return new_stamp + except IntegrityError: + row = db.session.get(UserSessionAuthStamp, user_id) + if row is None: + logger.exception( + "Failed to resolve session auth stamp after IntegrityError " + "for user_id=%s", + user_id, + ) + raise + row.stamp = new_stamp + return new_stamp Review Comment: **Suggestion:** The stamp-rotation function updates only the database row and returns without updating the shared cache, so other sessions can keep validating against a stale cached stamp until cache expiry. This delays cross-session invalidation after a password change; update or invalidate the cached value as part of the same rotation path. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Old browser sessions remain valid after password change. - ⚠️ Cross-session logout semantics not reliably enforced. - ⚠️ Security depends on cache eviction, not DB stamp. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. A user logs in, triggering `sync_session_auth_stamp_on_login` in `superset/utils/auth_session_stamp.py:165-177`, which calls `ensure_user_session_stamp_value` at lines 115-137 and then `_cache_user_session_stamp` at lines 85-93 to store the current stamp for `user_id` in the shared cache. 2. The instance is configured with `SESSION_AUTH_STAMP_REVALIDATION_SECONDS > 0` (read in `_get_revalidation_seconds` at lines 50-61), so `_stamp_cache_timeout_seconds` at lines 64-67 and `_should_skip_stamp_db_lookup` at lines 244-271 rely on the cached stamp to avoid frequent DB lookups on safe methods (`GET`, `HEAD`, `OPTIONS`). 3. The user changes their password via the new AUTH_DB password change flow, which rotates the per-user session stamp by calling `bump_user_session_auth_stamp(user_id)` defined at lines 185-207; this function updates `row.stamp = new_stamp` at line 205 and returns at line 206 without calling `_cache_user_session_stamp`, leaving the cache entry set to the old stamp. 4. Another active browser session for the same user sends a `GET` request; `validate_session_auth_stamp_for_request` at lines 312-390 runs on `before_request`, obtains `user_id` via `_resolve_stamp_check_user_id` at lines 209-242, and then calls `_should_skip_stamp_db_lookup` at lines 244-271. Because `SESSION_AUTH_STAMP_REVALIDATION_SECONDS > 0`, the method is safe, and the session carries the old stamp, `_get_cached_user_session_stamp` at lines 70-82 returns the stale value; `_should_skip_stamp_db_lookup` sees `cached_stamp == sess_stamp` and returns `True`, so the DB is not consulted and the outdated session remains valid even though the authoritative DB stamp has been rotated. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6cf94779dfa348e88f4faeea59716112&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6cf94779dfa348e88f4faeea59716112&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/auth_session_stamp.py **Line:** 205:206 **Comment:** *Security: The stamp-rotation function updates only the database row and returns without updating the shared cache, so other sessions can keep validating against a stale cached stamp until cache expiry. This delays cross-session invalidation after a password change; update or invalidate the cached value as part of the same rotation path. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=6c24ca5def8d063a0da1d9c3e398eb0b01bd570a22796ecb6a744cd67e3700b3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=6c24ca5def8d063a0da1d9c3e398eb0b01bd570a22796ecb6a744cd67e3700b3&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
