Repository navigation
End a changed password's secure sessions even while its sign-in limit is held full - #6955
Conversation
… secure sessions once the Matrix server accepts it Ending sessions after a password change was proven with the new password under the sign-in limit, which whoever holds the old password could keep full. The Matrix server accepting the change is now the proof. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rding Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 065aeb3b2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| password: typeof password === 'string' ? password : '', | ||
| }); | ||
| await setContextResponse(ctxt, new Response(null, { status: 204 })); | ||
| let newPassword = textOf(attributes, 'newPassword'); |
There was a problem hiding this comment.
During the ECS rolling deploy in .github/workflows/deploy-release.yml, an already-loaded old credential or reset page can send the former { username, password } body to a new task. Those old callers first change the Matrix password and deliberately ignore failure of this cleanup request; because this line now requires newPassword, the new task returns 400 and leaves every old secure/CLI session valid for up to a week. This violates ADR 005 §1.6 in exactly the leaked-password scenario, so the server contract needs a staged or versioned compatibility path rather than an atomic replacement.
AGENTS.md reference: AGENTS.md:L304-L304
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Agreed, and the window is wider than the rolling deploy: a reset page or credential frame keeps the code it loaded with, and an app tab can stay open for days. Fixed in e7a8c29. A body with username and password and no newPassword comes from a page that set the password with Synapse itself. The route ends that account's sessions once the new password checks out under the sign-in limit, as before, and answers 204. Only those pages are exposed to the limit starvation. ADR 005 §1.6 permits it with a MAY, the secure-session skill documents it, and a realm-server test covers the old password, the new one and the shared limit. CS-14356 tracks removing the shape once no such page can still be open. The other direction, a new page reaching an old task, fails visibly: the old task finds no username and answers 401, the password stays as it was, and the user tries again.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review covered the security reasoning: what card code, holding the app's Matrix token and able to drive the frame or send any Origin, can do with the route. It also checked the reset's email check, the response passthrough and redaction, the host and secure-origin callers, ADR 005 §1.6, and whether the tests pin their claims. It read Synapse v1.126.0's auth.py, account.py, set_password.py and ui_auth/checkers.py. I didn't run the realm-server suite. Its claims are verified by reading the code only.
No blocking issues. Synapse accepting account/password is now the proof, and no limit an old-password holder can fill sits between that and the UPDATE. Synapse's own _failed_uia_attempts_ratelimiter never counts a wrong UIA password, because check_ui_auth catches the LoginError and turns it into InteractiveAuthIncompleteError. So it can't be held full against confirmPassword either. The change path gives card code nothing beyond what it can already ask Synapse with the app's token.
- The frame should take the request's shape from its form, not from whether
accessTokenwas sent. See the comment onset-passwordincredentials.js. Should fix, non-blocking, pre-existing. - The host's
set-passwordargs still require anauthdict that a change never sends. Seecredential-fields.ts. Nit. - A reset with no
email, and the failure path that still answers 200, have no realm-server test. See the comment on the reset test. Nit. - The PR description cites a PR number and tells the change as before and after ("made the pages…", "no longer call…"). Under the evergreen rule, describe the current shape and leave the history to the commit messages and the ADR's Sources. Nit.
Adjacent, a follow-up and not this PR: SetPasswordHandler.set_password deletes devices and access tokens, but not unredeemed login_tokens. A Matrix login token minted from the old-password holder's secure session (POST /_secure-session/login-token) in the two minutes before a change can still be redeemed afterwards, for a Matrix device that logout_devices never saw. For a reset, the same holds during resetAccount's round trips, because the rows are still live then. The device can't sign in to a secure session or change the password, but it does outlive the change, which set-password's comment says a stolen session must not. One fix is for the realm server to delete the account's devices again through the admin API once login_via_existing_session.token_timeout has passed after a change. Worth a ticket if you agree.
| async 'set-password'({ auth, accessToken, account }) { | ||
| let result = await this.matrixRequest('account/password', { | ||
| accessToken, | ||
| body: { | ||
| auth: this.withPassword(plainObject(auth)), | ||
| new_password: this.value('password'), | ||
| logout_devices: true, | ||
| }, | ||
| }); | ||
| if (result.ok && typeof account === 'string' && account) { | ||
| try { | ||
| await this.endSessions({ | ||
| username: account, | ||
| password: this.value('password'), | ||
| }); | ||
| } catch { | ||
| // The password is changed whatever the answer, so a failure is not | ||
| // the app's to see; a session the realm server couldn't end runs out | ||
| // within its week. The app can name any account, but only the new | ||
| // password, which it never sees, gets a session ended. | ||
| } | ||
| let newPassword = this.value('password'); | ||
| let creds = plainObject(plainObject(auth).threepid_creds); | ||
| if (typeof accessToken === 'string' && accessToken) { | ||
| return await this.changePassword({ | ||
| newPassword, | ||
| accessToken, | ||
| currentPassword: this.value('current'), | ||
| }); | ||
| } | ||
| return result; | ||
| return await this.changePassword({ | ||
| newPassword, | ||
| sid: String(creds.sid ?? ''), | ||
| clientSecret: String(creds.client_secret ?? ''), | ||
| ...(typeof account === 'string' && account ? { email: account } : {}), | ||
| }); |
There was a problem hiding this comment.
[Claude Code 🤖] Should fix, non-blocking. Pre-existing, but this rewrite is the place to close it. The request's shape here depends on whether the app sent accessToken, not on which form this is. So card code driving the change-password form can leave the token out and pass auth.threepid_creds from a reset it validated on an account it controls. The password the user types as new then becomes the password of card code's account. The user's own password and secure sessions don't change, and the frame still answers ok: true. The old frame let the app's auth dict do the same, so this isn't a regression. But this function is now the one place that decides what the typed password authorizes, which the secure-origin skill says belongs in credentials.js.
Take the shape from the form. Among the set-password forms, only change-password has a current field:
if (this.fields.has('current')) {
if (typeof accessToken !== 'string' || !accessToken) {
return { ok: false, status: 0, body: { error: 'A change needs the app’s token.' } };
}
return await this.changePassword({ newPassword, accessToken, currentPassword: this.value('current') });
}
// reset: ignore accessTokenAdd a unit test that the change form, sent threepid creds and no token, sends nothing. Verified by reading the old and new set-password against performNow, which checks the operation for each form but never the shape.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 41e36d4. set-password takes its shape from the form. The form with a current field makes a change, and only with the app's access token: without one it answers ok: false and sends nothing. The reset form always makes a reset and ignores any token it's sent. A new unit test sends the change form reset credentials and no token and checks nothing is sent, then sends the reset form a token and checks it still makes a reset.
| auth: Record<string, unknown>; | ||
| accessToken?: string; | ||
| // The account whose password this is, by Matrix id or email address. | ||
| // Once the password is changed, the fields end the account's secure | ||
| // sessions at the realm server, proving it with the new password. | ||
| // For a reset, the email address the reset was sent to. The realm | ||
| // server sets the password and ends the secure sessions of the | ||
| // account this names once the new password checks out for it. A | ||
| // change made with `accessToken` ends the sessions of the token's | ||
| // account. | ||
| account?: string; |
There was a problem hiding this comment.
[Claude Code 🤖] Nit. For a change, the frame reads only accessToken and its own current field. But auth is still required here, and profile-settings-modal.gts still builds an m.login.password dict with user and identifier that nothing reads. The type should say what crosses the boundary. For example, make auth (really just threepid_creds) reset-only, and drop the dict from the change caller.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 41e36d4. set-password's arguments are now a union of what crosses to the frame: { accessToken } for a change, or the reset's m.login.email.identity auth with threepid_creds and an optional account. The settings modal sends only its token.
| ); | ||
| }); | ||
|
|
||
| test('a reset ends the sessions only of the account whose password it set', async function (assert) { |
There was a problem hiding this comment.
[Claude Code 🤖] Nit. Two branches of changePassword have no realm-server test. One is a reset with no email, which should answer 200, set the password and leave the account's rows alone. The other is ending the sessions failing after Synapse said 200, which should log on realm:secure-session and still answer 200. The secure-origin test that pinned "the password is reset even when its sessions can't be ended" was deleted, and that behavior now lives only on the server. The first is a couple of lines next to this test. For the second, a resetAccount that throws, for example with an email whose admin lookup fails, would do it.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 41e36d4. One test is a reset whose link names no email: it sets the password and leaves the account's CLI session working. The other calls changePassword directly with a database adapter that throws on the UPDATE. It answers { status: 200, body: {} }, the new password signs in, and realm:secure-session logs the failure without the password. Both pass locally, along with the rest of secure-session-test.ts (45/45).
…rguments, and test the unnamed reset and a failed ending Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…self
A reset page or credential frame keeps the code it loaded with across a
deploy, and an app tab can stay open for days. Such a page changes the
password with Synapse and sends {username, password}; refusing it would end
no session at all.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
[Claude Code 🤖] On the two findings in the review body:
|
Changing or resetting a password has to end the account's secure sessions. Those are the realm-server sessions the secure window (and
boxel keys sign-in) signs in for on the secure origin, and they are the only thing that can mint a key write ticket. A user who resets a leaked password needs this most: whoever signed in with the leaked password otherwise keeps a session that can change the user's keys for up to a week, the CLI's included.The pages that set a password end those sessions (#6866). Once Synapse accepts the new password, the page sends it to
POST /_secure-sessions/password-change, and the realm server checks it the way a password sign-in does before expiring everysecure_sessionsrow of the account. That check runs under the per-account limit on failed password attempts (SignInLimiter), and the limit is the problem. Whoever holds the old password can keep that limit full with wrong guesses. A refused guess costs them nothing, and the 429 tells them exactly when the next attempt is free. So a script polling at its own round-trip time takes every free attempt before the page's retry arrives. The page gives up after a minute, and the attacker keeps their sessions for the week. The limit can't simply be dropped either. Without it, anyone still holding a session could guess at the new password without limit, and watch for their own session to end.What this changes
The proof that the password changed is Synapse accepting the change, not a check of the new password afterwards. The pages that set a password send the change to the realm server instead of calling Synapse's
account/passwordthemselves. They send it, on the same route, and the realm server makes the Synapse request withlogout_devices: true. When Synapse answers 200, the realm server knows the password changed, and for which account, and expires the rows. Nothing about this goes throughSignInLimiter, so keeping the limit full stops nothing. The route answers with Synapse's own status and body, so the pages and the app read Synapse's answer (a wrong current password is Synapse'sM_FORBIDDEN, an unvalidated reset its 401).The route takes one of two shapes:
{ accessToken, currentPassword, newPassword }. The account is the token's (account/whoami), and Synapse checks the current password. Card code holds the app's token but never sees a password, so it can't use this to end the CLI's sessions. And whoever holds only the old password can't make a change once the password has changed, whatever token they mint, because Synapse asks for the current one. The device that made the change stays signed in; Synapse signs out every other one.{ sid, clientSecret, email, newPassword }. The realm server builds them.login.email.identityauth itself, so no other UIA session can ride along. Synapse finds the account from the validated email and doesn't say which account that was. So the realm server ends the sessions of the accountemailnames (the address the reset link was sent to), and only once the new password checks out for that account. Without that check, resetting an account you control could end anyone else's sessions. The check runs whatever the sign-in limit says. That does leave a way to test a guess at another account's password: reset your own account to the guess, name theirs, and a session you hold of theirs ends if the guess was right. But Synapse deletes a validation session the first time a reset uses it (EmailIdentityAuthChecker._check_threepidin v1.126.0), so every guess costs a confirmed reset email to an account the guesser controls, paced by Synapse's own limits on validation emails. That is a far worse rate than a guess every few seconds. A link that names no email resets the password and ends no session.If ending the sessions fails after Synapse has set the password (the admin API is down, say), the failure is logged on
realm:secure-session, and the route still answers 200. The password is set, a reset's validation session is spent, and the page can't usefully try again.Pages already open across a deploy
A reset page or a credential frame keeps the code it loaded with, and an app tab can stay open for days. Such a page changes the password with Synapse itself and then sends
{ username, password }. Refusing that shape would leave its change with no session ended at all. So the route takes it too, checks the new password under the sign-in limit (endSessionsForNewPassword), and answers 204. Only those pages meet the starvation above. CS-14356 removes the shape once no page that sends it can still be open.The other direction, a new page reaching an old task during the rolling deploy, fails visibly. The old task reads no
username, answers 401, and the password stays as it was, so the user tries again.I considered and rejected two other proofs, and recorded them in ADR 005:
POST /_secure-session/login-token). So they can get a fresh access token after the change and fill that limit too.Where the changes live
packages/realm-server/lib/secure-session.ts:changePasswordreplacesendSessionsForNewPassword. The handler inhandlers/handle-secure-session.tsreads the two shapes, and the route no longer takes the limiter (routes.ts).packages/secure-origin/src/security/session.js:changePasswordposts the change and returns{ ok, status, body }. It replaces the retryingendSessionsForNewPassword, since there is no limit left to retry against.matrix.jslosesresetPassword.packages/secure-origin/src/security/credentials.js:set-passwordsends a reset or a change to the realm server, with the passwords from its own fields.src/shell/password-reset/reset-password.gtssends its reset the same way.packages/host:set-password's arguments are a union of what crosses to the frame, the access token for a change, or the reset's validation session and email address for a reset. The settings modal sends only its token.secure-sessionandsecure-originskills describe the route and the frame.Testing
packages/realm-server/tests/secure-session-test.tsdrives real Synapse and smtp4dev. A reset requests the email, reads the link from smtp4dev, and confirms it with the same form POST a user's click makes:{ username, password }, ends the sessions with the new password, is refused with the old one, and shares the sign-in limit;The secure-origin unit and component tests cover the frame's two request shapes, that the shape comes from the form and never from what the app sends (a change form sent reset credentials and no token sends nothing), the redaction of Synapse's refusal, and the reset page's request and error handling.
Not in this PR
{ username, password }shape.🤖 Generated with Claude Code