From 163e34be78227e3ee0c5da6d77e08bba4431c056 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 21:31:24 +0000 Subject: [PATCH 1/2] fix(web): stop workflow status polls draining the shared AI rate-limit bucket #1507 added /api/workflows to AI_ROUTE_PREFIXES. isAiRoute classified by path prefix alone, so the polled GET status endpoint was metered against the AI budget (default 12/min) while pollVideoToActions polled at 40/min. The 12th request 429'd ~17s into a 30s window, before a transcript fetch plus an agent call could finish. The bucket is keyed by class, not path, so every AI prefix shares one ai: counter -- a single Studio run also 429'd /api/chat, /api/transcribe and /api/pipeline as collateral. Move the classifier into auth-paths.ts, which exists as the home for path policy free of Next.js request types so vitest can import it offline, and make it method-aware. GET/HEAD on /api/workflows falls to the general budget; POST stays AI-class because starting a run does real model work. The exemption is keyed per-prefix rather than exempting GET globally, so it cannot widen another route that later serves model work over GET. An omitted method defaults to POST so the failure mode is the stricter limit. Also retune the poller to 30 attempts x 2s: 30 req/min leaves roughly half the general allowance for the rest of the page, and the wall-clock window doubles to 60s, which better fits the work the run actually does. Closes #1517 --- apps/web/src/lib/__tests__/auth-paths.test.ts | 72 +++++++++++++++++++ apps/web/src/lib/auth-paths.ts | 54 +++++++++++++- apps/web/src/lib/studio-workflow.ts | 15 +++- apps/web/src/proxy.ts | 26 ++----- 4 files changed, 143 insertions(+), 24 deletions(-) diff --git a/apps/web/src/lib/__tests__/auth-paths.test.ts b/apps/web/src/lib/__tests__/auth-paths.test.ts index c9b961baa..a9a94ab96 100644 --- a/apps/web/src/lib/__tests__/auth-paths.test.ts +++ b/apps/web/src/lib/__tests__/auth-paths.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from 'vitest'; import { + isAiRoute, isPublicApiPath, isProtectedPagePath, needsAuthentication, @@ -111,3 +112,74 @@ describe('login gate mode (issue #1058)', () => { } }); }); + +describe('AI route classification (rate-limit budget)', () => { + it('meters model-backed routes against the AI budget', () => { + for (const path of [ + '/api/agents/dispatch', + '/api/chat', + '/api/extract-events', + '/api/pipeline', + '/api/realtime', + '/api/training', + '/api/transcribe', + '/api/video', + ]) { + expect(isAiRoute(path, 'POST')).toBe(true); + } + }); + + it('leaves non-AI API routes on the general budget', () => { + expect(isAiRoute('/api/billing/status', 'GET')).toBe(false); + expect(isAiRoute('/api/health', 'GET')).toBe(false); + expect(isAiRoute('/api/auth/session', 'GET')).toBe(false); + }); + + it('keeps starting a workflow run on the AI budget', () => { + // POST .../video-to-actions fetches a transcript and runs an action agent. + // That is genuine model work and must stay metered. + expect(isAiRoute('/api/workflows/video-to-actions', 'POST')).toBe(true); + }); + + it('exempts workflow status polls from the AI budget', () => { + // GET .../:runId reads stored run state and makes no model call. Metering + // it as AI-class throttles the poller to 12/min while it polls at 30/min. + expect(isAiRoute('/api/workflows/video-to-actions/run_123', 'GET')).toBe(false); + expect(isAiRoute('/api/workflows/video-to-actions/run_123', 'HEAD')).toBe(false); + }); + + it('does not let a status poll drain the shared AI bucket', () => { + // The bucket is keyed by class, not path: every AI prefix shares one + // `ai:` counter. If polls were AI-class, one Studio run would 429 + // /api/chat and /api/transcribe as collateral. This is the regression that + // motivated the method-aware check, so assert the pairing directly. + const poll = '/api/workflows/video-to-actions/run_123'; + expect(isAiRoute(poll, 'GET')).toBe(false); + expect(isAiRoute('/api/chat', 'GET')).toBe(true); + }); + + it('is case-insensitive about the method', () => { + expect(isAiRoute('/api/workflows/video-to-actions/run_1', 'get')).toBe(false); + }); + + it('keeps mutating verbs on workflows AI-class', () => { + // Only reads are exempt — a DELETE/PUT that reaches the workflow runtime + // must not slip onto the looser budget. + for (const method of ['POST', 'PUT', 'PATCH', 'DELETE']) { + expect(isAiRoute('/api/workflows/video-to-actions/run_1', method)).toBe(true); + } + }); + + it('defaults to the stricter budget when no method is supplied', () => { + // Fail safe: an omitted argument must not silently widen the allowance. + expect(isAiRoute('/api/workflows/video-to-actions/run_1')).toBe(true); + expect(isAiRoute('/api/chat')).toBe(true); + }); + + it('does not exempt GET on other AI routes', () => { + // The exemption is deliberately per-prefix. A blanket GET carve-out would + // open every model-backed route the moment one served work over GET. + expect(isAiRoute('/api/chat/history', 'GET')).toBe(true); + expect(isAiRoute('/api/transcribe/status', 'GET')).toBe(true); + }); +}); diff --git a/apps/web/src/lib/auth-paths.ts b/apps/web/src/lib/auth-paths.ts index 9d714b950..f9d5e975a 100644 --- a/apps/web/src/lib/auth-paths.ts +++ b/apps/web/src/lib/auth-paths.ts @@ -1,5 +1,5 @@ /** - * Shared auth path policy for middleware/proxy and unit tests. + * Shared auth and rate-limit path policy for middleware/proxy and unit tests. * Keep this free of Next.js request types so vitest can import it offline. */ @@ -78,6 +78,58 @@ export function shouldSkipRateLimit(pathname: string): boolean { return pathname === '/api/auth' || pathname.startsWith('/api/auth/'); } +/** Routes backed by model work, metered against the tighter AI budget. */ +const AI_ROUTE_PREFIXES = [ + '/api/agents/dispatch', + '/api/chat', + '/api/extract-events', + '/api/pipeline', + '/api/realtime', + '/api/training', + '/api/transcribe', + '/api/video', + '/api/workflows', +] as const; + +/** + * Methods that perform no model work on an otherwise AI-class prefix. + * + * `/api/workflows` covers both `POST .../video-to-actions` (starts a run: + * transcript fetch + action agent, genuinely AI-class) and + * `GET .../video-to-actions/:runId` (reads stored run state, no model call). + * Prefix matching alone cannot separate them, so the method has to reach the + * classifier. + * + * This matters more than "one endpoint is metered too tightly", because the + * rate-limit bucket is keyed by *class*, not by path — every AI prefix shares + * one `ai:` counter. A Studio run polls its status ~40x/min against an + * AI budget defaulting to 12/min, so without this exemption a single run + * exhausts the shared allowance in ~17s and 429s /api/chat, /api/transcribe + * and /api/pipeline along with itself. + * + * Deliberately keyed per-prefix rather than exempting GET globally: the other + * prefixes have no polling client, and a blanket GET exemption would be an + * abuse vector the moment any of them serves model work over GET. + */ +const AI_ROUTE_METHOD_EXEMPT: Record> = { + '/api/workflows': new Set(['GET', 'HEAD']), +}; + +/** + * Whether a request should be metered against the AI budget rather than the + * general one. + * + * `method` defaults to POST so an omitted argument fails *safe* (stricter + * limit) rather than silently widening the budget. + */ +export function isAiRoute(pathname: string, method: string = 'POST'): boolean { + const prefix = AI_ROUTE_PREFIXES.find((candidate) => + pathname.startsWith(candidate), + ); + if (!prefix) return false; + return !AI_ROUTE_METHOD_EXEMPT[prefix]?.has(method.toUpperCase()); +} + /** * How the login gate should behave for a given environment. * diff --git a/apps/web/src/lib/studio-workflow.ts b/apps/web/src/lib/studio-workflow.ts index d03eb9424..55985c07a 100644 --- a/apps/web/src/lib/studio-workflow.ts +++ b/apps/web/src/lib/studio-workflow.ts @@ -131,14 +131,23 @@ const TERMINAL = new Set(['completed', 'failed', 'cancelled']); /** * Poll until the run is terminal or attempts are exhausted. - * Default: 20 attempts × 1.5s ≈ 30s of wall time (workflow continues server-side). + * Default: 30 attempts × 2s ≈ 60s of wall time (workflow continues server-side). + * + * The cadence is chosen against the middleware rate limit, not just for UI + * responsiveness. Status reads are metered on the general budget + * (`UVAI_API_RATE_LIMIT_PER_MINUTE`, default 60/min) — see `isAiRoute` in + * `@/lib/auth-paths`, which exempts GET on `/api/workflows` from the much + * tighter AI budget. At 2s a run spends 30 req/min, leaving roughly half the + * general allowance for the rest of the page; the previous 1.5s cadence spent + * 40/min and left little margin. The wall-clock window doubles to 60s as a + * side effect, which better fits a transcript fetch plus an agent call. */ export async function pollVideoToActions( runId: string, opts?: { attempts?: number; delayMs?: number; signal?: AbortSignal }, ): Promise { - const attempts = opts?.attempts ?? 20; - const delayMs = opts?.delayMs ?? 1500; + const attempts = opts?.attempts ?? 30; + const delayMs = opts?.delayMs ?? 2000; let last: VideoToActionsPoll = { ok: false, status: 0, diff --git a/apps/web/src/proxy.ts b/apps/web/src/proxy.ts index da97c6914..dba9acaee 100644 --- a/apps/web/src/proxy.ts +++ b/apps/web/src/proxy.ts @@ -2,6 +2,7 @@ import { NextRequest, NextResponse } from 'next/server'; import type { Redis } from '@upstash/redis'; import { getToken } from 'next-auth/jwt'; import { + isAiRoute, needsAuthentication, resolveAuthGateMode, safeCallbackPath, @@ -29,18 +30,6 @@ const WINDOW_SECONDS = 60; const GENERAL_LIMIT = Number(process.env.UVAI_API_RATE_LIMIT_PER_MINUTE || 60); const AI_LIMIT = Number(process.env.UVAI_AI_RATE_LIMIT_PER_MINUTE || 12); -const AI_ROUTE_PREFIXES = [ - '/api/agents/dispatch', - '/api/chat', - '/api/extract-events', - '/api/pipeline', - '/api/realtime', - '/api/training', - '/api/transcribe', - '/api/video', - '/api/workflows', -]; - // Login gating (activate-when-configured) + server-to-server bypass. const INTERNAL_TOKEN = process.env.INTERNAL_REQUEST_TOKEN; const AUTH_SECRET = process.env.NEXTAUTH_SECRET; @@ -121,10 +110,6 @@ function getRedisClient(): Promise { return redisClientPromise; } -function isAiRoute(pathname: string): boolean { - return AI_ROUTE_PREFIXES.some((prefix) => pathname.startsWith(prefix)); -} - function getClientIp(request: NextRequest): string { // Prefer x-real-ip: set by Vercel's edge network and not client-controllable. const realIp = request.headers.get('x-real-ip'); @@ -144,8 +129,8 @@ function getClientIp(request: NextRequest): string { return 'unknown'; } -function getRateLimit(pathname: string): number { - return isAiRoute(pathname) ? AI_LIMIT : GENERAL_LIMIT; +function getRateLimit(pathname: string, method: string): number { + return isAiRoute(pathname, method) ? AI_LIMIT : GENERAL_LIMIT; } async function checkRedisLimit(redisClient: Redis, key: string, limit: number): Promise { @@ -188,9 +173,10 @@ function checkMemoryLimit(key: string, limit: number): RateLimitResult { async function checkRateLimit(request: NextRequest): Promise { const pathname = request.nextUrl.pathname; - const limit = getRateLimit(pathname); + const method = request.method; + const limit = getRateLimit(pathname, method); const clientIp = getClientIp(request); - const routeClass = isAiRoute(pathname) ? 'ai' : 'api'; + const routeClass = isAiRoute(pathname, method) ? 'ai' : 'api'; const key = `${routeClass}:${clientIp}`; const redisClient = await getRedisClient(); From 3c0d78dae48683c818d48321d9ef773a74f6a7c0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 21:34:49 +0000 Subject: [PATCH 2/2] fix(web): require a segment boundary before exempting a route from the AI budget Self-review follow-up. The exemption looked the prefix up with the same loose startsWith used for class membership, so a future sibling surface whose name merely starts with an exempted prefix -- /api/workflows-admin -- would silently inherit the GET carve-out and drop onto the looser budget. No route in the tree does this today (checked every directory under apps/web/src/app/api), so this is latent rather than live. It is worth closing while the file is open: the same shape, an incidental block quietly becoming an allow, is what #1486 had to fix in the SSRF guard. Class membership keeps its original loose matching. Narrowing that would move routes off the stricter budget, which this change has no business doing; the exemption is the widening, so only the exemption is tightened. --- apps/web/src/lib/__tests__/auth-paths.test.ts | 13 +++++++++++++ apps/web/src/lib/auth-paths.ts | 11 +++++++++++ 2 files changed, 24 insertions(+) diff --git a/apps/web/src/lib/__tests__/auth-paths.test.ts b/apps/web/src/lib/__tests__/auth-paths.test.ts index a9a94ab96..45ef4e4eb 100644 --- a/apps/web/src/lib/__tests__/auth-paths.test.ts +++ b/apps/web/src/lib/__tests__/auth-paths.test.ts @@ -176,6 +176,19 @@ describe('AI route classification (rate-limit budget)', () => { expect(isAiRoute('/api/chat')).toBe(true); }); + it('does not let the exemption leak to a route that merely shares the prefix', () => { + // The exemption is the widening in this change, so it requires a segment + // boundary. A future sibling surface like /api/workflows-admin is still + // AI-class on every method — otherwise adding a route whose name starts + // with an exempted prefix would silently hand it the looser budget. + expect(isAiRoute('/api/workflows-admin/secret', 'GET')).toBe(true); + expect(isAiRoute('/api/workflows-admin', 'GET')).toBe(true); + // ...while the real surface keeps its exemption, at the prefix itself and + // below it. + expect(isAiRoute('/api/workflows', 'GET')).toBe(false); + expect(isAiRoute('/api/workflows/video-to-actions/run_1', 'GET')).toBe(false); + }); + it('does not exempt GET on other AI routes', () => { // The exemption is deliberately per-prefix. A blanket GET carve-out would // open every model-backed route the moment one served work over GET. diff --git a/apps/web/src/lib/auth-paths.ts b/apps/web/src/lib/auth-paths.ts index f9d5e975a..a9957b3d6 100644 --- a/apps/web/src/lib/auth-paths.ts +++ b/apps/web/src/lib/auth-paths.ts @@ -127,6 +127,17 @@ export function isAiRoute(pathname: string, method: string = 'POST'): boolean { pathname.startsWith(candidate), ); if (!prefix) return false; + + // The exemption requires a segment boundary, while class membership above + // keeps its original loose `startsWith`. The asymmetry is deliberate: + // tightening membership would move routes off the stricter budget, which is + // a widening this change has no business making. But the exemption *is* the + // widening, so it must not leak to a route that merely shares the string + // prefix — `/api/workflows-admin/...` is a different surface than + // `/api/workflows/...` and stays AI-class. + const onExemptRoute = pathname === prefix || pathname.startsWith(`${prefix}/`); + if (!onExemptRoute) return true; + return !AI_ROUTE_METHOD_EXEMPT[prefix]?.has(method.toUpperCase()); }