Skip to content

Commit 07f9190

Browse files
waleedlatif1claude
andauthored
fix(chat): bound deployed-chat callers and stop leaking chat gate config (#7525)
* fix(chat): bound deployed-chat callers and stop leaking chat gate config Two authorization/throttling defects on chat deployments. **Denial of wallet on POST /api/chat/[identifier].** A deployed chat resolves its execution principal from the workflow's workspace, so the plan rate bucket, the usage/credit check and the concurrency reservation all belong to the owner while the request belongs to whoever found the link. Nothing bounded the caller, and an abort refunds none of it. Both the per-IP and the per-deployment bucket now run after auth and before `preprocessExecution`, on every execution regardless of `authType` — an email or SSO visitor is still not the payer. `GET /api/chat/validate` answered for any anonymous caller, so `available:false` inventoried live deployments; it now needs a session and a per-user bucket. **Chat gate config exposed at workflow `read` on GET /api/workflows/[id]/chat/ status.** The route reimplemented the admin-gated detail projection inline, serving the `allowedEmails` allow-list, `hasPassword` and the customization blob to any workspace viewer, and asserting no `deploy.chat` capability. It is now an adapter over `chat_deployments.list` — the same operation `GET /api/v2/chat- deployments` binds — returning only the deployment's id and identifier, which is all the editor reads before fetching the detail from `/api/chat/manage/{id}`. The two buckets are the existing `enforceIpRateLimitWithIndependentBackstop` plus a new `enforceResourceRateLimit` beside its siblings in `route-helpers`. The IP bucket is consulted first and returns on refusal, so one flooding IP cannot drain the deployment's budget and 429 the real audience with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWh9WFMYNZ6uTFQFUzF8Bj * fix(chat): drop the env knobs and put the ceiling under the plan bucket Two corrections to the execution throttle. The per-deployment ceiling was 300/min, at or above the workspace `sync` counter it debits on every plan but enterprise — 50 free, 150 pro, 300 team. A flood therefore drained that shared counter, which the owner's API, webhook and scheduled runs draw from too, before the ceiling ever refused: the availability half of the report went unmitigated on exactly the plans most workspaces are on. It is now 60/min sustained, under even the cheapest paid plan, with a test that pins it there against `RATE_LIMITS`. The per-IP bucket drops to 30/min so one host cannot take a deployment's whole allowance, and both gain the 2x burst allowance the plan buckets already use. Both limits go back to plain constants. Every sibling deployment throttle — password, OTP, SSO, on chat and on public file shares — is a hardcoded `TokenBucketConfig`, so the two env vars were the only configurable ones of their kind and bought speculative tuning for a control with sane defaults. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWh9WFMYNZ6uTFQFUzF8Bj * fix(chat): derive the chat ceiling from the plan table it must stay under The 60/min ceiling still sat above the free plan's 50/min sync rate, so on free the shared counter — the one the owner's API, webhook and scheduled runs also draw from — still emptied before the ceiling refused. Every plan rate is also operator-overridable through `RATE_LIMIT_*_SYNC`, which no hardcoded number can track. It is now derived: 80% of the smallest configured plan sync rate, which is 40/min with the defaults and stays under every plan by construction. The per-IP bucket follows at half that. Tests assert the invariant against each plan in `RATE_LIMITS`, on burst as well as sustained rate, rather than pinning numbers that would need editing the next time a plan default moves. This floor is shared by all plans, so enterprise is held to the same 40/min as free. Sizing the slice to the payer's own plan needs the subscription, which `preprocessExecution` resolves just after this runs — that is the follow-up, and the same hook bounds the generic-webhook surface that is still unbounded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWh9WFMYNZ6uTFQFUzF8Bj * docs(chat): note the one plan rate where the derived ceiling lands equal Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWh9WFMYNZ6uTFQFUzF8Bj * refactor(rate-limit): scope the per-IP bucket by resource id, not by bucket name The chat call interpolated the deployment id into `bucketName`, which produces a correct key but puts a per-deployment value into the field both log lines emit as `bucket` — high cardinality on a label meant to name a bucket family, and asymmetric with the `enforceResourceRateLimit` call beside it that takes the id as its own argument. `enforceIpRateLimitWithIndependentBackstop` now takes an optional `resourceId`, so the pair reads the same way and `resourceId` is logged as its own field. The unscoped key shape is unchanged for the existing callers, with a test pinning both shapes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWh9WFMYNZ6uTFQFUzF8Bj * test(rate-limit): drop needless any casts on the mock request createMockRequest already returns NextRequest, so the casts weakened the helper's input contract in the new tests for nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QWh9WFMYNZ6uTFQFUzF8Bj --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent c9945ef commit 07f9190

13 files changed

Lines changed: 655 additions & 165 deletions

File tree

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

Lines changed: 119 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
workflowsApiUtilsMock,
1515
workflowsApiUtilsMockFns,
1616
} from '@sim/testing'
17+
import { NextResponse } from 'next/server'
1718
import { beforeEach, describe, expect, it, vi } from 'vitest'
1819

1920
/**
@@ -65,10 +66,18 @@ const createMockStream = () => {
6566
})
6667
}
6768

68-
const { mockValidateChatAuth, mockSetChatAuthCookie, mockProcessChatFiles } = vi.hoisted(() => ({
69+
const {
70+
mockValidateChatAuth,
71+
mockSetChatAuthCookie,
72+
mockProcessChatFiles,
73+
mockEnforceIpRateLimit,
74+
mockEnforceResourceRateLimit,
75+
} = vi.hoisted(() => ({
6976
mockValidateChatAuth: vi.fn().mockResolvedValue({ authorized: true }),
7077
mockSetChatAuthCookie: vi.fn(),
7178
mockProcessChatFiles: vi.fn(),
79+
mockEnforceIpRateLimit: vi.fn(),
80+
mockEnforceResourceRateLimit: vi.fn(),
7281
}))
7382

7483
const mockCreateErrorResponse = workflowsApiUtilsMockFns.mockCreateErrorResponse
@@ -117,6 +126,12 @@ vi.mock('@/lib/core/utils/sse', () => ({
117126

118127
vi.mock('@/lib/core/security/encryption', () => encryptionMock)
119128

129+
vi.mock('@/lib/core/rate-limiter', () => ({
130+
enforceIpRateLimitWithIndependentBackstop: mockEnforceIpRateLimit,
131+
enforceResourceRateLimit: mockEnforceResourceRateLimit,
132+
}))
133+
134+
import { RATE_LIMITS } from '@/lib/core/rate-limiter/types'
120135
import { preprocessExecution } from '@/lib/execution/preprocessing'
121136
import { executeWorkflow } from '@/lib/workflows/executor/execute-workflow'
122137
import { createStreamingResponse } from '@/lib/workflows/streaming/streaming'
@@ -182,6 +197,8 @@ describe('Chat Identifier API Route', () => {
182197
})
183198

184199
mockValidateChatAuth.mockResolvedValue({ authorized: true })
200+
mockEnforceIpRateLimit.mockResolvedValue(null)
201+
mockEnforceResourceRateLimit.mockResolvedValue(null)
185202
mockProcessChatFiles.mockResolvedValue([])
186203
mockCreateErrorResponse.mockImplementation((message: string, status: number, code?: string) => {
187204
return new Response(
@@ -335,6 +352,107 @@ describe('Chat Identifier API Route', () => {
335352
expect(mockSetChatAuthCookie).toHaveBeenCalledWith(expect.anything(), passwordDeployment)
336353
})
337354

355+
describe('execution rate limit', () => {
356+
it.each([
357+
['per-IP', mockEnforceIpRateLimit],
358+
['per-deployment', mockEnforceResourceRateLimit],
359+
])("refuses on the %s bucket before the owner's budget is reserved", async (_, bucket) => {
360+
bucket.mockResolvedValue(
361+
NextResponse.json({ error: 'Rate limit exceeded' }, { status: 429 })
362+
)
363+
const req = createMockNextRequest('POST', { input: 'drain the wallet' })
364+
365+
const response = await POST(req, { params: Promise.resolve({ identifier: 'test-chat' }) })
366+
367+
expect(response.status).toBe(429)
368+
expect(preprocessExecution).not.toHaveBeenCalled()
369+
expect(createStreamingResponse).not.toHaveBeenCalled()
370+
expect(mockProcessChatFiles).not.toHaveBeenCalled()
371+
})
372+
373+
it('debits buckets keyed on the deployment, not the workflow', async () => {
374+
const req = createMockNextRequest('POST', { input: 'hello' })
375+
376+
await POST(req, { params: Promise.resolve({ identifier: 'test-chat' }) })
377+
378+
expect(mockEnforceIpRateLimit).toHaveBeenCalledWith(
379+
'chat-execute',
380+
req,
381+
expect.objectContaining({ refillIntervalMs: 60_000 }),
382+
'chat-id'
383+
)
384+
expect(mockEnforceResourceRateLimit).toHaveBeenCalledWith(
385+
'chat-execute',
386+
'chat-id',
387+
expect.objectContaining({ refillIntervalMs: 60_000 })
388+
)
389+
})
390+
391+
it('leaves the deployment bucket untouched when the IP bucket refuses', async () => {
392+
mockEnforceIpRateLimit.mockResolvedValue(NextResponse.json({}, { status: 429 }))
393+
const req = createMockNextRequest('POST', { input: 'flood' })
394+
395+
await POST(req, { params: Promise.resolve({ identifier: 'test-chat' }) })
396+
397+
expect(mockEnforceResourceRateLimit).not.toHaveBeenCalled()
398+
})
399+
400+
/**
401+
* The invariant the ceiling exists to hold. A chat execution debits the
402+
* workspace `sync` counter the owner's API, webhook and scheduled runs
403+
* share, so a ceiling at or above a plan's own rate never refuses before
404+
* that shared counter is drained — the availability half of the attack.
405+
* Asserted against every plan, including free, and on burst as well as
406+
* sustained rate, since either one reaching the plan bucket first is the
407+
* same hole.
408+
*/
409+
it.each(Object.keys(RATE_LIMITS))(
410+
'stays under the %s plan sync budget it debits',
411+
async (plan) => {
412+
const req = createMockNextRequest('POST', { input: 'hello' })
413+
414+
await POST(req, { params: Promise.resolve({ identifier: 'test-chat' }) })
415+
416+
const planBucket = RATE_LIMITS[plan as keyof typeof RATE_LIMITS].sync
417+
const [, , config] = mockEnforceResourceRateLimit.mock.calls[0]
418+
expect(config.refillRate).toBeLessThan(planBucket.refillRate)
419+
expect(config.maxTokens).toBeLessThan(planBucket.maxTokens)
420+
}
421+
)
422+
423+
/** One host must not be able to take the whole deployment's allowance. */
424+
it('holds the per-IP bucket under the per-deployment one', async () => {
425+
const req = createMockNextRequest('POST', { input: 'hello' })
426+
427+
await POST(req, { params: Promise.resolve({ identifier: 'test-chat' }) })
428+
429+
const [, , ipConfig] = mockEnforceIpRateLimit.mock.calls[0]
430+
const [, , deploymentConfig] = mockEnforceResourceRateLimit.mock.calls[0]
431+
expect(ipConfig.refillRate).toBeLessThan(deploymentConfig.refillRate)
432+
})
433+
434+
it('leaves the gate-configuration fetch unmetered', async () => {
435+
const passwordDeployment = {
436+
...mockChatResult[0],
437+
authType: 'password',
438+
password: 'encrypted-password',
439+
}
440+
dbChainMockFns.select.mockImplementation(() => ({
441+
from: vi.fn().mockReturnValue({
442+
where: vi.fn().mockReturnValue({
443+
limit: vi.fn().mockReturnValue([passwordDeployment]),
444+
}),
445+
}),
446+
}))
447+
const req = createMockNextRequest('POST', { password: 'test-password' })
448+
449+
await POST(req, { params: Promise.resolve({ identifier: 'password-protected-chat' }) })
450+
451+
expect(mockEnforceIpRateLimit).not.toHaveBeenCalled()
452+
expect(mockEnforceResourceRateLimit).not.toHaveBeenCalled()
453+
})
454+
})
455+
338456
it('should return 400 for requests without input', async () => {
339457
const req = createMockNextRequest('POST', {})
340458
const params = Promise.resolve({ identifier: 'test-chat' })

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

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,12 @@ 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 {
13+
enforceIpRateLimitWithIndependentBackstop,
14+
enforceResourceRateLimit,
15+
type TokenBucketConfig,
16+
} from '@/lib/core/rate-limiter'
17+
import { RATE_LIMITS } from '@/lib/core/rate-limiter/types'
1218
import { generateRequestId } from '@/lib/core/utils/request'
1319
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1420
import { preprocessExecution } from '@/lib/execution/preprocessing'
@@ -49,6 +55,56 @@ export const runtime = 'nodejs'
4955

5056
const CHAT_MAX_REQUEST_BYTES = Number.parseInt(env.CHAT_MAX_REQUEST_BYTES, 10) || 220 * 1024 * 1024
5157

58+
/** A sustained per-minute rate, with the 2x burst allowance the plan buckets use. */
59+
function executionsPerMinute(perMinute: number): TokenBucketConfig {
60+
return { maxTokens: perMinute * 2, refillRate: perMinute, refillIntervalMs: 60_000 }
61+
}
62+
63+
/**
64+
* What one deployed chat may spend of its owner's workspace allowance.
65+
*
66+
* A chat execution debits the workspace `sync` counter, which is the same
67+
* counter the owner's API, webhook and scheduled runs draw from. So this
68+
* ceiling only does its job while it sits *below* that counter: above it, a
69+
* flood empties the shared budget before this bucket ever refuses, and the
70+
* billing attack becomes an availability attack on unrelated production
71+
* workloads.
72+
*
73+
* Derived from the plan table rather than picked, because no fixed number holds
74+
* that invariant — the rates differ per plan and every one is operator
75+
* overridable through `RATE_LIMIT_*_SYNC`. A fraction of the smallest
76+
* configured rate keeps a public chat under the shared budget on every plan and
77+
* cannot drift if one of those defaults changes.
78+
*
79+
* The floor is deliberately shared by all plans for now. Sizing the slice to
80+
* the *payer's* own plan needs the subscription, which `preprocessExecution`
81+
* resolves a few lines after this runs, not here.
82+
*
83+
* A configured rate of `1` is the one value where this lands equal to the plan
84+
* rather than under it, because no positive integer is below 1. It is inert:
85+
* a workspace allowed one execution per minute has no capacity left to starve,
86+
* and the two buckets then exhaust together rather than one masking the other.
87+
*/
88+
const CHAT_EXECUTION_RATE_PER_MINUTE = Math.max(
89+
1,
90+
Math.floor(Math.min(...Object.values(RATE_LIMITS).map((plan) => plan.sync.refillRate)) * 0.8)
91+
)
92+
93+
const CHAT_EXECUTION_LIMIT = executionsPerMinute(CHAT_EXECUTION_RATE_PER_MINUTE)
94+
95+
/**
96+
* Executions one client IP may drive against a single deployed chat.
97+
*
98+
* Half the per-deployment rate, so a single source can never consume the whole
99+
* allowance and leave the rest of the audience with none. It is above one
100+
* person's chat cadence but not above a busy office behind one NAT — which
101+
* costs little in practice, since traffic that heavy from one address would
102+
* meet the per-deployment ceiling moments later anyway.
103+
*/
104+
const CHAT_EXECUTION_IP_LIMIT = executionsPerMinute(
105+
Math.max(1, Math.floor(CHAT_EXECUTION_RATE_PER_MINUTE / 2))
106+
)
107+
52108
export const POST = withRouteHandler(
53109
async (request: NextRequest, context: { params: Promise<{ identifier: string }> }) => {
54110
const { identifier } = await context.params
@@ -169,6 +225,23 @@ export const POST = withRouteHandler(
169225
return createErrorResponse('No input provided', 400)
170226
}
171227

228+
// Both buckets apply regardless of the chat's auth type: an email or SSO
229+
// visitor is still not the payer.
230+
const ipLimited = await enforceIpRateLimitWithIndependentBackstop(
231+
'chat-execute',
232+
request,
233+
CHAT_EXECUTION_IP_LIMIT,
234+
deployment.id
235+
)
236+
if (ipLimited) return ipLimited
237+
238+
const deploymentLimited = await enforceResourceRateLimit(
239+
'chat-execute',
240+
deployment.id,
241+
CHAT_EXECUTION_LIMIT
242+
)
243+
if (deploymentLimited) return deploymentLimited
244+
172245
const executionId = generateId()
173246

174247
const loggingSession = new LoggingSession(
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
/**
2+
* Tests for the chat identifier availability endpoint.
3+
*
4+
* @vitest-environment node
5+
*/
6+
import { authMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
7+
import { NextRequest, NextResponse } from 'next/server'
8+
import { beforeEach, describe, expect, it, vi } from 'vitest'
9+
10+
const { mockEnforceUserRateLimit } = vi.hoisted(() => ({
11+
mockEnforceUserRateLimit: vi.fn(),
12+
}))
13+
14+
vi.mock('@/lib/core/rate-limiter', () => ({
15+
enforceUserRateLimit: mockEnforceUserRateLimit,
16+
}))
17+
18+
import { GET } from '@/app/api/chat/validate/route'
19+
20+
function request(identifier: string) {
21+
return new NextRequest(`http://localhost:3000/api/chat/validate?identifier=${identifier}`)
22+
}
23+
24+
describe('chat identifier validation route', () => {
25+
beforeEach(() => {
26+
vi.clearAllMocks()
27+
resetDbChainMock()
28+
authMockFns.mockGetSession.mockResolvedValue({
29+
user: { id: 'user-1' },
30+
session: { id: 'session-1' },
31+
})
32+
mockEnforceUserRateLimit.mockResolvedValue(null)
33+
})
34+
35+
it('refuses an anonymous caller before answering', async () => {
36+
authMockFns.mockGetSession.mockResolvedValue(null)
37+
38+
const response = await GET(request('assistant'))
39+
40+
expect(response.status).toBe(401)
41+
expect(mockEnforceUserRateLimit).not.toHaveBeenCalled()
42+
})
43+
44+
it('reports a taken identifier to a signed-in caller', async () => {
45+
queueTableRows(schemaMock.chat, [{ id: 'chat-1' }])
46+
47+
const response = await GET(request('assistant'))
48+
49+
expect(response.status).toBe(200)
50+
expect(await response.json()).toEqual({
51+
available: false,
52+
error: 'This identifier is already in use',
53+
})
54+
})
55+
56+
it('reports a free identifier to a signed-in caller', async () => {
57+
const response = await GET(request('bot'))
58+
59+
expect(response.status).toBe(200)
60+
expect(await response.json()).toEqual({ available: true, error: null })
61+
})
62+
63+
it('caps how far one caller can walk a dictionary', async () => {
64+
mockEnforceUserRateLimit.mockResolvedValue(NextResponse.json({}, { status: 429 }))
65+
66+
const response = await GET(request('support'))
67+
68+
expect(response.status).toBe(429)
69+
expect(mockEnforceUserRateLimit).toHaveBeenCalledWith(
70+
'chat-identifier-check',
71+
'user-1',
72+
expect.objectContaining({ maxTokens: 60, refillIntervalMs: 60_000 })
73+
)
74+
})
75+
})

apps/sim/app/api/chat/validate/route.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,39 @@ import { and, eq, isNull } from 'drizzle-orm'
55
import type { NextRequest } from 'next/server'
66
import { identifierValidationQuerySchema } from '@/lib/api/contracts/chats'
77
import { getValidationErrorMessage } from '@/lib/api/server'
8+
import { getSession } from '@/lib/auth'
9+
import { enforceUserRateLimit, type TokenBucketConfig } from '@/lib/core/rate-limiter'
810
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
911
import { createErrorResponse, createSuccessResponse } from '@/app/api/workflows/utils'
1012

1113
const logger = createLogger('ChatValidateAPI')
1214

1315
/**
14-
* GET endpoint to validate chat identifier availability
16+
* Caps how far one caller can walk a dictionary of identifiers. Sized for a
17+
* debounced availability field, which sends one request per pause in typing.
18+
*/
19+
const IDENTIFIER_CHECK_RATE_LIMIT: TokenBucketConfig = {
20+
maxTokens: 60,
21+
refillRate: 60,
22+
refillIntervalMs: 60_000,
23+
}
24+
25+
/**
26+
* GET endpoint to validate chat identifier availability.
27+
*
28+
* Chat identifiers are globally unique, so availability cannot be scoped to a
29+
* workspace and there is no resource here to authorize. What the endpoint must
30+
* not be is anonymous: `available: false` names a live deployment, and the chat
31+
* behind it executes its owner's workflow on their budget for anyone holding
32+
* the identifier, so an unmetered answer is a deployment inventory.
1533
*/
1634
export const GET = withRouteHandler(async (request: NextRequest) => {
1735
try {
36+
const session = await getSession()
37+
if (!session?.user?.id) {
38+
return createErrorResponse('Unauthorized', 401)
39+
}
40+
1841
const { searchParams } = new URL(request.url)
1942
const identifier = searchParams.get('identifier')
2043

@@ -34,6 +57,13 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
3457
return createErrorResponse(errorMessage, 400)
3558
}
3659

60+
const rateLimited = await enforceUserRateLimit(
61+
'chat-identifier-check',
62+
session.user.id,
63+
IDENTIFIER_CHECK_RATE_LIMIT
64+
)
65+
if (rateLimited) return rateLimited
66+
3767
const { identifier: validatedIdentifier } = validation.data
3868

3969
const existingChat = await db

0 commit comments

Comments
 (0)