codeant-ai-for-open-source[bot] commented on code in PR #39469: URL: https://github.com/apache/superset/pull/39469#discussion_r3425688368
########## superset-frontend/src/utils/generateAuthDbPassword.ts: ########## @@ -0,0 +1,183 @@ +/** + * 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. + */ +/** + * Client-side generator aligned with default ``AUTH_DB_CONFIG`` / Python + * ``superset.utils.auth_db_password`` (minimum length, character classes, common list). + * Keep ``AUTH_DB_COMMON_PASSWORDS`` in sync with ``_COMMON_PASSWORDS`` in that module. + */ +export const AUTH_DB_PASSWORD_MIN_LENGTH = 12; +export interface AuthDbPasswordPolicy { + password_min_length: number; + password_require_uppercase: boolean; + password_require_lowercase: boolean; + password_require_digit: boolean; + password_require_special: boolean; + password_common_list_check: boolean; +} + +export const AUTH_DB_DEFAULT_PASSWORD_POLICY: AuthDbPasswordPolicy = { + password_min_length: AUTH_DB_PASSWORD_MIN_LENGTH, + password_require_uppercase: true, + password_require_lowercase: true, + password_require_digit: true, + password_require_special: true, + password_common_list_check: true, +}; + +/** Lowercased entries; keep in sync with ``_COMMON_PASSWORDS`` in auth_db_password.py */ +const AUTH_DB_COMMON_PASSWORDS = new Set( + [ + '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', + ].map(s => s.toLowerCase()), +); + +const UPPER = 'ABCDEFGHJKLMNPQRSTUVWXYZ'; +const LOWER = 'abcdefghijkmnopqrstuvwxyz'; +const DIGIT = '23456789'; +const SPECIAL = '!@#$%^&*-_=+'; +const ALPHANUM = UPPER + LOWER + DIGIT; + +export interface AuthDbPasswordPolicyChecks { + minLength: boolean; + uppercase: boolean; + lowercase: boolean; + digit: boolean; + special: boolean; + commonPassword: boolean; +} + +function secureRandomInt(maxExclusive: number): number { + const buf = new Uint32Array(1); + crypto.getRandomValues(buf); + return buf[0]! % maxExclusive; Review Comment: **Suggestion:** Using modulo reduction on random 32-bit values introduces statistical bias, so some characters are selected more often than others. Because this function is used for password generation, this weakens entropy and should use rejection sampling to produce an unbiased integer range. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Generated passwords have slightly biased character distribution. - ⚠️ Overall password entropy marginally lower than ideal uniform. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open either password-generation UI that uses AUTH_DB passwords: the self-service change password modal (`ChangePasswordModal` / `UserInfoModal` in `superset-frontend/src/features/userInfo/UserInfoModal.tsx:77-79,108-138,200-8`) or the admin reset password modal (`UserListResetPasswordModal` in `superset-frontend/src/features/users/UserListResetPasswordModal.tsx:18-23,41-53`). 2. In either modal, click the `GeneratePasswordInputSuffix` icon on the new-password field. This triggers `onGenerate={() => { const pwd = generateAuthDbPassword(); ... }}` at `UserInfoModal.tsx:200-8` or `UserListResetPasswordModal.tsx:45-50`, which calls `generateAuthDbPassword()` defined in `superset-frontend/src/utils/generateAuthDbPassword.ts:43-61`. 3. `generateAuthDbPassword()` constructs the password by repeatedly calling `pick()` to select characters from the UPPER/LOWER/DIGIT/SPECIAL pools (`generateAuthDbPassword.ts:47-55,9-13,30-32`). `pick()` in turn calls `secureRandomInt(pool.length)` (`generateAuthDbPassword.ts:30-32,24-28`). 4. `secureRandomInt()` obtains a 32-bit random integer from `crypto.getRandomValues` and returns `buf[0] % maxExclusive` (`generateAuthDbPassword.ts:24-27`). Because `maxExclusive` (e.g., the length of `ALPHANUM + SPECIAL`, currently 68) generally does not evenly divide `2^32`, some residues occur one time more than others in the modulo reduction, making certain characters slightly more probable. Every generated password goes through this biased selection path, so the distribution of characters is not perfectly uniform even though `crypto.getRandomValues` itself is unbiased. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ed3b9738c55241ea8110fafad509e0de&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=ed3b9738c55241ea8110fafad509e0de&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-frontend/src/utils/generateAuthDbPassword.ts **Line:** 106:106 **Comment:** *Security: Using modulo reduction on random 32-bit values introduces statistical bias, so some characters are selected more often than others. Because this function is used for password generation, this weakens entropy and should use rejection sampling to produce an unbiased integer range. 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=8955ab16ca1b9bfce93ba4c40b83766d7737bfac9b28a8193eaf52220cf5f632&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=8955ab16ca1b9bfce93ba4c40b83766d7737bfac9b28a8193eaf52220cf5f632&reaction=dislike'>👎</a> ########## superset-frontend/src/utils/generateAuthDbPassword.ts: ########## @@ -0,0 +1,183 @@ +/** + * 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. + */ +/** + * Client-side generator aligned with default ``AUTH_DB_CONFIG`` / Python + * ``superset.utils.auth_db_password`` (minimum length, character classes, common list). + * Keep ``AUTH_DB_COMMON_PASSWORDS`` in sync with ``_COMMON_PASSWORDS`` in that module. + */ +export const AUTH_DB_PASSWORD_MIN_LENGTH = 12; +export interface AuthDbPasswordPolicy { + password_min_length: number; + password_require_uppercase: boolean; + password_require_lowercase: boolean; + password_require_digit: boolean; + password_require_special: boolean; + password_common_list_check: boolean; +} + +export const AUTH_DB_DEFAULT_PASSWORD_POLICY: AuthDbPasswordPolicy = { + password_min_length: AUTH_DB_PASSWORD_MIN_LENGTH, + password_require_uppercase: true, + password_require_lowercase: true, + password_require_digit: true, + password_require_special: true, + password_common_list_check: true, +}; + +/** Lowercased entries; keep in sync with ``_COMMON_PASSWORDS`` in auth_db_password.py */ +const AUTH_DB_COMMON_PASSWORDS = new Set( + [ + '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', + ].map(s => s.toLowerCase()), +); + +const UPPER = 'ABCDEFGHJKLMNPQRSTUVWXYZ'; +const LOWER = 'abcdefghijkmnopqrstuvwxyz'; +const DIGIT = '23456789'; +const SPECIAL = '!@#$%^&*-_=+'; +const ALPHANUM = UPPER + LOWER + DIGIT; + +export interface AuthDbPasswordPolicyChecks { + minLength: boolean; + uppercase: boolean; + lowercase: boolean; + digit: boolean; + special: boolean; + commonPassword: boolean; +} + +function secureRandomInt(maxExclusive: number): number { + const buf = new Uint32Array(1); + crypto.getRandomValues(buf); + return buf[0]! % maxExclusive; +} + +function pick(pool: string): string { + return pool[secureRandomInt(pool.length)]!; +} + +function shuffleInPlace(chars: string[]): void { + for (let i = chars.length - 1; i > 0; i -= 1) { + const j = secureRandomInt(i + 1); + const t = chars[i]!; + chars[i] = chars[j]!; + chars[j] = t; + } +} + +/** True when the string satisfies default AUTH_DB rules (mirrors backend checks). */ +export function satisfiesDefaultAuthDbPasswordPolicy(password: string): boolean { + const checks = getAuthDbPasswordPolicyChecks( + password, + AUTH_DB_DEFAULT_PASSWORD_POLICY, + ); + return ( + checks.minLength && + checks.uppercase && + checks.lowercase && + checks.digit && + checks.special && + checks.commonPassword + ); +} + +/** Returns rule-by-rule checks for default AUTH_DB password policy. */ +export function getAuthDbPasswordPolicyChecks( + password: string, + policy: AuthDbPasswordPolicy = AUTH_DB_DEFAULT_PASSWORD_POLICY, +): AuthDbPasswordPolicyChecks { + const minLength = Math.max(1, Number(policy.password_min_length) || 1); + return { + minLength: password.length >= minLength, + uppercase: !policy.password_require_uppercase || /[A-Z]/.test(password), + lowercase: !policy.password_require_lowercase || /[a-z]/.test(password), + digit: !policy.password_require_digit || /\d/.test(password), + special: + !policy.password_require_special || + /[^\p{L}\p{N}\s]/u.test(password), + commonPassword: + !policy.password_common_list_check || + !AUTH_DB_COMMON_PASSWORDS.has(password.toLowerCase().trim()), + }; +} + +/** + * Returns a random password that should pass ``validate_auth_db_password`` with default + * ``AUTH_DB_CONFIG`` on the server. + */ +export function generateAuthDbPassword(): string { + const minLen = AUTH_DB_PASSWORD_MIN_LENGTH; + const maxAttempts = 64; + for (let attempt = 0; attempt < maxAttempts; attempt += 1) { + const chars: string[] = [ + pick(UPPER), + pick(LOWER), + pick(DIGIT), + pick(SPECIAL), + ]; + const pool = ALPHANUM + SPECIAL; + while (chars.length < minLen) { + chars.push(pick(pool)); + } + shuffleInPlace(chars); + const password = chars.join(''); + if (satisfiesDefaultAuthDbPasswordPolicy(password)) { + return password; + } + } + throw new Error('generateAuthDbPassword: exhausted retries'); +} Review Comment: **Suggestion:** The password generator is hard-wired to the default policy (`AUTH_DB_PASSWORD_MIN_LENGTH` + `satisfiesDefaultAuthDbPasswordPolicy`) and cannot honor a customized `AUTH_DB_CONFIG`. In deployments with stricter policy (for example longer minimum length), generated passwords will frequently fail server-side validation and break the "generate password" flow. Make the generator accept a policy argument and validate against that policy instead of the defaults. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Self-service password generator fails with stricter AUTH_DB_CONFIG. - ⚠️ Admin reset generator produces passwords rejected by backend. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure a stricter AUTH_DB password policy in `superset_config.py`, for example `AUTH_DB_CONFIG = {"password_min_length": 16, ...}` as documented in `docs/admin_docs/configuration/configuring-superset.mdx:15-29`. On the backend this is enforced by `validate_auth_db_password()` in `superset/utils/auth_db_password.py:164-176`, which reads `password_min_length` from the merged config. 2. Start Superset with `AUTH_TYPE = AUTH_DB`, log in as a regular user, and open the "Reset password" dialog, which is implemented by `ChangePasswordModal`/`UserInfoModal` in `superset-frontend/src/features/userInfo/UserInfoModal.tsx:77-79,46-52`. This modal fetches the effective policy from `/api/v1/me/password/policy` and stores it in `passwordPolicy` (`UserInfoModal.tsx:54-68`). 3. In the "New password" field of `ResetPasswordFields` (`UserInfoModal.tsx:108-139`), click the generate icon suffix. The `onGenerate` handler at `UserInfoModal.tsx:200-8` calls `generateAuthDbPassword()` from `superset-frontend/src/utils/generateAuthDbPassword.ts:43-61`, which uses the hard-coded constant `AUTH_DB_PASSWORD_MIN_LENGTH` (12) instead of the fetched `passwordPolicy`. 4. The generated password is exactly 12 characters long (loop at `generateAuthDbPassword.ts:44-55`) and is then validated client-side by `getPasswordPolicyError()` (`UserInfoModal.tsx:75-82`), which calls `getAuthDbPasswordPolicyChecks(password, passwordPolicy)` (`generateAuthDbPassword.ts:60-66`). With `passwordPolicy.password_min_length` = 16, `checks.minLength` is false, so the form validator rejects the generated password (showing "Password must be at least 16 characters long"), even though the generation logic believes it produced a policy-compliant password. The same generator is also used in the admin reset modal `UserListResetPasswordModal` (`superset-frontend/src/features/users/UserListResetPasswordModal.tsx:45-50`), where generated 12-character passwords will be accepted by the frontend but rejected by backend `validate_auth_db_password()`, causing API errors. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a1e21183c9724f089d08c22c8e6c078f&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=a1e21183c9724f089d08c22c8e6c078f&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-frontend/src/utils/generateAuthDbPassword.ts **Line:** 162:183 **Comment:** *Api Mismatch: The password generator is hard-wired to the default policy (`AUTH_DB_PASSWORD_MIN_LENGTH` + `satisfiesDefaultAuthDbPasswordPolicy`) and cannot honor a customized `AUTH_DB_CONFIG`. In deployments with stricter policy (for example longer minimum length), generated passwords will frequently fail server-side validation and break the "generate password" flow. Make the generator accept a policy argument and validate against that policy instead of the defaults. 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=821b27650230577bf30159564312ea3e3d5abcc49aac0da716f0bb156edf7568&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39469&comment_hash=821b27650230577bf30159564312ea3e3d5abcc49aac0da716f0bb156edf7568&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]
