Skip to content

Commit f9c22fd

Browse files
authored
fix(auth): revoke stale email share access (#7314)
1 parent cb4a84d commit f9c22fd

14 files changed

Lines changed: 502 additions & 213 deletions

File tree

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

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ const {
2929
mockSendEmail,
3030
mockRenderOTPEmail,
3131
mockSetChatAuthCookie,
32+
mockIsEmailAllowed,
3233
mockGetStorageMethod,
3334
mockZodParse,
3435
mockAfterResponse,
@@ -48,6 +49,11 @@ const {
4849
const mockSendEmail = vi.fn()
4950
const mockRenderOTPEmail = vi.fn()
5051
const mockSetChatAuthCookie = vi.fn()
52+
const mockIsEmailAllowed = vi.fn((email: string, allowedEmails: string[]) => {
53+
if (allowedEmails.includes(email)) return true
54+
const domain = email.slice(email.indexOf('@') + 1)
55+
return allowedEmails.includes(`@${domain}`)
56+
})
5157
const mockGetStorageMethod = vi.fn()
5258
const mockZodParse = vi.fn()
5359
const mockAfterResponse = vi.fn()
@@ -62,6 +68,7 @@ const {
6268
mockSendEmail,
6369
mockRenderOTPEmail,
6470
mockSetChatAuthCookie,
71+
mockIsEmailAllowed,
6572
mockGetStorageMethod,
6673
mockZodParse,
6774
mockAfterResponse,
@@ -101,15 +108,7 @@ vi.mock('@/components/emails', () => ({
101108
}))
102109

103110
vi.mock('@/lib/core/security/deployment', () => ({
104-
isEmailAllowed: (email: string, allowedEmails: string[]) => {
105-
if (allowedEmails.includes(email)) return true
106-
const atIndex = email.indexOf('@')
107-
if (atIndex > 0) {
108-
const domain = email.substring(atIndex + 1)
109-
if (domain && allowedEmails.some((allowed: string) => allowed === `@${domain}`)) return true
110-
}
111-
return false
112-
},
111+
isEmailAllowed: mockIsEmailAllowed,
113112
}))
114113

115114
vi.mock('@/app/api/chat/utils', () => ({
@@ -173,7 +172,7 @@ describe('Chat OTP API Route', () => {
173172

174173
/** Queues the chat-deployment row the route reads before touching OTP storage. */
175174
const queueDeployment = (row: Record<string, unknown>) => {
176-
queueTableRows(schemaMock.chat, [row])
175+
queueTableRows(schemaMock.chat, [{ allowedEmails: [mockEmail], ...row }])
177176
}
178177

179178
const emailDeployment = {
@@ -483,6 +482,15 @@ describe('Chat OTP API Route', () => {
483482

484483
expect(mockRedisGet).toHaveBeenCalledWith(`otp:${mockEmail}:${mockChatId}`)
485484
expect(mockRedisDel).toHaveBeenCalledWith(`otp:${mockEmail}:${mockChatId}`)
485+
expect(mockSetChatAuthCookie).toHaveBeenCalledWith(
486+
expect.anything(),
487+
expect.objectContaining({
488+
id: mockChatId,
489+
authType: 'email',
490+
allowedEmails: [mockEmail],
491+
}),
492+
mockEmail
493+
)
486494
expect(dbChainMockFns.select).toHaveBeenCalledTimes(1)
487495
})
488496
})
@@ -514,6 +522,22 @@ describe('Chat OTP API Route', () => {
514522
expect(mockRedisGet).not.toHaveBeenCalled()
515523
expect(mockSetChatAuthCookie).not.toHaveBeenCalled()
516524
})
525+
526+
it('rejects verification when the email is no longer allowed', async () => {
527+
mockIsEmailAllowed.mockReturnValueOnce(false)
528+
queueDeployment({ id: mockChatId, authType: 'email' })
529+
530+
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
531+
method: 'PUT',
532+
body: JSON.stringify({ email: mockEmail, otp: mockOTP }),
533+
})
534+
535+
await PUT(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
536+
537+
expect(mockCreateErrorResponse).toHaveBeenCalledWith('Email not authorized', 403)
538+
expect(mockRedisGet).not.toHaveBeenCalled()
539+
expect(mockSetChatAuthCookie).not.toHaveBeenCalled()
540+
})
517541
})
518542

519543
describe('PUT - Verify OTP (Database path)', () => {

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { db } from '@sim/db'
22
import { chat } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
4+
import { normalizeEmail } from '@sim/utils/string'
45
import { and, eq, isNull } from 'drizzle-orm'
56
import type { NextRequest } from 'next/server'
67
import { getOtpSubject, renderOTPEmail } from '@/components/emails'
@@ -103,7 +104,7 @@ export const POST = withRouteHandler(
103104
createErrorResponse(getValidationErrorMessage(error, 'Invalid request'), 400),
104105
})
105106
if (!parsed.success) return parsed.response
106-
const { email } = parsed.data.body
107+
const email = normalizeEmail(parsed.data.body.email)
107108

108109
const deploymentResult = await db
109110
.select({
@@ -157,7 +158,8 @@ export const PUT = withRouteHandler(
157158
createErrorResponse(getValidationErrorMessage(error, 'Invalid request'), 400),
158159
})
159160
if (!parsed.success) return parsed.response
160-
const { email, otp } = parsed.data.body
161+
const { otp } = parsed.data.body
162+
const email = normalizeEmail(parsed.data.body.email)
161163

162164
const deploymentResult = await db
163165
.select({
@@ -167,6 +169,7 @@ export const PUT = withRouteHandler(
167169
customizations: chat.customizations,
168170
authType: chat.authType,
169171
password: chat.password,
172+
allowedEmails: chat.allowedEmails,
170173
outputConfigs: chat.outputConfigs,
171174
includeThinking: chat.includeThinking,
172175
includeToolCalls: chat.includeToolCalls,
@@ -187,6 +190,9 @@ export const PUT = withRouteHandler(
187190
if (deployment.authType !== 'email') {
188191
return createErrorResponse('This chat does not use email authentication', 400)
189192
}
193+
if (!isEmailAllowed(email, deployment.allowedEmails)) {
194+
return createErrorResponse('Email not authorized', 403)
195+
}
190196

191197
const storedValue = await getOTP('chat', deployment.id, email)
192198
if (!storedValue) {
@@ -222,7 +228,7 @@ export const PUT = withRouteHandler(
222228
includeThinking: deployment.includeThinking ?? false,
223229
includeToolCalls: deployment.includeToolCalls ?? false,
224230
})
225-
setChatAuthCookie(response, deployment.id, deployment.authType, deployment.password)
231+
setChatAuthCookie(response, deployment, email)
226232

227233
return response
228234
} catch (error) {

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

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -65,13 +65,11 @@ const createMockStream = () => {
6565
})
6666
}
6767

68-
const { mockValidateChatAuth, mockSetChatAuthCookie, mockValidateAuthToken, mockProcessChatFiles } =
69-
vi.hoisted(() => ({
70-
mockValidateChatAuth: vi.fn().mockResolvedValue({ authorized: true }),
71-
mockSetChatAuthCookie: vi.fn(),
72-
mockValidateAuthToken: vi.fn().mockReturnValue(false),
73-
mockProcessChatFiles: vi.fn(),
74-
}))
68+
const { mockValidateChatAuth, mockSetChatAuthCookie, mockProcessChatFiles } = vi.hoisted(() => ({
69+
mockValidateChatAuth: vi.fn().mockResolvedValue({ authorized: true }),
70+
mockSetChatAuthCookie: vi.fn(),
71+
mockProcessChatFiles: vi.fn(),
72+
}))
7573

7674
const mockCreateErrorResponse = workflowsApiUtilsMockFns.mockCreateErrorResponse
7775
const mockCreateSuccessResponse = workflowsApiUtilsMockFns.mockCreateSuccessResponse
@@ -82,12 +80,6 @@ vi.mock('@sim/db', () => ({
8280
workflow: {},
8381
}))
8482

85-
vi.mock('@/lib/core/security/deployment', () => ({
86-
validateAuthToken: mockValidateAuthToken,
87-
setDeploymentAuthCookie: vi.fn(),
88-
isEmailAllowed: vi.fn().mockReturnValue(false),
89-
}))
90-
9183
vi.mock('@/app/api/chat/utils', () => ({
9284
validateChatAuth: mockValidateChatAuth,
9385
setChatAuthCookie: mockSetChatAuthCookie,
@@ -190,7 +182,6 @@ describe('Chat Identifier API Route', () => {
190182
})
191183

192184
mockValidateChatAuth.mockResolvedValue({ authorized: true })
193-
mockValidateAuthToken.mockReturnValue(false)
194185
mockProcessChatFiles.mockResolvedValue([])
195186
mockCreateErrorResponse.mockImplementation((message: string, status: number, code?: string) => {
196187
return new Response(
@@ -316,6 +307,18 @@ describe('Chat Identifier API Route', () => {
316307

317308
describe('POST endpoint', () => {
318309
it('should return chat config on successful authentication', async () => {
310+
const passwordDeployment = {
311+
...mockChatResult[0],
312+
authType: 'password',
313+
password: 'encrypted-password',
314+
}
315+
dbChainMockFns.select.mockImplementation(() => ({
316+
from: vi.fn().mockReturnValue({
317+
where: vi.fn().mockReturnValue({
318+
limit: vi.fn().mockReturnValue([passwordDeployment]),
319+
}),
320+
}),
321+
}))
319322
const req = createMockNextRequest('POST', { password: 'test-password' })
320323
const params = Promise.resolve({ identifier: 'password-protected-chat' })
321324

@@ -329,7 +332,7 @@ describe('Chat Identifier API Route', () => {
329332
expect(data).toHaveProperty('customizations')
330333
expect(data.customizations).toHaveProperty('welcomeMessage', 'Welcome to the test chat')
331334

332-
expect(mockSetChatAuthCookie).toHaveBeenCalled()
335+
expect(mockSetChatAuthCookie).toHaveBeenCalledWith(expect.anything(), passwordDeployment)
333336
})
334337

335338
it('should return 400 for requests without input', async () => {

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

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import { parseRequest } from '@/lib/api/server'
99
import { releaseExecutionSlot } from '@/lib/billing/calculations/usage-reservation'
1010
import { admissionRejectedResponse, tryAdmit } from '@/lib/core/admission/gate'
1111
import { env } from '@/lib/core/config/env'
12-
import { validateAuthToken } from '@/lib/core/security/deployment'
1312
import { generateRequestId } from '@/lib/core/utils/request'
1413
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1514
import { preprocessExecution } from '@/lib/execution/preprocessing'
@@ -158,8 +157,8 @@ export const POST = withRouteHandler(
158157
if ((password || email) && !input) {
159158
const response = createSuccessResponse(toChatConfigResponse(deployment))
160159

161-
if (deployment.authType !== 'sso') {
162-
setChatAuthCookie(response, deployment.id, deployment.authType, deployment.password)
160+
if (deployment.authType === 'password') {
161+
setChatAuthCookie(response, deployment)
163162
}
164163

165164
return response
@@ -394,18 +393,6 @@ export const GET = withRouteHandler(
394393
return createErrorResponse('This chat is currently unavailable', 403)
395394
}
396395

397-
const cookieName = `chat_auth_${deployment.id}`
398-
const authCookie = request.cookies.get(cookieName)
399-
400-
if (
401-
deployment.authType !== 'public' &&
402-
deployment.authType !== 'sso' &&
403-
authCookie &&
404-
validateAuthToken(authCookie.value, deployment.id, deployment.authType, deployment.password)
405-
) {
406-
return createSuccessResponse(toChatConfigResponse(deployment))
407-
}
408-
409396
const authResult = await validateChatAuth(requestId, deployment, request)
410397
if (!authResult.authorized) {
411398
logger.info(

apps/sim/app/api/chat/utils.test.ts

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -100,12 +100,10 @@ describe('Chat API Utils', () => {
100100
} as any
101101

102102
const result = await validateChatAuth('request-id', deployment, mockRequest)
103-
expect(mockValidateAuthToken).toHaveBeenCalledWith(
104-
'valid-token',
105-
'chat-id',
106-
'password',
107-
'encrypted-password'
108-
)
103+
expect(mockValidateAuthToken).toHaveBeenCalledWith({
104+
token: 'valid-token',
105+
resource: deployment,
106+
})
109107
expect(result.authorized).toBe(true)
110108
})
111109

@@ -136,15 +134,19 @@ describe('Chat API Utils', () => {
136134
cookies: { set: vi.fn() },
137135
} as unknown as NextResponse
138136

139-
setChatAuthCookie(mockResponse, 'test-chat-id', 'password')
137+
const deployment = {
138+
id: 'test-chat-id',
139+
authType: 'password',
140+
password: 'encrypted-password',
141+
}
142+
setChatAuthCookie(mockResponse, deployment)
140143

141-
expect(mockSetDeploymentAuthCookie).toHaveBeenCalledWith(
142-
mockResponse,
143-
'chat',
144-
'test-chat-id',
145-
'password',
146-
undefined
147-
)
144+
expect(mockSetDeploymentAuthCookie).toHaveBeenCalledWith({
145+
response: mockResponse,
146+
cookiePrefix: 'chat',
147+
resource: deployment,
148+
verifiedEmail: undefined,
149+
})
148150
})
149151
})
150152

apps/sim/app/api/chat/utils.ts

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,27 @@ import { chat, workflow } from '@sim/db/schema'
33
import { authorizeWorkflowByWorkspacePermission } from '@sim/platform-authz/workflow'
44
import { and, eq, isNull } from 'drizzle-orm'
55
import type { NextRequest, NextResponse } from 'next/server'
6-
import { setDeploymentAuthCookie } from '@/lib/core/security/deployment'
76
import {
7+
type DeploymentAuthResource,
8+
setDeploymentAuthCookie,
9+
} from '@/lib/core/security/deployment'
10+
import {
11+
type DeploymentAuthBody,
812
type DeploymentAuthResult,
913
validateDeploymentAuth,
1014
} from '@/lib/core/security/deployment-auth'
1115

1216
export function setChatAuthCookie(
1317
response: NextResponse,
14-
chatId: string,
15-
type: string,
16-
encryptedPassword?: string | null
18+
deployment: DeploymentAuthResource,
19+
verifiedEmail?: string
1720
): void {
18-
setDeploymentAuthCookie(response, 'chat', chatId, type, encryptedPassword)
21+
setDeploymentAuthCookie({
22+
response,
23+
cookiePrefix: 'chat',
24+
resource: deployment,
25+
verifiedEmail,
26+
})
1927
}
2028

2129
/**
@@ -85,9 +93,9 @@ export async function checkChatAccess(
8593
*/
8694
export async function validateChatAuth(
8795
requestId: string,
88-
deployment: any,
96+
deployment: DeploymentAuthResource,
8997
request: NextRequest,
90-
parsedBody?: any
98+
parsedBody?: DeploymentAuthBody
9199
): Promise<DeploymentAuthResult> {
92100
return validateDeploymentAuth(requestId, deployment, request, parsedBody, 'chat')
93101
}

apps/sim/app/api/files/public/[token]/otp/route.test.ts

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,7 @@ describe('PUT /api/files/public/[token]/otp', () => {
223223
beforeEach(() => {
224224
vi.clearAllMocks()
225225
mockResolveActiveShareByToken.mockResolvedValue(emailShare)
226+
mockIsEmailAllowed.mockReturnValue(true)
226227
mockGetOTP.mockResolvedValue('123456:0')
227228
mockDecodeOTPValue.mockReturnValue({ otp: '123456', attempts: 0 })
228229
})
@@ -232,13 +233,23 @@ describe('PUT /api/files/public/[token]/otp', () => {
232233
expect(res.status).toBe(200)
233234
expect(await res.json()).toEqual({ authType: 'email' })
234235
expect(mockDeleteOTP).toHaveBeenCalledWith('file', 'sh_1', 'user@acme.com')
235-
expect(mockSetDeploymentAuthCookie).toHaveBeenCalledWith(
236-
expect.anything(),
237-
'file',
238-
'sh_1',
239-
'email',
240-
null
241-
)
236+
expect(mockSetDeploymentAuthCookie).toHaveBeenCalledWith({
237+
response: expect.anything(),
238+
cookiePrefix: 'file',
239+
resource: emailShare.share,
240+
verifiedEmail: 'user@acme.com',
241+
})
242+
})
243+
244+
it('rejects a valid code when the email is no longer allowed', async () => {
245+
mockIsEmailAllowed.mockReturnValueOnce(false)
246+
247+
const res = await PUT(put('user@acme.com', '123456'), params())
248+
249+
expect(res.status).toBe(403)
250+
expect(mockGetOTP).not.toHaveBeenCalled()
251+
expect(mockDeleteOTP).not.toHaveBeenCalled()
252+
expect(mockSetDeploymentAuthCookie).not.toHaveBeenCalled()
242253
})
243254

244255
it('rejects a wrong code with 400 and increments attempts', async () => {

0 commit comments

Comments
 (0)