feat(auth): implement password change API for AUTH_DB authentication#39469
feat(auth): implement password change API for AUTH_DB authentication#39469shantanukhond wants to merge 42 commits into
Conversation
github-actions
Bot
added
risk:db-migration
dosubot
Bot
added
authentication
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #6616fd
Actionable Suggestions - 1
-
superset/daos/auth_audit_log.py - 1
- Use timezone-aware datetime instead utcnow · Line 51-51
Additional Suggestions - 3
-
docs/admin_docs/configuration/configuring-superset.mdx - 1
-
Incorrect default values path · Line 369-369The documentation states that defaults for AUTH_DB_CONFIG are merged from superset/config.py, but the actual defaults (AUTH_DB_DEFAULTS) are defined in superset/utils/auth_db_password.py. This inaccuracy could mislead users searching for default values. The code shows config.py imports AUTH_DB_DEFAULTS from auth_db_password.py and uses it for merging.
Code suggestion
@@ -369,1 +369,1 @@ - Any keys you omit are merged with the defaults from `superset/config.py`. + Any keys you omit are merged with the defaults from `superset/utils/auth_db_password.py`.
-
-
superset/utils/auth_db_password.py - 1
-
Misleading error message · Line 121-123The error message for the special character requirement states 'non-alphanumeric' but the code excludes spaces from counting as special. This could mislead users into thinking spaces satisfy the requirement. Change to 'special character (not a letter, digit, or space)' for clarity.
Code suggestion
@@ -120,3 +120,3 @@ - ): - errors.append( - "Password must contain at least one special (non-alphanumeric) character." + ): + errors.append( + "Password must contain at least one special character (not a letter, digit, or space)."
-
-
superset/daos/auth_audit_log.py - 1
-
Redundant field assignment · Line 45-51The `AuthAuditLog` model already defines a default value for `created_at` using `datetime.utcnow()`. Explicitly setting it in the DAO is unnecessary and can be removed to rely on the model's default, simplifying the code without changing behavior.
Code suggestion
@@ -45,8 +45,7 @@ - entry = AuthAuditLog( - user_id=user_id, - event_type=event_type, - ip_address=ip_address, - user_agent=user_agent, - event_metadata=metadata, - created_at=datetime.utcnow(), - ) + entry = AuthAuditLog( + user_id=user_id, + event_type=event_type, + ip_address=ip_address, + user_agent=user_agent, + event_metadata=metadata, + )
-
Review Details
-
Files reviewed - 22 · Commit Range:
a1339a4..a1339a4- docs/admin_docs/configuration/configuring-superset.mdx
- superset-frontend/src/components/GeneratePasswordInputSuffix.tsx
- superset-frontend/src/features/userInfo/UserInfoModal.tsx
- superset-frontend/src/features/users/UserListResetPasswordModal.tsx
- superset-frontend/src/features/users/utils.ts
- superset-frontend/src/pages/UserInfo/index.tsx
- superset-frontend/src/pages/UsersList/UsersList.test.tsx
- superset-frontend/src/pages/UsersList/index.tsx
- superset-frontend/src/utils/generateAuthDbPassword.test.ts
- superset-frontend/src/utils/generateAuthDbPassword.ts
- superset/config.py
- superset/daos/auth_audit_log.py
- superset/migrations/versions/2026-04-19_14-30_b7e4f2a891c3_add_auth_audit_log_table.py
- superset/models/__init__.py
- superset/models/auth_audit_log.py
- superset/security/manager.py
- superset/utils/auth_db_password.py
- superset/views/users/api.py
- superset/views/users/schemas.py
- tests/integration_tests/users/api_tests.py
- tests/unit_tests/daos/test_auth_audit_log_dao.py
- tests/unit_tests/utils/test_auth_db_password.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39469 +/- ##
==========================================
+ Coverage 65.13% 65.19% +0.05%
==========================================
Files 2753 2760 +7
Lines 154584 155288 +704
Branches 35463 35600 +137
==========================================
+ Hits 100695 101234 +539
- Misses 51975 52102 +127
- Partials 1914 1952 +38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hey @shantanukhond! 👋 This review was performed by Claude (Amin's AI assistant) — I did an independent review and then cross-checked my findings against a second AI reviewer (Codex) for additional confidence. Key Findings (both reviewers agreed)Blockers
Medium Concerns
Minor
Overall this is a well-structured feature with good separation of concerns and solid test coverage. The session ordering issue (#1) is the most important fix. Nice work! 🙌 |
Hi, thanks @shantanukhond for getting this started. I’m currently focused on other priorities, but as soon as I have some availability I plan to move this SIP forward. In the meantime, we can sync. |
|
Hi @shantanukhond, sharing notes from the SIP-author perspective focused on alignment rather than the implementation, which the prior review covers thoroughly. +1 in particular to the session-clear-before-commit, atomicity, and rate-limiting points raised there. Scope suggest splitting this PR The change currently bundles Pillar 1 (authenticated password change) with parts of Pillar 3 (admin reset).
Pillar 3 — design alignment needed The admin "set password directly" flow conflicts with the SIP. Per Rejected Alternative 6, admins should trigger an email reset link rather than ever seeing or setting the user's password (zero-knowledge principle, since admins can leave or change roles). Pillar 1 — items still missing vs. the SIP • Multi-session invalidation across all active sessions for the user, not only the current one. I suggest reading the linked SIP carefully before further development. Happy to track these on the SIP side as the work progresses. |
|
@EnxDev Thank you so much for your guidance. I will try to finish first part i.e. Pillar 1 by this week. I will keep you posted. I will also go through the SIP. Thank you. |
EnxDev's Review Agent — #39469 · HEAD f0de405request changes — two blockers: (1) the new migration forks the Alembic tree into two heads, so every DB-dependent CI job fails at Re-review; supersedes my prior review (HEAD 5e2a6bc). The plaintext-logging blocker below was raised there and is not addressed in this revision. The core password-change flow otherwise remains well built. 🔴 Functional
🟡 Should-fix
🔵 Nits
🙌 Praise
Verified as non-issues so they aren't re-raised: |
| if ( | ||
| not user | ||
| or not user.is_active | ||
| or not verify_auth_db_password(user.password, password) |
There was a problem hiding this comment.
Suggestion: This direct hash check bypasses the security manager authentication flow, so failed-attempt accounting/timing-balancing logic is skipped and this command behaves differently from normal login protections. Route credential validation through the security manager auth method so lockout/rate-limit and failure bookkeeping are consistently enforced. [security]
Severity Level: Major ⚠️
- ❌ Factory reset CLI ignores AUTH_DB login failure tracking.
- ⚠️ Brute-force attempts via CLI bypass lockout protections.Steps of Reproduction ✅
1. Invoke the Superset CLI entrypoint `superset` defined in `superset/cli/main.py:55-73`,
which auto-loads subcommands from the `superset.cli` package.
2. Ensure the factory reset command is enabled by setting
`FEATURE_FLAGS["ENABLE_FACTORY_RESET_COMMAND"] = True` so the check at
`superset/cli/reset.py:55-57` passes.
3. Run `superset factory_reset` from the CLI, enter an existing admin username and an
incorrect password at the prompts implemented in `superset/cli/reset.py:69-70`; execution
reaches the validation block at lines 72-76.
4. In `superset/cli/reset.py:72-76`, the condition `or not
verify_auth_db_password(user.password, password)` (line 75) directly checks the stored
hash and, on failure, prints "Invalid credentials" and calls `sys.exit(1)` (lines 77-78)
without ever invoking `SupersetSecurityManager.auth_user_db()` in
`superset/security/manager.py:84-115`, so the usual failure bookkeeping via
`update_user_auth_stat()` and fake-hash timing-balancing logic are skipped for this
command.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/cli/reset.py
**Line:** 75:75
**Comment:**
*Security: This direct hash check bypasses the security manager authentication flow, so failed-attempt accounting/timing-balancing logic is skipped and this command behaves differently from normal login protections. Route credential validation through the security manager auth method so lockout/rate-limit and failure bookkeeping are consistently enforced.
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
| if sess_stamp is None or sess_stamp != db_stamp: | ||
| # Sessions authenticated before stamp tracking, or a login that did not | ||
| # persist the stamp into the signed cookie, carry no sess stamp. Adopt | ||
| # the current DB value so the request can proceed; password rotation | ||
| # still invalidates sessions that carry an outdated non-null stamp. | ||
| if sess_stamp is None and db_stamp is not None: | ||
| session[SESSION_AUTH_STAMP_SESSION_KEY] = db_stamp | ||
| _mark_session_stamp_validated(db_stamp) | ||
| _cache_user_session_stamp(user_id, db_stamp) | ||
| return |
There was a problem hiding this comment.
Suggestion: This logic accepts any authenticated session that has no stamp by copying the current DB stamp into the session and allowing the request. That means stamp-less sessions (for example older sessions or flows that missed stamp write) are not invalidated after a password change, which breaks the cross-session invalidation guarantee. Treat missing session stamp as invalid once a DB stamp exists, rather than auto-adopting it. [security]
Severity Level: Major ⚠️
❌ Stamp-less sessions remain valid after password rotation.
⚠️ Cross-device invalidation weaker than documented guarantee.
⚠️ Older pre-feature sessions not forced to reauthenticate.Steps of Reproduction ✅
1. A user changes their AUTH_DB password via `PUT /api/v1/me/password`, which hits
`CurrentUserRestApi.update_my_password` at `superset/views/users/api.py:323-379`. This
view verifies the current password, hashes the new password, and calls
`bump_user_session_auth_stamp(g.user.id)` (imported at `superset/views/users/api.py:48`
and implemented in `superset/utils/auth_session_stamp.py:183-204`) to rotate the per-user
DB stamp.
2. The user has another authenticated browser session whose signed cookie lacks
`_auth_session_stamp` (for example, a session created before this feature or one that
missed stamp persistence). This scenario is mirrored in
`test_validate_stamp_adopts_missing_session_stamp` at
`superset/tests/unit_tests/utils/test_auth_session_stamp.py:235-257`, where `session` has
no `_auth_session_stamp` but the DB row’s `stamp` is set to `"db-stamp"`.
3. On the next request from that stamp-less session (e.g., `GET /api/v1/me/` as in the
tests), `validate_session_auth_stamp_for_request()` at
`superset/utils/auth_session_stamp.py:310-392` runs via the `before_request` hook
registered in `register_session_auth_stamp_hook()` at line 118, resolving `user_id`,
loading `db_stamp` from `UserSessionAuthStamp`, and reading `sess_stamp =
session.get(SESSION_AUTH_STAMP_SESSION_KEY)`, which is `None`.
4. Because `sess_stamp is None or sess_stamp != db_stamp` evaluates true, the branch at
lines 368-377 executes. Since `sess_stamp is None and db_stamp is not None`, the code
copies `db_stamp` into the session (`SESSION_AUTH_STAMP_SESSION_KEY`), marks the stamp as
validated, caches it, and returns without calling `logout_user()` or raising
`Unauthorized`. As a result, this stamp-less pre-rotation session remains authenticated
even after the password change, so some previously authenticated sessions are not
invalidated despite the rotated DB stamp.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/auth_session_stamp.py
**Line:** 368:377
**Comment:**
*Security: This logic accepts any authenticated session that has no stamp by copying the current DB stamp into the session and allowing the request. That means stamp-less sessions (for example older sessions or flows that missed stamp write) are not invalidated after a password change, which breaks the cross-session invalidation guarantee. Treat missing session stamp as invalid once a DB stamp exists, rather than auto-adopting it.
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…management - Updated the down_revision in the migration script to reflect the latest changes. - Improved the password reset logic in SupersetSecurityManager to rotate the session auth stamp for users when their password is reset, ensuring better security. - Added hooks to manage per-user session stamps during login and password changes, preventing sensitive information from being logged. - Enhanced unit tests to verify that password changes do not log sensitive data and that session stamps are correctly managed during resets.
- Reformatted conditional statement for better clarity. - Cleaned up assertions in user API tests for improved readability.
Code Review Agent Run #370093Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| SESSION_AUTH_STAMP_SESSION_KEY = "_auth_session_stamp" | ||
| SESSION_AUTH_STAMP_VALIDATED_AT_KEY = "_auth_session_stamp_validated_at" | ||
| SESSION_AUTH_STAMP_VALIDATED_DB_STAMP_KEY = "_auth_session_stamp_validated_db_stamp" |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the new session-stamp key constants to satisfy the type-hint requirement for relevant module-level variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
These newly added module-level constants are plain assignments without type annotations. The custom rule requires type hints for new Python variables that can be annotated, so this is a real violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/auth_session_stamp.py
**Line:** 35:37
**Comment:**
*Custom Rule: Add explicit type annotations for the new session-stamp key constants to satisfy the type-hint requirement for relevant module-level variables.
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| _SAFE_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) | ||
| _STAMP_CACHE_KEY_PREFIX = "auth_session_stamp:" | ||
| _DEFAULT_STAMP_CACHE_TIMEOUT_SECONDS = 300 |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for these cache and method constants so all relevant new variables are typed. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
These are also newly introduced module-level constants without explicit type hints. Since they are relevant variables that can be annotated, they violate the type-hint requirement.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/auth_session_stamp.py
**Line:** 39:41
**Comment:**
*Custom Rule: Add explicit type annotations for these cache and method constants so all relevant new variables are typed.
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
| resolved: str = algorithm or get_auth_db_password_hash_algorithm() | ||
| if resolved == "argon2": | ||
| return _argon2_hasher.hash(password) | ||
| if resolved == "bcrypt": | ||
| try: | ||
| encoded: bytes = password.encode("utf-8") | ||
| except UnicodeEncodeError as exc: | ||
| raise ValueError("Password cannot be encoded as UTF-8.") from exc | ||
| 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}") |
There was a problem hiding this comment.
Suggestion: The explicit algorithm argument is used verbatim, unlike config-derived values which are normalized in get_auth_db_password_hash_algorithm; passing values like "BCRYPT" or " argon2 " will incorrectly raise ValueError at runtime. Normalize algorithm with strip().lower() before comparison so direct callers get the same contract as config-based resolution. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Direct hash_auth_db_password callers passing uppercase algorithm crash.
- ⚠️ Inconsistent behavior versus AUTH_DB_CONFIG normalization causes confusion.
- ⚠️ Future password utilities relying on explicit algorithms become fragile.Steps of Reproduction ✅
1. Locate the hash helper `hash_auth_db_password` in
`superset/utils/auth_db_password_hash.py:50-69`, which accepts an optional `algorithm`
parameter and sets `resolved: str = algorithm or get_auth_db_password_hash_algorithm()`.
2. Inspect the configuration resolver `get_auth_db_password_hash_algorithm` in
`superset/utils/auth_db_password.py:15-37`, which normalizes config values with `algorithm
= str(raw_algorithm).strip().lower()` before validating against
`_SUPPORTED_HASH_ALGORITHMS`.
3. Note that `hash_auth_db_password` does not normalize its explicit `algorithm` argument;
it only compares `resolved` directly to `"argon2"` and `"bcrypt"` (lines 57-59 in
`auth_db_password_hash.py`).
4. In any calling code (for example a new helper or script) invoke
`hash_auth_db_password("secret", "BCRYPT")` or `hash_auth_db_password("secret", " argon2
")`; since `resolved` becomes `"BCRYPT"` / `" argon2 "`, neither `if resolved == "argon2"`
nor `if resolved == "bcrypt"` matches, and the function raises `ValueError("Unsupported
AUTH_DB hash algorithm: BCRYPT")` at line 69 instead of hashing successfully, unlike the
normalized config-driven path.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/auth_db_password_hash.py
**Line:** 56:69
**Comment:**
*Api Mismatch: The explicit `algorithm` argument is used verbatim, unlike config-derived values which are normalized in `get_auth_db_password_hash_algorithm`; passing values like `"BCRYPT"` or `" argon2 "` will incorrectly raise `ValueError` at runtime. Normalize `algorithm` with `strip().lower()` before comparison so direct callers get the same contract as config-based resolution.
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
Code Review Agent Run #f84088Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| const StrengthWrapper = styled.div` | ||
| ${({ theme }) => css` | ||
| display: flex; | ||
| align-items: center; | ||
| gap: ${theme.sizeUnit * 2}px; | ||
| `} | ||
| `; |
There was a problem hiding this comment.
would make sense to use the <Flex /> component here?
| .ant-progress { | ||
| margin-bottom: 0; | ||
| } | ||
| `} | ||
| `; |
There was a problem hiding this comment.
do we have another soulution instead of using the css class?
| `; | ||
|
|
||
| const StrengthBarContainer = styled.div` | ||
| ${({ theme }) => css` |
| const policy = { | ||
| ...AUTH_DB_DEFAULT_PASSWORD_POLICY, | ||
| password_hash_algorithm: 'argon2' as const, | ||
| password_require_uppercase: false, | ||
| password_require_lowercase: false, | ||
| password_require_digit: false, | ||
| password_require_special: false, | ||
| password_common_list_check: false, | ||
| password_min_length: 1, |
There was a problem hiding this comment.
Would it make sense to centralize this instead of repeating it in multiple places?
|
login_rate_limit, login_max_failures, and login_lockout_duration_minutes are shipped in AUTH_DB_DEFAULTS and documented, but the actual login path (auth_user_db) applies no rate limiting or lockout. The only consumer of get_auth_db_login_rate_limit_string() is the password-change endpoint (_rate_limit_me_password_change). |
… Flex component for improved layout; update password policy tests to use a new minimal policy configuration
| """ | ||
| Return non-secret AUTH_DB password policy options for frontend validation UI. | ||
| """ | ||
| merged_cfg = cfg if cfg is not None else get_merged_auth_db_config() |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this local config variable so it complies with the required type-hint coverage. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This is new Python code in the added file, and the local variable merged_cfg is assigned a config mapping without an explicit type annotation even though it can be annotated. That matches the type-hint requirement.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/auth_db_password.py
**Line:** 137:137
**Comment:**
*Custom Rule: Add an explicit type annotation for this local config variable so it complies with the required type-hint coverage.
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
| if (raw && typeof raw === 'object') { | ||
| return (Object.values(raw).flat() as string[]).join(' '); | ||
| } | ||
| return t('Something went wrong while saving the user info'); |
There was a problem hiding this comment.
Suggestion: This fallback error message is shown for both profile edit and password-change submissions, but it explicitly says “saving the user info,” which is incorrect for password reset failures and will mislead users. Use an operation-appropriate fallback message for the password flow. [comment mismatch]
Severity Level: Minor 🧹
⚠️ Password reset errors misreported as user info issues.
⚠️ Can confuse users diagnosing password change failures.Steps of Reproduction ✅
1. Inspect the shared error-message helper in
`superset-frontend/src/features/userInfo/UserInfoModal.tsx:81-93`.
`getSubmitErrorMessage()` falls back at line 92 to `t('Something went wrong while saving
the user info')` when neither `clientError.error` nor `clientError.message` provides a
usable string.
2. Examine the form submission handler in the same file at `handleFormSubmit()` on lines
104-137. For edit mode (lines 106-112), it PUTs `/api/v1/me/` to update profile info; for
password reset (lines 113-133), it PUTs `/api/v1/me/password` to change the password and
re-authenticates the session.
3. Note the shared catch block on lines 134-137: `catch (error) { addDangerToast(await
getSubmitErrorMessage(error)); throw error; }`. This is used for both the profile edit
flow and the password-change flow, meaning the fallback string from step 1 is shown on any
generic failure regardless of which endpoint failed.
4. Trigger a generic backend or network error on the password-change endpoint (e.g., cause
a 500 or a timeout so that `getClientErrorObject()` yields no specific message). The
password reset submission from the ChangePasswordModal (wired in
`superset-frontend/src/pages/UserInfo/index.tsx:245-253`) results in a toast with the
fallback `Something went wrong while saving the user info`, which inaccurately describes a
password-change failure as a user-info save issue and can mislead users diagnosing
password problems.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/userInfo/UserInfoModal.tsx
**Line:** 92:92
**Comment:**
*Comment Mismatch: This fallback error message is shown for both profile edit and password-change submissions, but it explicitly says “saving the user info,” which is incorrect for password reset failures and will mislead users. Use an operation-appropriate fallback message for the password flow.
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| const pwd = generateAuthDbPassword(passwordPolicy); | ||
| form.setFieldsValue({ | ||
| new_password: pwd, | ||
| confirm_password: pwd, | ||
| }); | ||
| // setFieldsValue does not fire the form's change handlers, so | ||
| // validate the affected fields to recompute submit state. | ||
| form | ||
| .validateFields(['new_password', 'confirm_password']) | ||
| .catch(() => {}); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Suggestion: The password generator call can throw (for example when server policy is impossible to satisfy, such as bcrypt with a minimum length above 72 bytes), but this click handler does not catch that exception. That will surface as an uncaught runtime error and break the modal instead of showing a user-facing validation/toast message. Catch generator failures in this handler and report a friendly error without crashing the form. [possible bug]
Severity Level: Major ⚠️
❌ Password generator click can crash reset password modal.
⚠️ Users lose feedback when policy or crypto misconfigured.Steps of Reproduction ✅
1. Open the password generator UI wiring in
`superset-frontend/src/features/userInfo/UserInfoModal.tsx:176-215`. On lines 198-213, the
`Input.Password` suffix renders `GeneratePasswordInputSuffix` with `onGenerate={() => {
const pwd = generateAuthDbPassword(passwordPolicy); ... }}` (lines 201-212).
2. Inspect the generator implementation at
`superset-frontend/src/utils/generateAuthDbPassword.ts:286-310`.
`generateAuthDbPassword()` (lines 290-309) calls helpers that can throw:
- `secureRandomInt()` (lines 139-148) throws if `globalThis.crypto.getRandomValues` is
unavailable.
- The generator loop throws `new Error('generateAuthDbPassword: exhausted retries')` at
line 309 if no password can satisfy the supplied `AuthDbPasswordPolicy` (e.g., an
impossible combination of minimum length and bcrypt byte limit).
3. Follow the user-facing path: the UserInfo page at
`superset-frontend/src/pages/UserInfo/index.tsx:91-269` opens `ChangePasswordModal` (lines
245-253), which renders `UserInfoModal` with `isEditMode={false}`. This causes
`ResetPasswordFields` (lines 162-260) to be shown, including the generator button wired to
the `onGenerate` handler above.
4. In an environment without Web Crypto (`globalThis.crypto.getRandomValues` missing) or
under a misconfigured password policy that cannot be satisfied, clicking the "Generate
password" button (rendered by `GeneratePasswordInputSuffix` in
`superset-frontend/src/components/GeneratePasswordInputSuffix.tsx:29-48`) will propagate
the exception from `generateAuthDbPassword()` through `onGenerate` without any try/catch.
React treats this as an uncaught error during the click event, which can break the modal
or hit a global error boundary instead of displaying a toast or validation error,
confirming the lack of defensive handling on this path.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/userInfo/UserInfoModal.tsx
**Line:** 201:212
**Comment:**
*Possible Bug: The password generator call can throw (for example when server policy is impossible to satisfy, such as bcrypt with a minimum length above 72 bytes), but this click handler does not catch that exception. That will surface as an uncaught runtime error and break the modal instead of showing a user-facing validation/toast message. Catch generator failures in this handler and report a friendly error without crashing the form.
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
| do_upgrade(session) | ||
| try: | ||
| session.commit() | ||
| session.commit() # pylint: disable=consider-using-transaction |
There was a problem hiding this comment.
Suggestion: Calling session.commit() inside an Alembic migration breaks the outer Alembic-managed transaction boundary. If a failure happens after this commit (for example while stamping migration state), the permission changes can be permanently committed while the revision is not fully recorded, leaving migration state inconsistent on reruns. Keep this work inside Alembic’s transaction and flush the session instead of committing here. [logic error]
Severity Level: Critical 🚨
- ❌ Migration data committed while Alembic version not recorded.
- ⚠️ Rerunning migrations sees inconsistent permissions versus revision.Steps of Reproduction ✅
1. From a Superset instance using this PR code, run `superset db upgrade` so Alembic
executes migrations via `superset/migrations/env.py:64-84`, where
`context.run_migrations()` is wrapped inside `context.begin_transaction()`.
2. During the run, Alembic loads revision module
`superset/migrations/versions/2026-06-23_03-23_a7d3f1b9c2e4_cleanup_stale_can_import_pvm.py`
and calls its `upgrade()` function defined around line 99 in the PR hunk.
3. Inside `upgrade()`, `Session(bind=op.get_bind())` is created, `do_upgrade(session)`
mutates permission rows, and `session.commit()` at line 104 commits those changes directly
on the Alembic-managed connection, independently of Alembic’s outer transaction in
`env.py:82`.
4. Introduce a later migration that intentionally raises an exception after this revision
(for example, a failing DDL in another script), rerun `superset db upgrade`, and observe
that Alembic aborts and may not fully record the revision chain while the permission
updates committed by `upgrade()` remain persisted, demonstrating desynchronized data
versus migration version tracking.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-06-23_03-23_a7d3f1b9c2e4_cleanup_stale_can_import_pvm.py
**Line:** 104:104
**Comment:**
*Logic Error: Calling `session.commit()` inside an Alembic migration breaks the outer Alembic-managed transaction boundary. If a failure happens after this commit (for example while stamping migration state), the permission changes can be permanently committed while the revision is not fully recorded, leaving migration state inconsistent on reruns. Keep this work inside Alembic’s transaction and flush the session instead of committing here.
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
| # How often (seconds) to re-check the session auth stamp against the database on | ||
| # read-only requests (GET/HEAD/OPTIONS). State-changing requests always | ||
| # revalidate. Set to 0 to check on every authenticated request. | ||
| SESSION_AUTH_STAMP_REVALIDATION_SECONDS: int = 300 |
There was a problem hiding this comment.
Suggestion: Setting the default revalidation interval to 300 seconds introduces a security window where stale sessions can keep passing GET/HEAD/OPTIONS checks without a DB lookup. This is especially problematic for password changes performed outside the self-service flow (for example admin resets), where the stamp is bumped but the shared stamp cache is not immediately refreshed, allowing old sessions to continue reading data until the interval expires. Use a default of 0 (always revalidate) or ensure every stamp bump path updates the shared cache immediately. [security]
Severity Level: Critical 🚨
❌ Admin ResetPasswordView leaves compromised sessions valid during TTL.
❌ GET /api/v1/me/ still authorized after admin reset.
⚠️ Cross-node session invalidation delayed for AUTH_DB admin resets.Steps of Reproduction ✅
1. Start Superset with AUTH_DB enabled and the default configuration in
`superset/config.py` lines 465–476, where `AUTH_TYPE = AUTH_DB` and
`SESSION_AUTH_STAMP_REVALIDATION_SECONDS: int = 300` is set.
2. Log in as an AUTH_DB user to create a session stamp; on successful login,
`SupersetSecurityManager.on_user_login` in `superset/security/manager.py:1232–1241` calls
`sync_session_auth_stamp_on_login(user)` from
`superset/utils/auth_session_stamp.py:153–165`, which writes the stamp into the signed
session cookie, marks it validated via `_mark_session_stamp_validated` at
`auth_session_stamp.py:261–265`, and caches it with `_cache_user_session_stamp`.
3. As an administrator, reset this same user’s password via the FAB “Reset Password” flow
(ResetPasswordView), which routes through `SupersetSecurityManager.reset_password` in
`superset/security/manager.py:1161–1218`; when `AUTH_TYPE == AUTH_DB`, this method calls
`bump_user_session_auth_stamp(target_user_id)` at `auth_session_stamp.py:172–194` to
rotate the DB-stored stamp but does not call `cache_user_session_auth_stamp`
(`auth_session_stamp.py:167–170`), leaving the shared cache and existing browser sessions
with the old stamp.
4. Within 300 seconds of the last stamp validation, have the affected user reuse their
existing browser session to issue a read-only request such as `GET /api/v1/me/`, handled
by `CurrentUserRestApi.get_me` in `superset/views/users/api.py:192–217`; the
`before_request` hook `_validate_user_session_auth_stamp` in
`auth_session_stamp.py:108–111` invokes `validate_session_auth_stamp_for_request`
(`auth_session_stamp.py:300–382`), which consults `_should_skip_stamp_db_lookup` at
`auth_session_stamp.py:232–258`. Because `SESSION_AUTH_STAMP_REVALIDATION_SECONDS` is 300,
the session’s `SESSION_AUTH_STAMP_VALIDATED_AT_KEY` timestamp is recent, and the cached
stamp still matches the old session stamp (no cache bump happened in `reset_password`),
`_should_skip_stamp_db_lookup` returns `True` and the DB lookup is skipped, allowing the
stale session to continue reading data until the revalidation interval and cache TTL
expire.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 476:476
**Comment:**
*Security: Setting the default revalidation interval to 300 seconds introduces a security window where stale sessions can keep passing GET/HEAD/OPTIONS checks without a DB lookup. This is especially problematic for password changes performed outside the self-service flow (for example admin resets), where the stamp is bumped but the shared stamp cache is not immediately refreshed, allowing old sessions to continue reading data until the interval expires. Use a default of 0 (always revalidate) or ensure every stamp bump path updates the shared cache immediately.
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| current_app.config.get( | ||
| "AUTH_DB_FAKE_PASSWORD_HASH_CHECK", | ||
| DEFAULT_AUTH_DB_FAKE_PASSWORD_HASH_CHECK, | ||
| ), | ||
| "password", | ||
| ) | ||
| logger.info(LOGMSG_WAR_SEC_LOGIN_FAILED, username) |
There was a problem hiding this comment.
Suggestion: The failed-login timing equalization path ignores the attacker-supplied password and always checks the literal string "password" against a Werkzeug hash. Because real-user checks use verify_auth_db_password(user.password, password) (bcrypt/argon2/legacy) while missing-user checks use a different verifier and fixed input, login timing can diverge enough to leak account existence. Use the same candidate password and the same verification path for both branches (or a fake hash compatible with the same verifier) to avoid user-enumeration side channels. [security]
Severity Level: Critical 🚨
- ❌ AUTH_DB login leaks account existence via timing variance.
- ⚠️ Enables username enumeration with crafted long passwords.Steps of Reproduction ✅
1. Inspect AUTH_DB login helper `auth_user_db()` in
`superset/security/manager.py:4599-131` (loaded via BulkRead), which authenticates
database users when `AUTH_TYPE` = `AUTH_DB`.
2. For a missing or inactive user, follow the branch at `manager.py:113-125`: the code
calls `check_password_hash(current_app.config.get("AUTH_DB_FAKE_PASSWORD_HASH_CHECK",
DEFAULT_AUTH_DB_FAKE_PASSWORD_HASH_CHECK), "password")`, using the literal `"password"`
instead of the attacker-supplied candidate.
3. For an existing active user, follow the branch at `manager.py:126-131`: it calls
`verify_auth_db_password(user.password, password)` with the attacker-supplied `password`.
In `superset/utils/auth_db_password_hash.py:72-103`, the bcrypt path (`lines 81-88`)
encodes the candidate and short-circuits to `False` when `len(encoded) >
BCRYPT_MAX_PASSWORD_BYTES`.
4. Configure AUTH_DB to use bcrypt (default in
`superset/utils/auth_db_password.py:29-42`), create a real user with a bcrypt hash, and
compare login timings for that username versus a non-existent username using an overlong
password (>72 UTF-8 bytes): the real-user path for bcrypt short-circuits quickly, while
the missing-user path always runs the full PBKDF2-based `check_password_hash` on
`"password"`, producing measurable timing differences that can be used to infer account
existence.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 4615:4621
**Comment:**
*Security: The failed-login timing equalization path ignores the attacker-supplied password and always checks the literal string `"password"` against a Werkzeug hash. Because real-user checks use `verify_auth_db_password(user.password, password)` (bcrypt/argon2/legacy) while missing-user checks use a different verifier and fixed input, login timing can diverge enough to leak account existence. Use the same candidate password and the same verification path for both branches (or a fake hash compatible with the same verifier) to avoid user-enumeration side channels.
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| "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, |
There was a problem hiding this comment.
Suggestion: These lockout/reset policy keys are introduced as configurable behavior but are not consumed anywhere in the backend auth flow, so changing them has no effect at runtime. This creates a misleading security contract for operators who expect lockout and reset controls to be enforced. Either wire these settings into the corresponding authentication/reset logic or remove them until enforcement is implemented. [incomplete implementation]
Severity Level: Critical 🚨
- ⚠️ Operators misled; lockout settings silently ineffective.
- ⚠️ Increased risk of password-guessing without configured lockout.Steps of Reproduction ✅
1. Inspect `AUTH_DB_DEFAULTS` in `superset/utils/auth_db_password.py:29-42` (BulkRead
output): it introduces `reset_token_expiry_minutes`, `reset_rate_limit`,
`login_rate_limit`, `login_max_failures`, and `login_lockout_duration_minutes` as
configurable AUTH_DB policy options.
2. Use Grep across `/workspace/superset` (tool results included) for each key:
`login_max_failures`, `reset_rate_limit`, `reset_token_expiry_minutes`, and
`login_lockout_duration_minutes` only appear in `auth_db_password.py:29-42` and are never
referenced elsewhere; `login_rate_limit` is used via
`get_auth_db_login_rate_limit_string()` in `superset/views/users/api.py:40-46,88-91` to
rate-limit `PUT /api/v1/me/password`, confirming the other knobs are truly unused.
3. Inspect the AUTH_DB login flow in `superset/security/manager.py:4599-131`:
`auth_user_db()` calls `find_user()` and `verify_auth_db_password()` to decide success and
uses `update_user_auth_stat()` on failure, but no code reads `login_max_failures` or
`login_lockout_duration_minutes`, so failed-login counts and lockout duration are not
enforced.
4. Inspect the password change API in `superset/views/users/api.py:340-64`: it loads the
request body, verifies the current password via `verify_auth_db_password()`, validates and
hashes the new password, and commits the change, but never consults
`reset_token_expiry_minutes` or `reset_rate_limit`. As a result, operators can set these
AUTH_DB_CONFIG values expecting lockout or reset semantics, yet the backend
authentication/reset flows ignore them and system behavior does not change.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/auth_db_password.py
**Line:** 37:41
**Comment:**
*Incomplete Implementation: These lockout/reset policy keys are introduced as configurable behavior but are not consumed anywhere in the backend auth flow, so changing them has no effect at runtime. This creates a misleading security contract for operators who expect lockout and reset controls to be enforced. Either wire these settings into the corresponding authentication/reset logic or remove them until enforcement is implemented.
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
|
EnxDev's Review Agent — #39469 · HEAD 2ec20b8 request changes — Pillar 1 is now substantively delivered and the split was honored, but the new migration forks the Alembic tree (again) and CI is red at this HEAD. Supersedes my review at f0de405. Both blockers raised there are addressed in spirit: the plaintext-password logging is fixed (manual _log_audit_event with an id-only payload, guarded by test_put_my_password_does_not_log_passwords on success and failure paths), and the migration was re-pointed — but at a revision that has since gained another child on master, so the tree forks again. Pillar 1 status vs. the SIP (your May 3 checklist)
Split status: ✅ admin-reset UI/API and auth_audit_log are out of the diff (existing Log reused per dpgaspar). Prior blockers verified fixed at this HEAD: doubled UserLoggedIn audits, cache-before-commit race, session-clear-before-commit, CSRF rotation via SupersetClient.reAuthenticate(). 🔴 Functional
🟡 Should-fix
🔵 Nits
🙌 Praise
|
…ation to reflect correct lineage
Code Review Agent Run #187689Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
@shantanukhond I think we're in a really good place with this overall; thanks for driving this Pillar 1 forward!
Let me leave just couple of points that I think are worth addressing before we merge.
One thing I noticed is that the source IP mentioned in SIP-201 still seems to be missing from the audit metadata.
What do you think about that here?
| @@ -1334,22 +1349,50 @@ def reset_password(self, userid: Union[int, str], password: str) -> None: | |||
| bypassed. We distinguish the two by comparing the acting user | |||
| (``g.user``) against the target ``userid``: they match for a | |||
| self-service reset and differ for an admin reset. | |||
|
|
|||
| On ``AUTH_DB``, the per-user session auth stamp is rotated so every | |||
| other session for the target account is invalidated — including when an | |||
| administrator resets a compromised password. | |||
| """ | |||
| # pylint: disable=import-outside-toplevel | |||
| from flask_appbuilder.const import AUTH_DB | |||
|
|
|||
| super().reset_password(userid, password) | |||
|
|
|||
| try: | |||
| target_user_id = int(userid) | |||
| except (TypeError, ValueError): | |||
| target_user_id = None | |||
|
|
|||
| acting_user = getattr(g, "user", None) | |||
| acting_user_id = getattr(acting_user, "id", None) | |||
| # ``userid`` arrives as a string (the ``pk`` request arg) on the admin | |||
| # path, so coerce both sides before comparing. | |||
| is_self_service = acting_user_id is not None and self._same_user( | |||
| acting_user_id, userid | |||
| is_self_service = ( | |||
| acting_user_id is not None | |||
| and target_user_id is not None | |||
| and self._same_user(acting_user_id, userid) | |||
| ) | |||
| if is_self_service: | |||
|
|
|||
| if ( | |||
| current_app.config.get("AUTH_TYPE") == AUTH_DB | |||
| and target_user_id is not None | |||
| ): | |||
| from superset.utils.auth_session_stamp import ( | |||
| bump_user_session_auth_stamp, | |||
| sync_session_auth_stamp_on_login, | |||
| ) | |||
|
|
|||
| bump_user_session_auth_stamp(target_user_id) | |||
| if is_self_service and acting_user is not None: | |||
| sync_session_auth_stamp_on_login(acting_user) | |||
|
|
|||
| if is_self_service and target_user_id is not None: | |||
| from superset.security.password_change import ( | |||
| clear_password_must_change, | |||
| ) | |||
|
|
|||
| clear_password_must_change(int(userid)) | |||
| clear_password_must_change(target_user_id) | |||
There was a problem hiding this comment.
@shantanukhond would it make sense to add an _log_audit_event here so we can track who changed the password?
|
|
||
| cache_user_session_auth_stamp(g.user.id, new_stamp) | ||
| _reestablish_login_session(user_after) | ||
| _log_audit_event("UserPasswordChanged", {"user_id": g.user.id}) |
There was a problem hiding this comment.
Would it make sense to add more details here?
There was a problem hiding this comment.
Adding initiated_by and source_ip. Anything else you can suggest?
| if sess_stamp is None and db_stamp is not None: | ||
| session[SESSION_AUTH_STAMP_SESSION_KEY] = db_stamp |
There was a problem hiding this comment.
I had one thought about the stamp adoption logic. It looks like when a session is restored from a remember-me cookie, there's no stamp in the session, so the current code adopts the latest DB stamp and the session remains valid.
Would it make sense to only adopt the stamp for fresh (session["_fresh"]) interactive logins, and instead reject non-fresh restored sessions (logout + clear the remember cookie + 401)?
| raw_algorithm: Any = merged_cfg.get( | ||
| "password_hash_algorithm", AUTH_DB_DEFAULTS["password_hash_algorithm"] | ||
| ) | ||
| algorithm = str(raw_algorithm).strip().lower() |
There was a problem hiding this comment.
Suggestion: Add an explicit type hint for this derived local value to satisfy the rule requiring type hints on relevant variables. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This new local variable is a derived string value that can be explicitly annotated, but the code omits a type hint. That is a real instance of the type-hint rule violation.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/auth_db_password.py
**Line:** 155:155
**Comment:**
*Custom Rule: Add an explicit type hint for this derived local value to satisfy the rule requiring type hints on relevant variables.
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
| do_downgrade(session) | ||
| try: | ||
| session.commit() | ||
| session.commit() # pylint: disable=consider-using-transaction |
There was a problem hiding this comment.
Suggestion: The same explicit session.commit() in downgrade() has the same transaction-boundary problem and can desynchronize downgrade behavior from Alembic’s transaction handling; this should also remain under Alembic-managed transaction control with flush-only ORM persistence. [logic error]
Severity Level: Critical 🚨
- ❌ Downgrade transaction boundaries inconsistent with Alembic env.py control.
- ⚠️ Rollback of failed downgrade cannot fully revert permission changes.Steps of Reproduction ✅
1. Run database downgrades via Superset (for example `superset db downgrade`), which uses
Alembic’s env runner at `superset/migrations/env.py:87-142` to call
`run_migrations_online()` and, for MySQL/SQLite, sets `transaction_per_migration=True` and
`transactional_ddl=True` (line 118-119) while wrapping `context.run_migrations()` inside
`context.begin_transaction()` (lines 131-133).
2. During this migration run, Alembic executes revision `a7d3f1b9c2e4`’s `downgrade()`
function defined at
`superset/migrations/versions/2026-06-23_03-23_a7d3f1b9c2e4_cleanup_stale_can_import_pvm.py:110-120`,
which creates `session = Session(bind=bind)` (line 112) and calls `do_downgrade(session)`
(line 113), currently a documented no-op (lines 86-96).
3. After `do_downgrade()` returns, `downgrade()` explicitly calls `session.commit()` at
line 115, committing the database transaction from inside the revision instead of leaving
transaction control to Alembic’s `context.begin_transaction()` in `env.py`.
4. If a subsequent downgrade step in the same Alembic-managed transaction fails and
Alembic attempts to roll back, the manual commit has already finalized this revision’s
effects, so downgrade behavior becomes desynchronized from Alembic’s transactional
guarantees and rollback reliability now depends on this explicit commit rather than the
configured per-migration transaction.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-06-23_03-23_a7d3f1b9c2e4_cleanup_stale_can_import_pvm.py
**Line:** 115:115
**Comment:**
*Logic Error: The same explicit `session.commit()` in `downgrade()` has the same transaction-boundary problem and can desynchronize downgrade behavior from Alembic’s transaction handling; this should also remain under Alembic-managed transaction control with flush-only ORM persistence.
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
| current_app.config.get( | ||
| "AUTH_DB_FAKE_PASSWORD_HASH_CHECK", | ||
| DEFAULT_AUTH_DB_FAKE_PASSWORD_HASH_CHECK, | ||
| ), | ||
| "password", | ||
| ) |
There was a problem hiding this comment.
Suggestion: The fake password check for unknown/inactive users always uses a Werkzeug PBKDF2 hash, while real users are now verified with bcrypt/argon2 via verify_auth_db_password; this creates a measurable timing difference between “user exists” and “user does not exist” paths and reintroduces username-enumeration risk. Use a fake hash generated with the same configured AUTH_DB algorithm (or route both paths through the same verifier) so failed-login timing stays consistent. [security]
Severity Level: Major ⚠️
- ❌ AUTH_DB login exposes username existence via timing side-channel.
- ⚠️ Facilitates remote enumeration of valid AUTH_DB user accounts.
- ⚠️ Undermines protection provided by fake password hash checks.Steps of Reproduction ✅
1. Observe how the security manager is wired into Superset:
`superset/initialization/__init__.py:1209-1218` sets `appbuilder.security_manager_class =
SupersetSecurityManager`, and `SupersetSecurityManager.register_views` in
`superset/security/manager.py:4880-4887` registers `SupersetAuthView` from
`superset/views/auth.py:33-42` as the `/login` view, so database logins ultimately
authenticate through this manager.
2. Inspect `SupersetSecurityManager.auth_user_db` in
`superset/security/manager.py:4847-4879`: when `user is None or (not user.is_active)` at
line 4861, the code executes the fake password check at lines 4863-4868, calling
`werkzeug.security.check_password_hash` with
`current_app.config["AUTH_DB_FAKE_PASSWORD_HASH_CHECK"]` (falling back to
`DEFAULT_AUTH_DB_FAKE_PASSWORD_HASH_CHECK`, a PBKDF2 hash defined at lines 125-132); when
a user exists, line 4874 instead calls `verify_auth_db_password(user.password, password)`.
3. Inspect the real verifier and password algorithm defaults: `verify_auth_db_password` in
`superset/utils/auth_db_password_hash.py:72-103` uses bcrypt (`bcrypt.checkpw`) when the
stored hash matches the bcrypt format and argon2 (`argon2.PasswordHasher.verify`) when it
starts with `"$argon2"`, only falling back to Werkzeug `check_password_hash` for legacy
hashes; `superset/utils/auth_db_password.py:28-37` and `144-166` show that
`AUTH_DB_CONFIG["password_hash_algorithm"]` defaults to `"bcrypt"`, so newly created
AUTH_DB users will typically have bcrypt hashes.
4. Run Superset with `AUTH_TYPE = AUTH_DB` and default `AUTH_DB_CONFIG`, create an active
AUTH_DB user `alice` (whose password hash will be bcrypt by default), then from an HTTP
client issue repeated login attempts: (a) POST to `/login` as `alice` with an incorrect
password, which drives the login stack to call `auth_user_db` and execute the
bcrypt/argon2 verification path via `verify_auth_db_password`; (b) POST to `/login` with a
non-existent username such as `ghost` and any password, which drives `auth_user_db` into
the `user is None` branch and executes only the PBKDF2 fake hash check at
`superset/security/manager.py:4863-4868`. Measuring average response times for case (a) vs
case (b) over many requests will reveal a timing difference attributable to the different
hash algorithms/parameters on these two failure paths, enabling remote username
enumeration despite the intended fake-check mitigation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 4863:4868
**Comment:**
*Security: The fake password check for unknown/inactive users always uses a Werkzeug PBKDF2 hash, while real users are now verified with bcrypt/argon2 via `verify_auth_db_password`; this creates a measurable timing difference between “user exists” and “user does not exist” paths and reintroduces username-enumeration risk. Use a fake hash generated with the same configured `AUTH_DB` algorithm (or route both paths through the same verifier) so failed-login timing stays consistent.
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| return str( | ||
| get_merged_auth_db_config().get( | ||
| "login_rate_limit", AUTH_DB_DEFAULTS["login_rate_limit"] | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Suggestion: This coerces any configured login_rate_limit value to a string, which turns invalid values like None into the literal string "None" and can break Flask-Limiter parsing at request time (causing endpoint failures instead of a controlled fallback). Validate the value and fall back to the default when it is missing or malformed instead of unconditional str(...) coercion. [logic error]
Severity Level: Major ⚠️
- ❌ Password change endpoint 500s when login_rate_limit misconfigured.
- ⚠️ Misconfigured limiter string blocks self-service password changes.Steps of Reproduction ✅
1. Enable request rate limiting so the password-change endpoint uses Flask-Limiter by
setting `RATELIMIT_ENABLED = True` as defined in `superset/config.py:12-15`, and ensure a
`limiter` instance is attached to `security_manager` (checked in
`_rate_limit_me_password_change` at `superset/views/users/api.py:82-86`).
2. Configure an invalid AUTH_DB login rate limit by setting
`app.config["AUTH_DB_CONFIG"]["login_rate_limit"] = None` (or another non-string/invalid
object); `get_merged_auth_db_config` in `superset/utils/auth_db_password.py:104-115`
merges this value without validating the `login_rate_limit` field.
3. Issue a `PUT /api/v1/me/password` request, which is handled by
`CurrentUserRestApi.update_my_password` decorated with `@_rate_limit_me_password_change`
at `superset/views/users/api.py:310-319`; inside the wrapper,
`limiter.limit(get_auth_db_login_rate_limit_string(),
key_func=_me_password_rate_limit_key, methods=["PUT"])` is called at
`superset/views/users/api.py:28-32`.
4. `get_auth_db_login_rate_limit_string` at `superset/utils/auth_db_password.py:118-128`
coerces the invalid `login_rate_limit` value to the literal string `"None"` via
`str(...)`; when Flask-Limiter attempts to parse this string as a rate-limit expression,
it raises an exception, causing the password-change request to fail with an internal error
instead of applying a safe default limit or allowing the request.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/auth_db_password.py
**Line:** 124:128
**Comment:**
*Logic Error: This coerces any configured `login_rate_limit` value to a string, which turns invalid values like `None` into the literal string `"None"` and can break Flask-Limiter parsing at request time (causing endpoint failures instead of a controlled fallback). Validate the value and fall back to the default when it is missing or malformed instead of unconditional `str(...)` coercion.
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- Updated the logging of password changes to include the initiator and source IP address. - Introduced a new function to invalidate stale authentication sessions, ensuring proper session management after password changes. - Adjusted tests to verify the new logging structure and session invalidation behavior for both self-service and admin-initiated password changes.
SUMMARY
Pillar 1 of SIP-201 / #37927: self-service password change for
AUTH_DBusers.PUT /api/v1/me/password(AUTH_DB only) acceptscurrent_password,new_password, andconfirm_password; verifies the current password, validates the new password againstAUTH_DB_CONFIG, hashes with bcrypt (default) or argon2, clearspassword_must_changeon success, rotates a per-user session auth stamp (invalidating other browser sessions), and re-establishes the current session safely.GET /api/v1/me/password/policyreturns non-secret policy options for real-time UI validation.PUT /api/v1/me/still updates name fields but rejectspasswordwhenAUTH_TYPEisAUTH_DB(breaking change).AUTH_DB_CONFIGdefines minimum length, required character classes, common-password blocklist, hash algorithm, and rate limits; validation is shared between backend enforcement and frontend helpers (generateAuthDbPassword, policy indicator).UPDATING.mddescribe the new flow, policy knobs, session invalidation, and the breakingPUT /api/v1/me/behavior.AUTH_DB.user_session_auth_stamptable for cross-session invalidation.Related: #37927
Migration standards context: SIP-59 / #13351
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
AUTH_TYPE = AUTH_DB(and a DB auth user). Runsuperset db upgradeso the new migration is applied.PUT /api/v1/me/still updates name fields; confirm password is not accepted on that endpoint forAUTH_DB.auth_audit_log(or equivalent check your environment allows).superset db downgradeone step andsuperset db upgradeagain in a dev database to confirm the migration is reversible (adjust if your review process requires a specific DB engine).ADDITIONAL INFORMATION