codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3502746759
########## superset/utils/auth_db_password_hash.py: ########## @@ -0,0 +1,94 @@ +# 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. +"""Hash and verify AUTH_DB passwords using bcrypt or argon2.""" + +from __future__ import annotations + +import re + +import bcrypt +from argon2 import PasswordHasher +from argon2.exceptions import InvalidHash, VerifyMismatchError +from werkzeug.security import check_password_hash + +from superset.utils.auth_db_password import ( + BCRYPT_MAX_PASSWORD_BYTES, + get_auth_db_password_hash_algorithm, +) + +_BCRYPT_HASH_RE = re.compile(r"^\$2[aby]\$\d{2}\$") +_ARGON2_HASH_PREFIX = "$argon2" + +_argon2_hasher: PasswordHasher = PasswordHasher() + + +def is_bcrypt_password_hash(password_hash: str) -> bool: + """Return True when ``password_hash`` uses the bcrypt wire format.""" + return bool(_BCRYPT_HASH_RE.match(password_hash)) + + +def is_argon2_password_hash(password_hash: str) -> bool: + """Return True when ``password_hash`` uses the argon2 wire format.""" + return password_hash.startswith(_ARGON2_HASH_PREFIX) + + +def hash_auth_db_password(password: str, algorithm: str | None = None) -> str: + """ + Hash a plaintext password for AUTH_DB storage. + + Uses ``AUTH_DB_CONFIG["password_hash_algorithm"]`` when ``algorithm`` is omitted. + """ + resolved = algorithm or get_auth_db_password_hash_algorithm() + if resolved == "argon2": + return _argon2_hasher.hash(password) + if resolved == "bcrypt": + encoded = password.encode("utf-8") + if len(encoded) > BCRYPT_MAX_PASSWORD_BYTES: + raise ValueError( + f"Password exceeds bcrypt's {BCRYPT_MAX_PASSWORD_BYTES}-byte limit." + ) + return bcrypt.hashpw(encoded, bcrypt.gensalt()).decode("utf-8") + raise ValueError(f"Unsupported AUTH_DB hash algorithm: {resolved}") + + +def verify_auth_db_password(password_hash: str, password: str) -> bool: + """ + Verify a candidate password against a stored hash. + + Supports bcrypt, argon2, and legacy Werkzeug hashes (for example scrypt, pbkdf2) + so existing users can log in until they rotate their password. + """ + if not password_hash: + return False + if is_bcrypt_password_hash(password_hash): + try: + return bcrypt.checkpw( + password.encode("utf-8"), + password_hash.encode("utf-8"), + ) + except (ValueError, TypeError): + return False Review Comment: **Suggestion:** The bcrypt verification path does not enforce the 72-byte input limit, so passwords that differ only after byte 72 can still authenticate as equal. Add the same byte-length guard used during hashing and return False when the candidate exceeds the bcrypt limit to prevent truncated-password equivalence. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Different long passwords treated as same bcrypt credential. ⚠️ AUTH_DB login semantics inconsistent for overlong bcrypt passwords. ⚠️ Password change verification accepts ambiguous overlong bcrypt inputs. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure Superset to use AUTH_DB so database password authentication goes through `_auth_user_db`, which calls `verify_auth_db_password(user.password, password)` (`superset/security/manager.py:4360-4375`). 2. Ensure there is an AUTH_DB user record whose stored bcrypt password hash was generated from a plaintext longer than `BCRYPT_MAX_PASSWORD_BYTES` (72 bytes), for example by inserting a hash via SQL or from an earlier system that did not enforce the length limit (user is loaded at `superset/views/users/api.py:16-20` and `superset/security/manager.py:4360-4375`). 3. Log in as that user through the standard AUTH_DB login flow so `_auth_user_db` invokes `verify_auth_db_password()` in `superset/utils/auth_db_password_hash.py:68-84` with the stored bcrypt hash and the candidate password provided by the user. 4. Provide a candidate password whose first 72 UTF-8 bytes match the original but which differs after byte 72; in `verify_auth_db_password()` the bcrypt branch at `superset/utils/auth_db_password_hash.py:77-83` passes `password.encode("utf-8")` directly to `bcrypt.checkpw()` without enforcing the 72-byte limit, so bcrypt silently truncates the candidate to 72 bytes and returns `True`, allowing multiple distinct longer passwords with the same 72-byte prefix to authenticate as the same credential. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5c3c1f0bdb5e468681f9b8199bd963a9&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=5c3c1f0bdb5e468681f9b8199bd963a9&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_hash.py **Line:** 77:84 **Comment:** *Security: The bcrypt verification path does not enforce the 72-byte input limit, so passwords that differ only after byte 72 can still authenticate as equal. Add the same byte-length guard used during hashing and return False when the candidate exceeds the bcrypt limit to prevent truncated-password equivalence. 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=24c2da3fa2dfee7d5d40167e50d131459364cb38c89004897275234674019724&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=24c2da3fa2dfee7d5d40167e50d131459364cb38c89004897275234674019724&reaction=dislike'>👎</a> ########## superset/utils/auth_db_password_hash.py: ########## @@ -0,0 +1,94 @@ +# 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. +"""Hash and verify AUTH_DB passwords using bcrypt or argon2.""" + +from __future__ import annotations + +import re + +import bcrypt +from argon2 import PasswordHasher +from argon2.exceptions import InvalidHash, VerifyMismatchError +from werkzeug.security import check_password_hash + +from superset.utils.auth_db_password import ( + BCRYPT_MAX_PASSWORD_BYTES, + get_auth_db_password_hash_algorithm, +) + +_BCRYPT_HASH_RE = re.compile(r"^\$2[aby]\$\d{2}\$") +_ARGON2_HASH_PREFIX = "$argon2" + +_argon2_hasher: PasswordHasher = PasswordHasher() + + +def is_bcrypt_password_hash(password_hash: str) -> bool: + """Return True when ``password_hash`` uses the bcrypt wire format.""" + return bool(_BCRYPT_HASH_RE.match(password_hash)) + + +def is_argon2_password_hash(password_hash: str) -> bool: + """Return True when ``password_hash`` uses the argon2 wire format.""" + return password_hash.startswith(_ARGON2_HASH_PREFIX) + + +def hash_auth_db_password(password: str, algorithm: str | None = None) -> str: + """ + Hash a plaintext password for AUTH_DB storage. + + Uses ``AUTH_DB_CONFIG["password_hash_algorithm"]`` when ``algorithm`` is omitted. + """ + resolved = algorithm or get_auth_db_password_hash_algorithm() + if resolved == "argon2": + return _argon2_hasher.hash(password) + if resolved == "bcrypt": + encoded = password.encode("utf-8") + if len(encoded) > BCRYPT_MAX_PASSWORD_BYTES: + raise ValueError( + f"Password exceeds bcrypt's {BCRYPT_MAX_PASSWORD_BYTES}-byte limit." + ) + return bcrypt.hashpw(encoded, bcrypt.gensalt()).decode("utf-8") + raise ValueError(f"Unsupported AUTH_DB hash algorithm: {resolved}") + + +def verify_auth_db_password(password_hash: str, password: str) -> bool: + """ + Verify a candidate password against a stored hash. + + Supports bcrypt, argon2, and legacy Werkzeug hashes (for example scrypt, pbkdf2) + so existing users can log in until they rotate their password. + """ + if not password_hash: + return False + if is_bcrypt_password_hash(password_hash): + try: + return bcrypt.checkpw( + password.encode("utf-8"), + password_hash.encode("utf-8"), + ) + except (ValueError, TypeError): + return False + if is_argon2_password_hash(password_hash): + try: + _argon2_hasher.verify(password_hash, password) + return True + except (VerifyMismatchError, InvalidHash): + return False Review Comment: **Suggestion:** The argon2 verify branch only handles mismatch/invalid-hash exceptions, but argon2 can also raise broader verification errors for malformed or corrupted hashes. Catch the broader verification exception class as well and return False so authentication failures do not bubble into 500 errors. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ❌ AUTH_DB login can 500 on corrupted argon2 hashes. ❌ Password change API 500s when argon2 hash invalid. ⚠️ CLI factory reset fails if admin argon2 hash bad. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Set `AUTH_DB_CONFIG["password_hash_algorithm"]` to `"argon2"` so `hash_auth_db_password()` at `superset/utils/auth_db_password_hash.py:49-57` stores argon2 hashes for AUTH_DB users. 2. Create or modify an AUTH_DB user such that the `password` column contains a malformed or corrupted argon2 hash (for example, truncating the stored hash via direct SQL), so `_argon2_hasher.verify()` is likely to raise a verification-related exception that is not `VerifyMismatchError` or `InvalidHash`. 3. Attempt to log in with that user through the AUTH_DB login flow; `_auth_user_db` in `superset/security/manager.py:4360-4375` calls `verify_auth_db_password(user.password, password)` with the corrupted argon2 hash. 4. Inside `verify_auth_db_password()` at `superset/utils/auth_db_password_hash.py:85-90`, the argon2 branch calls `_argon2_hasher.verify(password_hash, password)` and only catches `VerifyMismatchError` and `InvalidHash`, so a different argon2 verification exception propagates up the call stack, turning the authentication attempt (or the password change flow in `superset/views/users/api.py:11-24`) into an unhandled error that can surface as an HTTP 500 response instead of a clean authentication failure. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9c4cf584e19c45f6a89c1638a4166913&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=9c4cf584e19c45f6a89c1638a4166913&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_hash.py **Line:** 85:90 **Comment:** *Api Mismatch: The argon2 verify branch only handles mismatch/invalid-hash exceptions, but argon2 can also raise broader verification errors for malformed or corrupted hashes. Catch the broader verification exception class as well and return False so authentication failures do not bubble into 500 errors. 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=ceae6c0dd39ee9764a7de184e17f93dc5522c2aabc11b661ced03cad97974766&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=ceae6c0dd39ee9764a7de184e17f93dc5522c2aabc11b661ced03cad97974766&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]
