From d9f4e149022ef664df696fb8ba2e91420176f2d7 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 28 Jul 2026 11:38:54 -0700 Subject: [PATCH] fix(api): report the real rate-limit ceiling on every v1 endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-RateLimit-Limit was the bucket's refill rate while X-RateLimit-Remaining was tokens left in the bucket, and createBucketConfig sets maxTokens = refillRate * burstMultiplier. The two headers described different quantities, so remaining routinely exceeded limit — observed live on a team plan as limit 200 alongside remaining 399 — and any client computing used = limit - remaining got a negative number. Report the bucket capacity, which is what remaining counts down from. This lives in the shared checkRateLimit, so it applies to every v1 endpoint: workflows, logs, tables, files, knowledge, audit-logs and copilot. Also stops publishing rate-limit headers on an authentication failure. That path never reaches the bucket, so the previous placeholder advertised a quota that does not exist and told unauthenticated callers they had been throttled. Separately, the shared id schemas reported Zod's default "expected string, received undefined" when a required field was omitted, because .min(1) only fires for a present-but-empty string. Adding the message to the z.string() constructor makes an omitted workspaceId/organizationId/workflowId/fileId name the field it is complaining about — the first thing an API consumer sees on a malformed request. Documents all three headers in the OpenAPI spec as reusable components, including the burst-capacity semantics and the fact that they are absent on authentication failures. They were previously undocumented. Adds the first tests for the v1 middleware. --- apps/docs/openapi.json | 35 ++++- apps/sim/app/api/v1/middleware.test.ts | 134 ++++++++++++++++++ apps/sim/app/api/v1/middleware.ts | 33 ++++- apps/sim/lib/api/contracts/primitives.test.ts | 47 ++++++ apps/sim/lib/api/contracts/primitives.ts | 20 ++- 5 files changed, 257 insertions(+), 12 deletions(-) create mode 100644 apps/sim/app/api/v1/middleware.test.ts diff --git a/apps/docs/openapi.json b/apps/docs/openapi.json index 1db7ab8e662..79c22942f33 100644 --- a/apps/docs/openapi.json +++ b/apps/docs/openapi.json @@ -7762,13 +7762,22 @@ } }, "RateLimited": { - "description": "Rate limit exceeded. Wait for the duration specified in the Retry-After header before retrying.", + "description": "Rate limit exceeded. Wait for the duration specified in the Retry-After header before retrying. The X-RateLimit-* headers below accompany every authenticated response, not just this one; they are omitted when a request fails authentication, since no rate-limit bucket is consulted in that case.", "headers": { "Retry-After": { "description": "Number of seconds to wait before retrying the request.", "schema": { "type": "integer" } + }, + "X-RateLimit-Limit": { + "$ref": "#/components/headers/RateLimitLimit" + }, + "X-RateLimit-Remaining": { + "$ref": "#/components/headers/RateLimitRemaining" + }, + "X-RateLimit-Reset": { + "$ref": "#/components/headers/RateLimitReset" } }, "content": { @@ -7833,6 +7842,30 @@ } } } + }, + "headers": { + "RateLimitLimit": { + "description": "Maximum number of requests the bucket holds \u2014 its burst capacity, which is the per-minute allowance for your plan multiplied by a burst factor. `X-RateLimit-Remaining` counts down from this value, so `X-RateLimit-Limit - X-RateLimit-Remaining` is the number of requests currently consumed.", + "schema": { + "type": "integer", + "example": 400 + } + }, + "RateLimitRemaining": { + "description": "Requests still available in the current bucket. Never exceeds `X-RateLimit-Limit`.", + "schema": { + "type": "integer", + "example": 399 + } + }, + "RateLimitReset": { + "description": "ISO 8601 timestamp at which the bucket refills.", + "schema": { + "type": "string", + "format": "date-time", + "example": "2026-07-28T18:28:48.354Z" + } + } } } } diff --git a/apps/sim/app/api/v1/middleware.test.ts b/apps/sim/app/api/v1/middleware.test.ts new file mode 100644 index 00000000000..6b0efaabf3a --- /dev/null +++ b/apps/sim/app/api/v1/middleware.test.ts @@ -0,0 +1,134 @@ +/** + * @vitest-environment node + * + * Pins the rate-limit headers every v1 endpoint publishes. `X-RateLimit-Limit` + * and `X-RateLimit-Remaining` must describe the same quantity — the token + * bucket's capacity and the tokens left in it — or a client computing + * `used = limit - remaining` gets a negative number. + */ + +import { createMockRequest } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockAuthenticateV1Request, mockGetSubscription, mockCheckRateLimit, mockGetRateLimit } = + vi.hoisted(() => ({ + mockAuthenticateV1Request: vi.fn(), + mockGetSubscription: vi.fn(), + mockCheckRateLimit: vi.fn(), + mockGetRateLimit: vi.fn(), + })) + +vi.mock('@/app/api/v1/auth', () => ({ + authenticateV1Request: mockAuthenticateV1Request, +})) + +vi.mock('@/lib/billing/core/subscription', () => ({ + getHighestPrioritySubscription: mockGetSubscription, +})) + +vi.mock('@/lib/core/rate-limiter', () => ({ + getRateLimit: mockGetRateLimit, + RateLimiter: class { + checkRateLimitWithSubscription = mockCheckRateLimit + }, +})) + +import { checkRateLimit, createRateLimitResponse } from '@/app/api/v1/middleware' + +/** Mirrors `createBucketConfig`: capacity is the per-minute rate x burst multiplier. */ +const TEAM_BUCKET = { maxTokens: 400, refillRate: 200, refillIntervalMs: 60_000 } + +function request() { + return createMockRequest('GET', undefined, {}, 'http://localhost:3000/api/v1/workflows') +} + +describe('checkRateLimit', () => { + beforeEach(() => { + vi.clearAllMocks() + mockAuthenticateV1Request.mockResolvedValue({ + authenticated: true, + userId: 'user-1', + keyType: 'personal', + }) + mockGetSubscription.mockResolvedValue({ plan: 'team' }) + mockGetRateLimit.mockReturnValue(TEAM_BUCKET) + mockCheckRateLimit.mockResolvedValue({ + allowed: true, + remaining: 399, + resetAt: new Date('2026-07-28T18:28:48.354Z'), + }) + }) + + it('reports the bucket capacity as the limit, not the refill rate', async () => { + const result = await checkRateLimit(request(), 'workflows') + + expect(result.limit).toBe(TEAM_BUCKET.maxTokens) + expect(result.limit).not.toBe(TEAM_BUCKET.refillRate) + }) + + it('never reports more remaining than the limit', async () => { + const result = await checkRateLimit(request(), 'workflows') + + expect(result.remaining).toBeLessThanOrEqual(result.limit) + }) + + it('reports a zero limit when authentication fails, since no bucket was consulted', async () => { + mockAuthenticateV1Request.mockResolvedValue({ + authenticated: false, + error: 'API key required', + }) + + const result = await checkRateLimit(request(), 'workflows') + + expect(result.allowed).toBe(false) + expect(result.limit).toBe(0) + expect(result.error).toBe('API key required') + expect(mockCheckRateLimit).not.toHaveBeenCalled() + }) + + it('reports a zero limit when the checker itself throws', async () => { + mockCheckRateLimit.mockRejectedValue(new Error('redis down')) + + const result = await checkRateLimit(request(), 'workflows') + + expect(result.allowed).toBe(false) + expect(result.limit).toBe(0) + }) +}) + +describe('createRateLimitResponse', () => { + const throttled = { + allowed: false, + remaining: 0, + limit: 400, + resetAt: new Date('2026-07-28T18:28:48.354Z'), + retryAfterMs: 30_000, + } + + it('publishes consistent headers on a 429', () => { + const response = createRateLimitResponse(throttled) + + expect(response.status).toBe(429) + const limit = Number(response.headers.get('X-RateLimit-Limit')) + const remaining = Number(response.headers.get('X-RateLimit-Remaining')) + expect(remaining).toBeLessThanOrEqual(limit) + expect(response.headers.get('X-RateLimit-Reset')).toBe(throttled.resetAt.toISOString()) + expect(response.headers.get('Retry-After')).toBe('30') + }) + + it('omits rate-limit headers on an auth failure, which never reached the bucket', async () => { + const response = createRateLimitResponse({ + allowed: false, + remaining: 0, + limit: 0, + resetAt: new Date(), + error: 'API key required', + }) + + expect(response.status).toBe(401) + expect(response.headers.get('X-RateLimit-Limit')).toBeNull() + expect(response.headers.get('X-RateLimit-Remaining')).toBeNull() + expect(response.headers.get('X-RateLimit-Reset')).toBeNull() + await expect(response.json()).resolves.toEqual({ error: 'API key required' }) + }) +}) diff --git a/apps/sim/app/api/v1/middleware.ts b/apps/sim/app/api/v1/middleware.ts index c9f757d91df..eebebc04973 100644 --- a/apps/sim/app/api/v1/middleware.ts +++ b/apps/sim/app/api/v1/middleware.ts @@ -41,6 +41,11 @@ export interface RateLimitResult { allowed: boolean remaining: number resetAt: Date + /** + * Bucket capacity, matching what `remaining` counts down from. Zero on the + * paths that never reached the bucket (auth failure, checker error); those + * carry `error` and publish no rate-limit headers. + */ limit: number retryAfterMs?: number userId?: string @@ -65,7 +70,7 @@ export async function checkRateLimit( return { allowed: false, remaining: 0, - limit: 10, + limit: 0, resetAt: new Date(), error: auth.error, } @@ -96,7 +101,16 @@ export async function checkRateLimit( allowed: result.allowed, remaining: result.remaining, resetAt: result.resetAt, - limit: config.refillRate, + /** + * The bucket's capacity, not its refill rate. `remaining` is the token + * count left in that bucket, and `createBucketConfig` sets + * `maxTokens = refillRate * burstMultiplier` — so reporting `refillRate` + * here published an `X-RateLimit-Limit` smaller than the + * `X-RateLimit-Remaining` beside it (e.g. limit 200, remaining 399), and + * any client computing `used = limit - remaining` got a negative number. + * Both headers must describe the same quantity. + */ + limit: config.maxTokens, retryAfterMs: result.retryAfterMs, userId, workspaceId: auth.workspaceId, @@ -107,7 +121,7 @@ export async function checkRateLimit( return { allowed: false, remaining: 0, - limit: 10, + limit: 0, resetAt: new Date(Date.now() + 60000), error: 'Rate limit check failed', } @@ -131,16 +145,21 @@ export async function authenticateRequest( } export function createRateLimitResponse(result: RateLimitResult): NextResponse { + /** + * An authentication failure never reaches the token bucket, so there is no + * limit to report. Publishing a placeholder told unauthenticated callers they + * had been throttled and handed monitoring a quota that does not exist. + */ + if (result.error) { + return NextResponse.json({ error: result.error || 'Unauthorized' }, { status: 401 }) + } + const headers = { 'X-RateLimit-Limit': result.limit.toString(), 'X-RateLimit-Remaining': result.remaining.toString(), 'X-RateLimit-Reset': result.resetAt.toISOString(), } - if (result.error) { - return NextResponse.json({ error: result.error || 'Unauthorized' }, { status: 401, headers }) - } - const retryAfterSeconds = result.retryAfterMs ? Math.ceil(result.retryAfterMs / 1000) : Math.ceil((result.resetAt.getTime() - Date.now()) / 1000) diff --git a/apps/sim/lib/api/contracts/primitives.test.ts b/apps/sim/lib/api/contracts/primitives.test.ts index 727a42a65f1..204e6e9d2fe 100644 --- a/apps/sim/lib/api/contracts/primitives.test.ts +++ b/apps/sim/lib/api/contracts/primitives.test.ts @@ -4,8 +4,12 @@ import { describe, expect, it } from 'vitest' import { customPatternSchema, + organizationIdSchema, piiStagePolicySchema, piiStagesSchema, + workflowIdSchema, + workspaceFileIdSchema, + workspaceIdSchema, } from '@/lib/api/contracts/primitives' describe('customPatternSchema', () => { @@ -102,3 +106,46 @@ describe('piiStagesSchema', () => { expect(parsed.blockOutputs.enabled).toBe(true) }) }) + +/** + * `.min(1)` only fires for a present-but-empty string, so without the + * `z.string({ error })` form an omitted field falls back to Zod's default + * "expected string, received undefined" — which does not name the field the + * caller left out. These are the shared id schemas every contract builds on, so + * the wording here is the first thing an API consumer sees on a malformed + * request. + */ +describe('shared id schemas name the field when it is missing', () => { + const cases = [ + ['workspaceIdSchema', workspaceIdSchema, 'Workspace ID is required'], + ['organizationIdSchema', organizationIdSchema, 'Organization ID is required'], + ['workflowIdSchema', workflowIdSchema, 'Workflow ID is required'], + ['workspaceFileIdSchema', workspaceFileIdSchema, 'File ID is required'], + ] as const + + for (const [name, schema, message] of cases) { + it(`${name}: omitted value reports "${message}"`, () => { + const result = schema.safeParse(undefined) + + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.message).toBe(message) + }) + + it(`${name}: empty string reports "${message}"`, () => { + const result = schema.safeParse('') + + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.message).toBe(message) + }) + + it(`${name}: a valid id still passes`, () => { + expect(schema.safeParse('abc-123').success).toBe(true) + }) + } + + it('does not leak Zod default wording for a missing field', () => { + const result = workspaceIdSchema.safeParse(undefined) + + expect(result.error?.issues[0]?.message).not.toContain('received undefined') + }) +}) diff --git a/apps/sim/lib/api/contracts/primitives.ts b/apps/sim/lib/api/contracts/primitives.ts index 6e5b83454d3..79e6243aab4 100644 --- a/apps/sim/lib/api/contracts/primitives.ts +++ b/apps/sim/lib/api/contracts/primitives.ts @@ -36,20 +36,32 @@ export const nonEmptyIdSchema = z.string().min(1) * Non-empty `workspaceId` field. Same constraint as `nonEmptyIdSchema` with a * stable, human-readable message. Use to deduplicate the * `z.string().min(1, 'Workspace ID is required')` pattern across contracts. + * + * The message is given twice on purpose: `.min(1)` only fires for a present but + * empty string, so without the `z.string({ error })` form an *omitted* field + * falls back to Zod's default `Invalid input: expected string, received + * undefined`, which does not name the field. The same applies to the sibling id + * schemas below. */ -export const workspaceIdSchema = z.string().min(1, 'Workspace ID is required') +export const workspaceIdSchema = z + .string({ error: 'Workspace ID is required' }) + .min(1, 'Workspace ID is required') /** * Non-empty `organizationId` field. Same constraint as `nonEmptyIdSchema` with a * stable, human-readable message. */ -export const organizationIdSchema = z.string().min(1, 'Organization ID is required') +export const organizationIdSchema = z + .string({ error: 'Organization ID is required' }) + .min(1, 'Organization ID is required') /** * Non-empty `workflowId` field. Same constraint as `nonEmptyIdSchema` with a * stable, human-readable message. */ -export const workflowIdSchema = z.string().min(1, 'Workflow ID is required') +export const workflowIdSchema = z + .string({ error: 'Workflow ID is required' }) + .min(1, 'Workflow ID is required') /** * A `workspace_files.id` value. The column is a free-form `text` primary key, so @@ -59,7 +71,7 @@ export const workflowIdSchema = z.string().min(1, 'Workflow ID is required') * UUID-only schema — a `.uuid()` constraint here silently 400s every `wf_` file. */ export const workspaceFileIdSchema = z - .string() + .string({ error: 'File ID is required' }) .min(1, 'File ID is required') .max(128, 'File ID is too long') .regex(/^[A-Za-z0-9_-]+$/, 'Invalid file id')