codeant-ai-for-open-source[bot] commented on code in PR #40670: URL: https://github.com/apache/superset/pull/40670#discussion_r3384779952
########## superset/security/password_complexity.py: ########## @@ -0,0 +1,105 @@ +# 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. +"""Superset password-complexity validator. + +Wired in via ``FAB_PASSWORD_COMPLEXITY_VALIDATOR`` (with +``FAB_PASSWORD_COMPLEXITY_ENABLED``). Flask-AppBuilder runs this callable from +both the WTForms password fields (self-registration, user edit, reset password) +and the User REST API, so a single function enforces the policy across all +password-setting flows. + +The default policy is a minimum length plus a common-password blocklist — +intentionally less draconian than FAB's built-in ``default_password_complexity`` +(which requires 2 uppercase, 1 special, 2 digits, 3 lowercase and length 10). +""" + +from __future__ import annotations + +from flask import current_app +from flask_appbuilder.exceptions import PasswordComplexityValidationError +from flask_babel import gettext as __ + +# A small built-in blocklist of the most common/guessable passwords. Operators +# can extend it with AUTH_PASSWORD_COMMON_BLOCKLIST. (A fuller list or a +# Have-I-Been-Pwned k-anonymity check is a possible follow-up.) +COMMON_PASSWORDS: frozenset[str] = frozenset( + { + "123456", + "123456789", + "12345678", + "1234567890", + "12345", + "111111", + "123123", + "000000", + "password", + "password1", + "password123", + "passw0rd", + "qwerty", + "qwerty123", + "qwertyuiop", + "abc123", + "letmein", + "welcome", + "welcome1", + "admin", + "admin123", + "administrator", + "root", + "superset", + "changeme", + "iloveyou", + "monkey", + "dragon", + "sunshine", + "princess", + "football", + "baseball", + "trustno1", + "login", + "master", + "hello123", + "secret", + "default", + } +) + +DEFAULT_MIN_LENGTH = 8 + + +def validate_password_complexity(password: str) -> None: + """Validate a plaintext password against the configured policy. + + :raises PasswordComplexityValidationError: if the password is too short or + appears in the common-password blocklist. + """ + min_length = current_app.config.get("AUTH_PASSWORD_MIN_LENGTH", DEFAULT_MIN_LENGTH) + if len(password) < min_length: + raise PasswordComplexityValidationError( + __( + "Password must be at least %(min_length)s characters long.", + min_length=min_length, + ) + ) + + extra = current_app.config.get("AUTH_PASSWORD_COMMON_BLOCKLIST") or [] + blocklist = COMMON_PASSWORDS | {str(item).lower() for item in extra} Review Comment: **Suggestion:** The extra blocklist value is treated as a generic iterable, so if an operator provides a single string (for example from environment-driven config), it is split into characters and the intended password entries are never blocked. Normalize this setting to a list of full password strings (or explicitly reject plain strings) before building the blocklist. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Organization-specific weak passwords remain allowed despite blocklist. - ⚠️ Password policy weaker than operators expect or document. - ⚠️ Harder to enforce compliance with internal password standards. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Superset wires the password complexity validator in `superset/config.py:7-19`, documenting that policy is tuned via `AUTH_PASSWORD_COMMON_BLOCKLIST` and setting `FAB_PASSWORD_COMPLEXITY_VALIDATOR = _validate_password_complexity` so all password-setting flows go through `validate_password_complexity()`. 2. The Docker config loader at `superset/docker/pythonpath_dev/superset_config.py:139-148` imports `superset_config_docker`, which can override `AUTH_PASSWORD_COMMON_BLOCKLIST`; an operator may define `AUTH_PASSWORD_COMMON_BLOCKLIST = os.getenv("AUTH_PASSWORD_COMMON_BLOCKLIST", "AcmeCorp2024")`, leaving it as a single string instead of a list. 3. At runtime, when a password is set via any FAB-backed flow (self-registration, user edit/reset, or User REST API as described in `superset/config.py:7-11`), Flask-AppBuilder calls `validate_password_complexity()` in `superset/security/password_complexity.py:85-105`. 4. In `validate_password_complexity()`, line `100` assigns `extra = current_app.config.get("AUTH_PASSWORD_COMMON_BLOCKLIST") or []`, so `extra` is the string `"AcmeCorp2024"`, and line `101` builds `blocklist = COMMON_PASSWORDS | {str(item).lower() for item in extra}`, iterating over characters instead of full passwords; the check at line `102` (`if password.lower() in blocklist:`) does not match `"acmecorp2024"`, so the weak, organization-specific password is accepted despite the operator's intent to block it. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b4a4e858dfb54a26b0f1f86a55032c04&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b4a4e858dfb54a26b0f1f86a55032c04&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/password_complexity.py **Line:** 100:101 **Comment:** *Security: The extra blocklist value is treated as a generic iterable, so if an operator provides a single string (for example from environment-driven config), it is split into characters and the intended password entries are never blocked. Normalize this setting to a list of full password strings (or explicitly reject plain strings) before building the blocklist. 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%2F40670&comment_hash=4042fa7d6ebae86d3cc8b4a175ceb760e7ac5b67d413db3e28df141f7b82dab1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40670&comment_hash=4042fa7d6ebae86d3cc8b4a175ceb760e7ac5b67d413db3e28df141f7b82dab1&reaction=dislike'>👎</a> ########## superset/security/password_complexity.py: ########## @@ -0,0 +1,105 @@ +# 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. +"""Superset password-complexity validator. + +Wired in via ``FAB_PASSWORD_COMPLEXITY_VALIDATOR`` (with +``FAB_PASSWORD_COMPLEXITY_ENABLED``). Flask-AppBuilder runs this callable from +both the WTForms password fields (self-registration, user edit, reset password) +and the User REST API, so a single function enforces the policy across all +password-setting flows. + +The default policy is a minimum length plus a common-password blocklist — +intentionally less draconian than FAB's built-in ``default_password_complexity`` +(which requires 2 uppercase, 1 special, 2 digits, 3 lowercase and length 10). +""" + +from __future__ import annotations + +from flask import current_app +from flask_appbuilder.exceptions import PasswordComplexityValidationError +from flask_babel import gettext as __ + +# A small built-in blocklist of the most common/guessable passwords. Operators +# can extend it with AUTH_PASSWORD_COMMON_BLOCKLIST. (A fuller list or a +# Have-I-Been-Pwned k-anonymity check is a possible follow-up.) +COMMON_PASSWORDS: frozenset[str] = frozenset( + { + "123456", + "123456789", + "12345678", + "1234567890", + "12345", + "111111", + "123123", + "000000", + "password", + "password1", + "password123", + "passw0rd", + "qwerty", + "qwerty123", + "qwertyuiop", + "abc123", + "letmein", + "welcome", + "welcome1", + "admin", + "admin123", + "administrator", + "root", + "superset", + "changeme", + "iloveyou", + "monkey", + "dragon", + "sunshine", + "princess", + "football", + "baseball", + "trustno1", + "login", + "master", + "hello123", + "secret", + "default", + } +) + +DEFAULT_MIN_LENGTH = 8 + + +def validate_password_complexity(password: str) -> None: + """Validate a plaintext password against the configured policy. + + :raises PasswordComplexityValidationError: if the password is too short or + appears in the common-password blocklist. + """ + min_length = current_app.config.get("AUTH_PASSWORD_MIN_LENGTH", DEFAULT_MIN_LENGTH) + if len(password) < min_length: Review Comment: **Suggestion:** `AUTH_PASSWORD_MIN_LENGTH` is read from config without type coercion, so a common deployment pattern like `os.getenv("AUTH_PASSWORD_MIN_LENGTH", "8")` will pass a string and make `len(password) < min_length` raise a `TypeError` at runtime. Coerce and validate this setting to an integer before comparing so password-setting flows fail with a proper validation error instead of a server error. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Self-registration fails with 500 when length config mis-typed. - ❌ Password reset API fails under misconfigured minimum length. - ⚠️ Operators get no clear feedback on configuration errors. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Superset loads default password configuration from `superset/config.py:7-19`, which defines `AUTH_PASSWORD_MIN_LENGTH = 8` and wires `FAB_PASSWORD_COMPLEXITY_VALIDATOR = _validate_password_complexity` so Flask-AppBuilder uses `validate_password_complexity()` for all password-setting flows. 2. The Docker config loader at `superset/docker/pythonpath_dev/superset_config.py:139-148` imports `superset_config_docker`, whose documented purpose is to override defaults; an operator can define `AUTH_PASSWORD_MIN_LENGTH = os.getenv("AUTH_PASSWORD_MIN_LENGTH", "16")` there, making `current_app.config["AUTH_PASSWORD_MIN_LENGTH"]` a string. 3. When a user hits any FAB-backed password flow (self-registration, user edit/reset form, or User REST API as documented in `superset/config.py:7-11`), Flask-AppBuilder calls `superset.security.password_complexity.validate_password_complexity()` at `superset/security/password_complexity.py:85-92`. 4. Inside `validate_password_complexity()`, line `91` reads `min_length = current_app.config.get("AUTH_PASSWORD_MIN_LENGTH", DEFAULT_MIN_LENGTH)` and line `92` executes `if len(password) < min_length:` where `len(password)` is an `int` and `min_length` is the string `"16"`, causing a `TypeError: '<' not supported between instances of 'int' and 'str'` and turning the password-setting request into a 500 error instead of a clean validation message. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c96a23a00db743b0a8ff7ef2d453d470&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c96a23a00db743b0a8ff7ef2d453d470&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/password_complexity.py **Line:** 91:92 **Comment:** *Type Error: `AUTH_PASSWORD_MIN_LENGTH` is read from config without type coercion, so a common deployment pattern like `os.getenv("AUTH_PASSWORD_MIN_LENGTH", "8")` will pass a string and make `len(password) < min_length` raise a `TypeError` at runtime. Coerce and validate this setting to an integer before comparing so password-setting flows fail with a proper validation error instead of a server error. 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%2F40670&comment_hash=fb6a1d582299e36ac9fd52e849f78d2567cc1417e0d961333b296ebe16bf9e68&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40670&comment_hash=fb6a1d582299e36ac9fd52e849f78d2567cc1417e0d961333b296ebe16bf9e68&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]
