codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3596038377
########## superset/config.py: ########## @@ -463,6 +464,17 @@ def _try_json_readsha(filepath: str, length: int) -> str | None: # AUTH_REMOTE_USER : Is for using REMOTE_USER from web server AUTH_TYPE = AUTH_DB +# Consolidated database-auth (AUTH_DB) password and security tuning. +# ``superset.utils.auth_db_password.get_merged_auth_db_config()`` merges +# ``AUTH_DB_DEFAULTS`` with any ``AUTH_DB_CONFIG`` dict from +# ``superset_config`` (partial overrides supported). +AUTH_DB_CONFIG: dict[str, Any] = dict(AUTH_DB_DEFAULTS) + +# How often (seconds) to re-check the session auth stamp against the database on +# read-only requests (GET/HEAD/OPTIONS). State-changing requests always +# revalidate. Set to 0 to check on every authenticated request. +SESSION_AUTH_STAMP_REVALIDATION_SECONDS: int = 300 Review Comment: **Suggestion:** Setting the default revalidation interval to 300 seconds introduces a security window where stale sessions can keep passing GET/HEAD/OPTIONS checks without a DB lookup. This is especially problematic for password changes performed outside the self-service flow (for example admin resets), where the stamp is bumped but the shared stamp cache is not immediately refreshed, allowing old sessions to continue reading data until the interval expires. Use a default of 0 (always revalidate) or ensure every stamp bump path updates the shared cache immediately. [security] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx β Admin ResetPasswordView leaves compromised sessions valid during TTL. β GET /api/v1/me/ still authorized after admin reset. β οΈ Cross-node session invalidation delayed for AUTH_DB admin resets. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Start Superset with AUTH_DB enabled and the default configuration in `superset/config.py` lines 465β476, where `AUTH_TYPE = AUTH_DB` and `SESSION_AUTH_STAMP_REVALIDATION_SECONDS: int = 300` is set. 2. Log in as an AUTH_DB user to create a session stamp; on successful login, `SupersetSecurityManager.on_user_login` in `superset/security/manager.py:1232β1241` calls `sync_session_auth_stamp_on_login(user)` from `superset/utils/auth_session_stamp.py:153β165`, which writes the stamp into the signed session cookie, marks it validated via `_mark_session_stamp_validated` at `auth_session_stamp.py:261β265`, and caches it with `_cache_user_session_stamp`. 3. As an administrator, reset this same userβs password via the FAB βReset Passwordβ flow (ResetPasswordView), which routes through `SupersetSecurityManager.reset_password` in `superset/security/manager.py:1161β1218`; when `AUTH_TYPE == AUTH_DB`, this method calls `bump_user_session_auth_stamp(target_user_id)` at `auth_session_stamp.py:172β194` to rotate the DB-stored stamp but does not call `cache_user_session_auth_stamp` (`auth_session_stamp.py:167β170`), leaving the shared cache and existing browser sessions with the old stamp. 4. Within 300 seconds of the last stamp validation, have the affected user reuse their existing browser session to issue a read-only request such as `GET /api/v1/me/`, handled by `CurrentUserRestApi.get_me` in `superset/views/users/api.py:192β217`; the `before_request` hook `_validate_user_session_auth_stamp` in `auth_session_stamp.py:108β111` invokes `validate_session_auth_stamp_for_request` (`auth_session_stamp.py:300β382`), which consults `_should_skip_stamp_db_lookup` at `auth_session_stamp.py:232β258`. Because `SESSION_AUTH_STAMP_REVALIDATION_SECONDS` is 300, the sessionβs `SESSION_AUTH_STAMP_VALIDATED_AT_KEY` timestamp is recent, and the cached stamp still matches the old session stamp (no cache bump happened in `reset_password`), `_should_skip_stamp_db_lookup` returns `True` and the DB lookup is skipped, allowing the stale session to continue reading data until the revalidation interval and cache TTL expire. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e98cad2ff3b74a53ba70925058633fd4&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=e98cad2ff3b74a53ba70925058633fd4&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/config.py **Line:** 476:476 **Comment:** *Security: Setting the default revalidation interval to 300 seconds introduces a security window where stale sessions can keep passing GET/HEAD/OPTIONS checks without a DB lookup. This is especially problematic for password changes performed outside the self-service flow (for example admin resets), where the stamp is bumped but the shared stamp cache is not immediately refreshed, allowing old sessions to continue reading data until the interval expires. Use a default of 0 (always revalidate) or ensure every stamp bump path updates the shared cache immediately. 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=fd84ff003f88fe891d501146bf8a2052a052ffd13353e86eda0529209c092341&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=fd84ff003f88fe891d501146bf8a2052a052ffd13353e86eda0529209c092341&reaction=dislike'>π</a> ########## superset/security/manager.py: ########## @@ -4551,6 +4596,39 @@ def is_admin(self) -> bool: role.name for role in self.get_user_roles() ] + def auth_user_db(self, username: str, password: str) -> User | None: + """ + Authenticate a database user, verifying bcrypt/argon2 and legacy hashes. + """ + if username is None or username == "": + return None + first_user = self.get_first_user() + user = self.find_user(username=username) + if user is None: + user = self.find_user(email=username) + else: + # Balance failure and success + _ = self.find_user(email=username) + if user is None or (not user.is_active): + # Balance failure and success + check_password_hash( + current_app.config.get( + "AUTH_DB_FAKE_PASSWORD_HASH_CHECK", + DEFAULT_AUTH_DB_FAKE_PASSWORD_HASH_CHECK, + ), + "password", + ) + logger.info(LOGMSG_WAR_SEC_LOGIN_FAILED, username) Review Comment: **Suggestion:** The failed-login timing equalization path ignores the attacker-supplied password and always checks the literal string `"password"` against a Werkzeug hash. Because real-user checks use `verify_auth_db_password(user.password, password)` (bcrypt/argon2/legacy) while missing-user checks use a different verifier and fixed input, login timing can diverge enough to leak account existence. Use the same candidate password and the same verification path for both branches (or a fake hash compatible with the same verifier) to avoid user-enumeration side channels. [security] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx - β AUTH_DB login leaks account existence via timing variance. - β οΈ Enables username enumeration with crafted long passwords. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Inspect AUTH_DB login helper `auth_user_db()` in `superset/security/manager.py:4599-131` (loaded via BulkRead), which authenticates database users when `AUTH_TYPE` = `AUTH_DB`. 2. For a missing or inactive user, follow the branch at `manager.py:113-125`: the code calls `check_password_hash(current_app.config.get("AUTH_DB_FAKE_PASSWORD_HASH_CHECK", DEFAULT_AUTH_DB_FAKE_PASSWORD_HASH_CHECK), "password")`, using the literal `"password"` instead of the attacker-supplied candidate. 3. For an existing active user, follow the branch at `manager.py:126-131`: it calls `verify_auth_db_password(user.password, password)` with the attacker-supplied `password`. In `superset/utils/auth_db_password_hash.py:72-103`, the bcrypt path (`lines 81-88`) encodes the candidate and short-circuits to `False` when `len(encoded) > BCRYPT_MAX_PASSWORD_BYTES`. 4. Configure AUTH_DB to use bcrypt (default in `superset/utils/auth_db_password.py:29-42`), create a real user with a bcrypt hash, and compare login timings for that username versus a non-existent username using an overlong password (>72 UTF-8 bytes): the real-user path for bcrypt short-circuits quickly, while the missing-user path always runs the full PBKDF2-based `check_password_hash` on `"password"`, producing measurable timing differences that can be used to infer account existence. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b1fd0bc59f7a47248cf161fdba705303&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=b1fd0bc59f7a47248cf161fdba705303&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/security/manager.py **Line:** 4615:4621 **Comment:** *Security: The failed-login timing equalization path ignores the attacker-supplied password and always checks the literal string `"password"` against a Werkzeug hash. Because real-user checks use `verify_auth_db_password(user.password, password)` (bcrypt/argon2/legacy) while missing-user checks use a different verifier and fixed input, login timing can diverge enough to leak account existence. Use the same candidate password and the same verification path for both branches (or a fake hash compatible with the same verifier) to avoid user-enumeration side channels. 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=92262aa65dc14a0da71abc2681ba54163424aa857944dce27b5fb63d2647a763&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=92262aa65dc14a0da71abc2681ba54163424aa857944dce27b5fb63d2647a763&reaction=dislike'>π</a> ########## superset/utils/auth_db_password.py: ########## @@ -0,0 +1,265 @@ +# 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 collections.abc import Mapping +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, Review Comment: **Suggestion:** These lockout/reset policy keys are introduced as configurable behavior but are not consumed anywhere in the backend auth flow, so changing them has no effect at runtime. This creates a misleading security contract for operators who expect lockout and reset controls to be enforced. Either wire these settings into the corresponding authentication/reset logic or remove them until enforcement is implemented. [incomplete implementation] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx - β οΈ Operators misled; lockout settings silently ineffective. - β οΈ Increased risk of password-guessing without configured lockout. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Inspect `AUTH_DB_DEFAULTS` in `superset/utils/auth_db_password.py:29-42` (BulkRead output): it introduces `reset_token_expiry_minutes`, `reset_rate_limit`, `login_rate_limit`, `login_max_failures`, and `login_lockout_duration_minutes` as configurable AUTH_DB policy options. 2. Use Grep across `/workspace/superset` (tool results included) for each key: `login_max_failures`, `reset_rate_limit`, `reset_token_expiry_minutes`, and `login_lockout_duration_minutes` only appear in `auth_db_password.py:29-42` and are never referenced elsewhere; `login_rate_limit` is used via `get_auth_db_login_rate_limit_string()` in `superset/views/users/api.py:40-46,88-91` to rate-limit `PUT /api/v1/me/password`, confirming the other knobs are truly unused. 3. Inspect the AUTH_DB login flow in `superset/security/manager.py:4599-131`: `auth_user_db()` calls `find_user()` and `verify_auth_db_password()` to decide success and uses `update_user_auth_stat()` on failure, but no code reads `login_max_failures` or `login_lockout_duration_minutes`, so failed-login counts and lockout duration are not enforced. 4. Inspect the password change API in `superset/views/users/api.py:340-64`: it loads the request body, verifies the current password via `verify_auth_db_password()`, validates and hashes the new password, and commits the change, but never consults `reset_token_expiry_minutes` or `reset_rate_limit`. As a result, operators can set these AUTH_DB_CONFIG values expecting lockout or reset semantics, yet the backend authentication/reset flows ignore them and system behavior does not change. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=45d83ec48b484fd9b60837f9654c93e9&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=45d83ec48b484fd9b60837f9654c93e9&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:** 37:41 **Comment:** *Incomplete Implementation: These lockout/reset policy keys are introduced as configurable behavior but are not consumed anywhere in the backend auth flow, so changing them has no effect at runtime. This creates a misleading security contract for operators who expect lockout and reset controls to be enforced. Either wire these settings into the corresponding authentication/reset logic or remove them until enforcement is implemented. 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=6c82842cc9033d0ea5bafc95f5c5d06d3cba102b29ecdb6b0c0359ad983fe19e&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=6c82842cc9033d0ea5bafc95f5c5d06d3cba102b29ecdb6b0c0359ad983fe19e&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]
