EnxDev commented on PR #39469: URL: https://github.com/apache/superset/pull/39469#issuecomment-4901549743
## EnxDev's Review Agent โ apache/superset#39469 ยท HEAD f0de405 **request changes** โ two blockers: (1) the new migration forks the Alembic tree into two heads, so every DB-dependent CI job fails at `db upgrade`; (2) the password-change endpoint still writes plaintext credentials into the `logs` table. _Re-review; supersedes my [prior review](https://github.com/apache/superset/pull/39469#issuecomment-4896628917) (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 - **`superset/migrations/versions/2026-05-13_12-00_c6219cac9270_add_user_session_auth_stamp_table.py:31`** ยท _High_ โ `down_revision = "b4a3f2e1d0c9"` targets a **mid-graph** revision, not the head. `b4a3f2e1d0c9` already has a child (`7c4a8d09ca37`, `add_deleted_at_to_slices`), so this migration becomes a second child and forks the tree into two heads. `flask db upgrade` then aborts with `Error: Multiple head revisions are present for given argument 'head'` โ the exact error in the failing `test-sqlite` / `test-mysql` / `test-postgres` / `docker-build` logs, which is why the whole pipeline is red before any test runs. **Fix:** repoint `down_revision` to the current single head (`7c4a8d09ca37` today; re-run `alembic heads` after rebasing โ it moves). **regression test:** the existing single-head CI guard passes once `alembic heads` returns one revision. - **`superset/views/users/api.py:318`** ยท _High_ โ `update_my_password` is decorated with `@event_logger.log_this_with_context`. On context exit, `log_with_context` builds `payload = collect_request_payload()` (`utils/log.py:32`), which does `payload.update(request.get_json())` โ merging `current_password`, `new_password`, `confirm_password` verbatim โ then calls `self.log(records=[payload], โฆ)`. The default `EVENT_LOGGER = DBEventLogger` uses `records` directly (`utils/log.py`: `curated_payload` is only a fallback `if not records`), `json.dumps`-ing each into `Log.json` and committing. So both passwords land in the metadata `logs` table in **plaintext**, on success and failure paths alike โ directly contradicting this PR's own doc ("never stored in plaintext or logged", `configuring-superset.mdx`). Principal: any role with `can_read on Log` (Admin by default, but log read is commonly delegated to custom auditor roles, and `logs` is routinely shipped to external SIEMs/b ackups); leaking `current_password` in particular hands over a credential users often reuse elsewhere. **Fix:** drop `log_this_with_context` here and emit a manual event carrying only `{user_id, action}`, or scrub password-like keys in `collect_request_payload`. **regression test:** after a successful `PUT /me/password`, assert no `Log.json` row contains the new or current password. ### ๐ก Should-fix - **`superset/security/manager.py` (`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) leaves the attacker's existing session valid. Bump the stamp there too, or note it as an explicit Pillar-1 follow-up. - **`superset/utils/auth_session_stamp.py:377`** โ a signed session with no `_auth_session_stamp` is adopted to the current DB stamp rather than invalidated, so sessions authenticated *before* this feature deploys survive one password rotation. Bounded to the rollout window (the cookie is signed, so the stamp can't be stripped by an attacker); confirm that's the intended trade-off. ### ๐ต Nits - After repointing the migration, regenerate the file โ its `2026-05-13` date/`Create Date` predates its new parent, so the chain reads out of order. - `sync_session_auth_stamp_on_login` fires twice per login (once from `on_user_login`, once from the `user_logged_in` signal handler) โ idempotent but redundant; keep one path. - PR title: the breaking `PUT /api/v1/me/` password rejection warrants `feat(auth)!:` (the `risk:breaking-change` label is set; the `!` is not). ### ๐ Praise - **`_commit_user_password_change`** โ optimistic-concurrency `UPDATE โฆ WHERE password = old_hash` (โ `PasswordChangeConflictError` โ 400), with the hash write + stamp bump + `clear_password_must_change` all inside one nested-safe `@transaction()` (verified re-entrant), cache updated only post-commit, then session rebuilt. Correct and clean. - bcrypt's 72-byte truncation is guarded at the hash, verify, and policy layers, and `test_common_passwords_stay_in_sync_with_frontend` asserts the TS blocklist equals `_COMMON_PASSWORDS` โ exactly the drift guard this dual-validation design needs. --- _Verified as non-issues so they aren't re-raised: `noop_user_update` / `get_first_user` / `update_user_auth_stat` all exist on FAB's `BaseSecurityManager` (the `auth_user_db` override runs); `AUTH_DB_FAKE_PASSWORD_HASH_CHECK` uses `.get(โฆ, DEFAULT)` (no KeyError); frontend digit/length checks are Unicode-aware (`\p{Nd}`, code-point counting) and match the backend; the change clears `password_must_change` and the endpoint is in `_EXEMPT_ENDPOINTS`._ <!-- enxdev-review-agent:f0de405 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD f0de405._ -- 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]
