codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3655700976
########## superset/utils/auth_session_stamp.py: ########## @@ -0,0 +1,392 @@ +# 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: str = "_auth_session_stamp" +SESSION_AUTH_STAMP_VALIDATED_AT_KEY: str = "_auth_session_stamp_validated_at" +SESSION_AUTH_STAMP_VALIDATED_DB_STAMP_KEY: str = ( + "_auth_session_stamp_validated_db_stamp" +) + +_SAFE_METHODS: frozenset[str] = frozenset({"GET", "HEAD", "OPTIONS"}) +_STAMP_CACHE_KEY_PREFIX: str = "auth_session_stamp:" +_DEFAULT_STAMP_CACHE_TIMEOUT_SECONDS: int = 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 + + +def _resolve_stamp_check_user_id() -> int | None: + """Return the integer user id for the stamp check, or None to skip it.""" + from flask import current_app + from flask_appbuilder.const import AUTH_DB + from sqlalchemy.orm.exc import DetachedInstanceError + + if not has_request_context(): + return None + # The stamp is only ever bumped on AUTH_DB password changes; skip the DB + # hit for other auth backends where the stamp never changes after login. + if current_app.config.get("AUTH_TYPE") != AUTH_DB: + return None + if not getattr(current_user, "is_authenticated", False): + return None + if getattr(current_user, "is_guest_user", False): + return None + try: + raw_id = current_user.get_id() + except DetachedInstanceError: + # The user object was detached from the session (e.g. after a rollback + # in a previous before_request hook). Fail open — Flask-Login will + # reload the user on the next request. + logger.warning( + "Skipping session auth stamp check: current_user is detached", + exc_info=True, + ) + return None + if raw_id is None: + return None + try: + return int(raw_id) + except (TypeError, ValueError): + return None + + +def _should_skip_stamp_db_lookup( + user_id: int, method: str, sess_stamp: str | None +) -> bool: + """Return True when a recent DB check lets us skip another on read-only traffic.""" + if method not in _SAFE_METHODS or sess_stamp is None: + return False + + revalidation_seconds = _get_revalidation_seconds() + if revalidation_seconds <= 0: + return False + + validated_at = session.get(SESSION_AUTH_STAMP_VALIDATED_AT_KEY) + if validated_at is None: + return False + try: + elapsed = time.time() - float(validated_at) + except (TypeError, ValueError): + return False + if elapsed >= revalidation_seconds: + return False + + # Only skip when this session still carries the stamp we last validated and + # the shared cache agrees (so password rotation on another client is visible). + if session.get(SESSION_AUTH_STAMP_VALIDATED_DB_STAMP_KEY) != sess_stamp: + return False + cached_stamp = _get_cached_user_session_stamp(user_id) + return cached_stamp == sess_stamp + + +def _mark_session_stamp_validated(db_stamp: str | None = None) -> None: + session[SESSION_AUTH_STAMP_VALIDATED_AT_KEY] = time.time() + if db_stamp is not None: + session[SESSION_AUTH_STAMP_VALIDATED_DB_STAMP_KEY] = db_stamp + session.modified = True + + +def _load_db_stamp_for_user( + user_id: int, *, use_cache: bool = True +) -> tuple[str | None, bool]: + """ + Return ``(stamp, db_error)``. + + ``db_error`` is True when the database lookup failed and the check should + fail open. ``stamp`` is None when no row exists. + """ + from superset.models.user_session_auth_stamp import UserSessionAuthStamp + + if use_cache: + cached_stamp = _get_cached_user_session_stamp(user_id) + if cached_stamp is not None: + return cached_stamp, False + + try: + with db.session.begin_nested(): + row = db.session.get(UserSessionAuthStamp, user_id) + db_stamp = row.stamp if row is not None else None + except SQLAlchemyError: + logger.warning( + "Skipping session auth stamp check due to a database error", + exc_info=True, + ) + return None, True + + if db_stamp is not None: + _cache_user_session_stamp(user_id, db_stamp) Review Comment: **Suggestion:** This cache write can reinsert an obsolete stamp after a password rotation. A request that read the old database stamp before the rotation may finish afterward and overwrite the shared cache with that old value; read-only requests can then trust the stale cache and skip the database check, allowing old sessions to remain valid for the revalidation interval. [race condition] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Revoked browser sessions can remain valid temporarily. - ⚠️ Safe requests may skip authoritative stamp validation. - ⚠️ Password-rotation session invalidation depends on cache ordering. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure `SESSION_AUTH_STAMP_REVALIDATION_SECONDS` to a positive value and authenticate an AUTH_DB user; `validate_session_auth_stamp_for_request()` at `superset/utils/auth_session_stamp.py:314` enables cache-based skipping for safe requests. 2. Start a safe request using an old session stamp and pause it after `_load_db_stamp_for_user()` reads the old database stamp at `superset/utils/auth_session_stamp.py:299-301`, but before `_cache_user_session_stamp()` executes at lines 309-310. 3. Change the user's password through `CurrentUserRestApi.update_my_password()` at `superset/views/users/api.py:312-408`; `_commit_user_password_change()` rotates the database stamp at `superset/views/users/api.py:149`, and the new stamp is written to the cache at line 398. 4. Allow the paused request to continue. Lines 309-310 can overwrite the cache with the old stamp, after which `_should_skip_stamp_db_lookup()` at `superset/utils/auth_session_stamp.py:267-272` can trust that stale value for the revalidation interval, allowing the old session to pass safe-request validation. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=af43d5947a1247b1a9c721ad81659246&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=af43d5947a1247b1a9c721ad81659246&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:** 309:310 **Comment:** *Race Condition: This cache write can reinsert an obsolete stamp after a password rotation. A request that read the old database stamp before the rotation may finish afterward and overwrite the shared cache with that old value; read-only requests can then trust the stale cache and skip the database check, allowing old sessions to remain valid for the revalidation interval. 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=f4ef66961eb9c8c3de84ac010194d91c289447e3330b41bc48a2d556b69d693e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=f4ef66961eb9c8c3de84ac010194d91c289447e3330b41bc48a2d556b69d693e&reaction=dislike'>👎</a> ########## superset/views/users/api.py: ########## @@ -161,6 +309,142 @@ def update_me(self) -> Response: except ValidationError as error: return self.response_400(message=error.messages) + @expose("/password", methods=["PUT"]) + @protect() + @permission_name("write") + @safe + @statsd_metrics + @requires_json + @_rate_limit_me_password_change + def update_my_password(self) -> Response: + """Update the current user's password (AUTH_DB only) + --- + put: + summary: Update the current user's password + description: >- + Changes the authenticated user's password when ``AUTH_TYPE`` is ``AUTH_DB``. + Requires the current password and a new password that satisfies + ``AUTH_DB_CONFIG`` policy. + requestBody: + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/CurrentUserPasswordPutSchema' + responses: + 200: + description: Password updated successfully + content: + application/json: + schema: + type: object + properties: + result: + $ref: '#/components/schemas/UserResponseSchema' + 400: + $ref: '#/components/responses/400' + 401: + $ref: '#/components/responses/401' + 429: + $ref: '#/components/responses/400' + 500: + $ref: '#/components/responses/500' + """ + if app.config.get("AUTH_TYPE") != AUTH_DB: + return self.response_400( + message=( + "Password change is only available when AUTH_TYPE is AUTH_DB." + ), + ) + + try: + body = _load_password_change_body( + self.current_user_password_put_schema, + request.json, + ) + user_db = db.session.get(User, g.user.id) + if user_db is None: + return self.response_404() + + old_hash = user_db.password + if not verify_auth_db_password(old_hash, body["current_password"]): + return self.response_400(message="Incorrect current password.") + + new_hash = hash_auth_db_password(body["new_password"]) Review Comment: **Suggestion:** The configured hash algorithm is not validated or converted into an API error before hashing. For an unsupported `password_hash_algorithm`, `hash_auth_db_password` can raise a `ValueError` or similar exception, but this handler only catches `ValidationError`, causing the endpoint to return an unintended 500 instead of the documented 400 response. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Password changes fail under invalid hash configuration. - ⚠️ Clients receive an unintended server error. - ⚠️ Operators lack a clear configuration-validation response. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run Superset with `AUTH_TYPE` set to `AUTH_DB` and configure `AUTH_DB_CONFIG` with an unsupported password hash algorithm; the password-change endpoint checks this mode at `superset/views/users/api.py:353-358`. 2. Authenticate with an AUTH_DB user and send JSON containing `current_password`, `new_password`, and `confirm_password` to `PUT /api/v1/me/password`, implemented by `CurrentUserRestApi.update_my_password()` at `superset/views/users/api.py:312-319`. 3. The request passes `_load_password_change_body()` at `superset/views/users/api.py:107-114`, loads the user at lines 365-367, and verifies the current password at lines 369-371. 4. Line 373 calls `hash_auth_db_password()` from `superset/utils/auth_db_password_hash.py`. If that utility raises `ValueError` or another configuration error for the unsupported algorithm, the surrounding handler only catches `ValidationError` at lines 374-375, so the request reaches the generic server-error handling instead of returning the documented client-facing 400 response. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=de155600afcc4fc19dfd4f1fa1eea213&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=de155600afcc4fc19dfd4f1fa1eea213&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/views/users/api.py **Line:** 369:373 **Comment:** *Possible Bug: The configured hash algorithm is not validated or converted into an API error before hashing. For an unsupported `password_hash_algorithm`, `hash_auth_db_password` can raise a `ValueError` or similar exception, but this handler only catches `ValidationError`, causing the endpoint to return an unintended 500 instead of the documented 400 response. 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=f31ef5d43d698acf361d2cf4e6c5e568db928e755df89a2776b5ade6ccc7b3c0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=f31ef5d43d698acf361d2cf4e6c5e568db928e755df89a2776b5ade6ccc7b3c0&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]
