codeant-ai-for-open-source[bot] commented on code in PR #42934:
URL: https://github.com/apache/superset/pull/42934#discussion_r3744381527
##########
superset/views/users/api.py:
##########
@@ -49,12 +50,36 @@ class CurrentUserRestApi(BaseSupersetApi):
def pre_update(self, item: User, data: Dict[str, Any]) -> None:
item.changed_on = datetime.now()
item.changed_by_fk = g.user.id
+ # Pop unconditionally: this key is only meaningful for verifying a
+ # password change below, and it isn't a real column on the user
+ # model -- it must never reach ``UserDAO.update``'s ``setattr`` loop.
+ current_password = data.pop("current_password", None)
if "password" in data and data["password"]:
+ # An account with no password set yet (e.g. provisioned via an
+ # external auth backend) has nothing to prove knowledge of; for
+ # every other account, the caller must confirm the existing
+ # password before it can be replaced.
+ proof_ok = current_password and check_password_hash(
+ item.password, current_password
+ )
Review Comment:
**Suggestion:** When an account has no password and the request nevertheless
supplies a non-empty `current_password`, `check_password_hash` is called with
`item.password` equal to `None` before the later `item.password` guard is
evaluated. Werkzeug will fail while parsing the missing hash, producing an
unhandled server error instead of allowing the first password to be set or
returning validation feedback. Only call `check_password_hash` when
`item.password` is present. [null pointer]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Passwordless users receive an unhandled error on valid password updates.
- ⚠️ First-password setup fails when clients send extra password-proof data.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=53f5eac8009443479b47df8fffbf0b1c&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=53f5eac8009443479b47df8fffbf0b1c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/views/users/api.py
**Line:** 62:64
**Comment:**
*Null Pointer: When an account has no password and the request
nevertheless supplies a non-empty `current_password`, `check_password_hash` is
called with `item.password` equal to `None` before the later `item.password`
guard is evaluated. Werkzeug will fail while parsing the missing hash,
producing an unhandled server error instead of allowing the first password to
be set or returning validation feedback. Only call `check_password_hash` when
`item.password` is present.
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%2F42934&comment_hash=ab46fd05d721c10cc6180e0e0389d21e01c16b29ea2620aca90a15c967e137b5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42934&comment_hash=ab46fd05d721c10cc6180e0e0389d21e01c16b29ea2620aca90a15c967e137b5&reaction=dislike'>👎</a>
##########
superset/security/session_invalidation.py:
##########
@@ -187,6 +187,23 @@ def _stamp_existing() -> int:
_stamp_existing()
+def invalidate_sessions_for_user(user_id: int) -> None:
+ """Stamp the invalidation epoch for ``user_id`` from ordinary application
code.
+
+ Convenience wrapper around ``invalidate_user_sessions`` for callers that
+ don't have the raw ``Connection`` the ``after_update`` event listener
+ receives -- e.g. a password-change flow. The stamp is written through the
+ current session's own connection, so it participates in whatever
+ transaction the caller's other pending changes belong to; it is not
+ committed here, so the caller's own commit (or the next flush that
+ triggers one) is what makes it durable.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.extensions import db
+
+ invalidate_user_sessions(db.session.connection(), user_id)
Review Comment:
**Suggestion:** The wrapper can regress the invalidation epoch when
concurrent password changes or session-termination requests compute timestamps
before acquiring the row lock. A transaction with an older `now` value can
update the row after a transaction with a newer value commits, leaving the
older epoch persisted; sessions created after the newer invalidation but before
the older commit can then survive. Update the existing row using the maximum of
its current value and the newly computed timestamp (or otherwise
serialize/guard the write) so the epoch is monotonic. [race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Concurrent password changes can leave older sessions authenticated.
- ❌ Explicit admin session termination can be partially defeated.
- ⚠️ Affected sessions bypass logout until a later invalidation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=849d364fa266476598497d1ee58bfe6f&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=849d364fa266476598497d1ee58bfe6f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/session_invalidation.py
**Line:** 204:204
**Comment:**
*Race Condition: The wrapper can regress the invalidation epoch when
concurrent password changes or session-termination requests compute timestamps
before acquiring the row lock. A transaction with an older `now` value can
update the row after a transaction with a newer value commits, leaving the
older epoch persisted; sessions created after the newer invalidation but before
the older commit can then survive. Update the existing row using the maximum of
its current value and the newly computed timestamp (or otherwise
serialize/guard the write) so the epoch is monotonic.
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%2F42934&comment_hash=2d3d170210ba481f12a9e01668e5402093da4755986446b206e9f159a635b9c4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42934&comment_hash=2d3d170210ba481f12a9e01668e5402093da4755986446b206e9f159a635b9c4&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]
