EnxDev commented on PR #39469: URL: https://github.com/apache/superset/pull/39469#issuecomment-4896628917
## EnxDev's Review Agent โ apache/superset#39469 ยท HEAD 5e2a6bc **request changes** โ the new password endpoint writes plaintext passwords to the audit `logs` table on every change. _Supersedes my [prior review](https://github.com/apache/superset/pull/39469#issuecomment-4866465991) (HEAD 3ff511c). Both earlier blockers are fixed in this revision: the doubled `UserLoggedIn` audit event (`auth_user_db` is now a clean standalone override with no explicit `on_user_login` call โ single event confirmed by `test_auth_user_db_logs_single_user_logged_in_event`) and the cache-before-commit race (`bump_user_session_auth_stamp` no longer writes the cache; `cache_user_session_auth_stamp` runs post-commit, guarded by `test_bump_user_session_auth_stamp_does_not_write_cache`). Verified `get_first_user`/`noop_user_update` exist on the FAB base and FAB invokes `on_user_login` exactly once._ ### ๐ด Functional - **`superset/views/users/api.py:318`** ยท _High_ โ `update_my_password` is decorated with `@event_logger.log_this_with_context`. On every call, `AbstractEventLogger.log_context` runs `collect_request_payload()` (`superset/utils/log.py:40`), which merges `request.get_json()` verbatim into the payload, then passes it as `records=[payload]` to the logger. The default `EVENT_LOGGER = DBEventLogger()` `json.dumps`-es each record into `Log.json` โ so `current_password`, `new_password`, and `confirm_password` land in the metadata `logs` table in **plaintext**, on both success and failure paths. `curate_payload` does not apply here (it only guards `curated_payload`, which `DBEventLogger` uses only when `records` is empty โ it isn't). This contradicts the guarantee added in this same PR (`configuring-superset.mdx`: "they are never stored in plaintext or logged"). Principal: any role with `can_read on Log` โ Admin by default, but log read is commonly granted to custom auditor roles , and the `logs` table is routinely shipped to external SIEMs / backups. Leaking `current_password` in particular hands them a credential users very likely reuse on non-Superset systems, beyond the "Admin manages accounts" capability. **Fix:** drop `log_this_with_context` on this endpoint and emit a manual audit event carrying only `{user_id, action}` (no body), or scrub password-like keys in `collect_request_payload`. **regression test:** after a successful `PUT /me/password`, assert no `Log` row's `json` contains the new or current password. ### ๐ก Should-fix - **`superset/security/manager.py:1161`** (`reset_password`) and **`superset/cli/reset.py`** โ only the self-service path (`_commit_user_password_change`) calls `bump_user_session_auth_stamp`. Admin-initiated resets and the CLI don't rotate the stamp, so an admin resetting a *compromised* account (the case that matters most) leaves the attacker's existing session valid. If out of scope for Pillar 1, note it as a follow-up; otherwise call `bump_user_session_auth_stamp(user_id)` from `reset_password` too. ### ๐ต Nits - **`superset/security/manager.py:1212`** + the `user_logged_in` signal handler in `auth_session_stamp.py` โ `sync_session_auth_stamp_on_login` fires twice per login (once from `on_user_login`, once from the signal), each an `@transaction()` commit + cache write. Idempotent but redundant; keep one path. - PR title: the breaking `PUT /api/v1/me/` password rejection warrants `feat(auth)!:` per Conventional Commits (the `risk:breaking-change` label is set; the `!` is not). - `2026-06-23...a7d3f1b9c2e4` migration is edited only to add `# pylint: disable` comments to an already-merged file โ harmless but unusual. ### ๐ Praise - bcrypt's 72-byte truncation is handled at the hash, verify, and policy layers with a dedicated `test_bcrypt_verify_rejects_distinct_passwords_equal_after_truncation` โ closes the classic truncation-collision auth bypass. - The optimistic-locking UPDATE (`WHERE password == old_hash` โ `PasswordChangeConflictError`) with the post-commit cache write cleanly resolves the prior revision's cache-before-commit race. <!-- enxdev-review-agent:5e2a6bc --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 5e2a6bc._ -- 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]
