You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(payload): write only sessions in session updates so concurrent user changes survive - #19434
Session bookkeeping on login, refresh, logout and failed-login lockout wrote the whole user row back from a copy read earlier in the request. A change committed to the user in between (password change, a field changed by an admin or a hook) was silently reverted.
Why?
With auth.useSessions (the default), these writes pass the in-memory user to payload.db.updateOne as data:
Code path
Write
addSessionToUser (login, reset-password)
data: user
revokeSession (login error path, reset-password)
data: user
refreshOperation
data: { ...user, sessions }
logoutOperation
data: userWithSessions (whole row from findOne)
incrementLoginAttempts, the "parallel failed attempts reached the limit" branch
data: user
Every column of the row is rewritten, so the stale values of every field win over whatever was committed after the read. The updatedAt = null trick on these paths only keeps the timestamp from moving; it does not stop the other columns from being rewritten.
Observed downstream (Postgres, 3.90.2): a loop of about 8 logins per second against one account reverted a concurrent password change (the old password worked again), an account block, and a custom field reset. The effect is a security problem as well as a data-loss one: a password reset or block can be undone by anyone who keeps logging in.
How?
These writes now send only the columns they own: { sessions, updatedAt }.
addSessionToUser, revokeSession, refresh and logout: updatedAt: null (unchanged semantics: leave the column alone).
Lockout branch of incrementLoginAttempts: updatedAt keeps the refreshed value it already set.
Nothing else changes; the in-memory user mutations that callers rely on are untouched.
main (v4) has the same pattern (addSessionToUser, revokeSession, extendSession, logoutOperation and the same lockout branch in incrementLoginAttempts), so it is affected too. This PR only targets 3.x; I can send the matching change for main once the approach here is agreed, as the files differ.
Why the tests missed it
Every existing session test is sequential. Login, refresh and logout are each exercised alone; nothing changes the user between the read and the write inside one request, which is the only place a stale whole-row write loses anything.
The assertions look at sessions only. The tests check that sessions were added, removed or refreshed. No test checks that other fields of the user survive that write.
A concurrent change can't be produced from outside. The window between read and write is a few milliseconds inside one operation, so a test has to hold it open from inside (see below).
Test coverage added
New describe('session writes do not overwrite concurrent user changes') in test/auth/int.spec.ts. A helper wraps payload.db.updateOne and, right before the first write that carries sessions, commits a change through the original updateOne (separate transaction), so the race is deterministic rather than timing-based.
Test
Guards
field changed before the session is added on login
addSessionToUser
password changed before the session is added on login (old password must be rejected, new accepted)
addSessionToUser, the reported symptom
field changed before the session is refreshed
refreshOperation
field changed before the session is removed on logout
logoutOperation
field changed before the lockout branch removes recent sessions after a failed login
incrementLoginAttempts
revokeSession shares its write shape with addSessionToUser and is not covered by a separate test; it is only reached from the login/reset-password error paths.
Verification
Run on the unfixed source first, to confirm the failure reason, then with the fix.
1 failed (lockout: field reverted to its initial value); 4 skipped
1 passed; 4 skipped
Full test/auth/int.spec.ts with the fix: Postgres 101 passed / 1 skipped; MongoDB 97 passed / 5 skipped. pnpm test:int:postgres auth (all auth suites): 143 passed / 1 skipped.
tsc --noEmit clean for packages/payload, drizzle, db-mongodb, graphql.
ESLint: no new errors in the touched files (the test file has 12 errors at HEAD, 12 after); prettier clean.
Why 4 of the tests are skipped on MongoDB
Login, refresh and logout write inside the request's transaction. MongoDB aborts a transaction that writes a document changed after the transaction began (WriteConflict, HTTP 500) instead of overwriting it, both before and after this change. So the lost update cannot be reproduced there with these paths, and the tests skip. The lockout path runs without a transaction and fails/passes on both adapters. The Mongo WriteConflict on a login that races an update is a separate behaviour that I did not change.
Not verified
SQLite and the other drizzle adapters were not run; the change only narrows the data object, but I did not execute it there.
The reported symptom was measured on a downstream app (about 8 logins/s loop, Postgres, 3.90.2) with an equivalent patch to the built JS; the in-repo tests reproduce the same lost update deterministically rather than by load.
db-postgres was not typechecked (its dependency dist is not built in my checkout); the change does not touch it.
No e2e/admin UI tests; the change is on server-side auth operations.
main was inspected but not changed or tested.
Notes for review
Left alone on purpose: resetLoginAttempts already writes only lockUntil / loginAttempts.
Same whole-row pattern, deliberately not changed to keep this PR to session writes: verifyEmail (data: { ...user, _verified: true, ... }) and resetPassword (data: user with sessions = []). They are token-gated one-shot flows rather than something an attacker can trigger in a loop, but they have the same stale-write shape; happy to follow up if you want them narrowed too.
A password change through payload.update clears sessions; a login that read the user before it will still write its own sessions array. That remaining "stale sessions" behaviour is narrower than the full-row overwrite and is not changed here.
…er changes survive
Login (`addSessionToUser`), `revokeSession`, refresh, logout and the parallel-attempt lockout
branch of `incrementLoginAttempts` passed the whole user row, read earlier in the request, to
`payload.db.updateOne`. Any change committed between that read and the write (a password change,
a field changed by an admin or a hook) was overwritten with the stale values.
These writes now send only `sessions` and `updatedAt`. `updatedAt: null` still means "leave the
column untouched"; the lockout path keeps writing its own refreshed `updatedAt`.
On MongoDB the transactional paths (login, refresh, logout) fail with a WriteConflict instead of
overwriting, so the new tests for those paths are skipped there; the lockout path runs without a
transaction and is covered on both adapters.
No actionable comments were generated in the recent review.
Walkthrough
Authentication session updates now write only session-related data and timestamp fields instead of the full user object. Integration tests cover concurrent user and password changes during login, refresh, logout, and failed-login flows.
Logout, refresh, session creation and revocation, and failed-login cleanup now update session data rather than the full user object. The updates pass updatedAt: null where specified.
New tests inject concurrent user-field or password changes before session writes and verify that the changes remain stored. Selected race tests are skipped for Mongoose transactions.
payloadcms/payload#19045: Moves the refresh session extension and its database write into extendSession; this PR narrows that refresh write.
payloadcms/payload#19040: Builds on extendSession to redeem refresh tokens and revoke sessions; this PR narrows refresh and revocation session writes.
Suggested labels:v3
Suggested reviewers:alessiogr
Merge Risk: Minimal · up to e8fd1
Session writes now touch only session-related columns, so concurrent changes to other user fields such as password or custom fields are no longer overwritten. No merge-blocking risk was identified. The PR title scope fails the repository's title check and should be corrected before merge.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly describes the session write change and its purpose.
Description check
The description covers the session write changes, race tests, and verification limits.
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 5 files.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
ESLint
If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.
packages/payload/src/auth/operations/logout.ts
ESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox.
packages/payload/src/auth/operations/refresh.ts
ESLint skipped: the matched ESLint configuration already failed (missing-dependency).
packages/payload/src/auth/sessions.ts
ESLint skipped: the matched ESLint configuration already failed (missing-dependency).
2 others
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Session bookkeeping on login, refresh, logout and failed-login lockout wrote the whole user row back from a copy read earlier in the request. A change committed to the user in between (password change, a field changed by an admin or a hook) was silently reverted.
Why?
With
auth.useSessions(the default), these writes pass the in-memory user topayload.db.updateOneasdata:addSessionToUser(login, reset-password)data: userrevokeSession(login error path, reset-password)data: userrefreshOperationdata: { ...user, sessions }logoutOperationdata: userWithSessions(whole row fromfindOne)incrementLoginAttempts, the "parallel failed attempts reached the limit" branchdata: userEvery column of the row is rewritten, so the stale values of every field win over whatever was committed after the read. The
updatedAt = nulltrick on these paths only keeps the timestamp from moving; it does not stop the other columns from being rewritten.Observed downstream (Postgres, 3.90.2): a loop of about 8 logins per second against one account reverted a concurrent password change (the old password worked again), an account block, and a custom field reset. The effect is a security problem as well as a data-loss one: a password reset or block can be undone by anyone who keeps logging in.
How?
These writes now send only the columns they own:
{ sessions, updatedAt }.addSessionToUser,revokeSession, refresh and logout:updatedAt: null(unchanged semantics: leave the column alone).incrementLoginAttempts:updatedAtkeeps the refreshed value it already set.Nothing else changes; the in-memory
usermutations that callers rely on are untouched.main(v4) has the same pattern (addSessionToUser,revokeSession,extendSession,logoutOperationand the same lockout branch inincrementLoginAttempts), so it is affected too. This PR only targets3.x; I can send the matching change formainonce the approach here is agreed, as the files differ.Why the tests missed it
sessionsonly. The tests check that sessions were added, removed or refreshed. No test checks that other fields of the user survive that write.Test coverage added
New
describe('session writes do not overwrite concurrent user changes')intest/auth/int.spec.ts. A helper wrapspayload.db.updateOneand, right before the first write that carriessessions, commits a change through the originalupdateOne(separate transaction), so the race is deterministic rather than timing-based.addSessionToUseraddSessionToUser, the reported symptomrefreshOperationlogoutOperationincrementLoginAttemptsrevokeSessionshares its write shape withaddSessionToUserand is not covered by a separate test; it is only reached from the login/reset-password error paths.Verification
Run on the unfixed source first, to confirm the failure reason, then with the fix.
pnpm test:int:postgres auth/int.spec.ts -t "session writes")null; the old password still logs in)pnpm test:int auth/int.spec.ts -t "session writes")test/auth/int.spec.tswith the fix: Postgres 101 passed / 1 skipped; MongoDB 97 passed / 5 skipped.pnpm test:int:postgres auth(all auth suites): 143 passed / 1 skipped.tsc --noEmitclean forpackages/payload,drizzle,db-mongodb,graphql.HEAD, 12 after); prettier clean.Why 4 of the tests are skipped on MongoDB
Login, refresh and logout write inside the request's transaction. MongoDB aborts a transaction that writes a document changed after the transaction began (
WriteConflict, HTTP 500) instead of overwriting it, both before and after this change. So the lost update cannot be reproduced there with these paths, and the tests skip. The lockout path runs without a transaction and fails/passes on both adapters. The MongoWriteConflicton a login that races an update is a separate behaviour that I did not change.Not verified
dataobject, but I did not execute it there.db-postgreswas not typechecked (its dependencydistis not built in my checkout); the change does not touch it.mainwas inspected but not changed or tested.Notes for review
resetLoginAttemptsalready writes onlylockUntil/loginAttempts.verifyEmail(data: { ...user, _verified: true, ... }) andresetPassword(data: userwithsessions = []). They are token-gated one-shot flows rather than something an attacker can trigger in a loop, but they have the same stale-write shape; happy to follow up if you want them narrowed too.payload.updateclearssessions; a login that read the user before it will still write its ownsessionsarray. That remaining "stale sessions" behaviour is narrower than the full-row overwrite and is not changed here.