fix(oauth2): revoke refresh tokens on password reset#5177
Conversation
change_password updated the password hash but left the user's refresh tokens intact. refresh_access_token only re-checks that the user is still enabled, not whether the password changed, so a leaked refresh token could keep minting access tokens for up to REFRESH_TOKEN_EXPIRE_DAYS after a reset. Revoke the target's outstanding refresh tokens after updating the password. Addresses the last remaining finding from the xorbitsai#5058 audit (the others were fixed in xorbitsai#5088/xorbitsai#5089 and the setup-flow rework).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a security vulnerability by ensuring that resetting a user's password revokes all of their outstanding refresh tokens. It adds a call to delete_user_refresh_tokens in the change_password route and introduces comprehensive regression tests to verify this behavior. The review feedback points out that the test helper uses asyncio.get_event_loop(), which is deprecated since Python 3.10 and will fail in Python 3.12+, and suggests using asyncio.run() instead.
asyncio.get_event_loop() is deprecated when no loop is running and raises RuntimeError on Python 3.12+; asyncio.run manages the loop lifecycle and works across the 3.10-3.13 CI matrix.
qinxuye
left a comment
qinxuye
left a comment
There was a problem hiding this comment.
Two findings from the original #5058 report are still not fully resolved:
- Finding 4 remains raceable because password reset and refresh-token rotation are separate database operations.
- Finding 5 still affects upgrades: an existing account with
must_change_password=1can obtain unrestricted admin tokens. #5144 removes the vulnerable bootstrap path for fresh installs, but there is no migration or server-side enforcement for existing databases.
The other findings appear to be addressed. The focused auth tests pass, but they do not cover these two scenarios.
…rency The previous fix revoked refresh tokens after updating the password, but refresh_access_token performed its read-delete-insert rotation across three separate transactions. A refresh could read a still-valid token, the reset could delete all of the user's tokens, and the refresh could then insert a replacement that survived the reset -- keeping a leaked session alive. Move both flows into single BEGIN IMMEDIATE transactions that serialize on SQLite's write lock (matching create_first_user), including across processes: - Database.rotate_refresh_token: validate-delete-insert in one transaction, succeeding only if the old token still existed at delete time. - Database.update_password_and_revoke_tokens: update the hash and delete the user's refresh tokens in one transaction. - refresh_access_token bails out if the token was revoked before it took the write lock, so it never mints a session past a password reset. Add a multi-threaded regression test that interleaves refresh and reset; it reproduces surviving tokens against the old non-atomic code and passes here.
|
Thanks for the careful review. Finding 4 (race): fixed in e4da7f3 — see the inline reply. Rotation and password-reset revocation are now mutually atomic via Finding 5 (existing DBs with |
…Finding 5)
The must_change_password flag was only returned to the frontend as a hint;
the auth dependency never enforced it. An existing account carrying
must_change_password=1 (e.g. migrated from an older database) could obtain
fully-functional tokens -- including admin -- and use them without ever
changing the password.
Add a gate in AdvancedAuthService.__call__, placed after the enabled check
and before the admin bypass, so even an admin-scoped account is blocked while
flagged. A flagged user may make exactly one request: PUT on their own
/v1/admin/users/{id}/password; everything else returns 403. The exempt path
is matched by regex anchored on the trailing segments, independent of any
router mount prefix, and only for the caller's own user id.
Add tests covering the block, the own-password-change exemption, the
admin-before-bypass ordering, rejection of changing another user's password,
that unflagged users are unaffected, and that a GET to the password path is
not a bypass.
|
Finding 5: now enforced server-side in this PR (e75d827), as you suggested. Added a gate in Two design notes:
Full oauth2 suite (54 tests) and pre-commit pass. Tests: |
The Finding 5 gate only covered JWTs; the API-key branch returns the user before reaching it, so an existing API key owned by a legacy must_change_password=1 account could still call model endpoints, bypassing the flag. Block the API-key path outright when the owning account is still flagged. An API key can only reach model endpoints (it cannot change a password), so there is no exempt request: the key stays blocked until the owner clears the flag via a JWT password change, after which the same key works again. The check reads must_change_password from the live DB row, so clearing the flag takes effect immediately without recreating the key. Add regression tests: a flagged account's API key is rejected with 403, and the same key works again once the flag is cleared.
3 participants
Summary
change_passwordin the advanced OAuth2 layer updated the password hash but never revoked the target user's refresh tokens.refresh_access_tokenonly re-checks that the user is stillenabled— not whether the password changed — so a leaked/stolen refresh token could keep minting fresh access tokens for up toREFRESH_TOKEN_EXPIRE_DAYS(7 days) after an admin resets the user's password.This calls
delete_user_refresh_tokens(user_id)right after the hash update, so a password reset terminates all outstanding sessions for that user.Context
This is the last remaining finding from the #5058 security audit. Re-verifying all six findings against current
main:expcheck)users:manage→adminescalation via permission grant_reject_admin_target_takeover)must_change_passwordnot enforced + plaintext password loggedsetup_adminflow, so no random password is logged and no account relies onmust_change_passwordas a security boundaryuser_enabledcache check)Test
Adds
test_oauth2_password_reset_revokes_tokens.py, which drives the realchange_passwordhandler against a temporary SQLite DB and asserts the target's refresh tokens are deleted while other users' tokens are untouched.65 passedpre-commit(black, flake8, isort, mypy, codespell): clean