Skip to content

Commit 0d22e54

Browse files
committed
fix(otp): make a correct code single-use and never compare a code without expiry
A correct verification deleted the code unconditionally after an atomic reservation, so concurrent correct submissions each got a session and a slow success could delete a newer code issued meanwhile. Success now requires a conditional consume (delete only while the stored code is still the one compared), so exactly one request wins and newer codes survive. Reservations no longer delete; a code with every attempt spent just stops reserving, and the final wrong guess consumes it. The Redis reservation checks the remaining lifetime before anything else, so a key left without an expiry is never compared.
1 parent 887bece commit 0d22e54

2 files changed

Lines changed: 75 additions & 44 deletions

File tree

‎apps/sim/app/api/chat/[identifier]/otp/route.integration.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,22 @@ describe.each(['database', 'redis'] as const)('%s chat OTP verification', (backe
150150
expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE, cookie: null })
151151
})
152152

153+
it('lets only one of several concurrent correct submissions succeed', async () => {
154+
const results = await Promise.all(Array.from({ length: 10 }, () => verify(CODE)))
155+
156+
const accepted = results.filter((result) => result.status === 200)
157+
expect(accepted).toHaveLength(1)
158+
expect(accepted[0].cookie).toContain(`chat_auth_${chatId}=`)
159+
expect(results.filter((result) => result.cookie !== null)).toHaveLength(1)
160+
})
161+
162+
it.runIf(backend === 'redis')('treats a code with no expiry as expired', async () => {
163+
const key = `otp:${email}:${chatId}`
164+
await redisClient.current?.set(key, `${CODE}:${MAX_OTP_ATTEMPTS - 1}`)
165+
expect(await verify(CODE)).toMatchObject({ status: 400, error: NO_CODE, cookie: null })
166+
expect(await redisClient.current?.exists(key)).toBe(0)
167+
})
168+
153169
it('accepts the correct code on the last remaining attempt and mints the auth cookie', async () => {
154170
for (const guess of wrongGuesses(MAX_OTP_ATTEMPTS - 1)) {
155171
expect(await verify(guess)).toMatchObject({ status: 400, error: INVALID })

‎apps/sim/lib/core/security/otp.ts‎

Lines changed: 59 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -118,42 +118,50 @@ export async function storeOTP(
118118

119119
/**
120120
* Lua script that reserves one verification attempt in Redis. Returns the
121-
* code with the reserved attempt number, or nil when no code exists. Rewrites
122-
* keep the remaining lifetime to the millisecond; a code without one is
123-
* treated as expired so it can never become permanent.
121+
* code with the reserved attempt number, or nil when no code exists or its
122+
* attempts are spent. Rewrites keep the remaining lifetime to the
123+
* millisecond; a code without one is treated as expired so it can never
124+
* become permanent.
124125
*/
125126
const RESERVE_ATTEMPT_SCRIPT = `
126127
local val = redis.call('GET', KEYS[1])
127128
if not val then return nil end
128-
local colon = val:find(':([^:]*)$')
129-
local otp, attempts
130-
if colon then
131-
otp = val:sub(1, colon - 1)
132-
attempts = tonumber(val:sub(colon + 1)) or 0
133-
else
134-
otp = val
135-
attempts = 0
136-
end
137-
attempts = attempts + 1
138-
local newVal = otp .. ':' .. attempts
139-
if attempts >= tonumber(ARGV[1]) then
140-
redis.call('DEL', KEYS[1])
141-
return newVal
142-
end
129+
local otp, attempts = val:match('^(.*):(%d+)$')
130+
if not otp then return nil end
143131
local ttl = redis.call('PTTL', KEYS[1])
144132
if ttl <= 0 then
145133
redis.call('DEL', KEYS[1])
146134
return nil
147135
end
136+
attempts = tonumber(attempts)
137+
if attempts >= tonumber(ARGV[1]) then return nil end
138+
local newVal = otp .. ':' .. (attempts + 1)
148139
redis.call('SET', KEYS[1], newVal, 'PX', ttl)
149140
return newVal
150141
`
151142

143+
/**
144+
* Lua script that deletes the key only while it still holds the code in
145+
* ARGV[1], returning 1 when this call removed it.
146+
*/
147+
const CONSUME_SCRIPT = `
148+
local val = redis.call('GET', KEYS[1])
149+
if not val or val:match('^(.*):%d+$') ~= ARGV[1] then return 0 end
150+
return redis.call('DEL', KEYS[1])
151+
`
152+
153+
/**
154+
* The code and attempt count of a stored `code:attempts` value. Pattern
155+
* matching rather than split + cast: other `verification` rows sharing the
156+
* table must never fail a cast.
157+
*/
158+
const STORED_CODE = sql`substring(${verification.value} from '^(.*):[0-9]+$')`
159+
const STORED_ATTEMPTS = sql`substring(${verification.value} from ':([0-9]+)$')::int`
160+
152161
/**
153162
* Atomically spends one of the code's {@link MAX_OTP_ATTEMPTS} attempts and
154163
* returns the stored code with its reserved attempt count, or `null` when no
155-
* live code is left. The reservation that spends the last attempt also
156-
* deletes the code.
164+
* live code is left or all its attempts are spent.
157165
*/
158166
async function reserveOTPAttempt(
159167
kind: DeploymentKind,
@@ -171,41 +179,33 @@ async function reserveOTPAttempt(
171179
return typeof result === 'string' ? decodeOTPValue(result) : null
172180
}
173181

174-
const identifier = keys.dbIdentifier(email, deploymentId)
175-
// Pattern match, not split + cast: other `verification` rows must never fail a cast.
176-
const attemptsSql = sql`substring(${verification.value} from ':([0-9]+)$')::int`
177182
const [reserved] = await db
178183
.update(verification)
179184
.set({
180-
value: sql`substring(${verification.value} from '^(.*):[0-9]+$') || ':' || (${attemptsSql} + 1)`,
185+
value: sql`${STORED_CODE} || ':' || (${STORED_ATTEMPTS} + 1)`,
181186
updatedAt: new Date(),
182187
})
183188
.where(
184189
and(
185-
eq(verification.identifier, identifier),
190+
eq(verification.identifier, keys.dbIdentifier(email, deploymentId)),
186191
gt(verification.expiresAt, new Date()),
187-
lt(attemptsSql, MAX_OTP_ATTEMPTS)
192+
lt(STORED_ATTEMPTS, MAX_OTP_ATTEMPTS)
188193
)
189194
)
190195
.returning({ value: verification.value })
191196

192-
if (!reserved) return null
193-
const decoded = decodeOTPValue(reserved.value)
194-
if (decoded.attempts >= MAX_OTP_ATTEMPTS) {
195-
await db
196-
.delete(verification)
197-
.where(and(eq(verification.identifier, identifier), eq(verification.value, reserved.value)))
198-
}
199-
return decoded
197+
return reserved ? decodeOTPValue(reserved.value) : null
200198
}
201199

202200
export type OTPVerification = 'valid' | 'invalid' | 'locked' | 'missing'
203201

204202
/**
205203
* Checks a submitted code and consumes it on a match. An attempt is reserved
206204
* before the comparison, so at most {@link MAX_OTP_ATTEMPTS} guesses are ever
207-
* compared against one code however many requests run concurrently.
208-
* `'locked'` means this guess spent the last attempt and the code is gone.
205+
* compared against one code however many requests run concurrently, and only
206+
* one correct submission succeeds. `'locked'` means this guess spent the last
207+
* attempt and the code is gone; `'missing'` covers a code that is gone or was
208+
* consumed by a concurrent request.
209209
*/
210210
export async function verifyOTP(
211211
kind: DeploymentKind,
@@ -216,24 +216,39 @@ export async function verifyOTP(
216216
const reserved = await reserveOTPAttempt(kind, deploymentId, email)
217217
if (!reserved) return 'missing'
218218
if (safeCompare(reserved.otp, code)) {
219-
await deleteOTP(kind, deploymentId, email)
220-
return 'valid'
219+
return (await consumeOTP(kind, deploymentId, email, reserved.otp)) ? 'valid' : 'missing'
221220
}
222-
return reserved.attempts >= MAX_OTP_ATTEMPTS ? 'locked' : 'invalid'
221+
if (reserved.attempts < MAX_OTP_ATTEMPTS) return 'invalid'
222+
await consumeOTP(kind, deploymentId, email, reserved.otp)
223+
return 'locked'
223224
}
224225

225-
async function deleteOTP(kind: DeploymentKind, deploymentId: string, email: string): Promise<void> {
226+
/**
227+
* Deletes the stored code only if it is still `otp`, so concurrent correct
228+
* submissions get one success between them and a newer code issued meanwhile
229+
* survives. Returns whether this call removed it.
230+
*/
231+
async function consumeOTP(
232+
kind: DeploymentKind,
233+
deploymentId: string,
234+
email: string,
235+
otp: string
236+
): Promise<boolean> {
226237
const keys = OTP_KEYS[kind]
227238
const storageMethod = getStorageMethod()
228239

229240
if (storageMethod === 'redis') {
230241
const redis = getRedisClient()
231242
if (!redis) throw new Error('Redis configured but client unavailable')
232-
await redis.del(keys.redisKey(email, deploymentId))
233-
return
243+
const key = keys.redisKey(email, deploymentId)
244+
return (await redis.eval(CONSUME_SCRIPT, 1, key, otp)) === 1
234245
}
235246

236-
await db
247+
const consumed = await db
237248
.delete(verification)
238-
.where(eq(verification.identifier, keys.dbIdentifier(email, deploymentId)))
249+
.where(
250+
and(eq(verification.identifier, keys.dbIdentifier(email, deploymentId)), eq(STORED_CODE, otp))
251+
)
252+
.returning({ id: verification.id })
253+
return consumed.length > 0
239254
}

0 commit comments

Comments
 (0)