codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3460157571
########## superset/utils/auth_db_password.py: ########## @@ -0,0 +1,198 @@ +# 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. +"""Password policy helpers for AUTH_DB (database authentication).""" + +from __future__ import annotations + +import re +from typing import Any + +from flask import current_app as app +from marshmallow import ValidationError + +# Defaults are merged with ``AUTH_DB_CONFIG`` from Flask config (partial overrides). +AUTH_DB_DEFAULTS: dict[str, Any] = { + "password_min_length": 12, + "password_require_uppercase": True, + "password_require_lowercase": True, + "password_require_digit": True, + "password_require_special": True, + "password_common_list_check": True, + "password_hash_algorithm": "bcrypt", + "reset_token_expiry_minutes": 30, + "reset_rate_limit": "5 per 15 minutes", + "login_rate_limit": "10 per 5 minutes", + "login_max_failures": 5, + "login_lockout_duration_minutes": 15, +} + +_PUBLIC_PASSWORD_POLICY_KEYS = ( + "password_min_length", + "password_require_uppercase", + "password_require_lowercase", + "password_require_digit", + "password_require_special", + "password_common_list_check", +) + +_SUPPORTED_HASH_ALGORITHMS = frozenset({"bcrypt", "argon2"}) + +_COMMON_PASSWORDS = frozenset( + password.lower() + for password in { + "password", + "password1", + "password123", + "123456", + "12345678", + "123456789", + "qwerty", + "abc123", + "monkey", + "letmein", + "trustno1", + "dragon", + "baseball", + "iloveyou", + "master", + "sunshine", + "ashley", + "bailey", + "shadow", + "superman", + "qazwsx", + "michael", + "football", + "welcome", + "jesus", + "ninja", + "mustang", + "password1!", + "admin", + "admin123", + "root", + "toor", + "guest", + "p@ssw0rd", + "Passw0rd", + "Password1", + "Password123", + "Welcome1", + "Qwerty123", + } +) + + +def get_merged_auth_db_config() -> dict[str, Any]: + """Return ``AUTH_DB_DEFAULTS`` merged with ``AUTH_DB_CONFIG`` from app config.""" + overrides = app.config.get("AUTH_DB_CONFIG") or {} + return {**AUTH_DB_DEFAULTS, **overrides} Review Comment: **Suggestion:** `AUTH_DB_CONFIG` is assumed to be a mapping, but invalid config types (for example a string/list from `superset_config`) will raise `TypeError` when unpacked, causing password-related requests to fail with 500. Validate that overrides are a dict-like mapping before merging and raise a controlled validation error when misconfigured. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ `/api/v1/me/password/policy` fails with HTTP 500. - ❌ Password change rate limiting crashes when limiter enabled. - ⚠️ AUTH_DB password features brittle to config type mistakes. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure Superset for database auth by setting `AUTH_TYPE = AUTH_DB` so that the password endpoints in `CurrentUserRestApi` (superset/views/users/api.py:295-385) are reachable. 2. Misconfigure `AUTH_DB_CONFIG` in the Superset configuration so that `app.config["AUTH_DB_CONFIG"]` is a non-mapping value (for example a string), which is read in `get_merged_auth_db_config()` at `superset/utils/auth_db_password.py:100-103`. 3. From an authenticated session, call `GET /api/v1/me/password/policy`, which is implemented by `CurrentUserRestApi.get_my_password_policy()` at `superset/views/users/api.py:386-419` and calls `get_public_auth_db_password_policy()` imported from `superset/utils/auth_db_password.py:37-41`. 4. `get_public_auth_db_password_policy()` calls `get_merged_auth_db_config()` (superset/utils/auth_db_password.py:100-103), where `return {**AUTH_DB_DEFAULTS, **overrides}` at line 103 attempts to unpack the non-mapping `overrides` and raises `TypeError: type object argument after ** must be a mapping, not <type>`, causing Flask to return HTTP 500 instead of a controlled configuration error for all password-policy and rate-limited password change flows. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=548d5863e40b4251a8066335cad82873&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=548d5863e40b4251a8066335cad82873&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_db_password.py **Line:** 102:103 **Comment:** *Type Error: `AUTH_DB_CONFIG` is assumed to be a mapping, but invalid config types (for example a string/list from `superset_config`) will raise `TypeError` when unpacked, causing password-related requests to fail with 500. Validate that overrides are a dict-like mapping before merging and raise a controlled validation error when misconfigured. 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=5fa624f8e11370ea11e2c4e6204031982266e0f106ccdb3869fb2d6b472a815f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=5fa624f8e11370ea11e2c4e6204031982266e0f106ccdb3869fb2d6b472a815f&reaction=dislike'>👎</a> ########## superset/utils/auth_session_stamp.py: ########## @@ -0,0 +1,154 @@ +# 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 +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 + +from superset.extensions import db +from superset.utils.decorators import transaction + +logger = logging.getLogger(__name__) + +SESSION_AUTH_STAMP_SESSION_KEY = "_auth_session_stamp" + + +def register_session_auth_stamp_hook(app: Flask) -> None: + """Register a before_request handler that enforces 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 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 + stamp = ensure_user_session_stamp_value(int(uid)) + session[SESSION_AUTH_STAMP_SESSION_KEY] = stamp + + +@transaction() +def bump_user_session_auth_stamp(user_id: int) -> None: + """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 + 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 + + +def validate_session_auth_stamp_for_request() -> None: + """Drop login when the session cookie carries an outdated stamp for this user.""" + from superset.models.user_session_auth_stamp import UserSessionAuthStamp + + if not has_request_context(): + return + if not getattr(current_user, "is_authenticated", False): + return + if getattr(current_user, "is_guest_user", False): + return + raw_id = current_user.get_id() + if raw_id is None: + return + try: + user_id = int(raw_id) + except (TypeError, ValueError): + return + + row = db.session.get(UserSessionAuthStamp, user_id) + if row is None: + stamp = ensure_user_session_stamp_value(user_id) + session[SESSION_AUTH_STAMP_SESSION_KEY] = stamp + return Review Comment: **Suggestion:** The request hook performs database reads/writes without any exception handling, so DB-level failures (including a missing table during rolling deploy/migration mismatch) will raise out of `before_request` and turn authenticated requests into 500 errors. Wrap this check in a fail-open/fallback path (log and continue or safely log out) to avoid global request failures. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ All authenticated requests 500 during schema mismatch. - ⚠️ Rolling upgrades brittle to migration timing issues. - ⚠️ Session-stamp failures block otherwise healthy endpoints. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Deploy the PR code to an environment where the `user_session_auth_stamp` table has not yet been created (for example, migration `add_user_session_auth_stamp_table` has not run), so the ORM model `UserSessionAuthStamp` defined at `superset/models/user_session_auth_stamp.py:28-39` points to a missing table while other user tables exist. 2. Start Superset and create a login manager via `SupersetSecurityManager.create_login_manager()` at `superset/security/manager.py:19-25`, which imports `register_session_auth_stamp_hook` and registers the `_validate_user_session_auth_stamp` `before_request` hook at `superset/utils/auth_session_stamp.py:37-46`. 3. Log in as any user so that `current_user.is_authenticated` is `True` on subsequent requests, then invoke a simple authenticated endpoint such as `GET /api/v1/me/` implemented by `CurrentUserRestApi.get_me()` at `superset/views/users/api.py:175-201`. 4. For each authenticated request, the `before_request` handler calls `validate_session_auth_stamp_for_request()` (superset/utils/auth_session_stamp.py:123-153), which executes `row = db.session.get(UserSessionAuthStamp, user_id)` at line 141 while the underlying table is missing; SQLAlchemy raises an OperationalError/ProgrammingError, this exception is not caught anywhere in the hook or view, and Flask converts it into an HTTP 500 response for every authenticated request until the migration is applied or the hook is disabled. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9293901280a941fd8adc34341be8964f&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=9293901280a941fd8adc34341be8964f&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:** 141:145 **Comment:** *Possible Bug: The request hook performs database reads/writes without any exception handling, so DB-level failures (including a missing table during rolling deploy/migration mismatch) will raise out of `before_request` and turn authenticated requests into 500 errors. Wrap this check in a fail-open/fallback path (log and continue or safely log out) to avoid global request failures. 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=7d14b0a2cb78b08f8f30c11c6db0565e55a7616f1f2673e442fa3193e46fddfd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=7d14b0a2cb78b08f8f30c11c6db0565e55a7616f1f2673e442fa3193e46fddfd&reaction=dislike'>👎</a> ########## superset/utils/auth_db_password.py: ########## @@ -0,0 +1,198 @@ +# 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. +"""Password policy helpers for AUTH_DB (database authentication).""" + +from __future__ import annotations + +import re +from typing import Any + +from flask import current_app as app +from marshmallow import ValidationError + +# Defaults are merged with ``AUTH_DB_CONFIG`` from Flask config (partial overrides). +AUTH_DB_DEFAULTS: dict[str, Any] = { + "password_min_length": 12, + "password_require_uppercase": True, + "password_require_lowercase": True, + "password_require_digit": True, + "password_require_special": True, + "password_common_list_check": True, + "password_hash_algorithm": "bcrypt", + "reset_token_expiry_minutes": 30, + "reset_rate_limit": "5 per 15 minutes", + "login_rate_limit": "10 per 5 minutes", + "login_max_failures": 5, + "login_lockout_duration_minutes": 15, +} + +_PUBLIC_PASSWORD_POLICY_KEYS = ( + "password_min_length", + "password_require_uppercase", + "password_require_lowercase", + "password_require_digit", + "password_require_special", + "password_common_list_check", +) + +_SUPPORTED_HASH_ALGORITHMS = frozenset({"bcrypt", "argon2"}) + +_COMMON_PASSWORDS = frozenset( + password.lower() + for password in { + "password", + "password1", + "password123", + "123456", + "12345678", + "123456789", + "qwerty", + "abc123", + "monkey", + "letmein", + "trustno1", + "dragon", + "baseball", + "iloveyou", + "master", + "sunshine", + "ashley", + "bailey", + "shadow", + "superman", + "qazwsx", + "michael", + "football", + "welcome", + "jesus", + "ninja", + "mustang", + "password1!", + "admin", + "admin123", + "root", + "toor", + "guest", + "p@ssw0rd", + "Passw0rd", + "Password1", + "Password123", + "Welcome1", + "Qwerty123", + } +) + + +def get_merged_auth_db_config() -> dict[str, Any]: + """Return ``AUTH_DB_DEFAULTS`` merged with ``AUTH_DB_CONFIG`` from app config.""" + overrides = app.config.get("AUTH_DB_CONFIG") or {} + return {**AUTH_DB_DEFAULTS, **overrides} + + +def get_auth_db_login_rate_limit_string() -> str: + """ + Return the configured AUTH_DB ``login_rate_limit`` string for Flask-Limiter. + + Used for endpoints that verify a password (for example ``PUT /api/v1/me/password``). + """ + return str( + get_merged_auth_db_config().get( + "login_rate_limit", AUTH_DB_DEFAULTS["login_rate_limit"] + ) + ) + + +def get_public_auth_db_password_policy(cfg: dict[str, Any] | None = None) -> dict[str, Any]: + """ + Return non-secret AUTH_DB password policy options for frontend validation UI. + """ + merged_cfg = cfg if cfg is not None else get_merged_auth_db_config() + return { + key: merged_cfg.get(key, AUTH_DB_DEFAULTS[key]) + for key in _PUBLIC_PASSWORD_POLICY_KEYS + } + + +def get_auth_db_password_hash_algorithm(cfg: dict[str, Any] | None = None) -> str: + """ + Return the password hash algorithm for AUTH_DB. + + ``AUTH_DB_CONFIG["password_hash_algorithm"]`` accepts ``bcrypt`` (default) + or ``argon2``. + """ + merged_cfg = cfg if cfg is not None else get_merged_auth_db_config() + raw_algorithm = merged_cfg.get( + "password_hash_algorithm", AUTH_DB_DEFAULTS["password_hash_algorithm"] + ) + algorithm = str(raw_algorithm).strip().lower() + if algorithm not in _SUPPORTED_HASH_ALGORITHMS: + supported = ", ".join(sorted(_SUPPORTED_HASH_ALGORITHMS)) + raise ValidationError( + { + "password_hash_algorithm": [ + "Invalid AUTH_DB_CONFIG.password_hash_algorithm. " + f"Expected one of: {supported}." + ] + } + ) + return algorithm + + +def get_auth_db_password_hash_method(cfg: dict[str, Any] | None = None) -> str: + """Backward-compatible alias for :func:`get_auth_db_password_hash_algorithm`.""" + return get_auth_db_password_hash_algorithm(cfg) + + +def validate_auth_db_password(password: str, cfg: dict[str, Any] | None = None) -> None: + """ + Enforce AUTH_DB password policy. + + :param password: Candidate password (plaintext). + :param cfg: Optional merged config; loaded from the current app when omitted. + :raises marshmallow.ValidationError: When validation fails. + """ + cfg = cfg if cfg is not None else get_merged_auth_db_config() + errors: list[str] = [] + + min_len = int(cfg.get("password_min_length", AUTH_DB_DEFAULTS["password_min_length"])) Review Comment: **Suggestion:** Converting `password_min_length` with `int(...)` can throw `ValueError`/`TypeError` for invalid config values, which currently bubbles up as a server error instead of a user-facing validation response. Normalize and validate this config value explicitly before using it in request-time password checks. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ `/api/v1/me/password` returns 500 for bad policy config. - ⚠️ Users cannot change passwords until config is fixed. - ⚠️ Error messaging hides root cause from administrators. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure Superset for database auth by setting `AUTH_TYPE = AUTH_DB` so that `PUT /api/v1/me/password` is enabled via `CurrentUserRestApi.update_my_password()` in `superset/views/users/api.py:295-385`. 2. Set `AUTH_DB_CONFIG` in the Superset configuration to a dict with a non-numeric `password_min_length`, for example `{"password_min_length": "twelve"}`, so that `cfg["password_min_length"]` is a string when read in `validate_auth_db_password()` at `superset/utils/auth_db_password.py:168-172`. 3. As an authenticated user, call `PUT /api/v1/me/password` with a JSON body that passes schema validation (parsed by `CurrentUserPasswordPutSchema` and `_load_password_change_body()` at `superset/views/users/api.py:105-112`) and includes a `new_password` field. 4. `_load_password_change_body()` calls `validate_auth_db_password(body["new_password"])` (superset/views/users/api.py:109-112), which executes `min_len = int(cfg.get("password_min_length", AUTH_DB_DEFAULTS["password_min_length"]))` at `superset/utils/auth_db_password.py:171`; `int("twelve")` raises `ValueError`, which is not caught by `update_my_password()` (it only catches `marshmallow.ValidationError` at lines 359-360), so the request surfaces as an unhandled exception and returns HTTP 500 instead of a structured validation error. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ac696731b33b4f33b6cab08ab52eaf23&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=ac696731b33b4f33b6cab08ab52eaf23&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_db_password.py **Line:** 171:171 **Comment:** *Type Error: Converting `password_min_length` with `int(...)` can throw `ValueError`/`TypeError` for invalid config values, which currently bubbles up as a server error instead of a user-facing validation response. Normalize and validate this config value explicitly before using it in request-time password checks. 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=079ed4b1082726480440de557f73ee95ccaafa2d2062479a40e7e99606074c23&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=079ed4b1082726480440de557f73ee95ccaafa2d2062479a40e7e99606074c23&reaction=dislike'>👎</a> ########## superset/utils/auth_session_stamp.py: ########## @@ -0,0 +1,154 @@ +# 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 +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 + +from superset.extensions import db +from superset.utils.decorators import transaction + +logger = logging.getLogger(__name__) + +SESSION_AUTH_STAMP_SESSION_KEY = "_auth_session_stamp" + + +def register_session_auth_stamp_hook(app: Flask) -> None: + """Register a before_request handler that enforces 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 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 + stamp = ensure_user_session_stamp_value(int(uid)) + session[SESSION_AUTH_STAMP_SESSION_KEY] = stamp + + +@transaction() +def bump_user_session_auth_stamp(user_id: int) -> None: + """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 + 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 + + +def validate_session_auth_stamp_for_request() -> None: + """Drop login when the session cookie carries an outdated stamp for this user.""" + from superset.models.user_session_auth_stamp import UserSessionAuthStamp + + if not has_request_context(): + return + if not getattr(current_user, "is_authenticated", False): + return + if getattr(current_user, "is_guest_user", False): + return + raw_id = current_user.get_id() + if raw_id is None: + return + try: + user_id = int(raw_id) + except (TypeError, ValueError): + return + + row = db.session.get(UserSessionAuthStamp, user_id) + if row is None: + stamp = ensure_user_session_stamp_value(user_id) + session[SESSION_AUTH_STAMP_SESSION_KEY] = stamp + return + + sess_stamp = session.get(SESSION_AUTH_STAMP_SESSION_KEY) + if sess_stamp is None: + session[SESSION_AUTH_STAMP_SESSION_KEY] = row.stamp + return Review Comment: **Suggestion:** Treating a missing session stamp as valid allows pre-existing sessions to survive a password change. If another browser session has no `_auth_session_stamp` yet, this branch silently seeds it with the latest DB stamp instead of invalidating the session, so the "invalidate all sessions on password change" guarantee is bypassed. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Pre-existing sessions survive a user password change. - ❌ Password changes fail to revoke all active logins. - ⚠️ Increases risk of unauthorized access via stale browsers. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Before deploying this PR, have a user log in to Superset so that Flask-Login creates a browser session for that user without any `_auth_session_stamp` key in the session; the current code later reads this key via `session[SESSION_AUTH_STAMP_SESSION_KEY]` inside `validate_session_auth_stamp_for_request()` in `superset/utils/auth_session_stamp.py:147-150`. 2. Deploy the PR version (which registers `register_session_auth_stamp_hook()` in the login manager at `superset/security/manager.py:19-25` and defines the `UserSessionAuthStamp` model at `superset/models/user_session_auth_stamp.py:28-39`), then log in as the same user in a second browser and change the password through `PUT /api/v1/me/password` handled by `CurrentUserRestApi.update_my_password()` at `superset/views/users/api.py:295-385`. 3. The password change calls `_commit_user_password_change()` (superset/views/users/api.py:115-145), which invokes `bump_user_session_auth_stamp(user_id)` at line 139, updating `UserSessionAuthStamp.stamp` for that user in the database via the logic in `superset/utils/auth_session_stamp.py:100-120`, and then `_reestablish_login_session()` logs the user back in with the new stamp. 4. Returning to the original pre-upgrade browser session, make any authenticated request (for example `GET /api/v1/me/` at `superset/views/users/api.py:175-201`), which triggers the `_validate_user_session_auth_stamp` before-request hook at `superset/utils/auth_session_stamp.py:43-46`; inside `validate_session_auth_stamp_for_request()` (lines 123-153), `row = db.session.get(UserSessionAuthStamp, user_id)` at line 141 finds the updated row, but `sess_stamp = session.get(SESSION_AUTH_STAMP_SESSION_KEY)` at line 147 returns `None` for this legacy session, so the branch at lines 148-150 simply seeds `session[SESSION_AUTH_STAMP_SESSION_KEY] = row.stamp` and returns without calling `logout_user()`, leaving this pre-existing session fully authenticated even though the password was changed, contrary to the "invalidate all browser sessions when a user's password changes" guarantee documented in the module docstring at `superset/utils/auth_session_stamp.py:17` and in `UserSessionAuthStamp`'s comment at `superset/models/user_session_auth_stamp.py:30-33`. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=605dfdde664746688af638e7c9016ef8&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=605dfdde664746688af638e7c9016ef8&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:** 147:150 **Comment:** *Security: Treating a missing session stamp as valid allows pre-existing sessions to survive a password change. If another browser session has no `_auth_session_stamp` yet, this branch silently seeds it with the latest DB stamp instead of invalidating the session, so the "invalidate all sessions on password change" guarantee is bypassed. 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=cd499e1a4a94c3e12a69ffc1b01573b64ed6447c32edb99cea13bc3b639cccce&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=cd499e1a4a94c3e12a69ffc1b01573b64ed6447c32edb99cea13bc3b639cccce&reaction=dislike'>👎</a> ########## superset-frontend/src/utils/generateAuthDbPassword.ts: ########## @@ -0,0 +1,274 @@ +/** + * 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. + */ +import { t } from '@apache-superset/core/translation'; +/** + * Client-side generator aligned with default ``AUTH_DB_CONFIG`` / Python + * ``superset.utils.auth_db_password`` (minimum length, character classes, common list). + * Keep ``AUTH_DB_COMMON_PASSWORDS`` in sync with ``_COMMON_PASSWORDS`` in that module. + */ +export const AUTH_DB_PASSWORD_MIN_LENGTH = 12; +export interface AuthDbPasswordPolicy { + password_min_length: number; + password_require_uppercase: boolean; + password_require_lowercase: boolean; + password_require_digit: boolean; + password_require_special: boolean; + password_common_list_check: boolean; +} + +export const AUTH_DB_DEFAULT_PASSWORD_POLICY: AuthDbPasswordPolicy = { + password_min_length: AUTH_DB_PASSWORD_MIN_LENGTH, + password_require_uppercase: true, + password_require_lowercase: true, + password_require_digit: true, + password_require_special: true, + password_common_list_check: true, +}; + +/** Lowercased entries; keep in sync with ``_COMMON_PASSWORDS`` in auth_db_password.py */ +const AUTH_DB_COMMON_PASSWORDS = new Set( + [ + 'password', + 'password1', + 'password123', + '123456', + '12345678', + '123456789', + 'qwerty', + 'abc123', + 'monkey', + 'letmein', + 'trustno1', + 'dragon', + 'baseball', + 'iloveyou', + 'master', + 'sunshine', + 'ashley', + 'bailey', + 'shadow', + 'superman', + 'qazwsx', + 'michael', + 'football', + 'welcome', + 'jesus', + 'ninja', + 'mustang', + 'password1!', + 'admin', + 'admin123', + 'root', + 'toor', + 'guest', + 'p@ssw0rd', + 'Passw0rd', + 'Password1', + 'Password123', + 'Welcome1', + 'Qwerty123', + ].map(s => s.toLowerCase()), +); + +const UPPER = 'ABCDEFGHJKLMNPQRSTUVWXYZ'; +const LOWER = 'abcdefghijkmnopqrstuvwxyz'; +const DIGIT = '23456789'; +const SPECIAL = '!@#$%^&*-_=+'; +const ALPHANUM = UPPER + LOWER + DIGIT; + +export interface AuthDbPasswordPolicyChecks { + minLength: boolean; + uppercase: boolean; + lowercase: boolean; + digit: boolean; + special: boolean; + commonPassword: boolean; +} + +function getCodePointLength(text: string): number { + return Array.from(text).length; +} + +/** Mirrors ``int(cfg.get("password_min_length", default))`` in auth_db_password.py. */ +function resolveAuthDbPasswordMinLength( + passwordMinLength: number | undefined | null, +): number { + if (passwordMinLength === undefined || passwordMinLength === null) { + return AUTH_DB_PASSWORD_MIN_LENGTH; + } + const parsed = Number(passwordMinLength); + if (!Number.isFinite(parsed)) { + return AUTH_DB_PASSWORD_MIN_LENGTH; + } + return Math.trunc(parsed); +} + +function secureRandomInt(maxExclusive: number): number { + if (maxExclusive <= 0) { + throw new Error('secureRandomInt: maxExclusive must be positive'); + } + const maxUint32 = 0xffffffff; + const limit = maxUint32 - (maxUint32 % maxExclusive); + const buf = new Uint32Array(1); + let value: number; + do { + crypto.getRandomValues(buf); + value = buf[0]!; + } while (value >= limit); + return value % maxExclusive; +} + +function pick(pool: string): string { + return pool[secureRandomInt(pool.length)]!; +} + +function shuffleInPlace(chars: string[]): void { + for (let i = chars.length - 1; i > 0; i -= 1) { + const j = secureRandomInt(i + 1); + const t = chars[i]!; + chars[i] = chars[j]!; + chars[j] = t; + } +} + +function getRequiredCharacterPools( + policy: AuthDbPasswordPolicy, +): string[] { + const pools: string[] = []; + if (policy.password_require_uppercase) { + pools.push(UPPER); + } + if (policy.password_require_lowercase) { + pools.push(LOWER); + } + if (policy.password_require_digit) { + pools.push(DIGIT); + } + if (policy.password_require_special) { + pools.push(SPECIAL); + } + return pools; +} + +function getGenerationPool(policy: AuthDbPasswordPolicy): string { + let pool = ''; + if (policy.password_require_uppercase) { + pool += UPPER; + } + if (policy.password_require_lowercase) { + pool += LOWER; + } + if (policy.password_require_digit) { + pool += DIGIT; + } + if (policy.password_require_special) { + pool += SPECIAL; + } + return pool || ALPHANUM + SPECIAL; +} + +function satisfiesAuthDbPasswordPolicy( + password: string, + policy: AuthDbPasswordPolicy, +): boolean { + const checks = getAuthDbPasswordPolicyChecks(password, policy); + return Object.values(checks).every(Boolean); +} + +/** True when the string satisfies default AUTH_DB rules (mirrors backend checks). */ +export function satisfiesDefaultAuthDbPasswordPolicy(password: string): boolean { + return satisfiesAuthDbPasswordPolicy(password, AUTH_DB_DEFAULT_PASSWORD_POLICY); +} + +/** Returns rule-by-rule checks for default AUTH_DB password policy. */ +export function getAuthDbPasswordPolicyChecks( + password: string, + policy: AuthDbPasswordPolicy = AUTH_DB_DEFAULT_PASSWORD_POLICY, +): AuthDbPasswordPolicyChecks { + const minLength = resolveAuthDbPasswordMinLength(policy.password_min_length); + return { + minLength: getCodePointLength(password) >= minLength, + uppercase: !policy.password_require_uppercase || /[A-Z]/.test(password), + lowercase: !policy.password_require_lowercase || /[a-z]/.test(password), + digit: !policy.password_require_digit || /\d/.test(password), Review Comment: **Suggestion:** This digit check is narrower than backend validation: JavaScript `\d` matches only ASCII digits, while backend Python `\d` accepts Unicode decimal digits. That mismatch causes frontend-side false rejections for passwords the API would accept, breaking client/server policy consistency. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Password reset UI rejects backend-acceptable Unicode-digit passwords. - ⚠️ Breaks documented parity between frontend and backend password policy. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the Change Password modal implemented in `superset-frontend/src/features/userInfo/UserInfoModal.tsx` lines 86-177; the `ResetPasswordFields` component renders a `new_password` `<FormItem>` with a custom validator at lines 100-115. 2. When the user types a new password, the validator at `UserInfoModal.tsx:100-115` calls `getPasswordPolicyError(password)`, which forwards to `getAuthDbPasswordPolicyError` in `superset-frontend/src/utils/generateAuthDbPassword.ts:219-249` using the current AUTH_DB password policy loaded from `/api/v1/me/password/policy` (lines 3-9). 3. `getAuthDbPasswordPolicyError` calls `getAuthDbPasswordPolicyChecks` at `generateAuthDbPassword.ts:200-216`, where the `digit` check is implemented as `digit: !policy.password_require_digit || /\d/.test(password)` on line 209; enter a password containing only Unicode decimal digits (for example `'١٢٣٤AB!'` using Arabic-Indic digits) so that it contains no ASCII `0-9`. 4. On the backend, the same password would be validated by `validate_auth_db_password` in `superset/utils/auth_db_password.py:160-198`, whose digit rule uses `re.search(r"\d", password)` at line 181; Python `\d` matches Unicode decimal digits, so the backend considers the digit requirement satisfied, while the frontend `/\d/` check at line 209 fails and returns the message "Password must contain at least one digit.", blocking form submission and demonstrating a concrete client/server policy mismatch. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0939681d48e641af98285fc12a13dbb1&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=0939681d48e641af98285fc12a13dbb1&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-frontend/src/utils/generateAuthDbPassword.ts **Line:** 209:209 **Comment:** *Api Mismatch: This digit check is narrower than backend validation: JavaScript `\d` matches only ASCII digits, while backend Python `\d` accepts Unicode decimal digits. That mismatch causes frontend-side false rejections for passwords the API would accept, breaking client/server policy consistency. 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=450bb9eeef079f6222c02888df6b9d0ae6e87b10bddd8512597d33ea49d754a6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=450bb9eeef079f6222c02888df6b9d0ae6e87b10bddd8512597d33ea49d754a6&reaction=dislike'>👎</a> ########## superset/cli/reset.py: ########## @@ -68,7 +68,7 @@ def factory_reset( # Validate the user password = click.prompt("Admin Password", hide_input=True) user = security_manager.find_user(username) - if not user or not check_password_hash(user.password, password): + if not user or not verify_auth_db_password(user.password, password): click.secho("Invalid credentials", fg="red") sys.exit(1) Review Comment: **Suggestion:** This credential check bypasses account state validation by only verifying the password hash. A deactivated admin account can still pass this check and run a full factory reset if it has the `Admin` role. Reuse the standard DB auth flow (or explicitly require `user.is_active`) before allowing reset. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Inactive admin can run destructive factory reset via CLI. - ⚠️ Bypasses standard AUTH_DB account-state checks enforced at login. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Enable the factory reset CLI by setting the `ENABLE_FACTORY_RESET_COMMAND` feature flag so that the guard at `superset/cli/reset.py:53-56` allows `factory_reset` to run, and configure Superset to use AUTH_DB authentication. 2. Create an admin user with the `Admin` role, then deactivate the account (set `user.is_active = False`) via normal administration; note that the standard AUTH_DB login path `auth_user_db` in `superset/security/manager.py:7-35` explicitly rejects users where `user is None or (not user.is_active)` (line 20). 3. On the Superset host, invoke the CLI command `factory_reset` defined in `superset/cli/reset.py:26-80`, enter the deactivated admin's username when prompted at the `@click.option("--username", prompt="Admin Username")` (line 28), and enter their known password at `click.prompt("Admin Password", hide_input=True)` on line 69. 4. The command looks up the user via `security_manager.find_user(username)` (line 70) and then performs only a password check using `verify_auth_db_password(user.password, password)` at line 71; if this returns True and the user still has an `Admin` role (checked at line 74), `ResetSupersetCommand(silent, user, exclude_users, exclude_roles).run()` executes at lines 78-79, allowing a deactivated admin account to perform a full factory reset despite the normal login flow blocking `is_active=False` users. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0e8bb549dff74c819d1ca703ce3d8feb&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=0e8bb549dff74c819d1ca703ce3d8feb&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/cli/reset.py **Line:** 71:73 **Comment:** *Security: This credential check bypasses account state validation by only verifying the password hash. A deactivated admin account can still pass this check and run a full factory reset if it has the `Admin` role. Reuse the standard DB auth flow (or explicitly require `user.is_active`) before allowing reset. 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=16327d0d958d378ea9e59458723a8dea85ed10983ad32ecc54b46b0e92f0234d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=16327d0d958d378ea9e59458723a8dea85ed10983ad32ecc54b46b0e92f0234d&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]
