Skip to content

fix(auth): check reset link expiry against interactive login timestamp - #64126

Open
silverkszlo wants to merge 1 commit into
masterfrom
fix/password-resetting
Open

fix(auth): check reset link expiry against interactive login timestamp#64126
silverkszlo wants to merge 1 commit into
masterfrom
fix/password-resetting

Conversation

@silverkszlo

@silverkszlo silverkszlo commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

The bug

When the password of a user expires and they want to reset it while they have an open session somewhere, the "Forgot password" flow sends them into a loop and never allows them to reset their password: logging in is being refused because the password expired, and the emailed reset link is being refused as expired as well.

How to reproduce

  • use an existing user account or create one, e.g. "blume"
  • log in to [dev/prod_url] as blume and leave that window open(!)
  • expire the password:
    occ user:setting \ blume password_policy pwd_last_updated $(( $(date +%s) - 10*86400 ))
  • In another private window go to [dev/prod_url] and log in as blume; you'll see "Password is expired, please use forgot password method to reset".
  • go back to login form and click on "Forgot password?" and submit "blume"
  • open [local/remote_mail_server] and find the password-reset mail
  • click on "Reset your password"
  • it takes you to "Could not reset password because the token is expired."

This solution

VerificationToken compared the reset link's creation time against IUser::getLastLogin(). Despite its name that value is a "last seen" timestamp. Session::validateSession() refreshes it on every request of an already authenticated session, so it kept moving past the reset link's creation time while the user did nothing but leave a session open.

This PR records the time of an actual authentication separately (lastInteractiveLogin) and compares against that instead. IUser::getLastLogin() keeps its current meaning, so the administration user list and inactive-account handling are unaffected.

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

Signed-off-by: silver <s.szmajduch@posteo.de>
Assisted-by: ClaudeCode:claude-fable-5
@silverkszlo
silverkszlo requested a review from a team as a code owner September 8, 2026 15:11
@silverkszlo
silverkszlo requested review from Altahrim, blizzz, leftybournes, provokateurin and salmart-dev and removed request for a team September 8, 2026 15:11
@blizzz
blizzz requested a review from artonge September 8, 2026 21:38
@blizzz

blizzz commented Sep 8, 2026

Copy link
Copy Markdown
Member

it takes you to "Could not reset password because the token is expired."

this step is on the normal browser, or the private browser window? i understand the latter.

PDOException: SQLSTATE[HY000]: General error: 1 no such table: oc_appconfig in /home/runner/work/server/server/3rdparty/doctrine/dbal/src/Driver/PDO/Connection.php:59

Hm, there might be an edge case hidden? 🤔

@blizzz

blizzz commented Sep 8, 2026

Copy link
Copy Markdown
Member

Auth is a tough area to look into, and full of mines 🙃 Great that you looked into it! On first sight it looks fine, but there are actually a things to improve, also to avoid frequent writes to DB.

Findings from a claude review:

  • each basic auth request will lead to a DB write. This could be a thing with simple clients or script that send auth headers instead of using cookies… though cookies login would also cause a write (Claude does not say so, but I do). User::updateLastLoginTimestamp() has a safe guard, maybe the same mechanism can be utilized.
  • above also means each client that uses auth would invoke the recording, e.g. Thunderbird or possible DavX5. Worth to limit it to browsers only? To still cover this case. Or Worth a deliberate decision: if "interactive" means a human authenticated, record from the web login Chain (plus loginWithCookie()), or gate on session-token creation / $regenerateSessionId, rather than on every non-token completeLogin().
  • now a pending link survives the full TOKEN_LIFETIME for an account that is only used via app passwords not sure this is realistic, if a link is double checked for validity not a problem. But if it could be re-used any later point in time by some other party, wouldn't be ideal.
  • suggest to store the value flagged as lazy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Notify user before password expiration

2 participants