tabshift/payloadcms
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

fix: use atomic operation for incrementing login attempts (#13204)

Browse Source 
---
- To see the specific tasks where the Asana app for GitHub is being
used, see below:
  - https://app.asana.com/0/0/1210561338171141
This commit is contained in:
Alessio Gravili
2025-07-28 16:08:10 -07:00
committed by GitHub
parent aff2ce1b9b
commit 4fde0f23ce
5 changed files with 299 additions and 51 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
packages/payload/src/auth/isUserLocked.ts
View File

@@ -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()
}

44
packages/payload/src/auth/operations/login.ts
View File

@@ -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 <TSlug extends CollectionSlug>(
where: whereConstraint,
})
let user = await payload.db.findOne<any>({
let user = (await payload.db.findOne<TypedUser>({
collection: collectionConfig.slug,
req,
where: whereConstraint,
})
})) as TypedUser
checkLoginPermission({
loggingInWithUsername: Boolean(canLoginWithUsername && sanitizedUsername),
@@ -230,9 +235,16 @@ export const loginOperation = async <TSlug extends CollectionSlug>(
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 <TSlug extends CollectionSlug>(
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<TypedUser>({
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<typeof getFieldsToSign>[0] = {
collectionConfig,
email: sanitizedEmail!,

159
packages/payload/src/auth/strategies/local/incrementLoginAttempts.ts
View File

@@ -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<string, unknown> & 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<void> => {
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<TypedUser>({
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,
})
}
}
}
}

5
packages/payload/src/auth/strategies/local/resetLoginAttempts.ts
View File

@@ -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,
})
}

138
test/auth/int.spec.ts
View File

@@ -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 () => {