Skip to content

Commit 9615cb6

Browse files
committed
improvement(otp): centralize compare-and-consume in verifyOTP, tighten tests
Move the constant-time compare and consume-on-match into verifyOTP so both OTP callers share it; drop getOTP and the decodeOTPValue export now that nothing reads a code without reserving an attempt; remove the unreachable Lua branch; keep the lockout log free of the email address. Tests: assert the Redis TTL is preserved rather than reset, drop the non-null assertion, and use createMockRequest.
1 parent 5daef70 commit 9615cb6

6 files changed

Lines changed: 79 additions & 107 deletions

File tree

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

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,10 @@ import { chat, user, verification, workflow, workspace } from '@sim/db/schema'
33
import { readTestRedisUrl } from '@sim/db/testing/test-infrastructure'
44
import { deleteWorkspaceFixture, insertWorkspaceFixture } from '@sim/db/testing/workspace-fixtures'
55
import { createRouteContext } from '@sim/testing/helpers/http'
6+
import { createMockRequest } from '@sim/testing/mocks/request.mock'
67
import { generateId } from '@sim/utils/id'
78
import { eq, like } from 'drizzle-orm'
89
import Redis from 'ioredis'
9-
import { NextRequest } from 'next/server'
1010
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
1111

1212
const { storage, redisClient } = vi.hoisted(() => ({
@@ -35,10 +35,10 @@ const INVALID = 'Invalid verification code'
3535
const LOCKED = 'Too many failed attempts. Please request a new code.'
3636

3737
async function verify(otp: string) {
38-
const request = new NextRequest(`http://localhost:3000/api/chat/${identifier}/otp`, {
38+
const request = createMockRequest({
3939
method: 'PUT',
40-
headers: { 'content-type': 'application/json' },
41-
body: JSON.stringify({ email, otp }),
40+
url: `/api/chat/${identifier}/otp`,
41+
body: { email, otp },
4242
})
4343
const response = await PUT(request, createRouteContext({ identifier }))
4444
const body = (await response.json()) as { error?: string; message?: string }
@@ -99,8 +99,8 @@ afterAll(async () => {
9999
describe.each(['database', 'redis'] as const)('%s chat OTP verification', (backend) => {
100100
describe.runIf(backend === 'database' || Boolean(redisUrl))('real storage', () => {
101101
beforeAll(async () => {
102-
if (backend === 'redis' && !redisClient.current) {
103-
redisClient.current = new Redis(redisUrl!, { lazyConnect: true, maxRetriesPerRequest: 0 })
102+
if (backend === 'redis' && redisUrl && !redisClient.current) {
103+
redisClient.current = new Redis(redisUrl, { lazyConnect: true, maxRetriesPerRequest: 0 })
104104
await redisClient.current.connect()
105105
}
106106
})
@@ -132,20 +132,25 @@ describe.each(['database', 'redis'] as const)('%s chat OTP verification', (backe
132132
})
133133

134134
it('keeps the code expiry when a failed attempt is recorded', async () => {
135+
if (backend === 'redis') {
136+
const key = `otp:${email}:${chatId}`
137+
await redisClient.current?.expire(key, 60)
138+
expect(await verify('999999')).toMatchObject({ status: 400, error: INVALID })
139+
const ttl = await redisClient.current?.ttl(key)
140+
expect(ttl).toBeGreaterThan(55)
141+
expect(ttl).toBeLessThanOrEqual(60)
142+
return
143+
}
135144
const readExpiry = async () =>
136-
backend === 'redis'
137-
? redisClient.current?.ttl(`otp:${email}:${chatId}`)
138-
: (
139-
await db
140-
.select({ expiresAt: verification.expiresAt })
141-
.from(verification)
142-
.where(eq(verification.identifier, `chat-otp:${chatId}:${email}`))
143-
)[0]?.expiresAt.getTime()
145+
(
146+
await db
147+
.select({ expiresAt: verification.expiresAt })
148+
.from(verification)
149+
.where(eq(verification.identifier, `chat-otp:${chatId}:${email}`))
150+
)[0]?.expiresAt.getTime()
144151
const before = await readExpiry()
145152
expect(await verify('999999')).toMatchObject({ status: 400, error: INVALID })
146-
const after = await readExpiry()
147-
if (backend === 'redis') expect(after).toBeGreaterThan(0)
148-
else expect(after).toBe(before)
153+
expect(await readExpiry()).toBe(before)
149154
})
150155

151156
it.runIf(backend === 'database')('rejects an expired code', async () => {

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

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,6 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
2424

2525
const {
2626
mockRedisSet,
27-
mockRedisGet,
28-
mockRedisDel,
29-
mockRedisTtl,
3027
mockRedisEval,
3128
mockRedisClient,
3229
mockSetChatAuthCookie,
@@ -36,15 +33,9 @@ const {
3633
mockAfterResponse,
3734
} = vi.hoisted(() => {
3835
const mockRedisSet = vi.fn()
39-
const mockRedisGet = vi.fn()
40-
const mockRedisDel = vi.fn()
41-
const mockRedisTtl = vi.fn()
4236
const mockRedisEval = vi.fn()
4337
const mockRedisClient = {
4438
set: mockRedisSet,
45-
get: mockRedisGet,
46-
del: mockRedisDel,
47-
ttl: mockRedisTtl,
4839
eval: mockRedisEval,
4940
}
5041
const mockSetChatAuthCookie = vi.fn()
@@ -59,9 +50,6 @@ const {
5950

6051
return {
6152
mockRedisSet,
62-
mockRedisGet,
63-
mockRedisDel,
64-
mockRedisTtl,
6553
mockRedisEval,
6654
mockRedisClient,
6755
mockSetChatAuthCookie,
@@ -183,9 +171,6 @@ describe('Chat OTP API Route', () => {
183171

184172
mockGetRedisClient.mockReturnValue(mockRedisClient)
185173
mockRedisSet.mockResolvedValue('OK')
186-
mockRedisGet.mockResolvedValue(null)
187-
mockRedisDel.mockResolvedValue(1)
188-
mockRedisTtl.mockResolvedValue(600)
189174

190175
mockGetStorageMethod.mockReturnValue('redis')
191176

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

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { db } from '@sim/db'
22
import { chat } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
4-
import { safeCompare } from '@sim/security/compare'
54
import { normalizeEmail } from '@sim/utils/string'
65
import { and, eq, isNull } from 'drizzle-orm'
76
import type { NextRequest } from 'next/server'
@@ -11,14 +10,12 @@ import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
1110
import { RateLimiter } from '@/lib/core/rate-limiter'
1211
import { isEmailAllowed } from '@/lib/core/security/deployment'
1312
import {
14-
deleteOTP,
1513
generateOTP,
16-
MAX_OTP_ATTEMPTS,
1714
OTP_EMAIL_RATE_LIMIT,
1815
OTP_IP_RATE_LIMIT,
1916
OTP_RESOURCE_RATE_LIMIT,
20-
reserveOTPAttempt,
2117
storeOTP,
18+
verifyOTP,
2219
} from '@/lib/core/security/otp'
2320
import { afterResponse } from '@/lib/core/utils/after-response'
2421
import { generateRequestId, getClientIp } from '@/lib/core/utils/request'
@@ -195,24 +192,18 @@ export const PUT = withRouteHandler(
195192
return createCodedErrorResponse('Email not authorized', 403)
196193
}
197194

198-
const reserved = await reserveOTPAttempt('chat', deployment.id, email)
199-
if (!reserved) {
195+
const verification = await verifyOTP('chat', deployment.id, email, otp)
196+
if (verification === 'missing') {
200197
return createCodedErrorResponse('No verification code found, request a new one', 400)
201198
}
202-
203-
if (!safeCompare(reserved.otp, otp)) {
204-
if (reserved.attempts >= MAX_OTP_ATTEMPTS) {
205-
logger.warn(`OTP invalidated after max failed attempts for ${email}`)
206-
return createCodedErrorResponse(
207-
'Too many failed attempts. Please request a new code.',
208-
429
209-
)
210-
}
199+
if (verification === 'locked') {
200+
logger.warn('OTP invalidated after max failed attempts', { chatId: deployment.id })
201+
return createCodedErrorResponse('Too many failed attempts. Please request a new code.', 429)
202+
}
203+
if (verification === 'invalid') {
211204
return createCodedErrorResponse('Invalid verification code', 400)
212205
}
213206

214-
await deleteOTP('chat', deployment.id, email)
215-
216207
const response = createSuccessResponse({
217208
id: deployment.id,
218209
title: deployment.title,

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

Lines changed: 31 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { randomInt } from 'crypto'
22
import { db } from '@sim/db'
33
import { verification } from '@sim/db/schema'
4+
import { safeCompare } from '@sim/security/compare'
45
import { generateId } from '@sim/utils/id'
56
import { and, eq, gt, lt, sql } from 'drizzle-orm'
67
import { getRedisClient } from '@/lib/core/config/redis'
@@ -70,7 +71,7 @@ function encodeOTPValue(otp: string, attempts: number): string {
7071
return `${otp}:${attempts}`
7172
}
7273

73-
export function decodeOTPValue(value: string): { otp: string; attempts: number } {
74+
function decodeOTPValue(value: string): { otp: string; attempts: number } {
7475
const lastColon = value.lastIndexOf(':')
7576
if (lastColon === -1) return { otp: value, attempts: 0 }
7677
const attempts = Number.parseInt(value.slice(lastColon + 1), 10)
@@ -115,40 +116,9 @@ export async function storeOTP(
115116
})
116117
}
117118

118-
export async function getOTP(
119-
kind: DeploymentKind,
120-
deploymentId: string,
121-
email: string
122-
): Promise<string | null> {
123-
const keys = OTP_KEYS[kind]
124-
const storageMethod = getStorageMethod()
125-
126-
if (storageMethod === 'redis') {
127-
const redis = getRedisClient()
128-
if (!redis) throw new Error('Redis configured but client unavailable')
129-
return redis.get(keys.redisKey(email, deploymentId))
130-
}
131-
132-
const now = new Date()
133-
const [record] = await db
134-
.select({ value: verification.value })
135-
.from(verification)
136-
.where(
137-
and(
138-
eq(verification.identifier, keys.dbIdentifier(email, deploymentId)),
139-
gt(verification.expiresAt, now)
140-
)
141-
)
142-
.limit(1)
143-
144-
return record?.value ?? null
145-
}
146-
147119
/**
148120
* Lua script that reserves one verification attempt in Redis. Returns the
149-
* code with the reserved attempt number, or nil when no code exists or every
150-
* attempt is already spent. The reservation that spends the last attempt
151-
* deletes the key, so later requests find no code.
121+
* code with the reserved attempt number, or nil when no code exists.
152122
*/
153123
const RESERVE_ATTEMPT_SCRIPT = `
154124
local val = redis.call('GET', KEYS[1])
@@ -163,13 +133,8 @@ else
163133
attempts = 0
164134
end
165135
attempts = attempts + 1
166-
local max = tonumber(ARGV[1])
167-
if attempts > max then
168-
redis.call('DEL', KEYS[1])
169-
return nil
170-
end
171136
local newVal = otp .. ':' .. attempts
172-
if attempts == max then
137+
if attempts >= tonumber(ARGV[1]) then
173138
redis.call('DEL', KEYS[1])
174139
else
175140
local ttl = redis.call('TTL', KEYS[1])
@@ -184,12 +149,11 @@ return newVal
184149

185150
/**
186151
* Atomically spends one of the code's {@link MAX_OTP_ATTEMPTS} attempts and
187-
* returns the stored code for the caller to compare, or `null` when no live
188-
* code is left. Reserving before comparing bounds the guesses checked against
189-
* one code to the attempt limit however many requests run concurrently. The
190-
* reservation that spends the last attempt also deletes the code.
152+
* returns the stored code with its reserved attempt count, or `null` when no
153+
* live code is left. The reservation that spends the last attempt also
154+
* deletes the code.
191155
*/
192-
export async function reserveOTPAttempt(
156+
async function reserveOTPAttempt(
193157
kind: DeploymentKind,
194158
deploymentId: string,
195159
email: string
@@ -233,6 +197,29 @@ export async function reserveOTPAttempt(
233197
return decoded
234198
}
235199

200+
export type OTPVerification = 'valid' | 'invalid' | 'locked' | 'missing'
201+
202+
/**
203+
* Checks a submitted code and consumes it on a match. An attempt is reserved
204+
* before the comparison, so at most {@link MAX_OTP_ATTEMPTS} guesses are ever
205+
* compared against one code however many requests run concurrently.
206+
* `'locked'` means this guess spent the last attempt and the code is gone.
207+
*/
208+
export async function verifyOTP(
209+
kind: DeploymentKind,
210+
deploymentId: string,
211+
email: string,
212+
code: string
213+
): Promise<OTPVerification> {
214+
const reserved = await reserveOTPAttempt(kind, deploymentId, email)
215+
if (!reserved) return 'missing'
216+
if (safeCompare(reserved.otp, code)) {
217+
await deleteOTP(kind, deploymentId, email)
218+
return 'valid'
219+
}
220+
return reserved.attempts >= MAX_OTP_ATTEMPTS ? 'locked' : 'invalid'
221+
}
222+
236223
export async function deleteOTP(
237224
kind: DeploymentKind,
238225
deploymentId: string,

‎apps/sim/lib/projects/files/__integration__/content-sharing.integration.ts‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
publicShare,
1717
subscription,
1818
user,
19+
verification,
1920
workspace,
2021
workspaceFileSecretProvenance,
2122
workspaceFiles,
@@ -574,7 +575,14 @@ describe('Public file shares against PostgreSQL and private storage', () => {
574575
await (
575576
await requestPublicFileShareOtp({ token: share.token, email: 'outsider@shared.invalid' })
576577
).deliver()
577-
expect(await otpStore.getOTP('file', share.id, 'outsider@shared.invalid')).toBeNull()
578+
const storedCode = async (address: string) => {
579+
const [row] = await db
580+
.select({ value: verification.value })
581+
.from(verification)
582+
.where(eq(verification.identifier, `file-otp:${share.id}:${address}`))
583+
return row?.value.split(':')[0]
584+
}
585+
expect(await storedCode('outsider@shared.invalid')).toBeUndefined()
578586
const failure = { authorized: false, status: 400, error: 'Invalid verification code' }
579587
expect(
580588
await verifyPublicFileShareOtp({
@@ -587,9 +595,8 @@ describe('Public file shares against PostgreSQL and private storage', () => {
587595
failure
588596
)
589597
await (await requestPublicFileShareOtp({ token: share.token, email })).deliver()
590-
const stored = await otpStore.getOTP('file', share.id, email)
591-
if (!stored) throw new Error('Allowed email received no code')
592-
const { otp } = otpStore.decodeOTPValue(stored)
598+
const otp = await storedCode(email)
599+
if (!otp) throw new Error('Allowed email received no code')
593600
expect(
594601
await verifyPublicFileShareOtp({ token: share.token, email, otp: '000000' })
595602
).toMatchObject({ authorized: false, status: 400 })
@@ -606,9 +613,9 @@ describe('Public file shares against PostgreSQL and private storage', () => {
606613
status: 400,
607614
})
608615
await otpStore.storeOTP('file', share.id, email, '123456')
609-
const reserve = otpStore.reserveOTPAttempt
610-
const spy = vi.spyOn(otpStore, 'reserveOTPAttempt').mockImplementation(async (...args) => {
611-
const result = await reserve(...args)
616+
const verifyCode = otpStore.verifyOTP
617+
const spy = vi.spyOn(otpStore, 'verifyOTP').mockImplementation(async (...args) => {
618+
const result = await verifyCode(...args)
612619
await db
613620
.update(publicShare)
614621
.set({ allowedEmails: ['replacement@shared.invalid'] })
@@ -619,7 +626,7 @@ describe('Public file shares against PostgreSQL and private storage', () => {
619626
verifyPublicFileShareOtp({ token: share.token, email, otp: '123456' })
620627
).rejects.toMatchObject({ code: 'not_found' })
621628
spy.mockRestore()
622-
await otpStore.deleteOTP('file', share.id, email)
629+
expect(await storedCode(email)).toBeUndefined()
623630
}
624631
)
625632

‎apps/sim/lib/public-shares/application/credentials.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,14 @@
11
import { createLogger } from '@sim/logger'
2-
import { safeCompare } from '@sim/security/compare'
32
import { normalizeEmail } from '@sim/utils/string'
43
import { getOtpSubject, renderOTPEmail } from '@/components/emails'
54
import { RateLimiter, type TokenBucketConfig } from '@/lib/core/rate-limiter'
65
import { createDeploymentAuthCookie, isEmailAllowed } from '@/lib/core/security/deployment'
76
import {
8-
deleteOTP,
97
generateOTP,
108
OTP_EMAIL_RATE_LIMIT,
119
OTP_RESOURCE_RATE_LIMIT,
12-
reserveOTPAttempt,
1310
storeOTP,
11+
verifyOTP,
1412
} from '@/lib/core/security/otp'
1513
import { sendEmail } from '@/lib/messaging/email/mailer'
1614
import {
@@ -111,10 +109,9 @@ export async function verifyPublicFileShareOtp({
111109
isEmailAllowed(normalizedEmail, share.allowedEmails)
112110
)
113111
if (!allowed) return OTP_FAILURE
114-
const reserved = await reserveOTPAttempt('file', challenge.shareId, normalizedEmail)
112+
const verification = await verifyOTP('file', challenge.shareId, normalizedEmail, otp)
115113
await challenge.withCurrentPolicy(() => undefined)
116-
if (!reserved || !safeCompare(reserved.otp, otp)) return OTP_FAILURE
117-
await deleteOTP('file', challenge.shareId, normalizedEmail)
114+
if (verification !== 'valid') return OTP_FAILURE
118115
const resource = await challenge.withCurrentPolicy(({ share }) => share)
119116
const cookie = await createDeploymentAuthCookie({
120117
cookiePrefix: 'file',

0 commit comments

Comments
 (0)