From 4fde0f23ce51e9147ef137403f250758b7697c4a Mon Sep 17 00:00:00 2001 From: Alessio Gravili Date: Mon, 28 Jul 2025 16:08:10 -0700 Subject: [PATCH] fix: use atomic operation for incrementing login attempts (#13204) --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1210561338171141 --- packages/payload/src/auth/isUserLocked.ts | 4 +- packages/payload/src/auth/operations/login.ts | 44 ++++- .../local/incrementLoginAttempts.ts | 159 ++++++++++++++---- .../strategies/local/resetLoginAttempts.ts | 5 +- test/auth/int.spec.ts | 138 +++++++++++++-- 5 files changed, 299 insertions(+), 51 deletions(-) diff --git a/packages/payload/src/auth/isUserLocked.ts b/packages/payload/src/auth/isUserLocked.ts index a1ea9c2bf7..8a1b4c881d 100644 --- a/packages/payload/src/auth/isUserLocked.ts +++ b/packages/payload/src/auth/isUserLocked.ts @@ -1,6 +1,6 @@ -export const isUserLocked = (date: number): boolean => { +export const isUserLocked = (date: Date): boolean => { if (!date) { return false } - return date > Date.now() + return date.getTime() > Date.now() } diff --git a/packages/payload/src/auth/operations/login.ts b/packages/payload/src/auth/operations/login.ts index 4229ec81c2..48db4eecff 100644 --- a/packages/payload/src/auth/operations/login.ts +++ b/packages/payload/src/auth/operations/login.ts @@ -50,6 +50,11 @@ type CheckLoginPermissionArgs = { user: any } +/** + * Throws an error if the user is locked or does not exist. + * This does not check the login attempts, only the lock status. Whoever increments login attempts + * is responsible for locking the user properly, not whoever checks the login permission. + */ export const checkLoginPermission = ({ loggingInWithUsername, req, @@ -59,7 +64,7 @@ export const checkLoginPermission = ({ throw new AuthenticationError(req.t, Boolean(loggingInWithUsername)) } - if (isUserLocked(new Date(user.lockUntil).getTime())) { + if (isUserLocked(new Date(user.lockUntil))) { throw new LockedAuth(req.t) } } @@ -206,11 +211,11 @@ export const loginOperation = async ( where: whereConstraint, }) - let user = await payload.db.findOne({ + let user = (await payload.db.findOne({ collection: collectionConfig.slug, req, where: whereConstraint, - }) + })) as TypedUser checkLoginPermission({ loggingInWithUsername: Boolean(canLoginWithUsername && sanitizedUsername), @@ -230,9 +235,16 @@ export const loginOperation = async ( if (maxLoginAttemptsEnabled) { await incrementLoginAttempts({ collection: collectionConfig, - doc: user, payload: req.payload, req, + user, + }) + + // Re-check login permissions and max attempts after incrementing attempts, in case parallel updates occurred + checkLoginPermission({ + loggingInWithUsername: Boolean(canLoginWithUsername && sanitizedUsername), + req, + user, }) } @@ -243,6 +255,30 @@ export const loginOperation = async ( throw new UnverifiedEmail({ t: req.t }) } + /* + * Correct password accepted - re‑check that the account didn't + * get locked by parallel bad attempts in the meantime. + */ + if (maxLoginAttemptsEnabled) { + const { lockUntil, loginAttempts } = (await payload.db.findOne({ + collection: collectionConfig.slug, + req, + select: { + lockUntil: true, + loginAttempts: true, + }, + where: { id: { equals: user.id } }, + }))! + + user.lockUntil = lockUntil + user.loginAttempts = loginAttempts + + checkLoginPermission({ + req, + user, + }) + } + const fieldsToSignArgs: Parameters[0] = { collectionConfig, email: sanitizedEmail!, diff --git a/packages/payload/src/auth/strategies/local/incrementLoginAttempts.ts b/packages/payload/src/auth/strategies/local/incrementLoginAttempts.ts index 3a227f65f6..4fb86bb5e1 100644 --- a/packages/payload/src/auth/strategies/local/incrementLoginAttempts.ts +++ b/packages/payload/src/auth/strategies/local/incrementLoginAttempts.ts @@ -1,59 +1,154 @@ -import type { SanitizedCollectionConfig, TypeWithID } from '../../../collections/config/types.js' -import type { JsonObject, Payload } from '../../../index.js' +import type { SanitizedCollectionConfig } from '../../../collections/config/types.js' import type { PayloadRequest } from '../../../types/index.js' +import { type JsonObject, type Payload, type TypedUser } from '../../../index.js' +import { isUserLocked } from '../../isUserLocked.js' + type Args = { collection: SanitizedCollectionConfig - doc: Record & TypeWithID payload: Payload req: PayloadRequest + user: TypedUser } +// Note: this function does not use req in most updates, as we want those to be visible in parallel requests that are on a different +// transaction. At the same time, we want updates from parallel requests to be visible here. export const incrementLoginAttempts = async ({ collection, - doc, payload, req, + user, }: Args): Promise => { const { auth: { lockTime, maxLoginAttempts }, } = collection - if ('lockUntil' in doc && typeof doc.lockUntil === 'string') { - const lockUntil = new Date(doc.lockUntil).getTime() + const currentTime = Date.now() + let updatedLockUntil: null | string = null + let updatedLoginAttempts: null | number = null + + if (user.lockUntil && !isUserLocked(new Date(user.lockUntil))) { // Expired lock, restart count at 1 - if (lockUntil < Date.now()) { - await payload.update({ - id: doc.id, - collection: collection.slug, - data: { - lockUntil: null, - loginAttempts: 1, - }, - depth: 0, - req, - }) + const updatedUser = await payload.db.updateOne({ + id: user.id, + collection: collection.slug, + data: { + lockUntil: null, + loginAttempts: 1, + }, + req, + select: { + lockUntil: true, + loginAttempts: true, + }, + }) + updatedLockUntil = updatedUser.lockUntil + updatedLoginAttempts = updatedUser.loginAttempts + user.lockUntil = updatedLockUntil + } else { + const data: JsonObject = { + loginAttempts: { + $inc: 1, + }, } - return + const willReachMaxAttempts = + typeof user.loginAttempts === 'number' && user.loginAttempts + 1 >= maxLoginAttempts + // Lock the account if at max attempts and not already locked + if (willReachMaxAttempts) { + const lockUntil = new Date(currentTime + lockTime).toISOString() + data.lockUntil = lockUntil + } + + const updatedUser = await payload.db.updateOne({ + id: user.id, + collection: collection.slug, + data, + select: { + lockUntil: true, + loginAttempts: true, + }, + }) + + updatedLockUntil = updatedUser.lockUntil + updatedLoginAttempts = updatedUser.loginAttempts } - const data: JsonObject = { - loginAttempts: Number(doc.loginAttempts) + 1, + if (updatedLoginAttempts === null) { + throw new Error('Failed to update login attempts or lockUntil for user') } - // Lock the account if at max attempts and not already locked - if (typeof doc.loginAttempts === 'number' && doc.loginAttempts + 1 >= maxLoginAttempts) { - const lockUntil = new Date(Date.now() + lockTime).toISOString() - data.lockUntil = lockUntil - } + // Check updated latest lockUntil and loginAttempts in case there were parallel updates + const reachedMaxAttemptsForCurrentUser = + typeof updatedLoginAttempts === 'number' && updatedLoginAttempts - 1 >= maxLoginAttempts - await payload.update({ - id: doc.id, - collection: collection.slug, - data, - depth: 0, - req, - }) + const reachedMaxAttemptsForNextUser = + typeof updatedLoginAttempts === 'number' && updatedLoginAttempts >= maxLoginAttempts + + if (reachedMaxAttemptsForCurrentUser) { + user.lockUntil = updatedLockUntil + } + user.loginAttempts = updatedLoginAttempts - 1 // -1, as the updated increment is applied for the *next* login attempt, not the current one + + if ( + reachedMaxAttemptsForNextUser && + (!updatedLockUntil || !isUserLocked(new Date(updatedLockUntil))) + ) { + // If lockUntil reached max login attempts due to multiple parallel attempts but user was not locked yet, + const newLockUntil = new Date(currentTime + lockTime).toISOString() + + await payload.db.updateOne({ + id: user.id, + collection: collection.slug, + data: { + lockUntil: newLockUntil, + }, + returning: false, + }) + + if (reachedMaxAttemptsForCurrentUser) { + user.lockUntil = newLockUntil + } + + if (collection.auth.useSessions) { + // Remove all active sessions that have been created in a 20 second window. This protects + // against brute force attacks - example: 99 incorrect, 1 correct parallel login attempts. + // The correct login attempt will be finished first, as it's faster due to not having to perform + // an additional db update here. + // However, this request (the incorrect login attempt request) can kill the successful login attempt here. + + // Fetch user sessions separately (do not do this in the updateOne select in order to preserve the returning: true db call optimization) + const currentUser = await payload.db.findOne({ + collection: collection.slug, + select: { + sessions: true, + }, + where: { + id: { + equals: user.id, + }, + }, + }) + if (currentUser?.sessions?.length) { + // Does not hurt also removing expired sessions + currentUser.sessions = currentUser.sessions.filter((session) => { + const sessionCreatedAt = new Date(session.createdAt) + const twentySecondsAgo = new Date(currentTime - 20000) + + // Remove sessions created within the last 20 seconds + return sessionCreatedAt <= twentySecondsAgo + }) + + user.sessions = currentUser.sessions + + await payload.db.updateOne({ + id: user.id, + collection: collection.slug, + data: user, + returning: false, + }) + } + } + } } diff --git a/packages/payload/src/auth/strategies/local/resetLoginAttempts.ts b/packages/payload/src/auth/strategies/local/resetLoginAttempts.ts index b03e608f2f..8865612d0a 100644 --- a/packages/payload/src/auth/strategies/local/resetLoginAttempts.ts +++ b/packages/payload/src/auth/strategies/local/resetLoginAttempts.ts @@ -21,15 +21,14 @@ export const resetLoginAttempts = async ({ ) { return } - await payload.update({ + await payload.db.updateOne({ id: doc.id, collection: collection.slug, data: { lockUntil: null, loginAttempts: 0, }, - depth: 0, - overrideAccess: true, req, + returning: false, }) } diff --git a/test/auth/int.spec.ts b/test/auth/int.spec.ts index 29a7d92581..1ad8b84c5e 100644 --- a/test/auth/int.spec.ts +++ b/test/auth/int.spec.ts @@ -498,13 +498,21 @@ describe('Auth', () => { describe('Account Locking', () => { const userEmail = 'lock@me.com' - const tryLogin = async () => { - await restClient.POST(`/${slug}/login`, { - body: JSON.stringify({ - email: userEmail, - password: 'bad', - }), + const tryLogin = async (success?: boolean) => { + const res = await restClient.POST(`/${slug}/login`, { + body: JSON.stringify( + success + ? { + email: userEmail, + password, + } + : { + email: userEmail, + password: 'bad', + }, + ), }) + return await res.json() } beforeAll(async () => { @@ -530,10 +538,32 @@ describe('Auth', () => { }) }) + beforeEach(async () => { + await payload.db.updateOne({ + collection: slug, + data: { + lockUntil: null, + loginAttempts: 0, + }, + where: { + email: { + equals: userEmail, + }, + }, + }) + }) + + const lockedMessage = 'This user is locked due to having too many failed login attempts.' + const incorrectMessage = 'The email or password provided is incorrect.' + it('should lock the user after too many attempts', async () => { - await tryLogin() - await tryLogin() - await tryLogin() // Let it call multiple times, therefore the unlock condition has no bug. + const user1 = await tryLogin() + const user2 = await tryLogin() + const user3 = await tryLogin() // Let it call multiple times, therefore the unlock condition has no bug. + + expect(user1.errors[0].message).toBe(incorrectMessage) + expect(user2.errors[0].message).toBe(incorrectMessage) + expect(user3.errors[0].message).toBe(lockedMessage) const userResult = await payload.find({ collection: slug, @@ -546,10 +576,98 @@ describe('Auth', () => { }, }) - const { lockUntil, loginAttempts } = userResult.docs[0] + const { lockUntil, loginAttempts } = userResult.docs[0]! expect(loginAttempts).toBe(2) expect(lockUntil).toBeDefined() + + const successfulLogin = await tryLogin(true) + expect(successfulLogin.errors?.[0].message).toBe( + 'This user is locked due to having too many failed login attempts.', + ) + }) + + it('should lock the user after too many parallel attempts', async () => { + const tryLoginAttempts = 100 + const users = await Promise.allSettled( + Array.from({ length: tryLoginAttempts }, () => tryLogin()), + ) + + expect(users).toHaveLength(tryLoginAttempts) + + // Expect min. 8 locked message max. 2 incorrect messages. + const lockedMessages = users.filter( + (result) => + result.status === 'fulfilled' && result.value?.errors?.[0]?.message === lockedMessage, + ) + const incorrectMessages = users.filter( + (result) => + result.status === 'fulfilled' && + result.value?.errors?.[0]?.message === incorrectMessage, + ) + + const userResult = await payload.find({ + collection: slug, + limit: 1, + showHiddenFields: true, + where: { + email: { + equals: userEmail, + }, + }, + }) + + const { lockUntil, loginAttempts } = userResult.docs[0]! + + // loginAttempts does not have to be exactly the same amount of login attempts. If this ran sequentially, login attempts would stop + // incrementing after maxLoginAttempts is reached. Since this is run in parallel, it can increment more than maxLoginAttempts, but it is not + // expected to and can be less depending on the timing. + expect(loginAttempts).toBeGreaterThan(3) + expect(lockUntil).toBeDefined() + + expect(incorrectMessages.length).toBeLessThanOrEqual(2) + expect(lockedMessages.length).toBeGreaterThanOrEqual(tryLoginAttempts - 2) + + const successfulLogin = await tryLogin(true) + + expect(successfulLogin.errors?.[0].message).toBe( + 'This user is locked due to having too many failed login attempts.', + ) + }) + + it('ensure that login session expires if max login attempts is reached within narrow time-frame', async () => { + const tryLoginAttempts = 5 + + // If there are 100 parallel login attempts, 99 incorrect and 1 correct one, we do not want the correct one to be able to consistently be able + // to login successfully. + const user = await tryLogin(true) + const firstMeResponse = await restClient.GET(`/${slug}/me`, { + headers: { + Authorization: `JWT ${user.token}`, + }, + }) + + expect(firstMeResponse.status).toBe(200) + + const firstMeData = await firstMeResponse.json() + + expect(firstMeData.token).toBeDefined() + expect(firstMeData.user.email).toBeDefined() + + await Promise.allSettled(Array.from({ length: tryLoginAttempts }, () => tryLogin())) + + const secondMeResponse = await restClient.GET(`/${slug}/me`, { + headers: { + Authorization: `JWT ${user.token}`, + }, + }) + + expect(secondMeResponse.status).toBe(200) + + const secondMeData = await secondMeResponse.json() + + expect(secondMeData.user).toBeNull() + expect(secondMeData.token).not.toBeDefined() }) it('should unlock account once lockUntil period is over', async () => {