codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3655697928
########## tests/unit_tests/migrations/test_cleanup_stale_can_import_pvm_transaction.py: ########## @@ -0,0 +1,126 @@ +# 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. +"""Tests for migration ``a7d3f1b9c2e4_cleanup_stale_can_import_pvm``'s transaction +handling: ``upgrade()``/``downgrade()`` must flush, not commit, so the migration's +session stays inside Alembic's own transaction instead of committing early. +""" + +from importlib import import_module +from unittest.mock import patch + +import pytest +from sqlalchemy import create_engine +from sqlalchemy.orm import Session + +from superset.migrations.shared.security_converge import ( + _add_permission, + _add_permission_view, + _add_view_menu, + _find_pvm, + Base, + Role, +) + +migration = import_module( + "superset.migrations.versions." + "2026-06-23_03-23_a7d3f1b9c2e4_cleanup_stale_can_import_pvm" +) + +VIEW = "ImportExportRestApi" +STALE_PERM = "can_import" # no trailing underscore +LIVE_PERM = "can_import_" # trailing underscore (the real permission) + + [email protected] +def engine(): + engine = create_engine("sqlite:///:memory:") + Base.metadata.create_all(engine) + return engine + + +def _seed_stale_pvm(session: Session) -> None: + view_menu = _add_view_menu(session, VIEW) + stale_permission = _add_permission(session, STALE_PERM) + stale_pvm = _add_permission_view(session, stale_permission, view_menu) + role = Role(name="stale_import_role") + role.permissions.append(stale_pvm) + session.add(role) + session.commit() + + +def test_upgrade_flushes_without_committing(engine) -> None: + """upgrade() must not call session.commit(). The migration session shares + Alembic's connection, so an explicit commit there would end the outer + transaction early and let the permission changes land even if a later step + (including Alembic's own revision stamp write) fails.""" + with Session(engine) as seed: + _seed_stale_pvm(seed) + + with engine.begin() as conn: + with ( + patch.object(migration, "op") as mock_op, + patch.object(Session, "commit") as mock_commit, + ): + mock_op.get_bind.return_value = conn + migration.upgrade() + mock_commit.assert_not_called() Review Comment: **Suggestion:** The test only verifies that `commit()` was not called; it never verifies that the migration explicitly flushes its changes. Queries performed by the migration or later verification can trigger SQLAlchemy autoflush, allowing an implementation with no explicit `flush()` to pass while still violating the transaction-handling contract described by the test. Patch and assert `Session.flush()` as well, or otherwise make the test require an explicit flush. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Migration tests can miss regressions in transaction sequencing. - ⚠️ Autoflush-dependent migrations may behave inconsistently across database backends. - ❌ Failed migration steps may be harder to roll back predictably. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run `test_upgrade_flushes_without_committing()` in `tests/unit_tests/migrations/test_cleanup_stale_can_import_pvm_transaction.py:65-87`; it patches only `Session.commit()` at lines 75-80. 2. The test invokes `migration.upgrade()` from the dynamically imported migration at lines 38-41, but does not patch or assert `Session.flush()`. 3. Change the migration implementation so it omits an explicit `flush()` while retaining database queries that can trigger SQLAlchemy autoflush; the test still reaches `mock_commit.assert_not_called()` successfully. 4. The outer transaction at lines 73-82 can then commit the changes, and the verification queries at lines 85-87 pass, despite the test never proving that `upgrade()` explicitly flushed before completing. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f7753e8e02f7462fac70a141dfcd6dfa&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=f7753e8e02f7462fac70a141dfcd6dfa&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:** tests/unit_tests/migrations/test_cleanup_stale_can_import_pvm_transaction.py **Line:** 75:80 **Comment:** *Possible Bug: The test only verifies that `commit()` was not called; it never verifies that the migration explicitly flushes its changes. Queries performed by the migration or later verification can trigger SQLAlchemy autoflush, allowing an implementation with no explicit `flush()` to pass while still violating the transaction-handling contract described by the test. Patch and assert `Session.flush()` as well, or otherwise make the test require an explicit flush. 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=1666b3cec73a04c71549a6caa675264ee87e4e1341ba6c03f839029f80265723&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=1666b3cec73a04c71549a6caa675264ee87e4e1341ba6c03f839029f80265723&reaction=dislike'>👎</a> ########## superset/utils/auth_db_password.py: ########## @@ -0,0 +1,268 @@ +# 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, +} + +_PUBLIC_PASSWORD_POLICY_KEYS: tuple[str, ...] = ( + "password_min_length", + "password_require_uppercase", + "password_require_lowercase", + "password_require_digit", + "password_require_special", + "password_common_list_check", +) + +_SUPPORTED_HASH_ALGORITHMS: frozenset[str] = frozenset({"bcrypt", "argon2"}) + +# bcrypt silently truncates input beyond this UTF-8 byte limit. +BCRYPT_MAX_PASSWORD_BYTES: int = 72 + +_COMMON_PASSWORDS: frozenset[str] = 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: Any = app.config.get("AUTH_DB_CONFIG") or {} + if not isinstance(overrides, Mapping): + raise ValidationError( + { + "AUTH_DB_CONFIG": [ + "Invalid AUTH_DB_CONFIG. Expected a mapping of policy options." + ] + } + ) + 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``). + Falls back to the default when the configured value isn't a usable string (for + example ``None`` or a malformed override), so a misconfigured ``AUTH_DB_CONFIG`` + can't break rate limiting at request time. + """ + default = str(AUTH_DB_DEFAULTS["login_rate_limit"]) + raw = get_merged_auth_db_config().get("login_rate_limit", default) + if isinstance(raw, str) and raw.strip(): + return raw Review Comment: **Suggestion:** The implementation returns every non-empty string unchanged, even when it is not a valid Flask-Limiter rate-limit expression. The docstring promises malformed overrides fall back to the default, but malformed values can reach the limiter and cause request-time rate-limit parsing failures. Parse or validate the configured expression before returning it, and use the default when parsing fails. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ AUTH_DB password-change requests can fail with invalid configuration. - ⚠️ Rate limiting may be unavailable for password verification. - ⚠️ Operator configuration errors surface at request time. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure Superset's `AUTH_DB_CONFIG` with a non-empty but invalid `login_rate_limit` string, such as `"not-a-limiter-expression"`, which is read by `get_merged_auth_db_config()` at `superset/utils/auth_db_password.py:104-115`. 2. Invoke `get_auth_db_login_rate_limit_string()` at `superset/utils/auth_db_password.py:118`, which is used by the AUTH_DB password-verification rate-limiting setup for `PUT /api/v1/me/password`. 3. The function reaches `superset/utils/auth_db_password.py:129-130`; because the value is a non-empty string, it returns the malformed expression instead of the documented default. 4. When Flask-Limiter parses or applies that expression for the password endpoint, rate-limit configuration parsing can fail, preventing password-verification requests from being handled normally. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7e84928bede041b68c6e12ed6b889579&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=7e84928bede041b68c6e12ed6b889579&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:** 129:130 **Comment:** *Logic Error: The implementation returns every non-empty string unchanged, even when it is not a valid Flask-Limiter rate-limit expression. The docstring promises malformed overrides fall back to the default, but malformed values can reach the limiter and cause request-time rate-limit parsing failures. Parse or validate the configured expression before returning it, and use the default when parsing fails. 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=b2ec836d53555c5965678f66556f053fd8cc3756e3bc3dddd8a2e0d7d58041e0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=b2ec836d53555c5965678f66556f053fd8cc3756e3bc3dddd8a2e0d7d58041e0&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]
