codeant-ai-for-open-source[bot] commented on code in PR #40670: URL: https://github.com/apache/superset/pull/40670#discussion_r3401926850
########## superset/security/password_complexity.py: ########## @@ -0,0 +1,121 @@ +# 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. + """ + raw_min_length = current_app.config.get( + "AUTH_PASSWORD_MIN_LENGTH", DEFAULT_MIN_LENGTH + ) + # Operators commonly wire config via env vars, so AUTH_PASSWORD_MIN_LENGTH can + # arrive as a string (or be left unset/None). Coerce defensively and fall back + # to the default rather than blowing up every password-setting flow with a + # TypeError on the length comparison. + try: + min_length = int(raw_min_length) + except (TypeError, ValueError): + min_length = DEFAULT_MIN_LENGTH + + if len(password) < min_length: Review Comment: **Suggestion:** `AUTH_PASSWORD_MIN_LENGTH` is coerced to `int` but never validated to be positive. If an operator sets `0` or a negative value (for example via env), the length check is effectively disabled and very weak passwords can be accepted. Clamp invalid/non-positive values back to the secure default (or reject startup config) before comparing password length. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Password length policy can be disabled via misconfiguration. - ❌ Self-registration form accepts single-character passwords when misconfigured. - ❌ User REST API allows trivially weak passwords when misconfigured. - ❌ Violates ASVS password minimum length requirements. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `superset/config.py:371-383`, observe that `FAB_PASSWORD_COMPLEXITY_VALIDATOR` is set to `superset.security.password_complexity.validate_password_complexity` and that `AUTH_PASSWORD_MIN_LENGTH = 8` configures the minimum password length used by the validator for self-registration, user edit/reset forms, and the User REST API (as documented in the surrounding comments). 2. Following the pattern in `tests/unit_tests/security/test_password_complexity.py:25-33` (which modifies `current_app.config["AUTH_PASSWORD_MIN_LENGTH"]` before calling the validator), start an app context and set `current_app.config["AUTH_PASSWORD_MIN_LENGTH"] = 0` (or `-1`). 3. Call `validate_password_complexity("a")` from `superset.security.password_complexity` (see function definition at `superset/security/password_complexity.py:85-41`). In the body at `superset/security/password_complexity.py:91-24`, `raw_min_length` is read from config, `min_length = int(raw_min_length)` succeeds with value `0`, and the check `if len(password) < min_length:` (line 103 in the PR hunk) evaluates to `1 < 0`, which is false, so no `PasswordComplexityValidationError` is raised for a 1-character password. 4. In a running Superset instance where `AUTH_PASSWORD_MIN_LENGTH` is misconfigured to `0` or a negative value (via config or environment overrides), every password-setting flow that relies on FAB's validator (self-registration, user edit/reset forms, and the User REST API configured in `superset/config.py:371-383`) will accept arbitrarily short passwords, effectively disabling the minimum length policy and weakening authentication. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=81a03019845f4ea49e44c2d5fafda90a&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=81a03019845f4ea49e44c2d5fafda90a&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:** 98:103 **Comment:** *Security: `AUTH_PASSWORD_MIN_LENGTH` is coerced to `int` but never validated to be positive. If an operator sets `0` or a negative value (for example via env), the length check is effectively disabled and very weak passwords can be accepted. Clamp invalid/non-positive values back to the secure default (or reject startup config) before comparing password length. 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=f1564214c8b55aa5964e2652fdad09356a7137421089b644c0897df15a156e26&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40670&comment_hash=f1564214c8b55aa5964e2652fdad09356a7137421089b644c0897df15a156e26&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]
