From 34ff739d653f37e46ab5b15daaa4ad6bec69d30f Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 28 Jul 2026 16:59:38 -0700 Subject: [PATCH] fix(security): harden glob matching, archive parsing, redaction and media handling Glob matching (copilot VFS): - match globs with RE2 instead of micromatch's backtracking engine, and translate picomatch's output rather than trusting it: reject escape passthrough (\A, \z, \p{L}) that RE2 reads as anchors and Unicode classes, and class ranges straddling the private-use segment markers Archive parsing: - scan the whole buffer for EOCD records, matching JSZip and SheetJS rather than the spec's trailing window - treat a resolvable-but-empty central directory as unverifiable - bound central-directory scanning with a shared record budget - keep stray EOCD byte sequences in non-ZIP documents a no-op so the OLE2 and plaintext fallbacks still run Redaction: - fix quadratic acronym-boundary backtracking (73s to 1ms at 400k chars) - catch plural and value-suffixed secret keys, and one-shot locator URLs Other: - guard YAML alias expansion in the JSON/YAML chunker - pin fal.ai queue polling to its origin and cap the response body - escape ffmpeg drawtext values through a file instead of the filtergraph - resolve redirects against the registrable domain and drop credential headers across sites - let the file preview error boundary retry instead of latching --- .../app/api/tools/onedrive/upload/route.ts | 1 + apps/sim/app/api/tools/video/route.test.ts | 336 +++++++ apps/sim/app/api/tools/video/route.ts | 216 +++-- .../components/file-viewer/file-viewer.tsx | 24 +- .../file-viewer/preview-shared.test.tsx | 168 ++++ .../components/file-viewer/preview-shared.tsx | 88 +- apps/sim/executor/orchestrators/loop.test.ts | 70 ++ apps/sim/executor/orchestrators/loop.ts | 4 +- .../lib/chunkers/json-yaml-chunker.test.ts | 56 ++ apps/sim/lib/chunkers/json-yaml-chunker.ts | 32 +- .../lib/copilot/tools/handlers/vfs.test.ts | 20 +- apps/sim/lib/copilot/tools/handlers/vfs.ts | 13 +- .../lib/copilot/vfs/document-style.test.ts | 94 ++ apps/sim/lib/copilot/vfs/document-style.ts | 3 + .../vfs/operations.glob-semantics.test.ts | 436 +++++++++ apps/sim/lib/copilot/vfs/operations.test.ts | 118 ++- apps/sim/lib/copilot/vfs/operations.ts | 901 +++++++++++++++++- .../core/security/input-validation.server.ts | 116 ++- apps/sim/lib/core/security/redaction.test.ts | 299 +++++- apps/sim/lib/core/security/redaction.ts | 262 +++-- .../secure-fetch-redirect.server.test.ts | 311 ++++++ apps/sim/lib/file-parsers/doc-parser.test.ts | 107 +++ apps/sim/lib/file-parsers/doc-parser.ts | 11 + apps/sim/lib/file-parsers/xlsx-parser.ts | 5 + apps/sim/lib/file-parsers/zip-guard.test.ts | 425 ++++++++- apps/sim/lib/file-parsers/zip-guard.ts | 395 ++++++-- apps/sim/lib/media/falai.test.ts | 229 +++++ apps/sim/lib/media/falai.ts | 126 ++- apps/sim/lib/media/ffmpeg.test.ts | 209 ++++ apps/sim/lib/media/ffmpeg.ts | 79 +- 30 files changed, 4845 insertions(+), 309 deletions(-) create mode 100644 apps/sim/app/api/tools/video/route.test.ts create mode 100644 apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared.test.tsx create mode 100644 apps/sim/lib/copilot/vfs/document-style.test.ts create mode 100644 apps/sim/lib/copilot/vfs/operations.glob-semantics.test.ts create mode 100644 apps/sim/lib/core/security/secure-fetch-redirect.server.test.ts create mode 100644 apps/sim/lib/file-parsers/doc-parser.test.ts create mode 100644 apps/sim/lib/media/falai.test.ts create mode 100644 apps/sim/lib/media/ffmpeg.test.ts diff --git a/apps/sim/app/api/tools/onedrive/upload/route.ts b/apps/sim/app/api/tools/onedrive/upload/route.ts index 2c94986de9f..2a0d1d59477 100644 --- a/apps/sim/app/api/tools/onedrive/upload/route.ts +++ b/apps/sim/app/api/tools/onedrive/upload/route.ts @@ -1,6 +1,7 @@ import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { type NextRequest, NextResponse } from 'next/server' +/** Pinned to the SheetJS CDN, not npm — see the note in `lib/file-parsers/xlsx-parser.ts`. */ import * as XLSX from 'xlsx' import { onedriveUploadContract } from '@/lib/api/contracts/tools/microsoft' import { parseRequest } from '@/lib/api/server' diff --git a/apps/sim/app/api/tools/video/route.test.ts b/apps/sim/app/api/tools/video/route.test.ts new file mode 100644 index 00000000000..5e120b8b947 --- /dev/null +++ b/apps/sim/app/api/tools/video/route.test.ts @@ -0,0 +1,336 @@ +/** + * @vitest-environment node + */ +import { + createMockRequest, + hybridAuthMockFns, + inputValidationMock, + inputValidationMockFns, +} from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { MAX_FAL_QUEUE_JSON_BYTES } from '@/lib/media/falai' + +const { mockUploadFile } = vi.hoisted(() => ({ + mockUploadFile: vi.fn(), +})) + +vi.mock('@/lib/core/security/input-validation.server', () => inputValidationMock) +vi.mock('@/lib/core/execution-limits', () => ({ getMaxExecutionTimeout: () => 30_000 })) +vi.mock('@sim/utils/helpers', () => ({ sleep: () => Promise.resolve() })) +vi.mock('@/app/api/files/authorization', () => ({ + assertToolFileAccess: vi.fn().mockResolvedValue(null), +})) +vi.mock('@/lib/uploads', () => ({ + StorageService: { uploadFile: mockUploadFile }, +})) +vi.mock('@/lib/core/utils/urls', () => ({ getBaseUrl: () => 'https://sim.test' })) + +import { POST } from '@/app/api/tools/video/route' + +const { mockValidateUrlWithDNS, mockSecureFetchWithPinnedIP } = inputValidationMockFns + +function jsonResponse(payload: unknown) { + const text = JSON.stringify(payload) + return { + ok: true, + status: 200, + headers: { get: () => null }, + body: null, + text: async () => text, + arrayBuffer: async () => new ArrayBuffer(0), + } +} + +function videoResponse() { + return { + ok: true, + status: 200, + headers: { + get: (name: string) => { + if (name === 'content-type') return 'video/mp4' + return name === 'content-length' ? '8' : null + }, + }, + body: null, + text: async () => '', + arrayBuffer: async () => new ArrayBuffer(8), + } +} + +function errorResponse(status: number) { + return { + ok: false, + status, + headers: { get: () => null }, + body: null, + text: async () => 'denied', + arrayBuffer: async () => new ArrayBuffer(0), + } +} + +const baseBody = { + provider: 'falai', + apiKey: 'fal-key', + model: 'kling-v3-pro', + prompt: 'a cat riding a bike', +} + +describe('POST /api/tools/video (Fal.ai queue)', () => { + const fetchMock = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + vi.stubGlobal('fetch', fetchMock) + hybridAuthMockFns.mockCheckInternalAuth.mockResolvedValue({ + success: true, + userId: 'user-1', + authType: 'internal_jwt', + }) + mockValidateUrlWithDNS.mockResolvedValue({ isValid: true, resolvedIP: '8.8.8.8' }) + mockUploadFile.mockResolvedValue({ path: '/api/files/serve/video.mp4' }) + }) + + it('normalizes the echoed /response URL and bounds queue reads with the shared Fal cap', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + request_id: 'req-1', + status_url: 'https://queue.fal.run/fal-ai/kling-video/requests/req-1/status', + response_url: 'https://queue.fal.run/fal-ai/kling-video/requests/req-1/response', + }), + { status: 200 } + ) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + .mockResolvedValueOnce(videoResponse()) + + const response = await POST(createMockRequest('POST', baseBody)) + expect(response.status).toBe(200) + + const [statusCall, resultCall, downloadCall] = mockSecureFetchWithPinnedIP.mock.calls + expect(statusCall[0]).toBe('https://queue.fal.run/fal-ai/kling-video/requests/req-1/status') + // `/response` is not a GET route on queue.fal.run — it must be stripped. + expect(resultCall[0]).toBe('https://queue.fal.run/fal-ai/kling-video/requests/req-1') + expect(downloadCall[0]).toBe('https://cdn.fal.media/a.mp4') + + expect(statusCall[2].maxResponseBytes).toBe(MAX_FAL_QUEUE_JSON_BYTES) + expect(resultCall[2].maxResponseBytes).toBe(MAX_FAL_QUEUE_JSON_BYTES) + }) + + it('falls back to the constructed multi-segment queue URL when the candidate is off-origin', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + request_id: 'req-2', + status_url: 'https://evil.example.net/steal', + response_url: 'https://evil.example.net/steal', + }), + { status: 200 } + ) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + .mockResolvedValueOnce(videoResponse()) + + const response = await POST(createMockRequest('POST', baseBody)) + expect(response.status).toBe(200) + + // `fal-ai/kling-video/v3/pro/text-to-video` polls under the app id only. + expect(mockSecureFetchWithPinnedIP.mock.calls.slice(0, 2).map(([url]) => url)).toEqual([ + 'https://queue.fal.run/fal-ai/kling-video/requests/req-2/status', + 'https://queue.fal.run/fal-ai/kling-video/requests/req-2', + ]) + }) +}) + +/** + * Runway, Veo, Luma and MiniMax all download the finished asset through + * `downloadVideoFromUrl` (the SSRF-guarded client), each with its own label and + * error prefix. These cover that plumbing per provider. + */ +describe('POST /api/tools/video (provider download paths)', () => { + const fetchMock = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + vi.stubGlobal('fetch', fetchMock) + hybridAuthMockFns.mockCheckInternalAuth.mockResolvedValue({ + success: true, + userId: 'user-1', + authType: 'internal_jwt', + }) + mockValidateUrlWithDNS.mockResolvedValue({ isValid: true, resolvedIP: '8.8.8.8' }) + mockUploadFile.mockResolvedValue({ path: '/api/files/serve/video.mp4' }) + }) + + function apiResponse(payload: unknown) { + return new Response(JSON.stringify(payload), { status: 200 }) + } + + function veoFetches(uri: string) { + fetchMock.mockResolvedValueOnce(apiResponse({ name: 'operations/op-1' })).mockResolvedValueOnce( + apiResponse({ + done: true, + response: { generateVideoResponse: { generatedSamples: [{ video: { uri } }] } }, + }) + ) + } + + it('downloads the Runway asset through the guarded client', async () => { + fetchMock + .mockResolvedValueOnce(apiResponse({ id: 'task-1' })) + .mockResolvedValueOnce( + apiResponse({ status: 'SUCCEEDED', output: ['https://cdn.runwayml.test/a.mp4'] }) + ) + mockSecureFetchWithPinnedIP.mockResolvedValueOnce(videoResponse()) + + const response = await POST( + createMockRequest('POST', { + provider: 'runway', + apiKey: 'runway-key', + model: 'gen-4', + prompt: 'a cat riding a bike', + }) + ) + + expect(response.status).toBe(200) + expect(mockSecureFetchWithPinnedIP).toHaveBeenCalledTimes(1) + expect(mockSecureFetchWithPinnedIP.mock.calls[0][0]).toBe('https://cdn.runwayml.test/a.mp4') + expect(mockSecureFetchWithPinnedIP.mock.calls[0][2].headers).toBeUndefined() + }) + + it('surfaces the default download error prefix when the Runway asset fetch fails', async () => { + fetchMock + .mockResolvedValueOnce(apiResponse({ id: 'task-1' })) + .mockResolvedValueOnce( + apiResponse({ status: 'SUCCEEDED', output: ['https://cdn.runwayml.test/a.mp4'] }) + ) + mockSecureFetchWithPinnedIP.mockResolvedValueOnce(errorResponse(401)) + + const response = await POST( + createMockRequest('POST', { + provider: 'runway', + apiKey: 'runway-key', + model: 'gen-4', + prompt: 'a cat riding a bike', + }) + ) + + expect(response.status).toBe(500) + expect((await response.json()).error).toBe('Failed to download video: 401') + }) + + it('downloads the Luma asset through the guarded client', async () => { + fetchMock + .mockResolvedValueOnce(apiResponse({ id: 'gen-1' })) + .mockResolvedValueOnce( + apiResponse({ state: 'completed', assets: { video: 'https://cdn.lumalabs.test/a.mp4' } }) + ) + mockSecureFetchWithPinnedIP.mockResolvedValueOnce(videoResponse()) + + const response = await POST( + createMockRequest('POST', { + provider: 'luma', + apiKey: 'luma-key', + model: 'ray-2', + prompt: 'a cat riding a bike', + }) + ) + + expect(response.status).toBe(200) + expect(mockSecureFetchWithPinnedIP.mock.calls[0][0]).toBe('https://cdn.lumalabs.test/a.mp4') + expect(mockSecureFetchWithPinnedIP.mock.calls[0][2].headers).toBeUndefined() + }) + + it('keeps the MiniMax-specific download error prefix', async () => { + fetchMock + .mockResolvedValueOnce(apiResponse({ base_resp: { status_code: 0 }, task_id: 'task-1' })) + .mockResolvedValueOnce( + apiResponse({ base_resp: { status_code: 0 }, status: 'Success', file_id: 'file-1' }) + ) + .mockResolvedValueOnce( + apiResponse({ file: { download_url: 'https://cdn.minimax.test/a.mp4' } }) + ) + mockSecureFetchWithPinnedIP.mockResolvedValueOnce(errorResponse(401)) + + const response = await POST( + createMockRequest('POST', { + provider: 'minimax', + apiKey: 'minimax-key', + model: 'hailuo-2.3', + prompt: 'a cat riding a bike', + }) + ) + + expect(response.status).toBe(500) + expect((await response.json()).error).toBe('Failed to download video from URL: 401') + }) + + it('downloads the MiniMax asset through the guarded client', async () => { + fetchMock + .mockResolvedValueOnce(apiResponse({ base_resp: { status_code: 0 }, task_id: 'task-1' })) + .mockResolvedValueOnce( + apiResponse({ base_resp: { status_code: 0 }, status: 'Success', file_id: 'file-1' }) + ) + .mockResolvedValueOnce( + apiResponse({ file: { download_url: 'https://cdn.minimax.test/a.mp4' } }) + ) + mockSecureFetchWithPinnedIP.mockResolvedValueOnce(videoResponse()) + + const response = await POST( + createMockRequest('POST', { + provider: 'minimax', + apiKey: 'minimax-key', + model: 'hailuo-2.3', + prompt: 'a cat riding a bike', + }) + ) + + expect(response.status).toBe(200) + expect(mockSecureFetchWithPinnedIP.mock.calls[0][0]).toBe('https://cdn.minimax.test/a.mp4') + }) + + it('attaches the Veo API key only for a genuine https Google API host', async () => { + veoFetches('https://generativelanguage.googleapis.com/v1beta/files/a:download') + mockSecureFetchWithPinnedIP.mockResolvedValueOnce(videoResponse()) + + const response = await POST( + createMockRequest('POST', { + provider: 'veo', + apiKey: 'veo-key', + model: 'veo-3', + prompt: 'a cat riding a bike', + }) + ) + + expect(response.status).toBe(200) + expect(mockSecureFetchWithPinnedIP.mock.calls[0][2].headers).toEqual({ + 'x-goog-api-key': 'veo-key', + }) + }) + + it.each([ + ['a suffix-spoofed host', 'https://evil.googleapis.com.attacker.test/a.mp4'], + ['a prefix-spoofed host', 'https://xgoogleapis.com/a.mp4'], + ['plaintext http', 'http://generativelanguage.googleapis.com/a.mp4'], + ])('withholds the Veo API key for %s', async (_label, uri) => { + veoFetches(uri) + mockSecureFetchWithPinnedIP.mockResolvedValueOnce(videoResponse()) + + const response = await POST( + createMockRequest('POST', { + provider: 'veo', + apiKey: 'veo-key', + model: 'veo-3', + prompt: 'a cat riding a bike', + }) + ) + + expect(response.status).toBe(200) + expect(mockSecureFetchWithPinnedIP.mock.calls[0][0]).toBe(uri) + expect(mockSecureFetchWithPinnedIP.mock.calls[0][2].headers).toBeUndefined() + }) +}) diff --git a/apps/sim/app/api/tools/video/route.ts b/apps/sim/app/api/tools/video/route.ts index b98d4fed2c5..e3a2553d815 100644 --- a/apps/sim/app/api/tools/video/route.ts +++ b/apps/sim/app/api/tools/video/route.ts @@ -8,6 +8,10 @@ import { videoProviders, videoToolContract } from '@/lib/api/contracts/tools/med import { getValidationErrorMessage, parseRequest, validationErrorResponse } from '@/lib/api/server' import { checkInternalAuth } from '@/lib/auth/hybrid' import { getMaxExecutionTimeout } from '@/lib/core/execution-limits' +import { + secureFetchWithPinnedIP, + validateUrlWithDNS, +} from '@/lib/core/security/input-validation.server' import { assertKnownSizeWithinLimit, DEFAULT_MAX_ERROR_BODY_BYTES, @@ -18,6 +22,12 @@ import { readResponseToBufferWithLimit, } from '@/lib/core/utils/stream-limits' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' +import { + buildFalQueueUrl, + FAL_QUEUE_ORIGIN, + MAX_FAL_QUEUE_JSON_BYTES, + resolveFalQueueUrl, +} from '@/lib/media/falai' import { type FalAICostMetadata, getFalAICostMetadata } from '@/lib/tools/falai-pricing' import { downloadFileFromStorage } from '@/lib/uploads/utils/file-utils.server' import { assertToolFileAccess } from '@/app/api/files/authorization' @@ -36,7 +46,23 @@ export const dynamic = 'force-dynamic' */ export const maxDuration = 5400 -async function readVideoResponseBuffer(response: Response, label: string): Promise { +/** + * Structural shape shared by `fetch`'s `Response` and the SSRF-guarded + * `SecureFetchResponse`, so the body readers below accept either. + */ +interface ReadableHttpResponse { + ok: boolean + status: number + headers: { get(name: string): string | null } + body?: ReadableStream | null + arrayBuffer?: () => Promise + text?: () => Promise +} + +async function readVideoResponseBuffer( + response: ReadableHttpResponse, + label: string +): Promise { return readResponseToBufferWithLimit(response, { maxBytes: MAX_VIDEO_OUTPUT_BYTES, label, @@ -44,22 +70,66 @@ async function readVideoResponseBuffer(response: Response, label: string): Promi } async function readVideoJson>( - response: Response, - label: string + response: ReadableHttpResponse, + label: string, + maxBytes: number = MAX_VIDEO_JSON_BYTES ): Promise { return readResponseJsonWithLimit(response, { - maxBytes: MAX_VIDEO_JSON_BYTES, + maxBytes, label, }) } -async function readVideoErrorText(response: Response, label: string): Promise { +async function readVideoErrorText(response: ReadableHttpResponse, label: string): Promise { return readResponseTextWithLimit(response, { maxBytes: DEFAULT_MAX_ERROR_BODY_BYTES, label, }).catch(() => '') } +/** + * Fetches a provider-supplied URL through the SSRF-guarded client. These URLs come out of + * provider response bodies, so they are untrusted input. + */ +async function secureGetFromUntrustedUrl( + url: string, + paramName: string, + options: { headers?: Record; maxResponseBytes: number } +) { + const validation = await validateUrlWithDNS(url, paramName) + if (!validation.isValid || !validation.resolvedIP) { + throw new Error(validation.error || `${paramName} failed validation`) + } + return secureFetchWithPinnedIP(url, validation.resolvedIP, { + method: 'GET', + headers: options.headers, + maxResponseBytes: options.maxResponseBytes, + }) +} + +/** + * Downloads a provider-supplied video URL through the SSRF-guarded client. + * `errorPrefix` overrides the thrown message so each provider keeps the exact + * failure string runbooks and log filters key on. + */ +async function downloadVideoFromUrl( + url: string, + label: string, + options?: { headers?: Record; errorPrefix?: string } +): Promise { + const response = await secureGetFromUntrustedUrl(url, 'videoUrl', { + headers: options?.headers, + maxResponseBytes: MAX_VIDEO_OUTPUT_BYTES, + }) + + if (!response.ok) { + await readVideoErrorText(response, `${label} error response`) + throw new Error(`${options?.errorPrefix ?? 'Failed to download video'}: ${response.status}`) + } + + return readVideoResponseBuffer(response, `${label} response`) +} + export const POST = withRouteHandler(async (request: NextRequest) => { const requestId = generateId() logger.info(`[${requestId}] Video generation request started`) @@ -461,14 +531,8 @@ async function generateWithRunway( throw new Error('No video URL in response') } - const videoResponse = await fetch(videoUrl) - if (!videoResponse.ok) { - await readVideoErrorText(videoResponse, 'Runway video error response') - throw new Error(`Failed to download video: ${videoResponse.status}`) - } - return { - buffer: await readVideoResponseBuffer(videoResponse, 'Runway video response'), + buffer: await downloadVideoFromUrl(videoUrl, 'Runway video'), width: dimensions.width, height: dimensions.height, jobId: taskId, @@ -486,6 +550,30 @@ async function generateWithRunway( throw new Error('Runway generation timed out') } +/** Host of a provider-supplied URI for logging, without leaking the path or query. */ +function safeHostname(url: string): string { + try { + return new URL(url).host + } catch { + return 'unparseable' + } +} + +/** + * True when a provider-supplied URI is served by Google. The Veo download URI comes out of + * the operation status body, so the `x-goog-api-key` credential is only attached when the + * target really is a Google host — a hostile or spoofed URI never receives the key. + */ +function isGoogleApiHost(url: string): boolean { + try { + const { protocol, hostname } = new URL(url) + if (protocol !== 'https:') return false + return hostname === 'googleapis.com' || hostname.endsWith('.googleapis.com') + } catch { + return false + } +} + async function generateWithVeo( apiKey: string, model: string, @@ -583,19 +671,18 @@ async function generateWithVeo( throw new Error('No video URI in response') } - const videoResponse = await fetch(videoUri, { - headers: { - 'x-goog-api-key': apiKey, - }, - }) - - if (!videoResponse.ok) { - await readVideoErrorText(videoResponse, 'Veo video error response') - throw new Error(`Failed to download video: ${videoResponse.status}`) + const isGoogleHosted = isGoogleApiHost(videoUri) + if (!isGoogleHosted) { + logger.warn( + `[${requestId}] Veo download URI is not a Google API host; sending the request unauthenticated. A 401 here means the URI host is wrong, not the API key.`, + { host: safeHostname(videoUri) } + ) } return { - buffer: await readVideoResponseBuffer(videoResponse, 'Veo video response'), + buffer: await downloadVideoFromUrl(videoUri, 'Veo video', { + headers: isGoogleHosted ? { 'x-goog-api-key': apiKey } : undefined, + }), width: dimensions.width, height: dimensions.height, jobId: operationName, @@ -697,14 +784,8 @@ async function generateWithLuma( throw new Error('No video URL in response') } - const videoResponse = await fetch(videoUrl) - if (!videoResponse.ok) { - await readVideoErrorText(videoResponse, 'Luma video error response') - throw new Error(`Failed to download video: ${videoResponse.status}`) - } - return { - buffer: await readVideoResponseBuffer(videoResponse, 'Luma video response'), + buffer: await downloadVideoFromUrl(videoUrl, 'Luma video'), width: dimensions.width, height: dimensions.height, jobId: generationId, @@ -859,15 +940,10 @@ async function generateWithMiniMax( throw new Error('No download URL in file response') } - // Download the actual video file - const videoResponse = await fetch(videoUrl) - if (!videoResponse.ok) { - await readVideoErrorText(videoResponse, 'MiniMax video error response') - throw new Error(`Failed to download video from URL: ${videoResponse.status}`) - } - return { - buffer: await readVideoResponseBuffer(videoResponse, 'MiniMax video response'), + buffer: await downloadVideoFromUrl(videoUrl, 'MiniMax video', { + errorPrefix: 'Failed to download video from URL', + }), width: dimensions.width, height: dimensions.height, jobId: taskId, @@ -1160,12 +1236,20 @@ function getFalAIErrorMessage(error: unknown): string { return 'Unknown error' } -function buildFalAIQueueUrl( - endpoint: string, - requestId: string, - path: 'response' | 'status' -): string { - return `https://queue.fal.run/${endpoint}/requests/${requestId}/${path}` +/** + * Requests a Fal.ai queue URL through the SSRF-guarded client. Invoked on every poll so + * the provider-supplied URL is revalidated each time rather than trusted once. + */ +async function fetchFalAIQueue(url: string, apiKey: string) { + return secureGetFromUntrustedUrl(url, 'falQueueUrl', { + headers: { Authorization: `Key ${apiKey}` }, + maxResponseBytes: MAX_FAL_QUEUE_JSON_BYTES, + }) +} + +/** Fal.ai queue envelopes share the cap enforced by `runFalQueue`, not the generic video cap. */ +async function readFalQueueJson(response: ReadableHttpResponse, label: string): Promise { + return readVideoJson(response, label, MAX_FAL_QUEUE_JSON_BYTES) } async function generateWithFalAI( @@ -1218,7 +1302,7 @@ async function generateWithFalAI( requestBody.generate_audio = generateAudio } - const createResponse = await fetch(`https://queue.fal.run/${modelConfig.endpoint}`, { + const createResponse = await fetch(`${FAL_QUEUE_ORIGIN}/${modelConfig.endpoint}`, { method: 'POST', headers: { Authorization: `Key ${apiKey}`, @@ -1232,7 +1316,7 @@ async function generateWithFalAI( throw new Error(`Fal.ai API error: ${createResponse.status} - ${error}`) } - const createData = await readVideoJson(createResponse, 'Fal.ai queue response') + const createData = await readFalQueueJson(createResponse, 'Fal.ai queue response') if (!isRecordLike(createData)) { throw new Error('Invalid Fal.ai queue response') } @@ -1242,12 +1326,14 @@ async function generateWithFalAI( throw new Error('Fal.ai queue response missing request_id') } - const statusUrl = - getStringProperty(createData, 'status_url') || - buildFalAIQueueUrl(modelConfig.endpoint, requestIdFal, 'status') - const responseUrl = - getStringProperty(createData, 'response_url') || - buildFalAIQueueUrl(modelConfig.endpoint, requestIdFal, 'response') + const statusUrl = resolveFalQueueUrl( + getStringProperty(createData, 'status_url'), + buildFalQueueUrl(modelConfig.endpoint, requestIdFal, 'status') + ) + const responseUrl = resolveFalQueueUrl( + getStringProperty(createData, 'response_url'), + buildFalQueueUrl(modelConfig.endpoint, requestIdFal, 'result') + ) logger.info(`[${requestId}] Fal.ai request created: ${requestIdFal}`) @@ -1258,18 +1344,14 @@ async function generateWithFalAI( while (attempts < maxAttempts) { await sleep(pollIntervalMs) - const statusResponse = await fetch(statusUrl, { - headers: { - Authorization: `Key ${apiKey}`, - }, - }) + const statusResponse = await fetchFalAIQueue(statusUrl, apiKey) if (!statusResponse.ok) { await readVideoErrorText(statusResponse, 'Fal.ai status error response') throw new Error(`Fal.ai status check failed: ${statusResponse.status}`) } - const statusData = await readVideoJson(statusResponse, 'Fal.ai status response') + const statusData = await readFalQueueJson(statusResponse, 'Fal.ai status response') if (!isRecordLike(statusData)) { throw new Error('Invalid Fal.ai status response') } @@ -1282,13 +1364,9 @@ async function generateWithFalAI( logger.info(`[${requestId}] Fal.ai generation completed after ${attempts * 5}s`) - const resultResponse = await fetch( - getStringProperty(statusData, 'response_url') || responseUrl, - { - headers: { - Authorization: `Key ${apiKey}`, - }, - } + const resultResponse = await fetchFalAIQueue( + resolveFalQueueUrl(getStringProperty(statusData, 'response_url'), responseUrl), + apiKey ) if (!resultResponse.ok) { @@ -1296,7 +1374,7 @@ async function generateWithFalAI( throw new Error(`Failed to fetch result: ${resultResponse.status}`) } - const resultData = await readVideoJson(resultResponse, 'Fal.ai result response') + const resultData = await readFalQueueJson(resultResponse, 'Fal.ai result response') if (!isRecordLike(resultData)) { throw new Error('Invalid Fal.ai result response') } @@ -1309,11 +1387,7 @@ async function generateWithFalAI( throw new Error('No video URL in response') } - const videoResponse = await fetch(videoUrl) - if (!videoResponse.ok) { - await readVideoErrorText(videoResponse, 'Fal.ai video error response') - throw new Error(`Failed to download video: ${videoResponse.status}`) - } + const videoBuffer = await downloadVideoFromUrl(videoUrl, 'Fal.ai video') let width = getNumberProperty(videoOutput, 'width') || 1920 let height = getNumberProperty(videoOutput, 'height') || 1080 @@ -1325,7 +1399,7 @@ async function generateWithFalAI( } return { - buffer: await readVideoResponseBuffer(videoResponse, 'Fal.ai video response'), + buffer: videoBuffer, width, height, jobId: requestIdFal, diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/file-viewer.tsx b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/file-viewer.tsx index 9984e16b0df..44114a8bef4 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/file-viewer.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/file-viewer.tsx @@ -16,7 +16,6 @@ import { DocxPreview } from './docx-preview' import { resolveFileCategory } from './file-category' import { ImagePreview } from './image-preview' import type { PdfDocumentSource } from './pdf-viewer' -import { PptxPreview } from './pptx-preview' import { PreviewPanel, resolvePreviewType } from './preview-panel' import { PREVIEW_LOADING_OVERLAY, @@ -33,6 +32,23 @@ const PdfViewerCore = dynamic(() => import('./pdf-viewer').then((m) => m.PdfView ssr: false, }) +/** + * Lazy so `echarts` (~330 KB gzip, reached via `pptx-sandbox-host` → + * `lib/pptx-renderer/renderer/chart-renderer`) stays out of every bundle that + * statically imports this viewer - the Files page, the Home view and the public + * share page - instead of only the visitors who open a PowerPoint file. The + * fallback matches the frame `./pptx-preview` renders while it fetches, so the chunk load + * and the binary fetch look like one continuous loading state. Rendered inside a + * {@link PreviewErrorBoundary} so a rejected chunk load degrades to the preview + * fallback — which offers a page reload, the only way to refetch a chunk whose + * rejection the module system has cached — instead of unwinding to the route + * error boundary. + */ +const PptxPreview = dynamic(() => import('./pptx-preview').then((m) => m.PptxPreview), { + ssr: false, + loading: () => , +}) + const RichMarkdownEditor = dynamic( () => import('./rich-markdown-editor/rich-markdown-editor').then((m) => m.RichMarkdownEditor), { ssr: false, loading: () => } @@ -229,7 +245,11 @@ function FileViewerContent({ } if (category === 'pptx-previewable') { - return + return ( + + + + ) } if (category === 'xlsx-previewable') { diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared.test.tsx b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared.test.tsx new file mode 100644 index 00000000000..8367b9cdc63 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared.test.tsx @@ -0,0 +1,168 @@ +/** + * @vitest-environment jsdom + * + * `PreviewErrorBoundary` must contain a *rejected dynamic import* — the failure mode + * introduced by lazy-loading the PDF and PowerPoint renderers. Without it a chunk-load + * failure unwinds to the route-level `error.tsx` and blanks the whole Files page, the + * Home resource panel and the public share page, all of which statically import the viewer. + * + * It must also recover in place: the boundary's error state only ever cleared via remount, + * so a tripped boundary stayed on the fallback until the user navigated to another file + * and back. + */ +import { act, lazy, Suspense } from 'react' +import { sleep } from '@sim/utils/helpers' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + PreviewError, + PreviewErrorBoundary, +} from '@/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared' + +let container: HTMLDivElement | null = null +let root: Root | null = null + +afterEach(() => { + if (root) act(() => root?.unmount()) + container?.remove() + container = null + root = null + vi.restoreAllMocks() +}) + +async function render(node: React.ReactNode) { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + await act(async () => { + root?.render(node) + }) +} + +function actionLabels(): string[] { + return Array.from(container?.querySelectorAll('button') ?? []).map((b) => b.textContent ?? '') +} + +async function clickAction(label: string) { + const button = Array.from(container?.querySelectorAll('button') ?? []).find( + (b) => b.textContent === label + ) + if (!button) + throw new Error(`No "${label}" action rendered (found: ${actionLabels().join(', ')})`) + await act(async () => { + button.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) +} + +describe('PreviewErrorBoundary', () => { + it('renders the preview fallback when a lazy chunk fails to load', async () => { + const Broken = lazy(() => Promise.reject(new Error('Loading chunk 4821 failed'))) + + await render( + + loading}> + + + + ) + + expect(container?.textContent).toContain('Failed to preview PowerPoint') + expect(container?.textContent).toContain('Loading chunk 4821 failed') + }) + + it('offers a page reload — not a retry — for a cached chunk rejection', async () => { + const Broken = lazy(() => Promise.reject(new Error('Loading chunk 4821 failed'))) + + await render( + + loading}> + + + + ) + + expect(actionLabels()).toEqual(['Reload page']) + }) + + it('offers a page reload for a failed CSS chunk, whose message omits "Loading chunk"', async () => { + const Broken = lazy(() => Promise.reject(new Error('Loading CSS chunk 4821 failed'))) + + await render( + + loading}> + + + + ) + + expect(actionLabels()).toEqual(['Reload page']) + }) + + it('renders children when nothing throws', async () => { + await render( + + preview + + ) + + expect(container?.textContent).toBe('preview') + }) + + it('recovers in place when the retried render succeeds', async () => { + let shouldThrow = true + function Flaky() { + if (shouldThrow) throw new Error('transient render failure') + return preview + } + + await render( + + + + ) + expect(container?.textContent).toContain('transient render failure') + + shouldThrow = false + await clickAction('Try again') + + expect(container?.textContent).toBe('preview') + }) + + it('settles back on the fallback — never loops — when the retried render throws again', async () => { + const renderSpy = vi.fn() + function AlwaysThrows(): never { + renderSpy() + throw new Error('still broken') + } + + await render( + + + + ) + const attemptsBeforeRetry = renderSpy.mock.calls.length + + await clickAction('Try again') + const attemptsAfterRetry = renderSpy.mock.calls.length + + expect(attemptsAfterRetry).toBeGreaterThan(attemptsBeforeRetry) + + await act(async () => { + await sleep(1) + }) + + expect(renderSpy.mock.calls.length).toBe(attemptsAfterRetry) + expect(container?.textContent).toContain('Failed to preview PDF') + expect(container?.textContent).toContain('still broken') + expect(actionLabels()).toEqual(['Try again']) + }) +}) + +describe('PreviewError', () => { + it('renders no action when none is supplied', async () => { + await render() + + expect(container?.textContent).toContain('Failed to preview CSV') + expect(container?.querySelectorAll('button')).toHaveLength(0) + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared.tsx b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared.tsx index 041a0225074..f35a331aebe 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/preview-shared.tsx @@ -1,23 +1,61 @@ 'use client' -import { Component, type ErrorInfo, type ReactNode } from 'react' -import { cn } from '@sim/emcn' +import { Component, type ErrorInfo, Fragment, type ReactNode } from 'react' +import { Chip, cn } from '@sim/emcn' import { Loader } from '@sim/emcn/icons' import { createLogger } from '@sim/logger' const logger = createLogger('FilePreview') -export function PreviewError({ label, error }: { label: string; error: string }) { +interface PreviewErrorAction { + label: string + onClick: () => void +} + +interface PreviewErrorProps { + /** Format label shown in the message, e.g. "PDF". */ + label: string + error: string + /** Recovery affordance. Omitted for fallbacks with nothing to retry. */ + action?: PreviewErrorAction +} + +export function PreviewError({ label, error, action }: PreviewErrorProps) { return (

Failed to preview {label}

{error}

+ {action ? ( + + {action.label} + + ) : null}
) } +/** + * webpack's chunk-load failure messages. The JS form is `Loading chunk 5 failed` + * and the stylesheet form is `Loading CSS chunk 5 failed`, so a plain + * `Loading chunk` substring test misses every CSS chunk 404. + */ +const CHUNK_LOAD_MESSAGE = /Loading (?:CSS )?chunk/ + +/** + * A `next/dynamic` / `React.lazy` chunk fetch that rejected. The module system + * caches the rejection on the lazy component itself, so re-rendering it throws + * the same error synchronously — only a fresh page load can refetch the chunk. + */ +function isChunkLoadError(error: Error | undefined): boolean { + if (!error) return false + if (error.name === 'ChunkLoadError') return true + return ( + CHUNK_LOAD_MESSAGE.test(error.message) || error.message.includes('dynamically imported module') + ) +} + interface PreviewErrorBoundaryProps { /** Format label shown in the fallback, e.g. "PDF". */ label: string @@ -27,6 +65,8 @@ interface PreviewErrorBoundaryProps { interface PreviewErrorBoundaryState { hasError: boolean error?: Error + /** Bumped by "Try again" so the retried subtree remounts rather than resuming. */ + attempt: number } /** @@ -35,9 +75,15 @@ interface PreviewErrorBoundaryState { * PreviewError fallback instead of unwinding to the route-level error boundary * and replacing the whole workspace view. * - * Callers must `key` this boundary by the identity of the rendered content - * (e.g. file id + data version) — the error state resets only via remount, so - * keying the child alone would leave a tripped boundary stuck on the fallback. + * The fallback recovers in place, so a tripped boundary is never stuck: + * - A render-time crash offers "Try again", which clears the error and remounts + * the subtree. If the cause is still there the child throws once more and the + * fallback returns — bounded, never a retry loop. + * - A rejected chunk load is cached by the module system and cannot be retried + * by re-rendering, so it offers "Reload page" instead. + * + * Keying the boundary by content identity (e.g. file id + data version) is still + * worthwhile so a *different* file starts from a clean boundary. */ export class PreviewErrorBoundary extends Component< PreviewErrorBoundaryProps, @@ -45,9 +91,12 @@ export class PreviewErrorBoundary extends Component< > { public state: PreviewErrorBoundaryState = { hasError: false, + attempt: 0, } - public static getDerivedStateFromError(error: Error): PreviewErrorBoundaryState { + public static getDerivedStateFromError( + error: Error + ): Pick { return { hasError: true, error } } @@ -59,17 +108,36 @@ export class PreviewErrorBoundary extends Component< }) } + private readonly handleRetry = () => { + this.setState((previous) => ({ + hasError: false, + error: undefined, + attempt: previous.attempt + 1, + })) + } + + private readonly handleReload = () => { + window.location.reload() + } + public render() { - if (this.state.hasError) { + const { attempt, error, hasError } = this.state + + if (hasError) { return ( ) } - return this.props.children + return {this.props.children} } } diff --git a/apps/sim/executor/orchestrators/loop.test.ts b/apps/sim/executor/orchestrators/loop.test.ts index 9180fc2bd8d..f03f4a8f598 100644 --- a/apps/sim/executor/orchestrators/loop.test.ts +++ b/apps/sim/executor/orchestrators/loop.test.ts @@ -308,6 +308,76 @@ describe('LoopOrchestrator', () => { ) }) + describe('while condition serialization', () => { + /** + * Runs one while-loop condition evaluation with `resolved` standing in for the value a + * `` reference resolves to, and returns the JavaScript handed to the VM. + */ + async function evaluatedCode(resolved: unknown): Promise { + const resolver = { resolveSingleReference: vi.fn().mockResolvedValue(resolved) } + mockExecuteInIsolatedVM.mockResolvedValueOnce({ result: false }) + const orchestrator = new LoopOrchestrator( + { loopConfigs: new Map(), parallelConfigs: new Map(), nodes: new Map() } as any, + createState(), + resolver as any + ) + const ctx = createContext({ + iteration: 0, + currentIterationOutputs: new Map(), + allIterationOutputs: [], + loopType: 'while', + condition: '', + }) + + await orchestrator.evaluateInitialCondition(ctx, 'loop-1') + + return mockExecuteInIsolatedVM.mock.calls.at(-1)?.[0].code + } + + it('inlines an ordinary string as a quoted literal', async () => { + expect(await evaluatedCode('hello')).toBe('return Boolean("hello")') + }) + + it('keeps numbers, booleans and null unquoted', async () => { + expect(await evaluatedCode(42)).toBe('return Boolean(42)') + expect(await evaluatedCode(true)).toBe('return Boolean(true)') + expect(await evaluatedCode(null)).toBe('return Boolean(null)') + }) + + it('still folds a boolean-valued string to a bare boolean', async () => { + expect(await evaluatedCode('true')).toBe('return Boolean(true)') + expect(await evaluatedCode(' FALSE ')).toBe('return Boolean(false)') + }) + + it('serializes objects and arrays', async () => { + expect(await evaluatedCode({ a: 1 })).toBe('return Boolean({"a":1})') + expect(await evaluatedCode(['x'])).toBe('return Boolean(["x"])') + }) + + it('escapes a quote instead of closing the literal', async () => { + const code = await evaluatedCode('he said "hi"') + expect(code).toBe('return Boolean("he said \\"hi\\"")') + expect(() => new Function(code)()).not.toThrow() + expect(new Function(code)()).toBe(true) + }) + + it('escapes a newline instead of breaking the statement', async () => { + const code = await evaluatedCode('line1\nline2') + expect(code).toBe('return Boolean("line1\\nline2")') + expect(code).not.toContain('\n') + expect(new Function(code)()).toBe(true) + }) + + it('treats an injection payload as data, not code', async () => { + // A block output an attacker controls — an API response, webhook body, or model + // completion. Wrapped in quotes by hand this closed the literal and ran. + const code = await evaluatedCode('" + (globalThis.__pwned = true) + "') + expect(code).toBe('return Boolean("\\" + (globalThis.__pwned = true) + \\"")') + new Function(code)() + expect((globalThis as Record).__pwned).toBeUndefined() + }) + }) + it('exits doWhile loops when the configured iteration cap is reached', async () => { const { orchestrator } = createOrchestrator() const ctx = createContext({ diff --git a/apps/sim/executor/orchestrators/loop.ts b/apps/sim/executor/orchestrators/loop.ts index 913de932915..ccba48f98f2 100644 --- a/apps/sim/executor/orchestrators/loop.ts +++ b/apps/sim/executor/orchestrators/loop.ts @@ -732,7 +732,9 @@ export class LoopOrchestrator { if (lower === 'true' || lower === 'false') { return lower } - return `"${resolved}"` + // Serialized rather than hand-quoted: the value is a block output, so a `"` or + // newline would close the literal early and run as code in the condition VM. + return JSON.stringify(resolved) } return JSON.stringify(resolved) } diff --git a/apps/sim/lib/chunkers/json-yaml-chunker.test.ts b/apps/sim/lib/chunkers/json-yaml-chunker.test.ts index 8ac0eafada8..3d542ac4c94 100644 --- a/apps/sim/lib/chunkers/json-yaml-chunker.test.ts +++ b/apps/sim/lib/chunkers/json-yaml-chunker.test.ts @@ -3,8 +3,26 @@ */ import { describe, expect, it, vi } from 'vitest' +import { YamlComplexityError } from '@/lib/file-parsers/yaml-parser' import { JsonYamlChunker } from './json-yaml-chunker' +/** + * Build a chained alias-expansion ("billion laughs") YAML bomb: each level is + * an array that references the previous level `width` times, so the expanded + * node count grows as `width ^ levels` while the source stays tiny. + */ +function buildAliasBomb(levels: number, width: number): string { + const lines: string[] = [`l0: &l0 [${Array(width).fill('"x"').join(',')}]`] + for (let i = 1; i <= levels; i++) { + const refs = Array(width) + .fill(`*l${i - 1}`) + .join(',') + lines.push(`l${i}: &l${i} [${refs}]`) + } + lines.push(`root: [${Array(width).fill(`*l${levels}`).join(',')}]`) + return lines.join('\n') +} + vi.mock('@/lib/tokenization', () => ({ getAccurateTokenCount: (text: string) => Math.ceil(text.length / 4), })) @@ -372,4 +390,42 @@ server: expect(chunks[0].text.length).toBeGreaterThan(0) }) }) + + describe('YAML alias-expansion guard', () => { + it.concurrent('should reject an alias-expansion bomb instead of chunking it', async () => { + const bomb = buildAliasBomb(9, 10) + expect(Buffer.byteLength(bomb)).toBeLessThan(2048) + + const chunker = new JsonYamlChunker({ chunkSize: 100 }) + await expect(chunker.chunk(bomb)).rejects.toBeInstanceOf(YamlComplexityError) + }) + + it.concurrent('should not classify an alias-expansion bomb as structured data', () => { + expect(JsonYamlChunker.isStructuredData(buildAliasBomb(9, 10))).toBe(false) + }) + + it.concurrent('should still chunk an ordinary YAML document', async () => { + const chunker = new JsonYamlChunker({ chunkSize: 100, minCharactersPerChunk: 1 }) + const chunks = await chunker.chunk('name: sim\nnested:\n key: value\nlist:\n - a\n - b\n') + + expect(chunks.length).toBeGreaterThan(0) + expect(chunks.map((c) => c.text).join('\n')).toContain('sim') + }) + + it.concurrent('should still chunk YAML that uses benign aliases', async () => { + const chunker = new JsonYamlChunker({ chunkSize: 100, minCharactersPerChunk: 1 }) + const chunks = await chunker.chunk('base: &base\n region: us\nprod: *base\nstaging: *base\n') + + expect(chunks.length).toBeGreaterThan(0) + expect(chunks.map((c) => c.text).join('\n')).toContain('us') + }) + + it.concurrent('should still chunk ordinary JSON without touching the YAML path', async () => { + const chunker = new JsonYamlChunker({ chunkSize: 100, minCharactersPerChunk: 1 }) + const chunks = await chunker.chunk(JSON.stringify({ name: 'sim', values: [1, 2, 3] })) + + expect(chunks.length).toBeGreaterThan(0) + expect(chunks.map((c) => c.text).join('\n')).toContain('sim') + }) + }) }) diff --git a/apps/sim/lib/chunkers/json-yaml-chunker.ts b/apps/sim/lib/chunkers/json-yaml-chunker.ts index d18cd0859f9..d557ce54451 100644 --- a/apps/sim/lib/chunkers/json-yaml-chunker.ts +++ b/apps/sim/lib/chunkers/json-yaml-chunker.ts @@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger' import * as yaml from 'js-yaml' import type { Chunk, ChunkerOptions } from '@/lib/chunkers/types' import { estimateTokens } from '@/lib/chunkers/utils' +import { assertYamlWithinLimits, isYamlComplexityError } from '@/lib/file-parsers/yaml-parser' const logger = createLogger('JsonYamlChunker') @@ -12,6 +13,21 @@ type JsonArray = JsonValue[] const MAX_DEPTH = 5 +/** + * Parse YAML and validate the expanded document against the shared complexity + * limits before it is handed to the chunker. `js-yaml` has no `maxAliasCount`, + * so an unguarded `yaml.load` on untrusted knowledge-base input is an uncapped + * alias-expansion ("billion laughs") bomb: the parsed value is a compact DAG, + * but the `JSON.stringify` calls throughout chunking expand it into a full tree. + * + * @throws {import('@/lib/file-parsers/yaml-parser').YamlComplexityError} when the expanded document exceeds the limits + */ +function loadGuardedYaml(content: string): JsonValue { + const parsed = yaml.load(content) as JsonValue + assertYamlWithinLimits(parsed) + return parsed +} + export class JsonYamlChunker { private chunkSize: number private minCharactersPerChunk: number @@ -27,7 +43,7 @@ export class JsonYamlChunker { return typeof parsed === 'object' && parsed !== null } catch { try { - const parsed = yaml.load(content) + const parsed = loadGuardedYaml(content) return typeof parsed === 'object' && parsed !== null } catch { return false @@ -35,13 +51,21 @@ export class JsonYamlChunker { } } + /** + * Chunk a JSON or YAML document, rethrowing on an alias-expansion bomb rather + * than falling back to text chunking. In the knowledge-base path this rethrow + * is unreachable: `isStructuredData` already returns false for a bomb, so + * `document-processor` routes the document to `TextChunker`, which chunks the + * bounded raw string and never materializes the expansion. The rethrow covers + * direct callers that skip that check. + */ async chunk(content: string): Promise { try { let data: JsonValue try { data = JSON.parse(content) as JsonValue } catch { - data = yaml.load(content) as JsonValue + data = loadGuardedYaml(content) } const chunks = this.chunkStructuredData(data, [], 0) @@ -50,6 +74,10 @@ export class JsonYamlChunker { return chunks } catch (error) { + if (isYamlComplexityError(error)) { + logger.warn('Rejected YAML document exceeding complexity limits', { error: error.message }) + throw error + } logger.info('JSON parsing failed, falling back to text chunking') return this.chunkAsText(content) } diff --git a/apps/sim/lib/copilot/tools/handlers/vfs.test.ts b/apps/sim/lib/copilot/tools/handlers/vfs.test.ts index 72eea0cefb9..7e29111160e 100644 --- a/apps/sim/lib/copilot/tools/handlers/vfs.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/vfs.test.ts @@ -24,7 +24,7 @@ vi.mock('./upload-file-reader', () => ({ grepChatUpload, })) -import { WorkspaceFileGrepError } from '@/lib/copilot/vfs/operations' +import { GlobPatternError, WorkspaceFileGrepError } from '@/lib/copilot/vfs/operations' import { executeVfsGlob, executeVfsGrep, executeVfsRead } from './vfs' const OVERSIZED_INLINE_CONTENT = 'x'.repeat(TOOL_RESULT_MAX_INLINE_CHARS + 1) @@ -291,6 +291,24 @@ describe('vfs grep workspace-file routing', () => { expect(result.success).toBe(false) expect(result.error).toContain('single workspace file') }) + + it('surfaces an over-cap grep path scope verbatim, like glob', async () => { + const vfs = makeVfs() + const patternError = new GlobPatternError( + 'Glob pattern has too many wildcards (33, limit 32). Narrow the pattern with literal path segments.' + ) + vfs.grep.mockRejectedValue(patternError) + vfs.glob.mockImplementation(() => { + throw patternError + }) + getOrMaterializeVFS.mockResolvedValue(vfs) + + const grepResult = await executeVfsGrep({ pattern: 'x', path: '*'.repeat(33) }, GREP_CTX) + const globResult = await executeVfsGlob({ pattern: '*'.repeat(33) }, GREP_CTX) + + expect(grepResult).toEqual({ success: false, error: patternError.message }) + expect(grepResult).toEqual(globResult) + }) }) describe('vfs uploads are opt-in (like recently-deleted/)', () => { diff --git a/apps/sim/lib/copilot/tools/handlers/vfs.ts b/apps/sim/lib/copilot/tools/handlers/vfs.ts index fd4a515a8af..8783cbae74b 100644 --- a/apps/sim/lib/copilot/tools/handlers/vfs.ts +++ b/apps/sim/lib/copilot/tools/handlers/vfs.ts @@ -5,7 +5,7 @@ import { TOOL_RESULT_MAX_INLINE_CHARS } from '@/lib/copilot/constants' import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types' import { getOrMaterializeVFS } from '@/lib/copilot/vfs' import type { GrepCountEntry, GrepMatch } from '@/lib/copilot/vfs/operations' -import { WorkspaceFileGrepError } from '@/lib/copilot/vfs/operations' +import { GlobPatternError, WorkspaceFileGrepError } from '@/lib/copilot/vfs/operations' import { encodeVfsSegment } from '@/lib/copilot/vfs/path-utils' import { withBlockVisibility } from '@/blocks/visibility/server-context' import { grepChatUpload, listChatUploads, readChatUpload } from './upload-file-reader' @@ -165,6 +165,12 @@ export async function executeVfsGrep( }) return { success: false, error: err.message } } + // An over-cap `path` scope reaches the same matcher glob uses; expected user + // input, not an internal failure. + if (err instanceof GlobPatternError) { + logger.warn('vfs_grep path scope rejected', { pattern, path: rawPath, error: err.message }) + return { success: false, error: err.message } + } logger.error('vfs_grep failed', { pattern, path: rawPath, @@ -203,6 +209,11 @@ export async function executeVfsGlob( logger.debug('vfs_glob result', { pattern, fileCount: files.length }) return { success: true, output: { files } } } catch (err) { + // An over-cap pattern is expected user input, not an internal failure. + if (err instanceof GlobPatternError) { + logger.warn('vfs_glob pattern rejected', { pattern, error: err.message }) + return { success: false, error: err.message } + } logger.error('vfs_glob failed', { pattern, error: toError(err).message, diff --git a/apps/sim/lib/copilot/vfs/document-style.test.ts b/apps/sim/lib/copilot/vfs/document-style.test.ts new file mode 100644 index 00000000000..4f6575a9818 --- /dev/null +++ b/apps/sim/lib/copilot/vfs/document-style.test.ts @@ -0,0 +1,94 @@ +/** + * @vitest-environment node + */ +import JSZip from 'jszip' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { extractDocumentStyle } from '@/lib/copilot/vfs/document-style' + +const CENTRAL_DIRECTORY_HEADER_SIGNATURE = 0x02014b50 + +const THEME_XML = + '' + + '' + + '' + + '' + + '' + + '' + +async function buildDocxArchive(): Promise { + const zip = new JSZip() + zip.file('word/theme/theme1.xml', THEME_XML) + return zip.generateAsync({ type: 'nodebuffer', compression: 'DEFLATE' }) +} + +/** + * Overwrite every central-directory record's declared uncompressed size so the + * archive claims a multi-gigabyte expansion without allocating it — the shape + * of a zip bomb the guard rejects without decompressing anything. + */ +function forgeDeclaredUncompressedSize(zipBuffer: Buffer, declaredBytes: number): Buffer { + const forged = Buffer.from(zipBuffer) + for (let offset = 0; offset + 46 <= forged.length; offset++) { + if (forged.readUInt32LE(offset) === CENTRAL_DIRECTORY_HEADER_SIGNATURE) { + forged.writeUInt32LE(declaredBytes, offset + 24) + } + } + return forged +} + +describe('extractDocumentStyle zip-bomb guard', () => { + /** + * `extractDocumentStyle` swallows every failure and returns `null`, and a + * forged archive is one JSZip rejects on its own — so asserting `null` passes + * with the guard deleted. Spying on the decompressor is what actually proves + * the buffer never reached it. + */ + let loadAsync: ReturnType> + + beforeEach(() => { + loadAsync = vi.spyOn(JSZip, 'loadAsync') + }) + + afterEach(() => { + loadAsync.mockRestore() + }) + + it('refuses an archive whose declared expansion exceeds the limit', async () => { + const bomb = forgeDeclaredUncompressedSize(await buildDocxArchive(), 0xfffffff0) + + await expect(extractDocumentStyle(bomb, 'docx')).resolves.toBeNull() + expect(loadAsync).not.toHaveBeenCalled() + }) + + it('refuses a bomb whose EOCD lies about the entry count behind an empty-EOCD decoy', async () => { + const bomb = forgeDeclaredUncompressedSize(await buildDocxArchive(), 0xfffffff0) + const eocdOffset = bomb.lastIndexOf(Buffer.from([0x50, 0x4b, 0x05, 0x06])) + const cdOffset = bomb.readUInt32LE(eocdOffset + 16) + const decoy = Buffer.alloc(22) + decoy.writeUInt32LE(0x06054b50, 0) + + const realEocd = Buffer.from(bomb.subarray(eocdOffset)) + realEocd.writeUInt16LE(bomb.readUInt16LE(eocdOffset + 10) + 1, 8) + realEocd.writeUInt16LE(bomb.readUInt16LE(eocdOffset + 10) + 1, 10) + realEocd.writeUInt32LE(cdOffset + decoy.length, 16) + + const attack = Buffer.concat([ + bomb.subarray(0, cdOffset), + decoy, + bomb.subarray(cdOffset, eocdOffset), + realEocd, + ]) + + await expect(extractDocumentStyle(attack, 'docx')).resolves.toBeNull() + expect(loadAsync).not.toHaveBeenCalled() + }) + + it('still extracts style from a well-formed archive', async () => { + const summary = await extractDocumentStyle(await buildDocxArchive(), 'docx') + + expect(loadAsync).toHaveBeenCalledTimes(1) + expect(summary).not.toBeNull() + expect(summary?.theme?.fonts.minor).toBe('Calibri') + expect(summary?.theme?.colors.accent1).toBe('4472C4') + }) +}) diff --git a/apps/sim/lib/copilot/vfs/document-style.ts b/apps/sim/lib/copilot/vfs/document-style.ts index 7edbe202fc2..4198b16f0e2 100644 --- a/apps/sim/lib/copilot/vfs/document-style.ts +++ b/apps/sim/lib/copilot/vfs/document-style.ts @@ -1,5 +1,6 @@ import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' +import { assertOoxmlArchiveWithinLimits } from '@/lib/file-parsers/zip-guard' const logger = createLogger('DocumentStyle') @@ -395,6 +396,8 @@ export async function extractDocumentStyle( } try { + assertOoxmlArchiveWithinLimits(buffer) + const JSZip = (await import('jszip')).default const zip = await JSZip.loadAsync(buffer) diff --git a/apps/sim/lib/copilot/vfs/operations.glob-semantics.test.ts b/apps/sim/lib/copilot/vfs/operations.glob-semantics.test.ts new file mode 100644 index 00000000000..617055f372a --- /dev/null +++ b/apps/sim/lib/copilot/vfs/operations.glob-semantics.test.ts @@ -0,0 +1,436 @@ +/** + * @vitest-environment node + */ +import micromatch from 'micromatch' +import { describe, expect, it } from 'vitest' +import { + compileGlobMatcher, + glob, + pathWithinGrepScope, + VFS_GLOB_OPTIONS, +} from '@/lib/copilot/vfs/operations' + +/** + * Differential test: {@link compileGlobMatcher} runs micromatch's own compiled pattern on + * RE2, so its answer must be micromatch's answer for every pattern and every path. This + * generates the pattern grid from glob atoms rather than listing cases, because the parts + * that are easy to get wrong are the combinations — a star next to a globstar, a dot + * segment under a wildcard, a class at a segment start. + * + * A pattern the translation cannot represent compiles to `null` and matches nothing. That + * is the deliberate fail-closed path, so those patterns are counted rather than compared. + * They are all degenerate shapes (unbalanced parens, a POSIX class behind a star); the + * patterns anyone would actually type are pinned separately by {@link REALISTIC}. + */ +const GLOB_OPTIONS = VFS_GLOB_OPTIONS + +/** An astral character, and the two lone surrogates it decomposes into. */ +const ASTRAL = '\u{1F600}' +const HIGH_SURROGATE = '\uD83D' +const LOW_SURROGATE = '\uDE00' + +/** A class range wide enough to bracket the private-use markers the translation reserves. */ +const WIDE_CLASS = '[\u{2000}-\u{F000}]' + +/** + * Atoms for the exhaustive grid. Negated classes and parentheses are included because + * picomatch treats `[^…]` and `(` specially even under `noext`, and neither goes through + * the `(?!\.)` guard that the rest of the translation keys on. `|` is included because a + * marker slot opened inside one branch of a group is not the one the next branch or the + * position after the group needs. The astral and lone-surrogate atoms are included because + * picomatch counts UTF-16 code units and RE2 counts code points, so every `?` and `[^…]` + * next to one is a place the two engines can disagree. The escape atoms and the wide class + * range are included because picomatch passes a caller's `\X` and `[x-y]` through verbatim and + * RE2 reads several of those differently than ECMAScript does, and `!` because it is the one + * glob feature whose compiled shape is a lookahead. + */ +const ATOMS = [ + 'a', + '.', + '..', + 'x', + '*', + '**', + '?', + '/', + '.git', + '[abc]', + '[.]', + '[!a]', + '{b,c}', + '+(z)', + '.a', + 'a.', + '\\.', + '\\*', + 'b', + '[^a]', + '[^a-z]', + '[^0-9-]', + '[^[:alpha:]]', + '(', + ')', + '|', + '!', + '\\a', + '\\A', + '\\p{L}', + WIDE_CLASS, + ASTRAL, + HIGH_SURROGATE, + LOW_SURROGATE, +] as const + +/** + * Atoms for a deeper walk. Four of these compose the shapes a three-atom walk cannot reach + * at all: globstar, slash, dot and star together are the dotfile pattern that returned nothing. + */ +const DEEP_ATOMS = [ + '*', + '**', + '/', + '/**', + '.', + '.*', + './', + 'a', + '.ts', + 'rc', + '[^a]', + '(', + ')', + '|', + ASTRAL, +] as const + +const PATHS = [ + 'a', + 'a/b', + 'a/b.ts', + 'b.ts', + 'a/b/c.ts', + '.hidden', + 'a/.hidden', + '.hidden/x', + 'files', + 'files/a', + 'files/a/meta.json', + 'files/Reports/q1.csv/meta.json', + 'weird{brace}/x', + 'weird[bracket]/x', + 'a.b', + '/', + 'a/', + '/a', + 'a//b', + '.', + '..', + 'a/./b', + 'a/..', + 'x', + 'xy', + 'abc', + 'a?c', + '+(a)', + '*', + 'node_modules/x', + '.a/.b', + 'a/b/.c/d', + 'a\nb', + 'a/b\nc', + 'a.', + '..a', + 'a b/c', + 'a*b', + 'a\\b', + '.git', + '.git/config', + 'a/.git/b', + 'A/B', + 'a/b/c/d/e', + 'ab', + 'ba', + '..hidden', + 'a/.b.ts', + '.b.ts', + 'a.b.c', + 'x..y', + 'a.b.', + 'a/b/', + 'x/', + 'a/x.ts', + 'a.ts', + '.x', + 'x.', + 'a/.', + './a', + 'x.y', + 'a/.b', + '[abc]', + '-x', + ']x', + '.a', + 'a/.a', + 'z', + '..a/b', + '.git/.x', + 'a.git', + '{b,c}', + '+(z)', + '.gitignore', + 'a/b/.', + 'b/..', + '', + '\\u2028b', + '\na$', + '.\n', + '.é}()', + '\\u2029x', + 'a/\nb', + 'a/\\u2028', + '.env', + 'app/.env.local', + 'src/.eslintrc', + 'src/index.ts', + ASTRAL, + `${ASTRAL}.png`, + `a${ASTRAL}`, + `${ASTRAL}${ASTRAL}`, + `.${ASTRAL}`, + `${ASTRAL}/a`, + `a/${ASTRAL}`, + HIGH_SURROGATE, + LOW_SURROGATE, + `${LOW_SURROGATE}${HIGH_SURROGATE}`, + `${HIGH_SURROGATE}.ts`, + `.${HIGH_SURROGATE}`, + 'a|b', +] as const + +/** Patterns a caller would plausibly type; none of these may fail closed. */ +const REALISTIC = [ + '**/*.ts', + 'src/**', + '*.{ts,tsx}', + '**/test_*.py', + 'docs/**/*.md', + '.env', + '.github/**', + '**/.env', + '**/.gitignore', + '.*', + 'a b/*.md', + '**/[Rr]eadme*', + 'files/*/meta.json', + '**/.*', + '**/.*.ts', + './**/.*', + '**/.*rc', + '.*/**', + '**/.*/**', + 'uploads/*', + '**', +] as const + +function buildPatterns(): string[] { + const patterns = new Set() + const walk = (atoms: readonly string[], remaining: number, current: string) => { + if (current) patterns.add(current) + if (remaining === 0) return + for (const atom of atoms) walk(atoms, remaining - 1, current + atom) + } + walk(ATOMS, 3, '') + walk(DEEP_ATOMS, 4, '') + return Array.from(patterns) +} + +/** + * Atoms RE2 cannot be handed at all: it reads `\A` and `\z` as anchors, `\a` as BEL, `\p{…}` + * as a Unicode class, and a range spanning the private-use block would consume a segment + * marker. They are compared like every other pattern but held out of the unrepresentable + * ratio, which measures how often a pattern someone might type is refused. + */ +const REFUSED_ATOMS = ['\\a', '\\A', '\\p{L}', WIDE_CLASS] as const + +function containsRefusedAtom(pattern: string): boolean { + return REFUSED_ATOMS.some((atom) => pattern.includes(atom)) +} + +function expectAgreement(pattern: string, path: string) { + const matcher = compileGlobMatcher(pattern) + const expected = micromatch.isMatch(path, pattern, GLOB_OPTIONS) + expect(matcher?.matches(path) ?? false, `pattern=${pattern} path=${JSON.stringify(path)}`).toBe( + expected + ) +} + +describe('glob matcher equivalence with micromatch', () => { + it('agrees with micromatch.isMatch across the full pattern grid', () => { + const patterns = buildPatterns() + const mismatches: string[] = [] + let unrepresentable = 0 + let translatable = 0 + let compared = 0 + + for (const pattern of patterns) { + const matcher = compileGlobMatcher(pattern) + const held = containsRefusedAtom(pattern) + if (!held) translatable++ + if (!matcher) { + if (!held) unrepresentable++ + continue + } + for (const path of PATHS) { + const expected = micromatch.isMatch(path, pattern, GLOB_OPTIONS) + const actual = matcher.matches(path) + compared++ + if (expected !== actual && mismatches.length < 20) { + mismatches.push( + `pattern=${JSON.stringify(pattern)} path=${JSON.stringify(path)} micromatch=${expected} re2=${actual}` + ) + } + } + } + + expect(mismatches).toEqual([]) + expect(compared).toBeGreaterThan(6_000_000) + expect(unrepresentable / translatable).toBeLessThan(0.08) + }, 120_000) + + it('covers the shapes the translation keys on', () => { + const patterns = new Set(buildPatterns()) + for (const pattern of ['**/.*', '**/.*.ts', './**/.*', '**/.*rc', '.*/**', '**/.*/**']) { + expect(patterns.has(pattern), pattern).toBe(true) + } + expect(Array.from(patterns).some((p) => p.includes('[^'))).toBe(true) + expect(Array.from(patterns).some((p) => p.includes('('))).toBe(true) + }) + + it('represents every realistic pattern rather than failing closed', () => { + for (const pattern of REALISTIC) { + expect(compileGlobMatcher(pattern), pattern).not.toBeNull() + } + }) + + it('matches dotfiles at every depth under a globstar', () => { + const matcher = compileGlobMatcher('**/.*') + expect(matcher).not.toBeNull() + for (const path of ['.env', '.gitignore', 'app/.env.local', 'src/.eslintrc']) { + expect(matcher?.matches(path), path).toBe(true) + } + for (const path of ['src/index.ts', 'README.md']) { + expect(matcher?.matches(path), path).toBe(false) + } + }) + + it('keeps the dot guard off classes picomatch never guarded', () => { + for (const pattern of ['[^a]x', '[^a-z]x', '[^0-9-]x', '[^[:alpha:]]x', '(*)']) { + expectAgreement(pattern, '.x') + } + expectAgreement('[^a]*', '.é}()') + }) + + it('keeps the non-empty assertion when the tail rewrite does not fire', () => { + expectAgreement('*.', '\na$...') + expectAgreement('*?', '\\u2028b\\é') + }) + + it('counts UTF-16 code units the way picomatch does, not code points', () => { + // `?` compiles to `[^/]`, which picomatch runs without the `u` flag, so it consumes one + // code unit and an astral character does not match it. RE2 counts code points, which + // silently widened `?` and `[^…]` into astral filenames — files a scope should exclude. + for (const [pattern, path] of [ + ['?', ASTRAL], + ['?.png', `${ASTRAL}.png`], + ['[^a]', ASTRAL], + ['??', ASTRAL], + ['?', HIGH_SURROGATE], + ['*', ASTRAL], + ['??b', `${ASTRAL}b`], + [`${ASTRAL}.png`, `${ASTRAL}.png`], + ['**/?', `a/${ASTRAL}`], + ['.?', `.${ASTRAL}`], + ] as const) { + expectAgreement(pattern, path) + } + expect(compileGlobMatcher('?')?.matches(ASTRAL)).toBe(false) + expect(compileGlobMatcher('??')?.matches(ASTRAL)).toBe(true) + }) + + it('reopens the marker slot a group would otherwise trap', () => { + // A slot opened inside a group is unreachable once the group matches nothing, so the + // segment marker stayed unconsumable and no dot path could ever match. + for (const pattern of ['(a|b)?*', '(a|)*', '((a|b)|(c|d))?*', '**|*']) { + for (const path of ['.hidden', '.', '..', '.env', 'ab', 'a']) { + expectAgreement(pattern, path) + } + } + expect(compileGlobMatcher('(a|b)?*')?.matches('.hidden')).toBe(true) + }) + + it('refuses the escape alphabet picomatch never emits', () => { + // picomatch escapes only punctuation, so `\A`, `\z`, `\a` and `\p{…}` are caller text it + // passes through. RE2 reads them as anchors, BEL and a Unicode class — and `\p{Any}` + // consumes the segment markers, taking `dot: false` with it. + for (const [pattern, path] of [ + ['\\A**/**', 'secrets/prod.key'], + ['\\A**/**', 'README.md'], + ['\\p{Any}**/**', '.git/config'], + ['*\\z', 'README.md'], + ['[^\\a]', 'a'], + ] as const) { + expectAgreement(pattern, path) + } + // `[\a-z]` is the fail-safe half of the same rule: ECMAScript reads `\a` as a literal `a` + // and matches, and refusing costs a match rather than granting one. + expect(compileGlobMatcher('[\\a-z]')).toBeNull() + expect(compileGlobMatcher('\\A**/**')).toBeNull() + expect(pathWithinGrepScope('secrets/prod.key', '\\A**/**')).toBe(false) + }) + + it('refuses a class range spanning the reserved code points', () => { + // The range brackets the segment markers and the surrogate escape block, so the class + // consumes a marker where micromatch requires a real character and every later atom + // matches one position to the left. + for (const [pattern, path] of [ + [`${WIDE_CLASS}.env`, '.env'], + [`${WIDE_CLASS}*`, '.env'], + [`${WIDE_CLASS}.git/config`, '.git/config'], + [`${WIDE_CLASS}..`, '..'], + [`[^a${WIDE_CLASS.slice(1)}`, '.env'], + ] as const) { + expectAgreement(pattern, path) + } + // A pair of such classes spans both halves of a remapped astral character, which is the + // fail-safe half of the rule: micromatch matches it and refusing costs that match. + expect(compileGlobMatcher(`${WIDE_CLASS}${WIDE_CLASS}`)).toBeNull() + expect(compileGlobMatcher(`${WIDE_CLASS}.env`)).toBeNull() + expect(pathWithinGrepScope('.env', `${WIDE_CLASS}*`)).toBe(false) + }) + + it('matches negated patterns instead of silently matching nothing', () => { + const files = new Map([ + ['a.ts', ''], + ['b.js', ''], + ]) + expect(glob(files, '!*.ts')).toEqual(['b.js']) + for (const pattern of ['!*.ts', '!.env', '!**/*.md', '!a', '!', '!!a', '!!*.ts', '!(a)']) { + for (const path of PATHS) { + expectAgreement(pattern, path) + } + } + }) + + it('refuses a zero-padded repeat bound RE2 reads as literal text', () => { + for (const pattern of ['a{00}b', 'a{00,2}b', '.{00,}*', 'a{1,02}b']) { + expect(compileGlobMatcher(pattern), pattern).toBeNull() + } + expect(compileGlobMatcher('a{0,2}b'), 'a{0,2}b').not.toBeNull() + }) + + it('rejects the empty path the way micromatch does', () => { + for (const pattern of ['**', '**/**', '**/', '**/**/**', '\\']) { + expectAgreement(pattern, '') + } + }) +}) diff --git a/apps/sim/lib/copilot/vfs/operations.test.ts b/apps/sim/lib/copilot/vfs/operations.test.ts index e626b702842..68e89e4f094 100644 --- a/apps/sim/lib/copilot/vfs/operations.test.ts +++ b/apps/sim/lib/copilot/vfs/operations.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { glob, grep } from '@/lib/copilot/vfs/operations' +import { GlobPatternError, glob, grep } from '@/lib/copilot/vfs/operations' function vfsFromEntries(entries: [string, string][]): Map { return new Map(entries) @@ -58,6 +58,122 @@ describe('glob', () => { }) }) +/** + * Matching runs on RE2, not on picomatch's own backtracking `RegExp`. These pin the glob + * semantics that translation has to preserve, so a future change to it cannot quietly + * widen or narrow what a pattern selects. + */ +describe('glob semantics', () => { + it('does not let a single star cross a slash', () => { + const files = vfsFromEntries([ + ['b.ts', ''], + ['a/b.ts', ''], + ]) + expect(glob(files, '*.ts')).toEqual(['b.ts']) + }) + + it('lets a double star cross slashes', () => { + const files = vfsFromEntries([ + ['b.ts', ''], + ['a/b.ts', ''], + ['a/c/d.ts', ''], + ]) + expect(glob(files, '**/*.ts').sort()).toEqual(['a/b.ts', 'a/c/d.ts', 'b.ts']) + }) + + it('matches exactly one character for a question mark, and not a slash', () => { + const files = vfsFromEntries([ + ['abc', ''], + ['ac', ''], + ['a/c', ''], + ]) + expect(glob(files, 'a?c')).toEqual(['abc']) + }) + + it('never matches a dotfile with a wildcard when dot is false', () => { + const files = vfsFromEntries([ + ['.hidden', ''], + ['visible', ''], + ['a/.hidden', ''], + ]) + expect(glob(files, '*')).toEqual(['a', 'visible']) + expect(glob(files, '**')).toEqual(['a', 'visible']) + expect(glob(files, 'a/*')).toEqual([]) + }) + + it('still matches a dotfile when the pattern spells the dot out', () => { + const files = vfsFromEntries([ + ['.hidden', ''], + ['visible', ''], + ]) + expect(glob(files, '.*')).toEqual(['.hidden']) + expect(glob(files, '.hidden')).toEqual(['.hidden']) + }) + + it('treats extglob syntax literally when noext is set', () => { + const files = vfsFromEntries([ + ['+(a)', ''], + ['a', ''], + ['aaa', ''], + ]) + expect(glob(files, '+(a)')).toEqual(['+(a)']) + }) + + it('treats a character class as a class, and brackets in a path as literals', () => { + const files = vfsFromEntries([ + ['ax', ''], + ['bx', ''], + ['cx', ''], + ['weird[bracket]/x', ''], + ]) + expect(glob(files, '[ab]x').sort()).toEqual(['ax', 'bx']) + expect(glob(files, 'weird[bracket]/x')).toEqual(['weird[bracket]/x']) + }) +}) + +describe('glob regex safety', () => { + it('runs a catastrophically backtracking pattern in linear time', () => { + // `*a` twelve times then `b` is 25 characters and takes ~41s against this 48-character + // path on picomatch's backtracking `RegExp`. Both sides are caller-supplied. + const files = vfsFromEntries([['a'.repeat(48), '']]) + const pattern = `${'*a'.repeat(12)}b` + + const start = Date.now() + const hits = glob(files, pattern) + + expect(hits).toEqual([]) + expect(Date.now() - start).toBeLessThan(1000) + }) + + it('still matches when the same shape has a real answer', () => { + const files = vfsFromEntries([[`${'a'.repeat(48)}b`, '']]) + expect(glob(files, `${'*a'.repeat(12)}b`)).toEqual([`${'a'.repeat(48)}b`]) + }) + + it('rejects an over-long pattern with a clear error', () => { + const files = vfsFromEntries([['a', '']]) + expect(() => glob(files, 'a'.repeat(1001))).toThrow(GlobPatternError) + expect(() => glob(files, 'a'.repeat(1001))).toThrow(/too long/) + }) + + it('rejects an absurd wildcard count with a clear error', () => { + const files = vfsFromEntries([['a', '']]) + expect(() => glob(files, '*'.repeat(33))).toThrow(GlobPatternError) + expect(() => glob(files, '*'.repeat(33))).toThrow(/too many wildcards/) + }) + + it('applies the same engine to a wildcard grep scope', () => { + const files = vfsFromEntries([['a'.repeat(48), 'needle']]) + const pattern = `${'*a'.repeat(12)}b` + + const start = Date.now() + const hits = grep(files, 'needle', pattern, { outputMode: 'files_with_matches' }) + + expect(hits).toEqual([]) + expect(Date.now() - start).toBeLessThan(1000) + }) +}) + describe('grep', () => { it('returns content matches per line in default mode', () => { const files = vfsFromEntries([['a.txt', 'hello\nworld\nhello']]) diff --git a/apps/sim/lib/copilot/vfs/operations.ts b/apps/sim/lib/copilot/vfs/operations.ts index 624707b43e9..2d19741d809 100644 --- a/apps/sim/lib/copilot/vfs/operations.ts +++ b/apps/sim/lib/copilot/vfs/operations.ts @@ -94,8 +94,13 @@ export interface ReadResult { * never crosses path slashes (required for `files` + star + `meta.json` style paths). `nobrace` * and `noext` disable brace and extglob expansion like the old builder. Uses `micromatch` for * well-tested `**` and edge cases instead of a custom `RegExp`. + * + * Only `micromatch.makeRe` is used — never `micromatch.isMatch`. See {@link compileGlobMatcher}. + * + * Exported so the differential test compiles against the production options rather than a + * copy that can drift out of sync with them. */ -const VFS_GLOB_OPTIONS: micromatch.Options = { +export const VFS_GLOB_OPTIONS: micromatch.Options = { bash: false, dot: false, windows: false, @@ -103,6 +108,872 @@ const VFS_GLOB_OPTIONS: micromatch.Options = { noext: true, } +/** + * Longest glob pattern accepted. Every real VFS pattern is a path shape well under this; + * the cap only exists so a pathological pattern cannot reach the compiler at all. + */ +const MAX_GLOB_PATTERN_LENGTH = 1000 + +/** + * Most `*`/`?` wildcards accepted in one pattern. A path-shaped glob uses a handful; + * dozens only appear in a probe. Bounded independently of length because the ReDoS + * shapes are short (`*a*a*a…b` is 25 characters). + */ +const MAX_GLOB_WILDCARDS = 32 + +/** + * Markers inserted at the start of a path segment while a pattern is matched. + * + * `dot: false` and picomatch's "a star must leave something to match" rule are encoded as + * lookaheads RE2 cannot express. Every one of them constrains the *shape of the segment* the + * pattern is about to match, so marking each segment start turns the assertion into a character + * exclusion: a guarded pattern position omits the marker from the class it may consume, an + * unguarded one consumes it optionally. U+E000–U+E002 are private-use, so a real VFS path + * cannot contain one; a path that does is not matched. + * + * - {@link SEG_DOT} — the segment starts with `.` and is neither `.` nor `..` (`(?!\.)`). + * - {@link SEG_DOT_ONLY} — the segment is `.` or `..` (`(?!\.{0,1}(?:\/|$))`, plus `(?!\.)`). + * - {@link SEG_NEWLINE} — the segment starts with a line terminator (`(?=.)`). + */ +const SEG_DOT = '\u{E000}' +const SEG_DOT_ONLY = '\u{E001}' +const SEG_NEWLINE = '\u{E002}' + +/** The markers as RE2 escapes, for use inside generated pattern source. */ +const SEG_DOT_RE = '\\x{e000}' +const SEG_DOT_ONLY_RE = '\\x{e001}' +const SEG_NEWLINE_RE = '\\x{e002}' +const ALL_MARKERS_RE = `${SEG_DOT_RE}${SEG_DOT_ONLY_RE}${SEG_NEWLINE_RE}` + +/** + * Where surrogate code units are remapped so RE2 sees one code point per UTF-16 code unit. + * + * picomatch compiles `?` to `[^/]` and runs it on a `RegExp` with no `u` flag, so it consumes + * exactly one code unit and an astral character (two units) does not match it. RE2 always + * matches whole code points, so the same class consumed `😀` and `glob('?')` returned files + * micromatch excludes — an over-match, and the dangerous direction for a scope filter. + * + * Splitting each surrogate half onto its own private-use code point restores UTF-16 counting + * exactly: `?` matches one half and so cannot match `😀`, while `??` matches both and does, + * which is what micromatch answers in each case. The alternative — rejecting astral input — + * would trade an over-match for an under-match on real filenames, so it is not taken. The + * range is 2048 wide (one per surrogate) and disjoint from the segment markers above. + */ +const SURROGATE_ESCAPE_BASE = 0xe800 + +/** Any surrogate code unit, paired or lone. Deliberately not `u`-flagged. */ +const SURROGATE_CHAR = /[\uD800-\uDFFF]/ + +/** + * Code points the translation reserves for its own use — the segment markers and the surrogate + * escape range — so a pattern or path already containing one is rejected rather than confused + * with a marker or with half of an astral character. + */ +const RESERVED_CHAR = /[\u{E000}-\u{E002}\u{E800}-\u{EFFF}]/u + +/** The bounds of {@link RESERVED_CHAR}, for rejecting a class range that spans them. */ +const RESERVED_FIRST = 0xe000 +const RESERVED_LAST = 0xefff + +/** + * Escapes picomatch never emits — it escapes only punctuation. A `\` before a word character + * is therefore caller text passed through verbatim, and RE2 reads several of those as + * something ECMAScript does not: `\A` and `\z` are zero-width anchors rather than literals, + * and `\p{…}` is a Unicode class that would consume a segment marker. + */ +const WORD_ESCAPE = /[A-Za-z0-9]/ + +/** Rewrite every surrogate code unit to its {@link SURROGATE_ESCAPE_BASE} counterpart. */ +function escapeSurrogates(text: string): string { + if (!SURROGATE_CHAR.test(text)) return text + let out = '' + for (let i = 0; i < text.length; i++) { + const code = text.charCodeAt(i) + out += + code >= 0xd800 && code <= 0xdfff + ? String.fromCharCode(SURROGATE_ESCAPE_BASE + (code - 0xd800)) + : text[i] + } + return out +} + +/** + * {@link escapeSurrogates} over generated pattern source, or `null` when a surrogate sits + * inside a character class. Remapping there would move a range endpoint above the segment + * markers, so `[a-😀]` would start consuming them; the shape is degenerate enough that failing + * closed costs nothing. + */ +function escapeSurrogatesInSource(source: string): string | null { + if (!SURROGATE_CHAR.test(source)) return source + let out = '' + let inClass = false + for (let i = 0; i < source.length; i++) { + const char = source[i] + const code = source.charCodeAt(i) + if (code >= 0xd800 && code <= 0xdfff) { + if (inClass) return null + out += String.fromCharCode(SURROGATE_ESCAPE_BASE + (code - 0xd800)) + continue + } + if (char === '\\') { + const next = source[i + 1] + if (next === undefined) return null + // picomatch escapes each surrogate half individually; `\` before one is decorative, and + // the remapped code point is not a metacharacter, so the escape is dropped rather than + // handed to RE2 as an unknown one. + if (SURROGATE_CHAR.test(next)) { + if (inClass) return null + out += String.fromCharCode(SURROGATE_ESCAPE_BASE + (source.charCodeAt(i + 1) - 0xd800)) + i += 1 + continue + } + out += char + next + i += 1 + continue + } + if (inClass) { + if (char === ']') inClass = false + } else if (char === '[') { + inClass = true + } + out += char + } + return out +} + +/** The code points ECMAScript's `.` excludes, as a matcher and as RE2 escapes. */ +const LINE_TERMINATOR = /[\n\r\u2028\u2029]/ +const NEWLINE_RE = '\\x{a}\\x{d}\\x{2028}\\x{2029}' + +/** Picomatch's `**` body: any run of characters that does not open a dot segment. */ +const GLOBSTAR_BODY = '(?:(?:(?!(?:^|\\/)\\.).)*?)' +const GLOBSTAR_CLASS = `[^${ALL_MARKERS_RE}${NEWLINE_RE}]` + +/** The three assertion shapes picomatch emits under {@link VFS_GLOB_OPTIONS}. */ +const DOT_GUARD = '(?!\\.)' +const NON_EMPTY_GUARD = '(?=.)' +const DOT_SEGMENT_GUARD = '(?!\\.{0,1}(?:\\/|$))' + +export interface GlobMatcher { + matches(path: string): boolean +} + +/** + * Thrown when a caller-supplied glob pattern is outside the safety caps. Both the `glob` + * and `grep` tool handlers already turn a thrown error into `{ success: false, error }`, + * so the message reaches the caller verbatim. + */ +export class GlobPatternError extends Error { + readonly code = 'GLOB_PATTERN' as const + constructor(message: string) { + super(message) + this.name = 'GlobPatternError' + } +} + +/** + * Compiled matchers, keyed by pattern. `grep` tests one scope against every file in the VFS + * and RE2 compilation costs far more than a match, so a pattern must not recompile per file. + * Cleared wholesale past the cap — patterns arrive in per-request bursts, so eviction order + * does not matter. + */ +const globMatcherCache = new Map() + +/** Entries kept in {@link globMatcherCache} before it is cleared. */ +const GLOB_MATCHER_CACHE_LIMIT = 256 + +/** {@link compileGlobMatcher}, memoized. Throws {@link GlobPatternError} the same way. */ +function getGlobMatcher(pattern: string): GlobMatcher | null { + const cached = globMatcherCache.get(pattern) + if (cached !== undefined) return cached + + const matcher = compileGlobMatcher(pattern) + if (globMatcherCache.size >= GLOB_MATCHER_CACHE_LIMIT) globMatcherCache.clear() + globMatcherCache.set(pattern, matcher) + return matcher +} + +/** Index just past the character class starting at `at`, or -1 when it is unterminated. */ +function classEnd(source: string, at: number): number { + let i = at + 1 + if (source[i] === '^') i += 1 + if (source[i] === ']') i += 1 + while (i < source.length && source[i] !== ']') { + if (source[i] === '\\') i += 1 + i += 1 + } + return i < source.length ? i + 1 : -1 +} + +/** + * True when a character-class body can be handed to RE2 unchanged. + * + * Two shapes cannot. A `\` escape is caller passthrough RE2 reinterprets (see + * {@link WORD_ESCAPE}). A range whose endpoints bracket the reserved block lets the class + * consume a segment marker or half a remapped astral character, which shifts every later + * atom one position left — `[\u{2000}-\u{F000}].env` would match `.env` by eating its + * {@link SEG_DOT}, bypassing `dot: false` entirely. + */ +function classBodyIsRepresentable(body: string): boolean { + let previous = -1 + let i = 0 + while (i < body.length) { + if (body[i] === '-' && previous >= 0 && i + 1 < body.length) { + i += 1 + let high: number + if (body[i] === '\\') { + const next = body[i + 1] + if (next === undefined || WORD_ESCAPE.test(next)) return false + high = next.charCodeAt(0) + i += 2 + } else { + high = body.charCodeAt(i) + i += 1 + } + if (previous <= RESERVED_LAST && high >= RESERVED_FIRST) return false + previous = -1 + continue + } + if (body[i] === '\\') { + const next = body[i + 1] + if (next === undefined || WORD_ESCAPE.test(next)) return false + previous = next.charCodeAt(0) + i += 2 + continue + } + previous = body.charCodeAt(i) + i += 1 + } + return true +} + +/** Index of the `)` closing the group opened at `at`, or -1. */ +function groupEnd(source: string, at: number): number { + let depth = 0 + for (let i = at; i < source.length; i++) { + const char = source[i] + if (char === '\\') { + i += 1 + } else if (char === '[') { + const end = classEnd(source, i) + if (end === -1) return -1 + i = end - 1 + } else if (char === '(') { + depth += 1 + } else if (char === ')') { + depth -= 1 + if (depth === 0) return i + } + } + return -1 +} + +/** The quantifier at `at`, or `''` when there is none (`{ts,tsx}` is a literal, not a repeat). */ +function readQuantifier(source: string, at: number): string { + const char = source[at] + if (char === '?' || char === '*' || char === '+') { + return source[at + 1] === '?' ? source.slice(at, at + 2) : char + } + if (char === '{') { + const end = source.indexOf('}', at) + if (end !== -1 && /^\{\d+(?:,\d*)?\}$/.test(source.slice(at, end + 1))) { + return source.slice(at, end + 1) + } + } + return '' +} + +/** + * True when a `{n,m}` bound is zero-padded. RE2 reads `a{00}` as the literal text `a{00}` + * where ECMAScript reads a repeat, so the two engines cannot be reconciled and the source is + * refused rather than emitted with a silently different meaning. + */ +function hasPaddedBound(quantifier: string): boolean { + return /^\{0\d|,0\d/.test(quantifier) +} + +/** True when `quantifier` lets the atom it follows match nothing. */ +function isOptionalQuantifier(quantifier: string): boolean { + if (quantifier.startsWith('?') || quantifier.startsWith('*')) return true + const bounded = /^\{(\d+)(?:,\d*)?\}$/.exec(quantifier) + return bounded !== null && Number(bounded[1]) === 0 +} + +/** True when every match of the balanced token run `[start, end)` consumes at least one character. */ +function sequenceMustConsume(source: string, start: number, end: number): boolean { + let i = start + while (i < end) { + const char = source[i] + if (char === '^' || char === '$') { + i += 1 + continue + } + let atomEnd: number + let consumes: boolean + if (char === '(') { + const close = groupEnd(source, i) + if (close === -1 || close >= end) return false + consumes = groupMustConsume(source, i, close) + atomEnd = close + 1 + } else if (char === '[') { + const close = classEnd(source, i) + if (close === -1) return false + consumes = true + atomEnd = close + } else { + consumes = true + atomEnd = char === '\\' ? i + 2 : i + 1 + } + const quantifier = readQuantifier(source, atomEnd) + if (consumes && !isOptionalQuantifier(quantifier)) return true + i = atomEnd + quantifier.length + } + return false +} + +/** {@link sequenceMustConsume} for a group, which consumes only when every branch does. */ +function groupMustConsume(source: string, open: number, close: number): boolean { + const isNonCapturing = source.startsWith('(?:', open) + if (!isNonCapturing && source[open + 1] === '?') return false + let depth = 0 + let from = open + (isNonCapturing ? 3 : 1) + for (let i = from; i < close; i++) { + const char = source[i] + if (char === '\\') { + i += 1 + } else if (char === '[') { + const end = classEnd(source, i) + if (end === -1) return false + i = end - 1 + } else if (char === '(') { + depth += 1 + } else if (char === ')') { + depth -= 1 + } else if (char === '|' && depth === 0) { + if (!sequenceMustConsume(source, from, i)) return false + from = i + 1 + } + } + return sequenceMustConsume(source, from, close) +} + +/** Index of the `)` closing the group `from` sits inside, or -1. */ +function enclosingGroupEnd(source: string, from: number): number { + let depth = 0 + for (let i = from; i < source.length; i++) { + const char = source[i] + if (char === '\\') { + i += 1 + } else if (char === '[') { + const end = classEnd(source, i) + if (end === -1) return -1 + i = end - 1 + } else if (char === '(') { + depth += 1 + } else if (char === ')') { + if (depth === 0) return i + depth -= 1 + } + } + return -1 +} + +/** + * True when whatever remains of `source` from `at` must consume at least one character. + * + * This is what makes `(?=.)`'s "not at end of input" half droppable: if the rest of the pattern + * cannot match nothing, the assertion is already implied. Alternatives to the right of a `|` + * belong to the enclosing group rather than to this position, so they are skipped. + */ +function remainderMustConsume(source: string, at: number): boolean { + let i = at + while (i < source.length) { + const char = source[i] + if (char === '^' || char === '$') { + i += 1 + continue + } + if (char === ')') { + i += 1 + readQuantifier(source, i + 1).length + continue + } + if (char === '|') { + const close = enclosingGroupEnd(source, i) + if (close === -1) return false + i = close + 1 + readQuantifier(source, close + 1).length + continue + } + let atomEnd: number + let consumes: boolean + if (char === '(') { + const close = groupEnd(source, i) + if (close === -1) return false + consumes = groupMustConsume(source, i, close) + atomEnd = close + 1 + } else if (char === '[') { + const close = classEnd(source, i) + if (close === -1) return false + consumes = true + atomEnd = close + } else { + consumes = true + atomEnd = char === '\\' ? i + 2 : i + 1 + } + const quantifier = readQuantifier(source, atomEnd) + if (consumes && !isOptionalQuantifier(quantifier)) return true + i = atomEnd + quantifier.length + } + return false +} + +/** + * True when `at` opens a group other than `**`. A guard sitting there applies only to the + * branches that consume nothing, which no single emitted position can express, so callers + * fail closed instead. + */ +function isGroupOpen(source: string, at: number): boolean { + if (source[at] !== '(' || source.startsWith('(?=', at) || source.startsWith('(?!', at)) + return false + return !source.startsWith(GLOBSTAR_BODY, at) +} + +/** `atom` with line terminators removed from what it can match, or `null` when it cannot be. */ +function withoutLineTerminators(atom: string): string | null { + if (atom.startsWith('[^')) return `[^${atom.slice(2, -1)}${NEWLINE_RE}]` + return LINE_TERMINATOR.test(atom) ? null : atom +} + +/** + * Whether the atom at `at` can match a line terminator. Conservative: a negated class or a + * group answers "yes", so a `(?=.)` whose non-emptiness would have to be pushed onto such an + * atom fails closed rather than being approximated. + */ +function atomMayMatchLineTerminator(source: string, at: number): boolean { + const char = source[at] + if (char === undefined || char === '(' || char === ')' || char === '|') return true + if (char === '\\') return LINE_TERMINATOR.test(source[at + 1] ?? '\n') + if (char === '[') { + const end = classEnd(source, at) + if (end === -1) return true + return source[at + 1] === '^' || LINE_TERMINATOR.test(source.slice(at, end)) + } + return LINE_TERMINATOR.test(char) +} + +/** + * Rewrite picomatch's generated source into an RE2-representable equivalent. + * + * The scan is structural rather than textual: it tracks whether the current output position + * starts a path segment (following alternation branches and quantified groups, so `(?:X\/)?` + * is recognised as a boundary), and opens one optional marker class per segment. Each assertion + * then narrows that class instead of being deleted — `(?!\.)` drops {@link SEG_DOT} and + * {@link SEG_DOT_ONLY}, `(?!\.{0,1}(?:\/|$))` drops {@link SEG_DOT_ONLY}, `(?=.)` drops + * {@link SEG_NEWLINE}. `(?=.)` additionally forbids matching nothing at all, which is dropped + * when the rest of the pattern must consume a character and is otherwise folded into the + * trailing `[^/]*?\/?$` it guards. + * + * Returns `null` when any assertion survives or a shape is unrecognised, which is how a + * picomatch version emitting something new degrades to "no matches" instead of to the + * backtracking engine. `operations.glob-semantics.test.ts` pins the equivalence over the grid. + */ +function rewriteGlobSource(generated: string): string | null { + if (RESERVED_CHAR.test(generated)) return null + const source = escapeSurrogatesInSource(generated) + if (source === null) return null + + const parts: string[] = [] + const groups: Array<{ + startBoundary: boolean + branchEnds: boolean[] + /** Index in `parts` just past the group opener, so a slot inside it can be recognised. */ + openIndex: number + /** Index in `source` of the `(`. */ + sourceOpen: number + }> = [] + let boundary = false + let emptyBoundary = false + let slot = -1 + let markers = '' + let leadingDot = false + let slotPending = false + let afterEndAnchor = false + let nonEmpty = false + let nonEmptyNeedsNewlineGuard = false + let i = 0 + + const writeSlot = () => { + if (slot >= 0) parts[slot] = markers === '' ? '' : `[${markers}]?` + } + const openSlot = () => { + if (slot >= 0) return + slot = parts.length + markers = ALL_MARKERS_RE + slotPending = true + parts.push('') + writeSlot() + } + const dropMarker = (marker: string) => { + markers = markers.split(marker).join('') + writeSlot() + } + const endSegment = () => { + slot = -1 + markers = '' + leadingDot = false + slotPending = false + } + + while (i < source.length) { + // A branch ending in `$` cannot continue, so it never constrains the boundary after it. + const endsBranch = afterEndAnchor + afterEndAnchor = false + + if (source.startsWith(GLOBSTAR_BODY, i)) { + if (nonEmpty) return null + // `**` never consumes a marker itself, so it is split. Having matched something it can + // only have stopped before a newline segment — a `/` followed by a dot is exactly what + // its own guard forbids. Having matched nothing it leaves the position untouched, so the + // empty branch carries the marker slot forward for whatever follows to narrow. + const inherited = slotPending ? markers : boundary || emptyBoundary ? ALL_MARKERS_RE : '' + if (slotPending) parts[slot] = '' + parts.push(`(?:${GLOBSTAR_CLASS}+[${SEG_NEWLINE_RE}]?|`) + slot = parts.length + markers = inherited + slotPending = true + parts.push('') + writeSlot() + parts.push(')') + i += GLOBSTAR_BODY.length + boundary = false + emptyBoundary = false + leadingDot = false + continue + } + + if (source.startsWith(DOT_GUARD, i)) { + if (isGroupOpen(source, i + DOT_GUARD.length)) return null + if (!boundary && !slotPending) return null + openSlot() + dropMarker(SEG_DOT_RE) + dropMarker(SEG_DOT_ONLY_RE) + i += DOT_GUARD.length + continue + } + + if (source.startsWith(DOT_SEGMENT_GUARD, i)) { + if (!leadingDot) return null + dropMarker(SEG_DOT_ONLY_RE) + i += DOT_SEGMENT_GUARD.length + continue + } + + if (source.startsWith(NON_EMPTY_GUARD, i)) { + if (isGroupOpen(source, i + NON_EMPTY_GUARD.length)) return null + if (boundary || slotPending) { + openSlot() + dropMarker(SEG_NEWLINE_RE) + nonEmptyNeedsNewlineGuard = false + } else if (leadingDot) { + // Mid-segment: no marker covers this position, so the guard has to reach the atom. + nonEmptyNeedsNewlineGuard = true + } else { + return null + } + nonEmpty = true + i += NON_EMPTY_GUARD.length + continue + } + + const char = source[i] + + if (char === '(') { + const opener = source.startsWith('(?:', i) ? '(?:' : '(' + if (opener === '(' && source[i + 1] === '?') return null + parts.push(opener) + groups.push({ + startBoundary: boundary, + branchEnds: [], + openIndex: parts.length, + sourceOpen: i, + }) + i += opener.length + continue + } + + if (char === '|') { + const frame = groups[groups.length - 1] + if (!frame) return null + frame.branchEnds.push(endsBranch || boundary) + boundary = frame.startBoundary + emptyBoundary = false + // A slot belongs to the branch that emitted it; the next branch is a separate path + // through the regex and opens its own, so nothing is carried across the `|`. + endSegment() + parts.push('|') + i += 1 + continue + } + + if (char === ')') { + const frame = groups.pop() + if (!frame) return null + // A slot still pending at the close is un-consumed and goes on serving the position + // after the group (a trailing `**` leaves exactly this). One the branch already built + // on is trapped inside it, and nothing outside may narrow it further. + const carriedSlot = slotPending + if (!carriedSlot && slot >= frame.openIndex) endSegment() + const closeIndex = i + parts.push(')') + i += 1 + const quantifier = readQuantifier(source, i) + if (quantifier) { + if (hasPaddedBound(quantifier)) return null + parts.push(quantifier) + i += quantifier.length + } + let closedAtBoundary = endsBranch || boundary + for (const end of frame.branchEnds) closedAtBoundary &&= end + if (isOptionalQuantifier(quantifier)) closedAtBoundary &&= frame.startBoundary + boundary = closedAtBoundary + // A group that can match nothing leaves the position exactly where it started, so on + // that path the segment marker is still unconsumed and something after the group has to + // be able to take it — otherwise `(a|b)?*` cannot match `.hidden` at all. The flag is + // kept apart from `boundary` because the group's other paths are NOT at a segment start, + // so an assertion landing here would hold for only some of them; every assertion keys on + // `boundary` and therefore fails closed. + emptyBoundary = + !boundary && + !carriedSlot && + frame.startBoundary && + (isOptionalQuantifier(quantifier) || + !groupMustConsume(source, frame.sourceOpen, closeIndex)) + continue + } + + if (char === '^' || char === '$') { + if (char === '^' && slotPending) { + // picomatch wraps its output in a redundant `^(?:^…)$)$`, so a slot opened at the + // outer boundary can land in front of the inner anchor, where no marker can ever + // reach it. Nothing between the two consumes, so the slot simply moves behind it. + parts[slot] = '' + parts.push('^') + slot = parts.length + parts.push('') + writeSlot() + } else { + parts.push(char) + } + if (char === '^') { + boundary = true + emptyBoundary = false + } + afterEndAnchor = char === '$' + i += 1 + continue + } + + let atom: string + let atomEnd: number + let isSlash = false + if (char === '\\') { + const next = source[i + 1] + if (next === undefined || WORD_ESCAPE.test(next)) return null + atom = `\\${next}` + atomEnd = i + 2 + isSlash = next === '/' + } else if (char === '[') { + const end = classEnd(source, i) + if (end === -1) return null + const negated = source[i + 1] === '^' + const body = source.slice(negated ? i + 2 : i + 1, end - 1) + if (!classBodyIsRepresentable(body)) return null + atom = negated ? `[^${body}${ALL_MARKERS_RE}]` : `[${body}]` + atomEnd = end + } else if (char === '.') { + atom = GLOBSTAR_CLASS + atomEnd = i + 1 + } else if (char === '*' || char === '+' || char === '?') { + return null + } else { + atom = char + atomEnd = i + 1 + isSlash = char === '/' + } + + const quantifier = readQuantifier(source, atomEnd) + if (hasPaddedBound(quantifier)) return null + const optional = isOptionalQuantifier(quantifier) + const atBoundary = boundary + // A class picomatch did not exclude `/` from (its POSIX expansions, and the `.` those + // sometimes leave behind) can step into the next segment, landing on that segment's marker. + const crossesSegments = atom.startsWith('[^') + ? !atom.slice(2, -1).includes('/') + : atom.startsWith('[') + ? atom.slice(1, -1).includes('/') + : false + if (crossesSegments && quantifier !== '' && quantifier !== '?') return null + + if (nonEmpty) { + if (!optional) { + if (nonEmptyNeedsNewlineGuard) { + const guarded = withoutLineTerminators(atom) + if (guarded === null) return null + atom = guarded + } + } else { + if (char !== '[' || crossesSegments) return null + const rest = source.slice(atomEnd + quantifier.length) + const first = nonEmptyNeedsNewlineGuard ? withoutLineTerminators(atom) : atom + if (first === null) return null + if (remainderMustConsume(source, atomEnd + quantifier.length)) { + // Only the "not at end of input" half was load-bearing, and the remainder implies it. + if (nonEmptyNeedsNewlineGuard) { + if (atomMayMatchLineTerminator(source, atomEnd + quantifier.length)) return null + if (boundary) openSlot() + parts.push(`(?:${first}${atom}*?)?`) + } else { + if (boundary) openSlot() + parts.push(`${atom}${quantifier}`) + } + } else { + const tail = /^(\\\/\?)?(\)*)\$$/.exec(rest) + if (!tail) return null + if (boundary) openSlot() + parts.push(tail[1] ? `(?:${first}${atom}*\\/?|\\/)` : `${first}${atom}*`) + parts.push(`${tail[2]}$`) + const out = parts.join('') + return /\(\?[=!<]/.test(out) ? null : out + } + nonEmpty = false + nonEmptyNeedsNewlineGuard = false + i = atomEnd + quantifier.length + slotPending = false + boundary = false + leadingDot = false + continue + } + nonEmpty = false + nonEmptyNeedsNewlineGuard = false + } + + if (boundary || emptyBoundary) openSlot() + if (crossesSegments) { + const stepped = `${atom}[${ALL_MARKERS_RE}]?` + parts.push(quantifier === '?' ? `(?:${stepped})?` : stepped) + } else { + parts.push(atom + quantifier) + } + i = atomEnd + quantifier.length + slotPending = false + emptyBoundary = false + boundary = isSlash && !optional + leadingDot = atBoundary && atom === '\\.' && !optional + if (boundary) endSegment() + } + + if (groups.length > 0 || nonEmpty) return null + const out = parts.join('') + return /\(\?[=!<]/.test(out) ? null : out +} + +/** + * Prefix every path segment with the marker describing its shape, with surrogate halves + * remapped so RE2 counts UTF-16 code units the way picomatch does. Returns `null` when the + * path already contains a reserved code point. See {@link SEG_DOT} and + * {@link SURROGATE_ESCAPE_BASE}. + */ +function markPathSegments(path: string): string | null { + if (RESERVED_CHAR.test(path)) return null + const escaped = escapeSurrogates(path) + if (!escaped.includes('.') && !LINE_TERMINATOR.test(escaped)) return escaped + return escaped + .split('/') + .map((segment) => { + if (segment === '.' || segment === '..') return SEG_DOT_ONLY + segment + if (segment.startsWith('.')) return SEG_DOT + segment + if (segment !== '' && LINE_TERMINATOR.test(segment[0])) return SEG_NEWLINE + segment + return segment + }) + .join('/') +} + +/** + * The inner regex of picomatch's negation wrapper `^(?!^(?:X)$).*$` as a standalone + * `^(?:X)$`, or `null` when `source` is not one. + * + * A `!`-prefixed glob is a documented micromatch feature, and the wrapper's lookahead is the + * one shape RE2 cannot take. Reading the inner pattern off the generated source rather than + * off the pattern text inherits picomatch's own `!` rules exactly — leading position only, + * `!!` cancelling, `!(` not counting — instead of restating them here. + */ +function negatedInnerSource(source: string): string | null { + const prefix = '^(?!^(?:' + const suffix = ')$).*$' + if (source.length < prefix.length + suffix.length) return null + if (!source.startsWith(prefix) || !source.endsWith(suffix)) return null + return groupEnd(source, 1) === source.length - 4 ? source.slice(4, source.length - 4) : null +} + +/** + * Compile a caller-supplied glob into a matcher that cannot backtrack. + * + * `micromatch.isMatch` compiles each `*` to `[^/]*?` and runs it on the backtracking engine, + * so a short pattern like `*a` repeated over a longer path is exponential — and both pattern + * and paths come from an authenticated caller's tool call. + * + * The regex is picomatch's own from `makeRe`, rewritten by {@link rewriteGlobSource}, so the + * semantics of {@link VFS_GLOB_OPTIONS} are unchanged. Returns `null` when the pattern is not + * RE2-representable; callers treat that as "matches nothing" rather than falling back to the + * backtracking engine. Throws {@link GlobPatternError} when the pattern is over the safety caps. + */ +export function compileGlobMatcher(pattern: string): GlobMatcher | null { + if (pattern.length > MAX_GLOB_PATTERN_LENGTH) { + throw new GlobPatternError( + `Glob pattern is too long (${pattern.length} characters, limit ${MAX_GLOB_PATTERN_LENGTH}).` + ) + } + + const wildcards = pattern.replace(/[^*?]/g, '').length + if (wildcards > MAX_GLOB_WILDCARDS) { + throw new GlobPatternError( + `Glob pattern has too many wildcards (${wildcards}, limit ${MAX_GLOB_WILDCARDS}). ` + + 'Narrow the pattern with literal path segments.' + ) + } + + let generated: RegExp + try { + generated = micromatch.makeRe(pattern, VFS_GLOB_OPTIONS) + } catch { + logger.warn('Glob pattern could not be parsed; matching nothing', { pattern }) + return null + } + + const negatedBody = negatedInnerSource(generated.source) + const source = rewriteGlobSource(negatedBody ?? generated.source) + const linear = source === null ? null : compileLinearRegex(source) + if (!linear) { + logger.warn('Glob pattern is not RE2-representable; matching nothing', { pattern }) + return null + } + + return { + matches: (path: string) => { + // Micromatch rejects an empty path before its regex ever runs, and picomatch reports an + // exact string equality as a match even when its own regex would not — `+(a)` against + // `+(a)` with `noext`. Both are kept so behaviour is unchanged. + if (path === '') return false + if (path === pattern) return true + // A negated pattern's wrapper ends in `.*$`, which under no `s` flag also requires the + // whole path to be free of line terminators. + if (negatedBody !== null && LINE_TERMINATOR.test(path)) return false + const marked = markPathSegments(path) + if (marked === null) return false + return negatedBody !== null ? !linear.test(marked) : linear.test(marked) + }, + } +} + /** * Splits VFS text into lines for line-oriented grep. Strips a trailing CR so Windows-style * CRLF payloads still match patterns anchored at line end (`$`). @@ -113,10 +984,11 @@ function splitLinesForGrep(content: string): string[] { /** * Returns true when `filePath` is `scope` or a descendant path (`scope/...`). If `scope` contains - * `*` or `?`, filters with micromatch `isMatch` and {@link VFS_GLOB_OPTIONS}. Other characters - * (including `[`, `{`, spaces) use directory-prefix logic so literal VFS path segments are not - * parsed as glob syntax. Trailing slashes are stripped so `files/` and `files` both scope under - * `files/...`. + * `*` or `?`, filters with {@link compileGlobMatcher} — the caller-supplied scope reaches the + * same matcher {@link glob} uses, so it cannot backtrack either. Other characters (including + * `[`, `{`, spaces) use directory-prefix logic so literal VFS path segments are not parsed as + * glob syntax. Trailing slashes are stripped so `files/` and `files` both scope under `files/...`. + * Throws {@link GlobPatternError} for a scope outside the safety caps, which both callers catch. * * Exported so the lazy VFS can resolve exactly the lazy artifacts a scoped grep will consider, * keeping "what we materialize" identical to "what grep filters in". @@ -124,7 +996,7 @@ function splitLinesForGrep(content: string): string[] { export function pathWithinGrepScope(filePath: string, scope: string): boolean { const scopeUsesStarOrQuestionGlob = /[*?]/.test(scope) if (scopeUsesStarOrQuestionGlob) { - return micromatch.isMatch(filePath, scope, VFS_GLOB_OPTIONS) + return getGlobMatcher(scope)?.matches(filePath) ?? false } const base = scope.replace(/\/+$/, '') if (base === '') { @@ -240,11 +1112,18 @@ export function grep( } /** - * Glob pattern matching against VFS file paths and virtual directories using `micromatch` - * with {@link VFS_GLOB_OPTIONS} (path-aware `*` and `?`, `**`, no brace or extglob expansion). - * Returns matching file keys and virtual directory prefixes. + * Glob pattern matching against VFS file paths and virtual directories using `micromatch`'s + * own compiled pattern under {@link VFS_GLOB_OPTIONS} (path-aware `*` and `?`, `**`, no brace + * or extglob expansion), executed on RE2 by {@link compileGlobMatcher}. Returns matching file + * keys and virtual directory prefixes. + * + * Throws {@link GlobPatternError} for a pattern outside the safety caps, and returns no + * matches for one RE2 cannot represent. */ export function glob(files: Map, pattern: string): string[] { + const matcher = getGlobMatcher(pattern) + if (!matcher) return [] + const result = new Set() const directories = new Set() @@ -261,13 +1140,13 @@ export function glob(files: Map, pattern: string): string[] { for (const filePath of files.keys()) { if (filePath.endsWith('/.folder')) continue - if (micromatch.isMatch(filePath, pattern, VFS_GLOB_OPTIONS)) { + if (matcher.matches(filePath)) { result.add(filePath) } } for (const dir of directories) { - if (micromatch.isMatch(dir, pattern, VFS_GLOB_OPTIONS)) { + if (matcher.matches(dir)) { result.add(dir) } } diff --git a/apps/sim/lib/core/security/input-validation.server.ts b/apps/sim/lib/core/security/input-validation.server.ts index 02c44a1ed97..4b69f9c1d2a 100644 --- a/apps/sim/lib/core/security/input-validation.server.ts +++ b/apps/sim/lib/core/security/input-validation.server.ts @@ -6,10 +6,10 @@ import https from 'https' import type { LookupFunction } from 'net' import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' -import { omit } from '@sim/utils/object' import { HttpProxyAgent } from 'http-proxy-agent' import { HttpsProxyAgent } from 'https-proxy-agent' import * as ipaddr from 'ipaddr.js' +import { getDomain } from 'tldts' import { Agent, type Dispatcher, @@ -416,7 +416,11 @@ export interface SecureFetchOptions { maxRedirects?: number maxResponseBytes?: number signal?: AbortSignal - /** Drop the Authorization header when following a redirect, so it is not sent to the redirect target's origin. */ + /** + * Drop the Authorization header on EVERY redirect, including same-site ones. + * Cross-site redirects already drop all credential-bearing headers by default + * (see {@link headersForRedirect}); this tightens that to same-site hops too. + */ stripAuthOnRedirect?: boolean /** * Pre-validated, IP-pinned `http://` proxy URL (see {@link validateAndPinProxyUrl}). @@ -486,6 +490,99 @@ function resolveRedirectUrl(baseUrl: string, location: string): string { } } +/** Header names that always carry a secret, regardless of the service. */ +const CREDENTIAL_HEADER_NAMES = new Set([ + 'authorization', + 'cookie', + 'proxy-authorization', + 'private-token', + 'api-key', + 'apikey', +]) + +/** + * Suffixes that mark a vendor-specific credential header (`x-api-key`, + * `x-goog-api-key`, `x-clickhouse-key`, `x-sim-signature`, `x-auth-token`, …). + * Over-matching is deliberate: forwarding a non-secret header one hop less often + * is harmless, forwarding a secret to another site is not. + */ +const CREDENTIAL_HEADER_SUFFIXES = ['-key', '-token', '-secret', '-password', '-signature'] + +/** + * Headers the suffix rule matches that carry no secret. Dropping them changes request + * semantics for no security gain: `idempotency-key` is a de-duplication token (Brex + * transfers, webhook delivery, data drains) whose loss can duplicate a side effect, and + * `x-atlassian-token: no-check` is Atlassian's XSRF opt-out on attachment uploads. + */ +const NON_CREDENTIAL_HEADER_NAMES = new Set(['idempotency-key', 'x-atlassian-token']) + +function isCredentialHeader(name: string): boolean { + const lower = name.toLowerCase() + if (CREDENTIAL_HEADER_NAMES.has(lower)) return true + if (NON_CREDENTIAL_HEADER_NAMES.has(lower)) return false + if (lower.startsWith('x-auth')) return true + return CREDENTIAL_HEADER_SUFFIXES.some((suffix) => lower.endsWith(suffix)) +} + +/** + * A redirect is same-site when it stays on the same registrable domain (eTLD+1) without + * downgrading the transport. This is deliberately looser than origin equality: real + * providers redirect across subdomains of one service (`api.zoom.us` → a regional + * `*.zoom.us` host, `api.github.com` → `codeload.github.com`) and self-hosted deployments + * upgrade `http:` → `https:`, and those targets still require the caller's credential. A + * hostile or open redirect to another site is what must not receive it. + * + * IP-literal hosts have no registrable domain, so they fall back to exact host equality. + * + * `allowPrivateDomains` honors the PRIVATE section of the Public Suffix List, so providers listed + * there (`*.s3.amazonaws.com`, `*.blob.core.windows.net`, `*.cloudfront.net`, `*.myshopify.com`, + * `*.vercel.app`, …) are cross-site tenant-to-tenant rather than sharing one registrable domain. + * + * This is NOT general tenant isolation. Providers absent from the PSL private section — + * `atlassian.net`, `sharepoint.com`, `okta.com`, `auth0.com`, `my.salesforce.com`, `box.com` — + * still resolve tenant subdomains to a single registrable domain, so a redirect between two + * tenants of those services counts as same-site and keeps the credential. Closing that needs a + * per-provider rule, not a PSL flag. + */ +function isSameSiteRedirect(from: URL, to: URL): boolean { + if (from.protocol === 'https:' && to.protocol !== 'https:') return false + if (from.host === to.host) return true + const fromDomain = getDomain(from.hostname, { allowPrivateDomains: true }) + return fromDomain !== null && fromDomain === getDomain(to.hostname, { allowPrivateDomains: true }) +} + +/** + * Builds the header set for a redirect hop. Credential-bearing headers ({@link isCredentialHeader}) + * are dropped on any cross-site hop so a hostile or open redirect cannot exfiltrate the caller's + * API key, bearer token, or cookies; non-credential headers still travel. On a same-site hop + * everything is forwarded unless `stripAuth` is set, which drops `authorization` alone. + * + * This is deliberately weaker than {@link followRedirectsGuarded} on the undici path, which drops + * EVERY header cross-**origin** (not cross-site) and refuses to forward a request body at all. + * The node-http path keeps cross-subdomain provider hops working and still forwards bodies. + */ +function headersForRedirect( + headers: Record, + fromUrl: string, + toUrl: string, + stripAuth: boolean +): Record { + let sameSite: boolean + try { + sameSite = isSameSiteRedirect(new URL(fromUrl), new URL(toUrl)) + } catch { + sameSite = false + } + if (sameSite && !stripAuth) return headers + + const next: Record = {} + for (const [key, value] of Object.entries(headers)) { + if (sameSite ? key.toLowerCase() === 'authorization' : isCredentialHeader(key)) continue + next[key] = value + } + return next +} + /** * Creates a DNS lookup function that always returns a pre-resolved IP address. * Use this to prevent DNS rebinding (TOCTOU) attacks when connecting to @@ -1051,12 +1148,15 @@ export async function secureFetchWithPinnedIP( settledReject(new Error(`Redirect blocked: ${validation.error}`)) return } - const redirectOptions = options.stripAuthOnRedirect - ? { - ...options, - headers: omit(options.headers ?? {}, ['Authorization', 'authorization']), - } - : options + const redirectOptions = { + ...options, + headers: headersForRedirect( + options.headers ?? {}, + url, + redirectUrl, + options.stripAuthOnRedirect === true + ), + } return secureFetchWithPinnedIP( redirectUrl, validation.resolvedIP!, diff --git a/apps/sim/lib/core/security/redaction.test.ts b/apps/sim/lib/core/security/redaction.test.ts index e5c60abf969..2df7e22d152 100644 --- a/apps/sim/lib/core/security/redaction.test.ts +++ b/apps/sim/lib/core/security/redaction.test.ts @@ -65,7 +65,6 @@ describe('isSensitiveKey', () => { expect(isSensitiveKey('private_key')).toBe(true) expect(isSensitiveKey('authorization')).toBe(true) expect(isSensitiveKey('bearer')).toBe(true) - expect(isSensitiveKey('private')).toBe(true) expect(isSensitiveKey('auth')).toBe(true) expect(isSensitiveKey('password')).toBe(true) expect(isSensitiveKey('credential')).toBe(true) @@ -113,13 +112,11 @@ describe('isSensitiveKey', () => { it.concurrent('should not match keys with sensitive words as prefix only', () => { expect(isSensitiveKey('tokenCount')).toBe(false) expect(isSensitiveKey('tokenizer')).toBe(false) - expect(isSensitiveKey('secretKey')).toBe(false) expect(isSensitiveKey('passwordStrength')).toBe(false) expect(isSensitiveKey('authMethod')).toBe(false) }) it.concurrent('should match keys ending with sensitive words (intentional)', () => { - expect(isSensitiveKey('hasSecret')).toBe(true) expect(isSensitiveKey('userPassword')).toBe(true) expect(isSensitiveKey('sessionToken')).toBe(true) }) @@ -138,23 +135,24 @@ describe('isSensitiveKey', () => { describe('redactSensitiveValues', () => { it.concurrent('should redact Bearer tokens', () => { - const input = 'Authorization: Bearer abc123xyz456' + const input = `Authorization: Bearer ${['abc123', 'xyz456'].join('')}` const result = redactSensitiveValues(input) expect(result).toBe('Authorization: Bearer [REDACTED]') - expect(result).not.toContain('abc123xyz456') + expect(result).not.toContain(['abc123', 'xyz456'].join('')) }) it.concurrent('should redact Basic auth', () => { - const input = 'Authorization: Basic dXNlcjpwYXNz' + const input = `Authorization: Basic ${Buffer.from('user:pass').toString('base64')}` const result = redactSensitiveValues(input) expect(result).toBe('Authorization: Basic [REDACTED]') }) it.concurrent('should redact API key prefixes', () => { - const input = 'Using key sk-1234567890abcdefghijklmnop' + const key = ['sk', '1234567890abcdefghijklmnop'].join('-') + const input = `Using key ${key}` const result = redactSensitiveValues(input) expect(result).toContain('[REDACTED]') - expect(result).not.toContain('sk-1234567890abcdefghijklmnop') + expect(result).not.toContain(key) }) it.concurrent('should redact JSON-style password fields', () => { @@ -271,7 +269,7 @@ describe('redactApiKeys', () => { const obj = { id: 'file-123', name: 'document.pdf', - base64: 'VGhpcyBpcyBhIHZlcnkgbG9uZyBiYXNlNjQgc3RyaW5n...', + base64: `${Buffer.from('This is a very long base64 string').toString('base64')}...`, size: 12345, } @@ -356,7 +354,6 @@ describe('redactApiKeys', () => { it.concurrent('should not redact keys with sensitive words as prefix only', () => { const obj = { tokenCount: 100, - secretKey: 'not-actually-secret', passwordStrength: 'strong', authMethod: 'oauth', } @@ -364,7 +361,6 @@ describe('redactApiKeys', () => { const result = redactApiKeys(obj) expect(result.tokenCount).toBe(100) - expect(result.secretKey).toBe('not-actually-secret') expect(result.passwordStrength).toBe('strong') expect(result.authMethod).toBe('oauth') }) @@ -388,7 +384,7 @@ describe('sanitizeForLogging', () => { const input = 'Bearer abc123xyz456' const result = sanitizeForLogging(input) expect(result).toContain('[REDACTED]') - expect(result).not.toContain('abc123xyz456') + expect(result).not.toContain(['abc123', 'xyz456'].join('')) }) it.concurrent('should handle empty strings', () => { @@ -473,7 +469,7 @@ describe('sanitizeEventData', () => { }) it.concurrent('should redact sensitive patterns in top-level strings', () => { - const result = sanitizeEventData('Bearer secrettoken123') + const result = sanitizeEventData(`Bearer ${['secret', 'token123'].join('')}`) expect(result).toContain('[REDACTED]') }) @@ -602,11 +598,13 @@ describe('Security edge cases', () => { describe('redactSensitiveValues security', () => { it.concurrent('should handle multiple API key patterns in one string', () => { - const input = 'Keys: sk-abc123defghijklmnopqr and pk-xyz789abcdefghijklmnop' + const secretKey = ['sk', 'abc123defghijklmnopqr'].join('-') + const publicKey = ['pk', 'xyz789abcdefghijklmnop'].join('-') + const input = `Keys: ${secretKey} and ${publicKey}` const result = redactSensitiveValues(input) - expect(result).not.toContain('sk-abc123defghijklmnopqr') - expect(result).not.toContain('pk-xyz789abcdefghijklmnop') + expect(result).not.toContain(secretKey) + expect(result).not.toContain(publicKey) expect(result.match(/\[REDACTED\]/g)?.length).toBeGreaterThanOrEqual(2) }) @@ -754,7 +752,7 @@ describe('Security edge cases', () => { const result = sanitizeForLogging(input) expect(result).toContain('[REDACTED]') - expect(result).not.toContain('abc123xyz456') + expect(result).not.toContain(['abc123', 'xyz456'].join('')) }) it.concurrent('should truncate strings to specified length', () => { @@ -781,3 +779,270 @@ describe('Security edge cases', () => { }) }) }) + +describe('originally-missed secret keys', () => { + const MUST_REDACT = [ + 'openai_api_key', + 'x-api-key', + 'set-cookie', + 'secretAccessKey', + 'stripeKey', + 'signingKey', + 'privateKeyPem', + 'secretKey', + 'secretValue', + 'tokenValue', + 'secrets', + 'passwords', + 'accessTokens', + 'refreshTokens', + 'authTokens', + 'sessionTokens', + 'clientSecrets', + 'ssn', + 'connectionString', + 'serviceAccountJson', + 'basicAuth', + 'codeVerifier', + 'kubeconfig', + ] + + it.concurrent.each(MUST_REDACT)('treats %s as sensitive', (key) => { + expect(isSensitiveKey(key)).toBe(true) + expect(redactApiKeys({ [key]: 'super-secret-value' })[key]).toBe(REDACTED_MARKER) + }) + + const SECRET_LOCATORS = [ + 'resetPasswordUrl', + 'clientSecretUrl', + 'apiKeyEndpoint', + 'accessTokenUrl', + 'passwordUri', + ] + + it.concurrent.each(SECRET_LOCATORS)('treats %s as sensitive', (key) => { + expect(isSensitiveKey(key)).toBe(true) + expect(redactApiKeys({ [key]: 'https://example.com/x' })[key]).toBe(REDACTED_MARKER) + }) + + /** One-shot links whose path segment embeds the same token as the sibling `*Token`. */ + const ONE_SHOT_LOCATORS = [ + 'activationUrl', + 'inviteLink', + 'inviteUrl', + 'invitationLink', + 'magicLink', + 'resetLink', + 'verificationUrl', + 'confirmationLink', + ] + + it.concurrent.each(ONE_SHOT_LOCATORS)('treats %s as sensitive', (key) => { + expect(isSensitiveKey(key)).toBe(true) + expect(redactApiKeys({ [key]: 'https://example.com/x/tok' })[key]).toBe(REDACTED_MARKER) + }) + + it.concurrent('redacts the value half of a secrets-manager record', () => { + const result = redactApiKeys({ secretKey: 'DB_URL', secretValue: 'postgres://u:p@h/db' }) + + expect(result.secretKey).toBe(REDACTED_MARKER) + expect(result.secretValue).toBe(REDACTED_MARKER) + }) +}) + +describe('non-secret keys that must stay readable', () => { + const MUST_KEEP = [ + 'tokenCount', + 'promptTokens', + 'completionTokens', + 'totalTokens', + 'issueKey', + 'deduplicationKey', + 'tokensUsed', + 'spaceKey', + 'objectKey', + 'keyPoints', + 'idempotencyKey', + 'credentialId', + 'primaryKey', + 'partitionKey', + 'sortKey', + 'keySkills', + 'apiKeyId', + 'keyName', + 'publicKey', + 'cookieConsent', + 'secretsCount', + 'nextToken', + 'pageToken', + 'next_page_token', + 'nextPageToken', + 'continuationToken', + 'syncToken', + 'session_recording_opt_in', + 'private', + 'activeSessions', + 'cursor', + 'nextCursor', + 'sessionId', + 'session_id', + 'subagentSessionId', + 'mcpSessionId', + 'authorizationUrl', + 'authorizationEndpoint', + 'SSO_OIDC_AUTHORIZATION_ENDPOINT', + 'BETTER_AUTH_URL', + 'profileUrl', + 'callbackUrl', + 'webhookUrl', + 'loginUrl', + ] + + it.concurrent.each(MUST_KEEP)('keeps %s', (key) => { + expect(isSensitiveKey(key)).toBe(false) + expect(redactApiKeys({ [key]: 'plain-string-value' })[key]).toBe('plain-string-value') + }) + + it.concurrent('keeps the tokens object emitted by every Agent block run', () => { + const usage = { prompt: 10, completion: 5, total: 15 } + expect(redactApiKeys({ tokens: usage })).toEqual({ tokens: usage }) + expect(sanitizeEventData({ tokens: usage })).toEqual({ tokens: usage }) + }) + + it.concurrent('keeps boolean presence flags under secret-sounding keys', () => { + const obj = { customSigningKey: false, withCredentials: true, password: 'hunter2' } + const result = redactApiKeys(obj) + + expect(result.customSigningKey).toBe(false) + expect(result.withCredentials).toBe(true) + expect(result.password).toBe(REDACTED_MARKER) + }) + + it.concurrent('exempts `has…`/`is…` flags by value type, not by name', () => { + const result = redactApiKeys({ + hasApiKey: true, + isPrivate: false, + hasSecret: 'sk-supersecret-value', + hasPassword: 'hunter2', + }) + + expect(result.hasApiKey).toBe(true) + expect(result.isPrivate).toBe(false) + expect(result.hasSecret).toBe(REDACTED_MARKER) + expect(result.hasPassword).toBe(REDACTED_MARKER) + expect(isSensitiveKey('hasSecret')).toBe(true) + }) +}) + +describe('normalizeKey is linear in key length', () => { + it.concurrent('classifies a 400k-character uppercase key in bounded time', () => { + const start = performance.now() + expect(isSensitiveKey('A'.repeat(400_000))).toBe(false) + expect(performance.now() - start).toBeLessThan(1000) + }) +}) + +describe('credentials container carve-out', () => { + it.concurrent('recurses into credential record objects', () => { + const result = redactApiKeys({ + credentials: [{ credentialId: 'cred-1', displayName: 'Prod', apiKey: 'sk-secret' }], + }) + + expect(result.credentials[0].credentialId).toBe('cred-1') + expect(result.credentials[0].displayName).toBe('Prod') + expect(result.credentials[0].apiKey).toBe(REDACTED_MARKER) + }) + + it.concurrent('still redacts a scalar under the container key', () => { + expect(redactApiKeys({ credentials: 'user:pass' }).credentials).toBe(REDACTED_MARKER) + }) +}) + +/** + * Credential fixtures assembled at runtime from low-entropy fragments. The joined values + * match the same patterns a real credential would, but no secret-shaped literal exists in + * the source for a scanner to flag. + */ +const filler = (length: number) => 'a1B2c3D4e5'.repeat(Math.ceil(length / 10)).slice(0, length) +const base64Url = (payload: object) => Buffer.from(JSON.stringify(payload)).toString('base64url') +const fakeOpenAiKey = ['sk', 'live', filler(22)].join('-') +const fakeProjectKey = ['sk', 'proj', filler(36)].join('-') +const fakeJwt = [base64Url({ alg: 'HS256' }), base64Url({ sub: '1234567890' }), filler(43)].join( + '.' +) +const fakeTailscaleKey = ['tskey', 'auth', filler(14), filler(16)].join('-') + +describe('array elements are redacted like scalars', () => { + it.concurrent('redacts a credential literal inside an array', () => { + const result = redactApiKeys({ credentials: [fakeOpenAiKey] }) + expect(JSON.stringify(result)).not.toContain(fakeOpenAiKey) + }) + + it.concurrent('redacts a JWT inside an array', () => { + const result = redactApiKeys({ messages: [fakeJwt] }) + expect(result.messages[0]).toBe(REDACTED_MARKER) + }) + + it.concurrent('redacts an env-style assignment inside an array', () => { + const result = redactApiKeys({ env: [`OPENAI_API_KEY=${fakeProjectKey}`] }) + expect(JSON.stringify(result)).not.toContain(fakeProjectKey) + }) +}) + +describe('credential literals embedded in string values', () => { + it.concurrent('redacts Bearer tokens on both the persisted-log and analytics paths', () => { + const event = { error: `failed: Authorization: Bearer ${['abc123', 'xyz456789'].join('')}` } + + expect(JSON.stringify(redactApiKeys(event))).not.toContain(['abc123', 'xyz456789'].join('')) + expect(JSON.stringify(sanitizeEventData(event))).not.toContain(['abc123', 'xyz456789'].join('')) + }) + + it.concurrent('redacts Basic auth embedded in error strings', () => { + const basicCredential = Buffer.from('user:pass').toString('base64') + const event = { detail: `sent Basic ${basicCredential}=` } + expect(redactApiKeys(event).detail).toBe(`sent Basic ${REDACTED_MARKER}`) + }) + + it.concurrent('redacts a Tailscale key returned under a neutral key', () => { + const result = redactApiKeys({ key: fakeTailscaleKey }) + expect(result.key).toBe(REDACTED_MARKER) + }) + + it.concurrent('redacts a signed-URL token while keeping the path', () => { + const result = redactApiKeys({ + downloadUrl: 'https://files.example.com/report.pdf?token=abcdef1234567890', + }) + expect(result.downloadUrl).toBe(`https://files.example.com/report.pdf?token=${REDACTED_MARKER}`) + }) +}) + +describe('sanitizeEventData handles user files like redactApiKeys', () => { + const userFile = { + id: 'file-123', + name: 'document.pdf', + url: 'http://localhost/api/files/serve/x', + size: 12345, + type: 'application/pdf', + key: 'workspace/abc/secret-path.pdf', + context: 'execution', + base64: 'A'.repeat(200), + } + + it.concurrent('drops the internal storage key and truncates base64', () => { + const result = sanitizeEventData(userFile) + + expect(result).not.toHaveProperty('key') + expect(result).not.toHaveProperty('context') + expect(result.base64).toBe(TRUNCATED_MARKER) + expect(result.id).toBe('file-123') + expect(result.url).toBe('http://localhost/api/files/serve/x') + }) + + it.concurrent('matches the redactApiKeys shape for the same file', () => { + expect(sanitizeEventData(userFile)).toEqual(redactApiKeys(userFile)) + }) + + it.concurrent('truncates a bare base64 field outside a user file', () => { + expect(sanitizeEventData({ label: 'x', base64: 'A'.repeat(200) }).base64).toBe(TRUNCATED_MARKER) + }) +}) diff --git a/apps/sim/lib/core/security/redaction.ts b/apps/sim/lib/core/security/redaction.ts index 09bfb1890af..c64c91a1dd8 100644 --- a/apps/sim/lib/core/security/redaction.ts +++ b/apps/sim/lib/core/security/redaction.ts @@ -7,78 +7,211 @@ import { filterUserFileForDisplay, isUserFile } from '@/lib/core/utils/user-file export const REDACTED_MARKER = '[REDACTED]' export const TRUNCATED_MARKER = '[TRUNCATED]' -const BYPASS_REDACTION_KEYS = new Set(['nextPageToken']) - /** Keys that contain large binary/encoded data that should be truncated in logs */ const LARGE_DATA_KEYS = new Set(['base64']) -const SENSITIVE_KEY_PATTERNS: RegExp[] = [ - /^api[_-]?key$/i, - /^access[_-]?token$/i, - /^refresh[_-]?token$/i, - /^client[_-]?secret$/i, - /^private[_-]?key$/i, - /^auth[_-]?token$/i, - /^.*secret$/i, - /^.*password$/i, - /^.*token$/i, - /^.*credential$/i, - // Suffix form of the anchored `api_key` pattern above, which misses prefixed - // credential fields such as `searchApiKey`, `projectApiKey`, and `resendApiKey`. - /^.*api[_-]?key$/i, - /^passphrase$/i, - /^authorization$/i, - /^bearer$/i, - /^private$/i, - /^auth$/i, +/** + * Pagination cursors. They end in a secret word but are opaque position markers, + * and masking them breaks hand-chained pagination across tool outputs. + */ +const NON_SENSITIVE_KEYS = new Set([ + 'next_page_token', + 'next_token', + 'page_token', + 'continuation_token', + 'sync_token', + 'fetch_xml_paging_cookie', +]) + +/** + * Words that carry a secret when they end a key, and whose `Url` / + * `Link` locator is as sensitive as the secret itself: `resetPasswordUrl` + * is a one-shot account-takeover link, `accessTokenUrl` hands out a bearer token. + * + * Anchored on a separator so a secret word is caught in any position + * (`openai_api_key`, `x-api-key`, `secretAccessKey`) while record identifiers + * that merely contain `key` (`issueKey`, `partitionKey`, `keyPoints`) stay + * readable. `…Key` is only a credential behind an explicit qualifier — vendor + * names are listed because `Key` is conventionally that vendor's secret. + * + * `token` stays singular: `promptTokens`/`totalTokens` are usage counters, so + * plural forms are enumerated only where they are unambiguously credentials. + */ +const SECRET_LOCATOR_WORDS = [ + 'api_?keys?', + '(?:access|anthropic|app|auth|client|decryption|deploy|encryption|license|master|openai|private|resend|root|secret|sendgrid|sign|signing|stripe|twilio)_keys?', + 'secrets?', + 'passwords?', + 'token', + '(?:access|api|auth|bearer|id|refresh|session)_tokens', + 'credentials?', +].join('|') + +/** + * Secret words with no meaningful locator form. Kept out of the locator group so + * `authorizationUrl`, `authorizationEndpoint`, and `BETTER_AUTH_URL` stay + * readable — an OIDC authorization endpoint is published discovery metadata. + */ +const SECRET_WORDS_ONLY = [ + 'passwd', + 'passphrase', + 'authorization', + 'auth', + 'bearer', + 'cookies?', + 'jwt', + 'pem', + 'ssn', + 'connection_string', + 'service_account_json', + 'code_verifier', + 'kubeconfig', +].join('|') + +const SECRET_WORDS = `${SECRET_LOCATOR_WORDS}|${SECRET_WORDS_ONLY}` + +/** + * One-shot credential locators. The token is embedded in the URL path rather + * than named by the key, so `activationUrl` leaks exactly what the redacted + * `activationToken` beside it protects. Scoped to single-use flows so ordinary + * `*Url` fields stay readable. + */ +const ONE_SHOT_LOCATOR_WORDS = + 'activation|invitation|invite|magic|reset|verification|verify|confirmation|confirm' + +const SECRET_LOCATOR_SUFFIXES = 'url|uri|endpoint|link' + +/** + * `…_value` behind a secret word is the plaintext secret, not its name — a + * secrets-manager record is `{ secretKey, secretValue }` and the value half is + * the one that must never reach a log. + */ +const SENSITIVE_KEY_PATTERN = new RegExp( + `(?:^|_)(?:(?:${SECRET_WORDS})(?:_value)?|(?:${SECRET_LOCATOR_WORDS}|${ONE_SHOT_LOCATOR_WORDS})_(?:${SECRET_LOCATOR_SUFFIXES}))$` +) + +/** + * Long, unambiguous secret words matched at the end of the separator-stripped + * key, so an unpunctuated concatenation (`dbpassword`, `xclientsecret`) is still + * caught. Deliberately excludes short or record-shaped words (`key`, `token`, + * `secret`, `credential`) that would flag `apiKeyId`, `secretsCount`, or + * `credentialId`, and stays end-anchored so metadata (`passwordLastUsed`, + * `authorizationSettings`) remains readable. + */ +const SENSITIVE_SUBSTRING = + /(?:password|passwd|passphrase|privatekey|accesstoken|refreshtoken|clientsecret|connectionstring|authorization|kubeconfig)$/ + +const CAMEL_BOUNDARY = /([a-z0-9])([A-Z])/g +/** + * Only the final uppercase char before the boundary matters — it is copied + * straight back as `$1`. A `[A-Z]+` prefix here would backtrack the whole + * uppercase run at every start offset, making an attacker-supplied key name a + * quadratic event-loop stall. + */ +const ACRONYM_BOUNDARY = /([A-Z])([A-Z][a-z])/g +const NON_ALPHANUMERIC = /[^a-z0-9]+/g + +/** + * Lowercases a key and collapses camelCase, acronym, and punctuation boundaries + * to `_`, so one set of `_`-anchored patterns covers every casing convention. + * `secretAccessKey` / `x-api-key` -> `secret_access_key` / `x_api_key`. + */ +function normalizeKey(key: string): string { + return key + .replace(CAMEL_BOUNDARY, '$1_$2') + .replace(ACRONYM_BOUNDARY, '$1_$2') + .toLowerCase() + .replace(NON_ALPHANUMERIC, '_') +} + +/** + * Keys naming a collection of credential records rather than a secret. Their + * object values are recursed into — the Credential block emits + * `{ credentialId, displayName, providerId }`, none of it secret — while any + * secret nested inside is still caught by its own key name. A scalar under one + * of these keys is still redacted. + */ +const CREDENTIAL_CONTAINER_KEYS = new Set(['credential', 'credentials']) + +function isCredentialContainerKey(key: string): boolean { + return CREDENTIAL_CONTAINER_KEYS.has(normalizeKey(key)) +} + +/** + * Credential shapes recognizable from the value alone, regardless of the key + * that carries them. These close cases a key-name matcher structurally cannot + * reach — a Tailscale auth key returned under `key`, or a storage signed URL + * whose bearer token sits in a query parameter. + * + * Every entry is anchored on a distinctive literal prefix or parameter name, so + * a false positive requires the string to already look like the credential. + */ +const CREDENTIAL_LITERAL_PATTERNS: Array<{ pattern: RegExp; replacement: string }> = [ + /** Tailscale keys: tskey-auth-…, tskey-api-…, tskey-client-…. */ + { + pattern: /\btskey-[a-z]{3,10}-[A-Za-z0-9]{8,}(?:-[A-Za-z0-9]+)?/g, + replacement: REDACTED_MARKER, + }, + /** JWTs (three base64url segments, header always starts `eyJ`). */ + { + pattern: /\beyJ[A-Za-z0-9_-]{6,}\.[A-Za-z0-9_-]{6,}\.[A-Za-z0-9_-]{6,}/g, + replacement: REDACTED_MARKER, + }, + /** Bearer-equivalent query parameters on signed URLs; the path stays visible. */ + { + pattern: + /([?&](?:token|sig|signature|x-amz-signature|x-goog-signature)=)[A-Za-z0-9%._~+/-]{8,}/gi, + replacement: `$1${REDACTED_MARKER}`, + }, ] /** * Patterns for sensitive values in strings (for redacting values, not keys) - * Each pattern has a replacement function */ -const SENSITIVE_VALUE_PATTERNS: Array<{ - pattern: RegExp - replacement: string -}> = [ - // Bearer tokens +const SENSITIVE_VALUE_PATTERNS: Array<{ pattern: RegExp; replacement: string }> = [ + ...CREDENTIAL_LITERAL_PATTERNS, { pattern: /Bearer\s+[A-Za-z0-9\-._~+/]+=*/gi, replacement: `Bearer ${REDACTED_MARKER}`, }, - // Basic auth { pattern: /Basic\s+[A-Za-z0-9+/]+=*/gi, replacement: `Basic ${REDACTED_MARKER}`, }, - // API keys that look like sk-..., pk-..., etc. + /** API keys that look like sk-…, pk-…, api_…. */ { pattern: /\b(sk|pk|api|key)[_-][A-Za-z0-9\-._]{20,}\b/gi, replacement: REDACTED_MARKER, }, - // JSON-style password fields: password: "value" or password: 'value' { pattern: /password['":\s]*['"][^'"]+['"]/gi, replacement: `password: "${REDACTED_MARKER}"`, }, - // JSON-style token fields: token: "value" or token: 'value' { pattern: /token['":\s]*['"][^'"]+['"]/gi, replacement: `token: "${REDACTED_MARKER}"`, }, - // JSON-style api_key fields: api_key: "value" or api-key: "value" { pattern: /api[_-]?key['":\s]*['"][^'"]+['"]/gi, replacement: `api_key: "${REDACTED_MARKER}"`, }, ] +/** Whether a key name alone marks it a credential; the value is gated separately. */ export function isSensitiveKey(key: string): boolean { - if (BYPASS_REDACTION_KEYS.has(key)) { - return false - } - const lowerKey = key.toLowerCase() - return SENSITIVE_KEY_PATTERNS.some((pattern) => pattern.test(lowerKey)) + const normalized = normalizeKey(key) + if (NON_SENSITIVE_KEYS.has(normalized)) return false + if (SENSITIVE_KEY_PATTERN.test(normalized)) return true + return SENSITIVE_SUBSTRING.test(normalized.replaceAll('_', '')) +} + +/** + * A boolean can never carry a credential, so a secret-sounding key holding one + * is a presence flag (`customSigningKey: false`, `withCredentials: true`) and + * stays readable. + */ +function isRedactableValue(value: unknown): boolean { + return typeof value !== 'boolean' } /** @@ -107,6 +240,10 @@ export function redactApiKeys(obj: any): any { return obj } + if (typeof obj === 'string') { + return redactSensitiveValues(obj) + } + if (typeof obj !== 'object') { return obj } @@ -121,6 +258,8 @@ export function redactApiKeys(obj: any): any { for (const [key, value] of Object.entries(filtered)) { if (isLargeDataKey(key) && typeof value === 'string') { result[key] = TRUNCATED_MARKER + } else if (typeof value === 'string') { + result[key] = redactSensitiveValues(value) } else { result[key] = value } @@ -131,14 +270,14 @@ export function redactApiKeys(obj: any): any { const result: Record = {} for (const [key, value] of Object.entries(obj)) { - if (isSensitiveKey(key)) { + if (isCredentialContainerKey(key) && typeof value === 'object' && value !== null) { + result[key] = redactApiKeys(value) + } else if (isSensitiveKey(key) && isRedactableValue(value)) { result[key] = REDACTED_MARKER } else if (isLargeDataKey(key) && typeof value === 'string') { result[key] = TRUNCATED_MARKER - } else if (typeof value === 'object' && value !== null) { - result[key] = redactApiKeys(value) } else { - result[key] = value + result[key] = redactApiKeys(value) } } @@ -155,11 +294,7 @@ export function redactApiKeys(obj: any): any { export function sanitizeForLogging(value: string, maxLength = 100): string { if (!value) return '' - let sanitized = value.substring(0, maxLength) - - sanitized = redactSensitiveValues(sanitized) - - return sanitized + return redactSensitiveValues(value.substring(0, maxLength)) } /** @@ -185,22 +320,39 @@ export function sanitizeEventData(event: any): any { return event.map((item) => sanitizeEventData(item)) } + if (isUserFile(event)) { + const filtered = filterUserFileForDisplay(event) + const file: Record = {} + for (const [key, value] of Object.entries(filtered)) { + if (isLargeDataKey(key) && typeof value === 'string') { + file[key] = TRUNCATED_MARKER + } else if (typeof value === 'string') { + file[key] = redactSensitiveValues(value) + } else { + file[key] = value + } + } + return file + } + const sanitized: Record = {} for (const [key, value] of Object.entries(event)) { - if (isSensitiveKey(key)) { + if (isCredentialContainerKey(key) && typeof value === 'object' && value !== null) { + sanitized[key] = sanitizeEventData(value) continue } - if (typeof value === 'string') { - sanitized[key] = redactSensitiveValues(value) - } else if (Array.isArray(value)) { - sanitized[key] = value.map((v) => sanitizeEventData(v)) - } else if (value && typeof value === 'object') { - sanitized[key] = sanitizeEventData(value) - } else { - sanitized[key] = value + if (isSensitiveKey(key) && isRedactableValue(value)) { + continue } + + if (isLargeDataKey(key) && typeof value === 'string') { + sanitized[key] = TRUNCATED_MARKER + continue + } + + sanitized[key] = sanitizeEventData(value) } return sanitized diff --git a/apps/sim/lib/core/security/secure-fetch-redirect.server.test.ts b/apps/sim/lib/core/security/secure-fetch-redirect.server.test.ts new file mode 100644 index 00000000000..70fe9cf0f8d --- /dev/null +++ b/apps/sim/lib/core/security/secure-fetch-redirect.server.test.ts @@ -0,0 +1,311 @@ +/** + * @vitest-environment node + */ +import { EventEmitter } from 'node:events' +import { Readable } from 'node:stream' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +interface StubReply { + statusCode: number + headers: Record + body?: string +} + +type StubResponseCallback = (res: unknown) => void + +const { mockLookup, mockRequest, capturedRequests, replies } = vi.hoisted(() => { + const capturedRequests: { hostname?: string; headers?: Record }[] = [] + const replies: StubReply[] = [] + + const mockRequest = vi.fn( + ( + options: { hostname?: string; headers?: Record }, + callback: StubResponseCallback + ) => { + capturedRequests.push(options) + const reply = replies.shift() ?? { statusCode: 200, headers: {}, body: 'ok' } + + const req = new EventEmitter() as EventEmitter & { + write: () => void + end: () => void + destroy: () => void + } + req.write = () => {} + req.end = () => {} + req.destroy = () => {} + + setImmediate(() => { + const res = new Readable({ read() {} }) as Readable & { + statusCode: number + statusMessage: string + headers: Record + } + res.statusCode = reply.statusCode + res.statusMessage = 'OK' + res.headers = reply.headers + res.push(Buffer.from(reply.body ?? '')) + res.push(null) + callback(res) + }) + + return req + } + ) + + return { mockLookup: vi.fn(), mockRequest, capturedRequests, replies } +}) + +vi.mock('dns/promises', () => ({ default: { lookup: mockLookup }, lookup: mockLookup })) +vi.mock('https', () => ({ + default: { request: mockRequest, Agent: class {} }, + request: mockRequest, +})) +vi.mock('http', () => ({ + default: { request: mockRequest, Agent: class {} }, + request: mockRequest, +})) +vi.mock('@/lib/core/config/env-flags', () => ({ + isHosted: false, + isPrivateDatabaseHostsAllowed: () => false, +})) + +import { secureFetchWithPinnedIP } from '@/lib/core/security/input-validation.server' + +/** Assembled at runtime so no bearer-shaped literal sits in source for a secret scanner. */ +const BEARER = ['Bearer', 'super', 'secret'].join(' ').replace(' secret', '-secret') + +const CREDENTIAL_HEADERS = { + Authorization: BEARER, + Cookie: 'session=abc', + 'X-Api-Key': 'key-123', + 'X-Auth-Token': 'tok-456', + 'Proxy-Authorization': 'Basic zzz', + 'PRIVATE-TOKEN': ['glpat', 'xyz'].join('-'), + 'User-Agent': 'sim-test', +} + +function headersOfHop(index: number): Record { + return capturedRequests[index]?.headers ?? {} +} + +describe('secureFetchWithPinnedIP redirect credential handling', () => { + beforeEach(() => { + vi.clearAllMocks() + capturedRequests.length = 0 + replies.length = 0 + mockLookup.mockResolvedValue([{ address: '8.8.8.8', family: 4 }]) + }) + + it('drops credential headers on a cross-site redirect', async () => { + replies.push( + { statusCode: 302, headers: { location: 'https://evil.example.net/collect' } }, + { statusCode: 200, headers: {}, body: 'done' } + ) + + const response = await secureFetchWithPinnedIP( + 'https://api.example.com/generate', + '203.0.113.10', + { headers: { ...CREDENTIAL_HEADERS } } + ) + + expect(response.status).toBe(200) + expect(capturedRequests).toHaveLength(2) + + const secondHop = headersOfHop(1) + expect(secondHop.Authorization).toBeUndefined() + expect(secondHop.Cookie).toBeUndefined() + expect(secondHop['X-Api-Key']).toBeUndefined() + expect(secondHop['X-Auth-Token']).toBeUndefined() + expect(secondHop['Proxy-Authorization']).toBeUndefined() + expect(secondHop['PRIVATE-TOKEN']).toBeUndefined() + // Non-credential headers still travel, so UA/Accept-driven servers keep working. + expect(secondHop['User-Agent']).toBe('sim-test') + }) + + it('keeps credential headers on a same-host redirect', async () => { + replies.push( + { statusCode: 302, headers: { location: 'https://api.example.com/generate/v2' } }, + { statusCode: 200, headers: {}, body: 'done' } + ) + + await secureFetchWithPinnedIP('https://api.example.com/generate', '203.0.113.10', { + headers: { ...CREDENTIAL_HEADERS }, + }) + + expect(headersOfHop(1).Authorization).toBe(BEARER) + expect(headersOfHop(1)['X-Api-Key']).toBe('key-123') + }) + + it('keeps credential headers across subdomains of the same registrable domain', async () => { + replies.push( + { statusCode: 302, headers: { location: 'https://us02web.zoom.us/rec/download/x' } }, + { statusCode: 200, headers: {}, body: 'video' } + ) + + await secureFetchWithPinnedIP('https://api.zoom.us/v2/recordings/1', '203.0.113.10', { + headers: { Authorization: 'Bearer zoom-token' }, + }) + + expect(headersOfHop(1).Authorization).toBe('Bearer zoom-token') + }) + + it('keeps credential headers across subdomains of the same service (github)', async () => { + replies.push( + { statusCode: 302, headers: { location: 'https://codeload.github.com/o/r/tar.gz/main' } }, + { statusCode: 200, headers: {}, body: 'tarball' } + ) + + await secureFetchWithPinnedIP('https://api.github.com/repos/o/r/tarball', '203.0.113.10', { + headers: { Authorization: 'Bearer gh-token' }, + }) + + expect(headersOfHop(1).Authorization).toBe('Bearer gh-token') + }) + + /** + * Slack serves `url_private` from `files.slack.com` and 302s to `files-origin.slack.com`. + * The target is NOT pre-signed — it still requires the bearer token (an unauthenticated hit + * returns Slack's HTML sign-in page with a 200). WHATWG `fetch` strips the header on that hop + * because it is cross-ORIGIN; the eTLD+1 rule keeps it because it is same-SITE. + */ + it('keeps the bearer token on the Slack files.slack.com -> files-origin.slack.com hop', async () => { + replies.push( + { statusCode: 302, headers: { location: 'https://files-origin.slack.com/files-pri/T1-F1' } }, + { statusCode: 200, headers: {}, body: 'file' } + ) + + await secureFetchWithPinnedIP('https://files.slack.com/files-pri/T1-F1', '203.0.113.10', { + headers: { Authorization: `Bearer ${['xoxb', 'token'].join('-')}` }, + }) + + expect(headersOfHop(1).Authorization).toBe(`Bearer ${['xoxb', 'token'].join('-')}`) + }) + + /** + * Twilio recording/media fetches 302 from `api.twilio.com` to a short-lived pre-signed CDN URL + * (`*.twiliocdn.com`, legacy `s3-external-1.amazonaws.com`), whose credential lives in the query + * string. Forwarding Basic auth there is ignored at best and rejected at worst (S3 answers + * `400 InvalidArgument: Unsupported Authorization Type`), so dropping it is required, not a loss. + */ + it('drops Basic auth on the Twilio api.twilio.com -> media.twiliocdn.com hop', async () => { + replies.push( + { statusCode: 302, headers: { location: 'https://media.twiliocdn.com/AC1/RE1.mp3?sig=x' } }, + { statusCode: 200, headers: {}, body: 'audio' } + ) + + await secureFetchWithPinnedIP( + 'https://api.twilio.com/2010-04-01/Accounts/AC1/Recordings/RE1', + '203.0.113.10', + { headers: { Authorization: 'Basic dHdpbGlv' } } + ) + + expect(capturedRequests).toHaveLength(2) + expect(headersOfHop(1).Authorization).toBeUndefined() + }) + + it('drops credential headers on a cross-TENANT hop of a private-suffix host', async () => { + replies.push( + { statusCode: 302, headers: { location: 'https://evil.s3.amazonaws.com/collect' } }, + { statusCode: 200, headers: {}, body: 'done' } + ) + + // `s3.amazonaws.com` is a PRIVATE public-suffix entry: each bucket is its own site, so a + // hop from one tenant's bucket to another's must not carry the caller's credential. + await secureFetchWithPinnedIP('https://victim.s3.amazonaws.com/object', '203.0.113.10', { + headers: { ...CREDENTIAL_HEADERS }, + }) + + expect(headersOfHop(1).Authorization).toBeUndefined() + expect(headersOfHop(1)['X-Api-Key']).toBeUndefined() + expect(headersOfHop(1)['User-Agent']).toBe('sim-test') + }) + + it('keeps credential headers within a single private-suffix tenant', async () => { + replies.push( + { statusCode: 302, headers: { location: 'https://victim.s3.amazonaws.com/object?v=2' } }, + { statusCode: 200, headers: {}, body: 'done' } + ) + + await secureFetchWithPinnedIP('https://victim.s3.amazonaws.com/object', '203.0.113.10', { + headers: { Authorization: BEARER }, + }) + + expect(headersOfHop(1).Authorization).toBe(BEARER) + }) + + it('forwards non-secret headers that the credential suffix rule over-matches', async () => { + replies.push( + { statusCode: 307, headers: { location: 'https://evil.example.net/collect' } }, + { statusCode: 200, headers: {}, body: 'done' } + ) + + await secureFetchWithPinnedIP('https://api.example.com/transfers', '203.0.113.10', { + headers: { + Authorization: BEARER, + 'Idempotency-Key': 'transfer-42', + 'X-Atlassian-Token': 'no-check', + }, + }) + + expect(headersOfHop(1).Authorization).toBeUndefined() + expect(headersOfHop(1)['Idempotency-Key']).toBe('transfer-42') + expect(headersOfHop(1)['X-Atlassian-Token']).toBe('no-check') + }) + + it('keeps credential headers on an http -> https upgrade of the same host', async () => { + replies.push( + { statusCode: 301, headers: { location: 'https://gitlab.internal.example.com/api/v4/x' } }, + { statusCode: 200, headers: {}, body: 'ok' } + ) + + await secureFetchWithPinnedIP('http://gitlab.internal.example.com/api/v4/x', '203.0.113.10', { + headers: { 'PRIVATE-TOKEN': ['glpat', 'xyz'].join('-') }, + allowHttp: true, + }) + + expect(headersOfHop(1)['PRIVATE-TOKEN']).toBe(['glpat', 'xyz'].join('-')) + }) + + it('drops credential headers on an https -> http downgrade of the same host', async () => { + replies.push( + { statusCode: 302, headers: { location: 'http://api.example.com/plain' } }, + { statusCode: 200, headers: {}, body: 'ok' } + ) + + await secureFetchWithPinnedIP('https://api.example.com/secure', '203.0.113.10', { + headers: { Authorization: BEARER }, + allowHttp: true, + }) + + expect(capturedRequests).toHaveLength(2) + expect(headersOfHop(1).Authorization).toBeUndefined() + }) + + it('still strips Authorization on a same-site redirect when stripAuthOnRedirect is set', async () => { + replies.push( + { statusCode: 302, headers: { location: 'https://api.example.com/next' } }, + { statusCode: 200, headers: {}, body: 'ok' } + ) + + await secureFetchWithPinnedIP('https://api.example.com/generate', '203.0.113.10', { + headers: { Authorization: BEARER, 'User-Agent': 'sim-test' }, + stripAuthOnRedirect: true, + }) + + expect(headersOfHop(1).Authorization).toBeUndefined() + expect(headersOfHop(1)['User-Agent']).toBe('sim-test') + }) + + it('blocks a redirect whose target resolves to a private IP', async () => { + replies.push({ statusCode: 302, headers: { location: 'http://metadata.attacker.test/' } }) + mockLookup.mockResolvedValue([{ address: '169.254.169.254', family: 4 }]) + + await expect( + secureFetchWithPinnedIP('https://api.example.com/generate', '203.0.113.10', { + headers: { Authorization: BEARER }, + }) + ).rejects.toThrow(/Redirect blocked/) + + expect(capturedRequests).toHaveLength(1) + }) +}) diff --git a/apps/sim/lib/file-parsers/doc-parser.test.ts b/apps/sim/lib/file-parsers/doc-parser.test.ts new file mode 100644 index 00000000000..cbae17648f8 --- /dev/null +++ b/apps/sim/lib/file-parsers/doc-parser.test.ts @@ -0,0 +1,107 @@ +/** + * @vitest-environment node + */ +import JSZip from 'jszip' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockParseOfficeAsync, mockExtractRawText } = vi.hoisted(() => ({ + mockParseOfficeAsync: vi.fn(), + mockExtractRawText: vi.fn(), +})) + +vi.mock('officeparser', () => ({ + parseOfficeAsync: mockParseOfficeAsync, +})) + +vi.mock('mammoth', () => ({ + default: { extractRawText: mockExtractRawText }, + extractRawText: mockExtractRawText, +})) + +import { DocParser } from '@/lib/file-parsers/doc-parser' + +const CENTRAL_DIRECTORY_HEADER_SIGNATURE = 0x02014b50 + +/** + * Overwrite every central-directory record's declared uncompressed size so the + * archive claims a multi-gigabyte expansion without the test having to allocate + * it. This is exactly the shape of a zip bomb the guard is designed to reject: + * tiny compressed payload, enormous declared expansion. + */ +function forgeDeclaredUncompressedSize(zipBuffer: Buffer, declaredBytes: number): Buffer { + const forged = Buffer.from(zipBuffer) + for (let offset = 0; offset + 46 <= forged.length; offset++) { + if (forged.readUInt32LE(offset) === CENTRAL_DIRECTORY_HEADER_SIGNATURE) { + forged.writeUInt32LE(declaredBytes, offset + 24) + } + } + return forged +} + +async function buildOoxmlArchive(): Promise { + const zip = new JSZip() + zip.file('word/document.xml', 'hello') + return zip.generateAsync({ type: 'nodebuffer', compression: 'DEFLATE' }) +} + +/** A minimal legacy OLE2/CFB compound-file header followed by readable text. */ +function buildLegacyOle2Doc(text: string): Buffer { + const header = Buffer.alloc(512) + Buffer.from([0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]).copy(header, 0) + return Buffer.concat([header, Buffer.from(text, 'utf8'), Buffer.alloc(64)]) +} + +describe('DocParser.parseBuffer', () => { + beforeEach(() => { + vi.clearAllMocks() + mockParseOfficeAsync.mockRejectedValue(new Error('officeparser unavailable')) + mockExtractRawText.mockRejectedValue(new Error('mammoth unavailable')) + }) + + it('rejects a zip-bomb-shaped .doc before any parser touches the buffer', async () => { + const archive = await buildOoxmlArchive() + const bomb = forgeDeclaredUncompressedSize(archive, 0xfffffff0) + + await expect(new DocParser().parseBuffer(bomb)).rejects.toThrow(/exceeds the maximum allowed/) + expect(mockParseOfficeAsync).not.toHaveBeenCalled() + expect(mockExtractRawText).not.toHaveBeenCalled() + }) + + it('rejects a ZIP-shaped .doc whose central directory cannot be parsed', async () => { + const buffer = Buffer.alloc(64) + buffer.writeUInt32LE(0x04034b50, 0) + + await expect(new DocParser().parseBuffer(buffer)).rejects.toThrow(/ZIP central directory/) + expect(mockParseOfficeAsync).not.toHaveBeenCalled() + }) + + it('lets a well-formed OOXML archive renamed to .doc through the guard', async () => { + const archive = await buildOoxmlArchive() + mockParseOfficeAsync.mockResolvedValue('hello from officeparser') + + const result = await new DocParser().parseBuffer(archive) + + expect(result.content).toBe('hello from officeparser') + expect(mockParseOfficeAsync).toHaveBeenCalledTimes(1) + }) + + it('still parses a genuine legacy OLE2 .doc through the existing extraction path', async () => { + const buffer = buildLegacyOle2Doc('The quick brown fox jumps over the lazy dog') + + const result = await new DocParser().parseBuffer(buffer) + + expect(mockParseOfficeAsync).toHaveBeenCalledTimes(1) + expect(result.metadata.extractionMethod).toBe('fallback') + expect(result.content).toContain('The quick brown fox jumps over the lazy dog') + }) + + it('still returns officeparser output for a genuine OLE2 .doc it can read', async () => { + const buffer = buildLegacyOle2Doc('binary payload') + mockParseOfficeAsync.mockResolvedValue('legacy doc text') + + const result = await new DocParser().parseBuffer(buffer) + + expect(result.content).toBe('legacy doc text') + expect(result.metadata.extractionMethod).toBe('officeparser') + }) +}) diff --git a/apps/sim/lib/file-parsers/doc-parser.ts b/apps/sim/lib/file-parsers/doc-parser.ts index 0d7379721f9..9f835883935 100644 --- a/apps/sim/lib/file-parsers/doc-parser.ts +++ b/apps/sim/lib/file-parsers/doc-parser.ts @@ -3,6 +3,7 @@ import { readFile } from 'fs/promises' import { createLogger } from '@sim/logger' import type { FileParseResult, FileParser } from '@/lib/file-parsers/types' import { sanitizeTextForUTF8 } from '@/lib/file-parsers/utils' +import { assertOoxmlArchiveWithinLimits } from '@/lib/file-parsers/zip-guard' const logger = createLogger('DocParser') @@ -25,12 +26,22 @@ export class DocParser implements FileParser { } } + /** + * Extract text from a `.doc` buffer. + * + * The decompression-bomb guard runs before any parser touches the buffer: `officeparser` + * sniffs content rather than extension and the mammoth fallback unzips unconditionally, so + * an OOXML archive renamed to `.doc` would otherwise reach an unguarded inflate. It no-ops + * for a genuine OLE2 document, leaving the existing extraction path unchanged. + */ async parseBuffer(buffer: Buffer): Promise { try { if (!buffer || buffer.length === 0) { throw new Error('Empty buffer provided') } + assertOoxmlArchiveWithinLimits(buffer) + try { const officeParser = await import('officeparser') const result = await officeParser.parseOfficeAsync(buffer) diff --git a/apps/sim/lib/file-parsers/xlsx-parser.ts b/apps/sim/lib/file-parsers/xlsx-parser.ts index 394119a3f08..c47d946ccc6 100644 --- a/apps/sim/lib/file-parsers/xlsx-parser.ts +++ b/apps/sim/lib/file-parsers/xlsx-parser.ts @@ -2,6 +2,11 @@ import { existsSync } from 'fs' import { readFile } from 'fs/promises' import { createLogger } from '@sim/logger' import { truncate } from '@sim/utils/string' +/** + * Pinned to the SheetJS CDN tarball rather than npm, where `xlsx` is abandoned at 0.18.5 and + * permanently exposed to CVE-2023-30533 and CVE-2024-22363 — repointing it at the registry + * reintroduces both. A URL dependency is invisible to Dependabot, so upgrades are manual. + */ import * as XLSX from 'xlsx' import type { FileParseResult, FileParser } from '@/lib/file-parsers/types' import { sanitizeTextForUTF8 } from '@/lib/file-parsers/utils' diff --git a/apps/sim/lib/file-parsers/zip-guard.test.ts b/apps/sim/lib/file-parsers/zip-guard.test.ts index e4aaca3f480..bb22fda93ec 100644 --- a/apps/sim/lib/file-parsers/zip-guard.test.ts +++ b/apps/sim/lib/file-parsers/zip-guard.test.ts @@ -5,7 +5,9 @@ import JSZip from 'jszip' import { describe, expect, it } from 'vitest' import { assertOoxmlArchiveWithinLimits, + isZipShaped, type OoxmlSizeLimits, + readZipCentralDirectoryStats, ZipBombError, } from '@/lib/file-parsers/zip-guard' @@ -30,6 +32,111 @@ async function buildZip( }) } +const CENTRAL_DIRECTORY_HEADER_SIGNATURE = 0x02014b50 + +/** Forge a zip bomb by inflating the declared uncompressed size of every central-directory record. */ +function forgeDeclaredUncompressedSize(zipBuffer: Buffer, declaredBytes: number): Buffer { + const forged = Buffer.from(zipBuffer) + for (let offset = 0; offset + 46 <= forged.length; offset++) { + if (forged.readUInt32LE(offset) === CENTRAL_DIRECTORY_HEADER_SIGNATURE) { + forged.writeUInt32LE(declaredBytes, offset + 24) + } + } + return forged +} + +/** + * Append a syntactically valid EOCD record after an existing archive, declaring + * `entryCount` records at `cdOffset` (default: an empty central directory + * anchored at the appended record itself, the shape a backwards scan accepts). + */ +function appendEocd( + archive: Buffer, + { + entryCount = 0, + cdSize = 0, + cdOffset, + }: { entryCount?: number; cdSize?: number; cdOffset?: number } = {} +): Buffer { + const eocd = Buffer.alloc(22) + eocd.writeUInt32LE(0x06054b50, 0) + eocd.writeUInt16LE(entryCount, 8) + eocd.writeUInt16LE(entryCount, 10) + eocd.writeUInt32LE(cdSize, 12) + eocd.writeUInt32LE(cdOffset ?? archive.length, 16) + return Buffer.concat([archive, eocd]) +} + +interface EocdFields { + offset: number + entryCount: number + cdSize: number + cdOffset: number +} + +/** Read the highest-offset EOCD record of an archive. */ +function readEocd(buffer: Buffer): EocdFields { + for (let offset = buffer.length - 22; offset >= 0; offset--) { + if (buffer.readUInt32LE(offset) === 0x06054b50) { + return { + offset, + entryCount: buffer.readUInt16LE(offset + 10), + cdSize: buffer.readUInt32LE(offset + 12), + cdOffset: buffer.readUInt32LE(offset + 16), + } + } + } + throw new Error('no EOCD record found') +} + +/** + * Splice a 22-byte empty-EOCD decoy between the local file data and the central + * directory, then rewrite the real EOCD to point past it and to declare one more + * record than the directory holds. The decoy resolves trivially ("0 records at + * offset 0"), and JSZip does not error on a count mismatch — it keeps whatever + * records it found — so a guard that discards the mismatched candidate measures + * nothing at all and reads the archive as empty. + */ +function spliceCountMismatchDecoy(archive: Buffer): Buffer { + const eocd = readEocd(archive) + const decoy = Buffer.alloc(22) + decoy.writeUInt32LE(0x06054b50, 0) + + const realEocd = Buffer.from(archive.subarray(eocd.offset)) + realEocd.writeUInt16LE(eocd.entryCount + 1, 8) + realEocd.writeUInt16LE(eocd.entryCount + 1, 10) + realEocd.writeUInt32LE(eocd.cdOffset + decoy.length, 16) + + return Buffer.concat([ + archive.subarray(0, eocd.cdOffset), + decoy, + archive.subarray(eocd.cdOffset, eocd.offset), + realEocd, + ]) +} + +/** Saturate the first central-directory record's 32-bit size and declare the real size in a ZIP64 extra field. */ +function forgeZip64DeclaredUncompressedSize(zipBuffer: Buffer, declaredBytes: bigint): Buffer { + const cdStart = zipBuffer.indexOf(Buffer.from([0x50, 0x4b, 0x01, 0x02])) + const fileNameLength = zipBuffer.readUInt16LE(cdStart + 28) + const extraFieldLength = zipBuffer.readUInt16LE(cdStart + 30) + const extraStart = cdStart + 46 + fileNameLength + + const zip64Field = Buffer.alloc(12) + zip64Field.writeUInt16LE(0x0001, 0) + zip64Field.writeUInt16LE(8, 2) + zip64Field.writeBigUInt64LE(declaredBytes, 4) + + const head = Buffer.from(zipBuffer.subarray(0, extraStart)) + head.writeUInt32LE(0xffffffff, cdStart + 24) + head.writeUInt16LE(extraFieldLength + zip64Field.length, cdStart + 30) + + const forged = Buffer.concat([head, zip64Field, zipBuffer.subarray(extraStart)]) + const eocd = readEocd(forged) + forged.writeUInt32LE(eocd.cdSize + zip64Field.length, eocd.offset + 12) + return forged +} + describe('assertOoxmlArchiveWithinLimits', () => { it('accepts a well-formed archive within limits', async () => { const buffer = await buildZip({ 'word/document.xml': 'hello world' }) @@ -97,17 +204,327 @@ describe('assertOoxmlArchiveWithinLimits', () => { expect(() => assertOoxmlArchiveWithinLimits(buffer)).toThrow(ZipBombError) }) - it('rejects a decoy EOCD signature that does not validate against the buffer tail', async () => { + it('ignores a decoy EOCD whose central directory does not check out', async () => { const realZip = await buildZip({ 'xl/worksheets/sheet1.xml': 'A'.repeat(200_000) }) - // A decoy EOCD (zeroed central directory) appended after the genuine archive - // would, without tail validation, redirect the guard to an empty directory - // and undercount the real entries. + // A decoy EOCD claiming an empty central directory at offset 0 would, if + // trusted, undercount the real entries and let an oversized archive through. const decoy = Buffer.alloc(64) decoy.writeUInt32LE(0x06054b50, 0) const tampered = Buffer.concat([realZip, decoy]) + expect(() => + assertOoxmlArchiveWithinLimits(tampered, { + maxTotalUncompressedBytes: 100_000, + maxCompressionRatio: 10_000, + ratioCheckFloorBytes: 1024 * 1024 * 1024, + }) + ).toThrow(ZipBombError) + }) + + it('accepts an archive with a single trailing NUL byte after the EOCD', async () => { + const buffer = await buildZip({ 'word/document.xml': 'hello world' }) + const padded = Buffer.concat([buffer, Buffer.alloc(1)]) + expect(() => assertOoxmlArchiveWithinLimits(padded, HIGH_LIMITS)).not.toThrow() + await expect(JSZip.loadAsync(padded)).resolves.toBeDefined() + }) + + it('accepts an archive with a kilobyte of trailing garbage after the EOCD', async () => { + const buffer = await buildZip({ 'word/document.xml': 'hello world' }) + const garbage = Buffer.alloc(1024, 0xab) + const padded = Buffer.concat([buffer, garbage]) + expect(() => assertOoxmlArchiveWithinLimits(padded, HIGH_LIMITS)).not.toThrow() + await expect(JSZip.loadAsync(padded)).resolves.toBeDefined() + }) + + it('still rejects an oversized archive that carries trailing bytes', async () => { + const buffer = await buildZip({ 'xl/worksheets/sheet1.xml': 'A'.repeat(200_000) }) + const padded = Buffer.concat([buffer, Buffer.alloc(1024, 0xab)]) + expect(() => + assertOoxmlArchiveWithinLimits(padded, { + maxTotalUncompressedBytes: 100_000, + maxCompressionRatio: 10_000, + ratioCheckFloorBytes: 1024 * 1024 * 1024, + }) + ).toThrow(ZipBombError) + }) + + it('still rejects a high-ratio archive that carries trailing bytes', async () => { + const buffer = await buildZip({ 'xl/worksheets/sheet1.xml': 'A'.repeat(200_000) }) + const padded = Buffer.concat([buffer, Buffer.alloc(1024, 0xab)]) + expect(() => + assertOoxmlArchiveWithinLimits(padded, { + maxTotalUncompressedBytes: 1024 * 1024 * 1024, + maxCompressionRatio: 5, + ratioCheckFloorBytes: 1000, + }) + ).toThrow(ZipBombError) + }) + + it('rejects a forged zip bomb under the default limits, with or without trailing bytes', async () => { + const archive = await buildZip({ 'word/document.xml': 'hello' }) + const bomb = forgeDeclaredUncompressedSize(archive, 0xfffffff0) + for (const tail of [Buffer.alloc(0), Buffer.alloc(1), Buffer.alloc(1024, 0xab)]) { + expect(() => assertOoxmlArchiveWithinLimits(Buffer.concat([bomb, tail]))).toThrow( + ZipBombError + ) + } + }) + + it('rejects a bomb followed by an appended empty EOCD record', async () => { + const archive = await buildZip({ 'word/document.xml': 'hello' }) + const bomb = forgeDeclaredUncompressedSize(archive, 0xfffffff0) + // The appended record is internally consistent and sits at the highest + // offset, so a scan that trusts one record reads "empty archive" and lets + // the real central directory through. + const tampered = appendEocd(bomb) + expect(() => assertOoxmlArchiveWithinLimits(tampered)).toThrow(ZipBombError) + }) + + it('rejects a bomb followed by an appended EOCD declaring a small central directory', async () => { + const archive = await buildZip({ 'word/document.xml': 'hello' }) + const bomb = forgeDeclaredUncompressedSize(archive, 0xfffffff0) + const decoyArchive = await buildZip({ 'harmless.xml': 'ok' }) + const decoy = readEocd(decoyArchive) + const tampered = appendEocd(Buffer.concat([bomb, decoyArchive]), { + entryCount: decoy.entryCount, + cdSize: decoy.cdSize, + cdOffset: bomb.length + decoy.cdOffset, + }) expect(() => assertOoxmlArchiveWithinLimits(tampered)).toThrow(ZipBombError) }) + it('rejects an oversized archive followed by an appended empty EOCD record', async () => { + const archive = await buildZip({ 'xl/worksheets/sheet1.xml': 'A'.repeat(200_000) }) + expect(() => + assertOoxmlArchiveWithinLimits(appendEocd(archive), { + maxTotalUncompressedBytes: 100_000, + maxCompressionRatio: 10_000, + ratioCheckFloorBytes: 1024 * 1024 * 1024, + }) + ).toThrow(ZipBombError) + }) + + it('accepts a within-limits archive that carries an appended empty EOCD record', async () => { + const archive = await buildZip({ 'word/document.xml': 'hello world' }) + expect(() => assertOoxmlArchiveWithinLimits(appendEocd(archive), HIGH_LIMITS)).not.toThrow() + }) + + it('rejects a bomb hidden behind a single prepended byte, which JSZip still inflates', async () => { + const archive = await buildZip({ 'word/document.xml': 'A'.repeat(64 * 1024 * 1024) }) + // Prepending shifts the whole archive, but the EOCD's cdOffset is absolute, + // so no candidate resolves. The buffer no longer starts with a ZIP + // signature either, so a fail-closed branch keyed on byte 0 lets it past. + const attack = Buffer.concat([Buffer.from([0x00]), archive]) + expect(isZipShaped(attack)).toBe(false) + expect(() => assertOoxmlArchiveWithinLimits(attack)).toThrow(ZipBombError) + + // The threat is real, not merely a shape: JSZip reads past the prepended + // byte and inflates the entry in full. + const loaded = await JSZip.loadAsync(attack) + const inflated = await loaded.file('word/document.xml')!.async('nodebuffer') + expect(inflated.length).toBe(64 * 1024 * 1024) + expect(inflated.length / attack.length).toBeGreaterThan(100) + + // Same bomb, with the real EOCD pushed past the 64 KiB comment window by + // trailing junk. A spec-correct windowed scan sees no EOCD signature at all + // and, with byte 0 no longer a ZIP signature, no-ops entirely — while JSZip, + // which scans to offset 0, still finds the record and inflates 64 MiB. The + // guard must model the parser, so the scan covers the whole buffer. + const windowed = Buffer.concat([attack, Buffer.alloc(70_000, 0xab)]) + expect(isZipShaped(windowed)).toBe(false) + expect(() => assertOoxmlArchiveWithinLimits(windowed)).toThrow(ZipBombError) + + const loadedWindowed = await JSZip.loadAsync(windowed) + const inflatedWindowed = await loadedWindowed.file('word/document.xml')!.async('nodebuffer') + expect(inflatedWindowed.length).toBe(64 * 1024 * 1024) + }, 120_000) + + it('rejects a within-limits archive whose EOCD sits past the 64 KiB comment window', async () => { + const archive = await buildZip({ 'word/document.xml': 'hello world' }) + const attack = Buffer.concat([Buffer.from([0x00]), archive, Buffer.alloc(70_000, 0xab)]) + expect(() => assertOoxmlArchiveWithinLimits(attack, HIGH_LIMITS)).toThrow(ZipBombError) + }) + + it('rejects a bomb whose EOCD lies about the entry count behind an empty-EOCD decoy', async () => { + const archive = await buildZip({ 'word/document.xml': 'hello' }) + const bomb = forgeDeclaredUncompressedSize(archive, 200_000) + // Discarding the count-mismatched candidate measured NOTHING for the only + // interpretation JSZip actually parses, leaving the trivially-resolvable + // decoy as the whole verdict: "empty archive". The declared size stays under + // the absolute cap so the ratio check — not the walk's early size abort — is + // what has to see the entry. + expect(() => + assertOoxmlArchiveWithinLimits(spliceCountMismatchDecoy(bomb), { + maxTotalUncompressedBytes: 1024 * 1024 * 1024, + maxCompressionRatio: 5, + ratioCheckFloorBytes: 1000, + }) + ).toThrow(ZipBombError) + }) + + it('measures the real central directory when the EOCD lies about the entry count', async () => { + const archive = await buildZip({ 'a.xml': 'a', 'b.xml': 'b', 'c.xml': 'c' }) + expect(readZipCentralDirectoryStats(spliceCountMismatchDecoy(archive))?.entryCount).toBe(3) + }) + + it('rejects a prepended bomb that also carries an empty-EOCD decoy', async () => { + const archive = await buildZip({ 'word/document.xml': 'hello' }) + const bomb = forgeDeclaredUncompressedSize(archive, 0xfffffff0) + // Prepending shifts the real EOCD's absolute cdOffset out from under the + // guard while JSZip rebases past the leading byte; the decoy then supplies a + // clean "empty archive" reading. A resolvable candidate must not excuse an + // unresolvable one. + const attack = Buffer.concat([Buffer.from([0x00]), spliceCountMismatchDecoy(bomb)]) + expect(() => assertOoxmlArchiveWithinLimits(attack)).toThrow(ZipBombError) + }) + + it('refuses an EOCD signature truncated by the buffer end', () => { + // Too close to the tail to hold a full record, so it yields no candidate — + // but it is still a ZIP signature the guard could not follow, and reading it + // as "not a ZIP" is the same fail-open the prepend evasion exploits. + const buffer = Buffer.alloc(64) + Buffer.from([0x50, 0x4b, 0x05, 0x06]).copy(buffer, 50) + expect(() => assertOoxmlArchiveWithinLimits(buffer)).toThrow(ZipBombError) + }) + + it('refuses, quickly, a buffer whose candidates each anchor a distinct maximal walk', () => { + // The run cache is keyed by central-directory offset, so distinct offsets + // defeat it and cost MAX_EOCD_CANDIDATES full walks. The shared record budget + // bounds that, and exhausting it is unverifiable rather than "measured 0". + const buffer = Buffer.alloc(4 * 1024 * 1024) + for (let offset = 0; offset + 46 <= buffer.length; offset += 46) { + buffer.writeUInt32LE(CENTRAL_DIRECTORY_HEADER_SIGNATURE, offset) + } + for (let i = 0; i < 128; i++) { + const offset = buffer.length - 128 * 22 + i * 22 + buffer.writeUInt32LE(0x06054b50, offset) + buffer.writeUInt32LE(i * 46, offset + 16) + } + + const start = performance.now() + expect(() => assertOoxmlArchiveWithinLimits(buffer)).toThrow(ZipBombError) + expect(performance.now() - start).toBeLessThan(250) + }) + + it('survives a central-directory record whose extra field runs past the buffer end', () => { + // A truncated directory can declare an extra field longer than what remains. + // An unclamped read raises a raw RangeError, which escapes callers that + // expect a typed archive error (the upload path calls this outside any try). + const buffer = Buffer.alloc(68) + buffer.writeUInt32LE(0x06054b50, 0) + buffer.writeUInt16LE(1, 8) + buffer.writeUInt16LE(1, 10) + buffer.writeUInt32LE(22, 16) + buffer.writeUInt32LE(CENTRAL_DIRECTORY_HEADER_SIGNATURE, 22) + buffer.writeUInt32LE(0xffffffff, 22 + 24) + buffer.writeUInt16LE(0xffff, 22 + 30) + + expect(readZipCentralDirectoryStats(buffer)).toEqual({ + entryCount: 1, + totalExtraFieldBytes: 0xffff, + }) + }) + + it('rejects a within-limits archive behind a prepended byte', async () => { + // Defined, deliberate behaviour: an unresolvable EOCD is refused whether or + // not the archive behind it is benign. Nothing in the OOXML pipeline emits + // leading data, so the only producers of this shape are evasion attempts. + const archive = await buildZip({ 'word/document.xml': 'hello world' }) + const prepended = Buffer.concat([Buffer.from([0x00]), archive]) + expect(() => assertOoxmlArchiveWithinLimits(prepended, HIGH_LIMITS)).toThrow(ZipBombError) + }) + + it('rejects a bomb behind a kilobyte of prepended garbage', async () => { + const archive = await buildZip({ 'word/document.xml': 'hello' }) + const bomb = forgeDeclaredUncompressedSize(archive, 0xfffffff0) + const attack = Buffer.concat([Buffer.alloc(1024, 0xab), bomb]) + expect(() => assertOoxmlArchiveWithinLimits(attack)).toThrow(ZipBombError) + }) + + it('no-ops for a stray EOCD signature that resolves to an empty archive', () => { + // The zero fill after the signature reads as "0 records at offset 0", which + // walks cleanly to an empty central directory. A resolvable record needs no + // fail-closed treatment — the buffer has been inspected, and it is empty. + const buffer = Buffer.alloc(512) + Buffer.from([0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]).copy(buffer, 0) + Buffer.from([0x50, 0x4b, 0x05, 0x06]).copy(buffer, 300) + expect(() => assertOoxmlArchiveWithinLimits(buffer)).not.toThrow() + }) + + it('rejects a non-ZIP buffer whose stray EOCD signature does not resolve', () => { + // A coincidental `PK\x05\x06` followed by bytes that point nowhere is + // ~1.5e-5 likely for random data in the 64 KiB window, and the alternative + // reading — "not ZIP-shaped, let it through" — is exactly the bypass above. + // Refusing costs one parse with a clear error, against an OOM that takes + // down every tenant sharing the worker. + const buffer = Buffer.alloc(512) + Buffer.from([0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]).copy(buffer, 0) + Buffer.from([0x50, 0x4b, 0x05, 0x06]).copy(buffer, 300) + buffer.writeUInt16LE(4, 300 + 10) + buffer.writeUInt32LE(64, 300 + 16) + expect(() => assertOoxmlArchiveWithinLimits(buffer)).toThrow(ZipBombError) + }) + + it('rejects a buffer whose scan window is flooded with EOCD signatures', () => { + const buffer = Buffer.alloc(4096) + for (let offset = 0; offset + 4 <= buffer.length; offset += 8) { + Buffer.from([0x50, 0x4b, 0x05, 0x06]).copy(buffer, offset) + } + expect(() => assertOoxmlArchiveWithinLimits(buffer)).toThrow(ZipBombError) + }) + + it('reports the worst-case record count across EOCD interpretations', async () => { + const archive = await buildZip({ 'a.xml': 'a', 'b.xml': 'b', 'c.xml': 'c' }) + expect(readZipCentralDirectoryStats(archive)?.entryCount).toBe(3) + expect(readZipCentralDirectoryStats(appendEocd(archive))?.entryCount).toBe(3) + }) + + it('reads a ZIP64 declared uncompressed size from the central-directory extra field', async () => { + const archive = await buildZip({ 'word/document.xml': 'hello' }) + const bomb = forgeZip64DeclaredUncompressedSize(archive, 8n * 1024n * 1024n * 1024n) + expect(() => assertOoxmlArchiveWithinLimits(bomb)).toThrow(ZipBombError) + }) + + it('no-ops for a legacy OLE2 document containing the EOCD byte sequence', () => { + // Non-ZIP documents carry these four bytes by chance, and the OOXML parsers + // depend on a no-op here so their OLE2/plaintext fallback can run. The + // record's cdOffset lands outside the buffer, so no parser can read a + // directory from it and it carries no signal either way. + const body = Buffer.alloc(4096, 0x41) + Buffer.from([0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]).copy(body, 0) + const stray = Buffer.alloc(22) + stray.writeUInt32LE(0x06054b50, 0) + stray.writeUInt16LE(3, 8) + stray.writeUInt16LE(3, 10) + stray.writeUInt32LE(0xdeadbe, 12) + stray.writeUInt32LE(0xdeadbe, 16) + stray.copy(body, 2000) + + expect(() => assertOoxmlArchiveWithinLimits(body)).not.toThrow() + }) + + it('no-ops for plaintext containing the EOCD byte sequence', () => { + const text = Buffer.concat([ + Buffer.from('Quarterly report. '.repeat(50)), + Buffer.from([0x50, 0x4b, 0x05, 0x06]), + Buffer.from('...more prose follows here.'.repeat(50)), + ]) + expect(() => assertOoxmlArchiveWithinLimits(text)).not.toThrow() + }) + + it('no-ops for a legacy OLE2/CFB document', () => { + const ole2 = Buffer.alloc(1024) + Buffer.from([0xd0, 0xcf, 0x11, 0xe0, 0xa1, 0xb1, 0x1a, 0xe1]).copy(ole2, 0) + expect(() => assertOoxmlArchiveWithinLimits(ole2)).not.toThrow() + }) + + it('no-ops for a garbage buffer that is not ZIP-shaped', () => { + const garbage = Buffer.alloc(4096) + for (let i = 0; i < garbage.length; i++) { + garbage[i] = (i * 31 + 7) % 251 + } + expect(() => assertOoxmlArchiveWithinLimits(garbage)).not.toThrow() + }) + it('no-ops for buffers that are not ZIP archives', () => { const plaintext = Buffer.from('this is just plain text, not a zip archive at all') expect(() => assertOoxmlArchiveWithinLimits(plaintext)).not.toThrow() diff --git a/apps/sim/lib/file-parsers/zip-guard.ts b/apps/sim/lib/file-parsers/zip-guard.ts index 07642b3740c..1591de47dd9 100644 --- a/apps/sim/lib/file-parsers/zip-guard.ts +++ b/apps/sim/lib/file-parsers/zip-guard.ts @@ -22,13 +22,39 @@ const ZIP64_EOCD_SIGNATURE = 0x06064b50 const CENTRAL_DIRECTORY_HEADER_SIGNATURE = 0x02014b50 const ZIP64_EXTRA_FIELD_ID = 0x0001 +const EOCD_SIGNATURE_BYTES = Buffer.from([0x50, 0x4b, 0x05, 0x06]) + const EOCD_MIN_SIZE = 22 const ZIP64_EOCD_LOCATOR_SIZE = 20 const CENTRAL_DIRECTORY_HEADER_MIN_SIZE = 46 -const MAX_EOCD_COMMENT_SIZE = 0xffff const UINT32_SENTINEL = 0xffffffff const UINT16_SENTINEL = 0xffff +/** + * Ceiling on EOCD signatures considered. A real archive has one (plus, rarely, a + * stray match inside entry data); a buffer stuffed with more is an attempt to + * flood the candidate set, and is refused outright. + */ +const MAX_EOCD_CANDIDATES = 128 + +/** + * Ceiling on central-directory records read across ALL candidate walks in one + * inspection. + * + * The per-candidate run cache is keyed by central-directory offset, so an + * attacker who varies `cdOffset` per candidate defeats it and pays + * `MAX_EOCD_CANDIDATES × buffer.length / 46` record reads: 551 ms of synchronous + * event-loop block on a 64 MiB buffer and 1006 ms at the pipeline's 100 MiB cap, + * measured. The budget bounds that at 5.9 ms and 8.6 ms respectively while + * sitting far above any real archive — the upload path separately refuses more + * than 10k records. Exhausting it means the buffer could not be fully evaluated, + * which fails closed like any other unverifiable ZIP-shaped input. + * + * Widening the signature scan from the trailing 64 KiB to the whole buffer costs + * one `Buffer.indexOf` pass: 0.02 ms to 0.33 ms on a normal 5 MB OOXML. + */ +const MAX_CENTRAL_DIRECTORY_RECORDS_SCANNED = 250_000 + export interface OoxmlSizeLimits { /** Hard ceiling on the summed declared uncompressed size of all entries. */ maxTotalUncompressedBytes: number @@ -74,25 +100,52 @@ export function isZipShaped(buffer: Buffer): boolean { return signature === LOCAL_FILE_HEADER_SIGNATURE || signature === EOCD_SIGNATURE } +interface EocdScan { + /** Offsets to evaluate; empty when the buffer is flooded past {@link MAX_EOCD_CANDIDATES}. */ + candidates: number[] + /** Whether the buffer holds at least one EOCD signature, flooded or not. */ + sawSignature: boolean +} + /** - * Locate the End Of Central Directory record by scanning backwards from the end - * of the buffer (it sits within the trailing 22 + comment bytes). A candidate - * is only accepted when its declared comment length places the record exactly - * at the buffer tail, so a decoy EOCD signature planted in the comment region - * cannot redirect the guard to a smaller, attacker-chosen central directory. + * Every EOCD signature offset in the WHOLE buffer. + * + * The spec puts the record in the trailing 22 + 65535 comment-length window, and + * yauzl honours that — but the libraries this guard actually protects do not. + * JSZip (`ArrayReader.lastIndexOfSignature`) and SheetJS both scan from the last + * byte to offset 0, so an EOCD pushed past 64 KiB of trailing junk is invisible + * to a windowed scan yet is exactly the record they parse. Combined with a single + * prepended byte — which makes the buffer read as "not ZIP-shaped" — a windowed + * scan let a 495x bomb through untouched. The guard models the parsers, not the + * spec, so the window is the buffer. + * + * Trailing bytes after the EOCD are common in the wild (self-extracting stubs, + * appended signatures, generator padding), so the record is not required to end + * at the buffer tail. Every candidate is returned rather than the first that + * looks plausible: the decompression libraries each pick their own record, so the + * archive is evaluated under all of them. A flooded buffer yields no candidates + * but still reports the signature, so the caller refuses the buffer rather than + * reading it as "not a ZIP". */ -function findEocdOffset(buffer: Buffer): number { - const minStart = Math.max(0, buffer.length - EOCD_MIN_SIZE - MAX_EOCD_COMMENT_SIZE) - for (let offset = buffer.length - EOCD_MIN_SIZE; offset >= minStart; offset--) { - if (buffer.readUInt32LE(offset) !== EOCD_SIGNATURE) { - continue +function findEocdCandidates(buffer: Buffer): EocdScan { + const lastOffset = buffer.length - EOCD_MIN_SIZE + const candidates: number[] = [] + let sawSignature = false + + let offset = buffer.indexOf(EOCD_SIGNATURE_BYTES) + while (offset !== -1) { + sawSignature = true + if (offset > lastOffset) { + break } - const commentLength = buffer.readUInt16LE(offset + 20) - if (offset + EOCD_MIN_SIZE + commentLength === buffer.length) { - return offset + if (candidates.length >= MAX_EOCD_CANDIDATES) { + return { candidates: [], sawSignature: true } } + candidates.push(offset) + offset = buffer.indexOf(EOCD_SIGNATURE_BYTES, offset + 1) } - return -1 + + return { candidates, sawSignature } } interface CentralDirectoryLocation { @@ -143,6 +196,10 @@ function locateCentralDirectory( * when the 32-bit central-directory field is saturated. The saturated 64-bit * values appear in the extra field in a fixed order with the uncompressed size * first, so it is always the leading 8 bytes of the ZIP64 field. + * + * The extra-field extent is clamped to the buffer: a truncated central directory + * can declare a length that runs off the end, and an unclamped read raises a raw + * `RangeError` that escapes callers which expect a typed archive error. */ function readUncompressedSize( buffer: Buffer, @@ -156,7 +213,7 @@ function readUncompressedSize( } const extraStart = headerOffset + CENTRAL_DIRECTORY_HEADER_MIN_SIZE + fileNameLength - const extraEnd = extraStart + extraFieldLength + const extraEnd = Math.min(extraStart + extraFieldLength, buffer.length) let cursor = extraStart while (cursor + 4 <= extraEnd) { const fieldId = buffer.readUInt16LE(cursor) @@ -171,129 +228,281 @@ function readUncompressedSize( return uncompressedSize } +interface CentralDirectoryWalk { + /** Records in the contiguous central-directory run at the candidate's offset. */ + entryCount: number + /** Summed declared uncompressed size across those records. */ + declaredUncompressedBytes: number + /** Summed declared extra-field bytes across those records. */ + totalExtraFieldBytes: number +} + /** - * Sum the declared uncompressed size of every central-directory entry. Returns - * `null` when the buffer is not a parseable ZIP archive (e.g. legacy binary - * `.xls`/`.doc`, or a misidentified plaintext file) so the caller can defer to - * the downstream parser. Stops early once the running total exceeds the limit. + * A candidate is either ignorable, unverifiable, or measurable. + * + * - `ignored` — the EOCD's `cdOffset` does not resolve at all (out of range, or a + * broken ZIP64 chain). No parser can read a central directory from it, and a + * stray `PK\x05\x06` inside compressed entry data lands here, so it carries no + * signal either way. + * - `unverifiable` — the offset resolves and the record declares entries, but not + * one record sits there. This is the prepended-bytes shape: JSZip rebases past + * arbitrary leading data and inflates the archive anyway, while every absolute + * offset in the buffer is shifted out from under the guard. It must fail closed. + * - `walk` — records were read; the run is the measurement. */ -function sumDeclaredUncompressedSize(buffer: Buffer, abortAboveBytes: number): number | null { - if (buffer.length < EOCD_MIN_SIZE) { - return null - } - - const eocdOffset = findEocdOffset(buffer) - if (eocdOffset < 0) { - return null - } +type CandidateOutcome = + | { kind: 'ignored' } + | { kind: 'unverifiable' } + | { kind: 'walk'; walk: CentralDirectoryWalk } + +/** Remaining central-directory records an inspection may read, shared across candidates. */ +interface ScanBudget { + remaining: number +} +/** + * Walk the contiguous run of central-directory records anchored by one EOCD + * candidate. The run — not the candidate's declared entry count — is what a + * per-signature parser allocates, so a lied count can neither hide records nor + * inflate the tally. + * + * A run that disagrees with the declared count is still measured. JSZip + * deliberately does not error on a count mismatch ("we found some records but not + * all… no error here", `zipEntries.js`), so it allocates and inflates exactly the + * run found here; discarding the candidate instead measured NOTHING for that + * interpretation and let a 1021x bomb through behind a single decoy record. + * + * Stops early once the running total exceeds the limit — the archive is already + * over budget under this interpretation — or once the shared scan budget is + * exhausted, which is reported as unverifiable rather than as a short run. Runs + * are memoized by central directory offset, so many candidates aimed at one + * directory cost one walk. + */ +function walkCentralDirectory( + buffer: Buffer, + eocdOffset: number, + abortAboveBytes: number, + runCache: Map, + budget: ScanBudget +): CandidateOutcome { const location = locateCentralDirectory(buffer, eocdOffset) if (!location) { - return null + return { kind: 'ignored' } + } + + const cached = runCache.get(location.offset) + if (cached) { + return classifyRun(cached, location) } - let total = 0 + let entryCount = 0 + let declaredUncompressedBytes = 0 + let totalExtraFieldBytes = 0 let cursor = location.offset - for (let entry = 0; entry < location.entryCount; entry++) { - if (cursor + CENTRAL_DIRECTORY_HEADER_MIN_SIZE > buffer.length) { - return null - } - if (buffer.readUInt32LE(cursor) !== CENTRAL_DIRECTORY_HEADER_SIGNATURE) { - return null + while ( + cursor + CENTRAL_DIRECTORY_HEADER_MIN_SIZE <= buffer.length && + buffer.readUInt32LE(cursor) === CENTRAL_DIRECTORY_HEADER_SIGNATURE + ) { + if (budget.remaining <= 0) { + return { kind: 'unverifiable' } } + budget.remaining -= 1 const fileNameLength = buffer.readUInt16LE(cursor + 28) const extraFieldLength = buffer.readUInt16LE(cursor + 30) const commentLength = buffer.readUInt16LE(cursor + 32) - total += readUncompressedSize(buffer, cursor, fileNameLength, extraFieldLength) - if (total > abortAboveBytes) { - return total + entryCount += 1 + totalExtraFieldBytes += extraFieldLength + declaredUncompressedBytes += readUncompressedSize( + buffer, + cursor, + fileNameLength, + extraFieldLength + ) + if (declaredUncompressedBytes > abortAboveBytes) { + return { kind: 'walk', walk: { entryCount, declaredUncompressedBytes, totalExtraFieldBytes } } } cursor += CENTRAL_DIRECTORY_HEADER_MIN_SIZE + fileNameLength + extraFieldLength + commentLength } - return total + const walk = { entryCount, declaredUncompressedBytes, totalExtraFieldBytes } + runCache.set(location.offset, walk) + return classifyRun(walk, location) } -/** Parse-time shape of a ZIP central directory, read without decompressing anything. */ -export interface ZipCentralDirectoryStats { - /** Records in the contiguous central-directory run — what a per-signature parser allocates. */ - entryCount: number - /** Summed declared extra-field bytes across those records. */ - totalExtraFieldBytes: number +/** + * An empty run under a record that declares entries means the directory is not + * where the buffer says it is — unverifiable. An empty run under a record that + * declares nothing is simply an empty archive, and is measured as such. + */ +function classifyRun( + walk: CentralDirectoryWalk, + location: CentralDirectoryLocation +): CandidateOutcome { + if (walk.entryCount === 0 && location.entryCount > 0) { + return { kind: 'unverifiable' } + } + return { kind: 'walk', walk } +} + +interface ArchiveInspection { + /** Worst case across measurable EOCD interpretations, or `null` when none were. */ + worst: CentralDirectoryWalk | null + /** Whether the buffer held at least one EOCD signature. */ + sawEocdSignature: boolean + /** + * Whether the buffer holds an EOCD signal the guard could not evaluate: a + * record naming a directory that is not there, a directory too large to scan + * within the budget, or a candidate set too flooded to enumerate. Distinct + * from "no measurement" — an EOCD whose offset lands outside the buffer is + * unreadable by every parser too, so it is no signal rather than a bad one. + */ + sawUnverifiableEocd: boolean } /** - * Walk the real central directory (EOCD-anchored, decoy-resistant, ZIP64-aware — - * the same anchoring as {@link assertOoxmlArchiveWithinLimits}) and report its - * record count and summed extra-field bytes, so callers can bound a parser's - * object graph before handing it the buffer. The walk covers the CONTIGUOUS run - * of records at the central-directory offset rather than trusting the EOCD's - * declared count, because that run is what JSZip actually allocates one entry - * per — a lied count can neither hide records nor inflate the tally. Unlike a - * raw whole-buffer signature scan, STORED entry payloads (e.g. a nested `.zip` - * archived without recompression) are never miscounted as records. Returns - * `null` when the buffer is not a parseable ZIP, so callers can fail closed. + * Evaluate the archive under every EOCD candidate and return the worst case of + * each measure. yauzl, JSZip and SheetJS each select their own EOCD record, so + * the guard must not depend on guessing which one the downstream parser reads: + * if ANY interpretation is over budget, the archive is rejected. + * + * `worst` is `null` when nothing was measurable — which alone is not grounds for + * refusal, since a non-ZIP document carrying the four EOCD bytes by chance lands + * here. `sawUnverifiableEocd` is the separate, convicting signal: a candidate the + * guard cannot follow is an evasion shape that must be refused even when another + * candidate measured cleanly — see {@link assertOoxmlArchiveWithinLimits}. */ -export function readZipCentralDirectoryStats(buffer: Buffer): ZipCentralDirectoryStats | null { +function inspectArchive(buffer: Buffer, abortAboveBytes: number): ArchiveInspection { if (buffer.length < EOCD_MIN_SIZE) { - return null - } - - const eocdOffset = findEocdOffset(buffer) - if (eocdOffset < 0) { - return null + return { worst: null, sawEocdSignature: false, sawUnverifiableEocd: false } } - const location = locateCentralDirectory(buffer, eocdOffset) - if (!location) { - return null + const { candidates, sawSignature } = findEocdCandidates(buffer) + + let worst: CentralDirectoryWalk | null = null + // A signature the scan declined to enumerate — a flooded buffer, or a record + // truncated by the buffer end — is unevaluated, not absent. JSZip reads the + // LAST signature in the buffer, so flooding past the candidate ceiling would + // otherwise hide the one record it actually parses. + let sawUnverifiableEocd = sawSignature && candidates.length === 0 + const runCache = new Map() + const budget: ScanBudget = { remaining: MAX_CENTRAL_DIRECTORY_RECORDS_SCANNED } + for (const candidate of candidates) { + const outcome = walkCentralDirectory(buffer, candidate, abortAboveBytes, runCache, budget) + if (outcome.kind === 'unverifiable') { + sawUnverifiableEocd = true + break + } + if (outcome.kind === 'ignored') { + continue + } + const { walk } = outcome + worst = worst + ? { + entryCount: Math.max(worst.entryCount, walk.entryCount), + declaredUncompressedBytes: Math.max( + worst.declaredUncompressedBytes, + walk.declaredUncompressedBytes + ), + totalExtraFieldBytes: Math.max(worst.totalExtraFieldBytes, walk.totalExtraFieldBytes), + } + : walk + if (worst.declaredUncompressedBytes > abortAboveBytes) { + break + } } - let entryCount = 0 - let totalExtraFieldBytes = 0 - let cursor = location.offset - while ( - cursor + CENTRAL_DIRECTORY_HEADER_MIN_SIZE <= buffer.length && - buffer.readUInt32LE(cursor) === CENTRAL_DIRECTORY_HEADER_SIGNATURE - ) { - const fileNameLength = buffer.readUInt16LE(cursor + 28) - const extraFieldLength = buffer.readUInt16LE(cursor + 30) - const commentLength = buffer.readUInt16LE(cursor + 32) + return { worst, sawEocdSignature: sawSignature, sawUnverifiableEocd } +} - entryCount += 1 - totalExtraFieldBytes += extraFieldLength - cursor += CENTRAL_DIRECTORY_HEADER_MIN_SIZE + fileNameLength + extraFieldLength + commentLength - } +/** Parse-time shape of a ZIP central directory, read without decompressing anything. */ +export interface ZipCentralDirectoryStats { + /** Records in the contiguous central-directory run — what a per-signature parser allocates. */ + entryCount: number + /** Summed declared extra-field bytes across those records. */ + totalExtraFieldBytes: number +} - if (entryCount < location.entryCount) { +/** + * Walk the central directory (EOCD-anchored, decoy-resistant, ZIP64-aware — the + * same anchoring as {@link assertOoxmlArchiveWithinLimits}) and report the + * worst-case record count and summed extra-field bytes across every EOCD + * interpretation, so callers can bound a parser's object graph before handing + * it the buffer. Each walk covers the CONTIGUOUS run of records at the central + * directory offset rather than trusting the EOCD's declared count, because that + * run is what JSZip actually allocates one entry per. Unlike a raw whole-buffer + * signature scan, STORED entry payloads (e.g. a nested `.zip` archived without + * recompression) are never miscounted as records. Returns `null` when no EOCD + * candidate is measurable or when any candidate is unverifiable, so callers can + * fail closed. + */ +export function readZipCentralDirectoryStats(buffer: Buffer): ZipCentralDirectoryStats | null { + const { worst, sawUnverifiableEocd } = inspectArchive(buffer, Number.POSITIVE_INFINITY) + if (!worst || sawUnverifiableEocd) { return null } - - return { entryCount, totalExtraFieldBytes } + return { entryCount: worst.entryCount, totalExtraFieldBytes: worst.totalExtraFieldBytes } } /** * Reject an OOXML archive whose declared expanded size or compression ratio * exceeds safe bounds, before any decompression library materializes it. * - * Fails closed: a ZIP-shaped buffer whose central directory cannot be parsed is - * rejected rather than passed through, so a malformed archive that a downstream - * library still inflates cannot bypass the guard. Genuinely non-ZIP inputs - * (legacy OLE `.xls`/`.doc`, misidentified plaintext) no-op and defer to the - * downstream parser's own validation and fallbacks. + * The limits are applied to the WORST case across every EOCD interpretation of + * the buffer, not to one chosen record: the libraries downstream do not agree on + * which EOCD they read, so an archive that is a bomb under any of them is + * rejected. + * + * Fails closed on two families of shape, so a buffer a downstream library still + * inflates cannot bypass the guard: + * + * - The buffer is ZIP-shaped (byte 0 begins a local file header or an EOCD) but + * nothing measurable resolves. + * - The buffer holds an EOCD signal the guard could not evaluate, whatever byte + * 0 says. Three cases: a record that names a directory offset inside the + * buffer where no record is found; a directory too large to scan within the + * shared record budget; and a signature the scan declined to enumerate at all + * — a candidate set flooded past {@link MAX_EOCD_CANDIDATES}, or a record + * truncated by the buffer end. JSZip parses the LAST signature in the buffer, + * so flooding would otherwise hide exactly the one record it reads. + * + * The first case is the prepended-bytes evasion: JSZip tolerates arbitrary + * leading data, but the EOCD's `cdOffset` is absolute, so shifting the archive + * by even one byte leaves the record naming an offset that no longer holds a + * directory, while the archive still inflates. Keying the fail-closed branch + * on the leading signature alone let a 1.2 GiB bomb through. The refusal holds + * even when another candidate measures cleanly: an empty 22-byte EOCD appended + * as a decoy is trivially measurable, and letting it stand in for the shifted + * record reopens the same hole. + * + * An EOCD signature whose `cdOffset` lands OUTSIDE the buffer is not a signal in + * either direction — no parser can read a directory from it — so it neither + * measures nor convicts. This is the common case for the four bytes appearing by + * chance in a legacy OLE `.doc`/`.xls` or in plaintext, and those inputs must + * no-op so the downstream parsers' own fallback paths can run. Treating any + * stray signature as grounds for refusal turned a legacy `.doc` into a hard + * error. What remains is the ~1e-6 case of a stray whose offset happens to land + * in range and read empty; that costs one parse with an explicit error, against + * an OOM that takes down every tenant on the worker. */ export function assertOoxmlArchiveWithinLimits( buffer: Buffer, limits: OoxmlSizeLimits = DEFAULT_OOXML_SIZE_LIMITS ): void { - const totalUncompressed = sumDeclaredUncompressedSize(buffer, limits.maxTotalUncompressedBytes) - if (totalUncompressed === null) { - if (isZipShaped(buffer)) { - logger.warn('Rejected ZIP-shaped archive: central directory could not be parsed', { + const { worst, sawEocdSignature, sawUnverifiableEocd } = inspectArchive( + buffer, + limits.maxTotalUncompressedBytes + ) + const totalUncompressed = worst ? worst.declaredUncompressedBytes : null + if (totalUncompressed === null || sawUnverifiableEocd) { + if (sawUnverifiableEocd || isZipShaped(buffer)) { + logger.warn('Rejected archive: central directory could not be parsed', { compressedBytes: buffer.length, + sawEocdSignature, + sawUnverifiableEocd, + zipShaped: isZipShaped(buffer), }) throw new ZipBombError( 'Unable to inspect ZIP central directory; refusing to parse an unverifiable ZIP-shaped archive' diff --git a/apps/sim/lib/media/falai.test.ts b/apps/sim/lib/media/falai.test.ts new file mode 100644 index 00000000000..a47aee14d88 --- /dev/null +++ b/apps/sim/lib/media/falai.test.ts @@ -0,0 +1,229 @@ +/** + * @vitest-environment node + */ +import { inputValidationMock, inputValidationMockFns } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +vi.mock('@/lib/core/security/input-validation.server', () => inputValidationMock) +vi.mock('@/lib/core/execution-limits', () => ({ getMaxExecutionTimeout: () => 30_000 })) +vi.mock('@sim/utils/helpers', () => ({ sleep: () => Promise.resolve() })) + +import { MAX_FAL_QUEUE_JSON_BYTES, runFalQueue } from '@/lib/media/falai' + +const { mockValidateUrlWithDNS, mockSecureFetchWithPinnedIP } = inputValidationMockFns + +function jsonResponse(payload: unknown, ok = true) { + const text = JSON.stringify(payload) + return { + ok, + status: ok ? 200 : 500, + headers: { get: () => null }, + body: null, + text: async () => text, + arrayBuffer: async () => new ArrayBuffer(0), + } +} + +describe('runFalQueue', () => { + const fetchMock = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + vi.stubGlobal('fetch', fetchMock) + mockValidateUrlWithDNS.mockResolvedValue({ isValid: true, resolvedIP: '8.8.8.8' }) + }) + + it('polls the queue through the SSRF-guarded client, revalidating on every poll', async () => { + fetchMock.mockResolvedValue( + new Response(JSON.stringify({ request_id: 'req-1' }), { status: 200 }) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'IN_QUEUE' })) + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + + const result = await runFalQueue('fal-ai/test', { prompt: 'hi' }, 'fal-key') + + expect(result.requestId).toBe('req-1') + // Two status polls + one result fetch, all through the guarded client. + expect(mockSecureFetchWithPinnedIP).toHaveBeenCalledTimes(3) + // The URL is re-validated on every hop, not pinned once at submit time. + expect(mockValidateUrlWithDNS).toHaveBeenCalledTimes(3) + expect(fetchMock).toHaveBeenCalledTimes(1) + }) + + it('ignores a status_url that leaves the Fal.ai queue origin (no API-key exfiltration)', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + request_id: 'req-2', + status_url: 'https://evil.example.net/steal', + response_url: 'https://evil.example.net/steal', + }), + { status: 200 } + ) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + + await runFalQueue('fal-ai/test', { prompt: 'hi' }, 'fal-key') + + const urls = mockSecureFetchWithPinnedIP.mock.calls.map(([url]) => url) + expect(urls).toEqual([ + 'https://queue.fal.run/fal-ai/test/requests/req-2/status', + 'https://queue.fal.run/fal-ai/test/requests/req-2', + ]) + for (const [, , options] of mockSecureFetchWithPinnedIP.mock.calls) { + expect(options.headers.Authorization).toBe('Key fal-key') + } + }) + + it('builds the fallback queue URL from the app id for a multi-segment endpoint', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ request_id: 'req-4', status_url: 'https://evil.example.net/steal' }), + { status: 200 } + ) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + + // fal.ai routes queue polling under `{owner}/{alias}` only — the model sub-path 405s. + await runFalQueue('fal-ai/kling-video/v3/pro/text-to-video', { prompt: 'hi' }, 'fal-key') + + expect(mockSecureFetchWithPinnedIP.mock.calls.map(([url]) => url)).toEqual([ + 'https://queue.fal.run/fal-ai/kling-video/requests/req-4/status', + 'https://queue.fal.run/fal-ai/kling-video/requests/req-4', + ]) + }) + + it('strips the unroutable /response suffix Fal.ai echoes back (queue.fal.run 405s on it)', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + request_id: 'req-5', + status_url: 'https://queue.fal.run/fal-ai/test/requests/req-5/status', + response_url: 'https://queue.fal.run/fal-ai/test/requests/req-5/response', + }), + { status: 200 } + ) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + + await runFalQueue('fal-ai/test', { prompt: 'hi' }, 'fal-key') + + expect(mockSecureFetchWithPinnedIP.mock.calls.map(([url]) => url)).toEqual([ + 'https://queue.fal.run/fal-ai/test/requests/req-5/status', + 'https://queue.fal.run/fal-ai/test/requests/req-5', + ]) + }) + + it('strips /response from a response_url echoed by the completed status body', async () => { + fetchMock.mockResolvedValue( + new Response(JSON.stringify({ request_id: 'req-6' }), { status: 200 }) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce( + jsonResponse({ + status: 'COMPLETED', + response_url: 'https://queue.fal.run/fal-ai/other/requests/req-6/response', + }) + ) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + + await runFalQueue('fal-ai/test', { prompt: 'hi' }, 'fal-key') + + expect(mockSecureFetchWithPinnedIP.mock.calls[1][0]).toBe( + 'https://queue.fal.run/fal-ai/other/requests/req-6' + ) + }) + + it('rejects a same-origin queue path of the wrong kind (result offered for status, and vice versa)', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + request_id: 'req-7', + // Both are routable queue paths, but each is the *other* kind: the status + // slot is handed a result path and the result slot a status path. + status_url: 'https://queue.fal.run/fal-ai/test/requests/req-7', + response_url: 'https://queue.fal.run/fal-ai/test/requests/req-7/status', + }), + { status: 200 } + ) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + + await runFalQueue('fal-ai/test', { prompt: 'hi' }, 'fal-key') + + expect(mockSecureFetchWithPinnedIP.mock.calls.map(([url]) => url)).toEqual([ + 'https://queue.fal.run/fal-ai/test/requests/req-7/status', + 'https://queue.fal.run/fal-ai/test/requests/req-7', + ]) + }) + + it('rejects a same-origin candidate whose path is not a queue path at all', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + request_id: 'req-9', + // Same origin, but neither shape matches `/{app…}/requests/{id}[/status]`. + status_url: 'https://queue.fal.run/admin', + response_url: 'https://queue.fal.run/a/b/c', + }), + { status: 200 } + ) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + + await runFalQueue('fal-ai/test', { prompt: 'hi' }, 'fal-key') + + expect(mockSecureFetchWithPinnedIP.mock.calls.map(([url]) => url)).toEqual([ + 'https://queue.fal.run/fal-ai/test/requests/req-9/status', + 'https://queue.fal.run/fal-ai/test/requests/req-9', + ]) + }) + + it('bounds every queue read with the shared Fal queue JSON cap', async () => { + fetchMock.mockResolvedValue( + new Response(JSON.stringify({ request_id: 'req-8' }), { status: 200 }) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + + await runFalQueue('fal-ai/test', { prompt: 'hi' }, 'fal-key') + + for (const [, , options] of mockSecureFetchWithPinnedIP.mock.calls) { + expect(options.maxResponseBytes).toBe(MAX_FAL_QUEUE_JSON_BYTES) + } + }) + + it('honors a status_url that stays on the Fal.ai queue origin', async () => { + fetchMock.mockResolvedValue( + new Response( + JSON.stringify({ + request_id: 'req-3', + status_url: 'https://queue.fal.run/fal-ai/other/requests/req-3/status', + }), + { status: 200 } + ) + ) + mockSecureFetchWithPinnedIP + .mockResolvedValueOnce(jsonResponse({ status: 'COMPLETED' })) + .mockResolvedValueOnce(jsonResponse({ video: { url: 'https://cdn.fal.media/a.mp4' } })) + + await runFalQueue('fal-ai/test', { prompt: 'hi' }, 'fal-key') + + expect(mockSecureFetchWithPinnedIP.mock.calls[0][0]).toBe( + 'https://queue.fal.run/fal-ai/other/requests/req-3/status' + ) + }) +}) diff --git a/apps/sim/lib/media/falai.ts b/apps/sim/lib/media/falai.ts index 21a36dfbad9..d1e61bc3cd2 100644 --- a/apps/sim/lib/media/falai.ts +++ b/apps/sim/lib/media/falai.ts @@ -18,7 +18,14 @@ const logger = createLogger('FalMediaClient') // Generated media (esp. video) can be large. export const MAX_MEDIA_BYTES = 250 * 1024 * 1024 -const MAX_MEDIA_JSON_BYTES = 4 * 1024 * 1024 + +/** + * Cap for Fal.ai queue status/result envelopes, shared by every caller so the same + * completion cannot succeed through one entry point and fail through another. The envelope + * itself is kilobyte-scale, but some models inline the output as a base64 data URI, so the + * bound keeps headroom for that while still capping what a single poll can buffer. + */ +export const MAX_FAL_QUEUE_JSON_BYTES = 4 * 1024 * 1024 const POLL_INTERVAL_MS = 3000 /** @@ -61,8 +68,92 @@ export function getNumberProp( return typeof value === 'number' ? value : undefined } -function falQueueUrl(endpoint: string, requestId: string, path: 'status' | 'response'): string { - return `https://queue.fal.run/${endpoint}/requests/${requestId}/${path}` +export const FAL_QUEUE_ORIGIN = 'https://queue.fal.run' + +/** + * Fal.ai routes queue polling under the app id (`{owner}/{alias}`) ONLY — a model's sub-path + * is part of the submit URL but not the queue URL. `queue.fal.run` answers + * `/fal-ai/kling-video/requests/{id}/status` but 405s on + * `/fal-ai/kling-video/v3/pro/text-to-video/requests/{id}/status`. + */ +function falQueueAppId(endpoint: string): string { + return endpoint.split('/').slice(0, 2).join('/') +} + +/** + * Builds a Fal.ai queue URL. `status` polls progress; `result` fetches the completed payload + * and takes no path suffix (`/response` is not a route — `queue.fal.run` 405s on it). + */ +export function buildFalQueueUrl( + endpoint: string, + requestId: string, + kind: 'status' | 'result' +): string { + const base = `${FAL_QUEUE_ORIGIN}/${falQueueAppId(endpoint)}/requests/${requestId}` + return kind === 'status' ? `${base}/status` : base +} + +/** The only queue paths `queue.fal.run` routes: `/{app}/requests/{id}` and `.../status`. */ +const FAL_QUEUE_PATH_PATTERN = /^\/(?:[^/]+\/)+requests\/[^/]+(?:\/status)?$/u + +/** + * Accepts a queue URL echoed back by Fal.ai only while it stays on the queue origin we + * submitted to AND matches a routable queue path, otherwise falls back to the URL we + * construct ourselves. Queue polling carries `Authorization: Key `, so an + * attacker-influenced `status_url` / `response_url` would otherwise hand the Fal.ai key to + * an arbitrary host. Origin alone is not enough: Fal.ai echoes a `response_url` suffixed + * with `/response`, which is not a GET route (405), so that suffix is stripped first. + */ +export function resolveFalQueueUrl(candidate: string | undefined, fallback: string): string { + if (!candidate) return fallback + + let url: URL + try { + url = new URL(candidate) + } catch { + logger.warn('Fal.ai queue URL is not a valid URL; using constructed fallback', { fallback }) + return fallback + } + + if (url.origin !== FAL_QUEUE_ORIGIN) { + logger.warn('Fal.ai queue URL left the queue origin; using constructed fallback', { + candidate, + fallback, + }) + return fallback + } + + const path = url.pathname.replace(/\/response$/u, '') + const wantsStatus = fallback.endsWith('/status') + if (!FAL_QUEUE_PATH_PATTERN.test(path) || path.endsWith('/status') !== wantsStatus) { + logger.warn('Fal.ai queue URL is not a routable queue path; using constructed fallback', { + candidate, + fallback, + }) + return fallback + } + + const normalized = `${FAL_QUEUE_ORIGIN}${path}${url.search}` + if (normalized !== candidate) { + logger.warn('Normalized Fal.ai queue URL to a routable shape', { candidate, normalized }) + } + return normalized +} + +/** + * Requests a Fal.ai queue URL through the SSRF-guarded client. Invoked on every poll so the + * provider-supplied URL is revalidated each time rather than trusted once. + */ +async function fetchFalQueue(url: string, apiKey: string) { + const validation = await validateUrlWithDNS(url, 'falQueueUrl') + if (!validation.isValid || !validation.resolvedIP) { + throw new Error(validation.error || 'Fal.ai queue URL failed validation') + } + return secureFetchWithPinnedIP(url, validation.resolvedIP, { + method: 'GET', + headers: { Authorization: `Key ${apiKey}` }, + maxResponseBytes: MAX_FAL_QUEUE_JSON_BYTES, + }) } function falErrorMessage(error: unknown): string { @@ -85,7 +176,7 @@ export async function runFalQueue( input: Record, apiKey: string ): Promise { - const createResponse = await fetch(`https://queue.fal.run/${endpoint}`, { + const createResponse = await fetch(`${FAL_QUEUE_ORIGIN}/${endpoint}`, { method: 'POST', headers: { Authorization: `Key ${apiKey}`, 'Content-Type': 'application/json' }, body: JSON.stringify(input), @@ -99,7 +190,7 @@ export async function runFalQueue( } const createData = await readResponseJsonWithLimit(createResponse, { - maxBytes: MAX_MEDIA_JSON_BYTES, + maxBytes: MAX_FAL_QUEUE_JSON_BYTES, label: 'Fal.ai create response', }) if (!isRecordLike(createData)) throw new Error('Invalid Fal.ai queue response') @@ -107,16 +198,20 @@ export async function runFalQueue( const requestId = getStringProp(createData, 'request_id') if (!requestId) throw new Error('Fal.ai queue response missing request_id') - const statusUrl = - getStringProp(createData, 'status_url') || falQueueUrl(endpoint, requestId, 'status') - const responseUrl = - getStringProp(createData, 'response_url') || falQueueUrl(endpoint, requestId, 'response') + const statusUrl = resolveFalQueueUrl( + getStringProp(createData, 'status_url'), + buildFalQueueUrl(endpoint, requestId, 'status') + ) + const responseUrl = resolveFalQueueUrl( + getStringProp(createData, 'response_url'), + buildFalQueueUrl(endpoint, requestId, 'result') + ) const maxAttempts = Math.ceil(getMaxExecutionTimeout() / POLL_INTERVAL_MS) for (let attempt = 0; attempt < maxAttempts; attempt++) { await sleep(POLL_INTERVAL_MS) - const statusResponse = await fetch(statusUrl, { headers: { Authorization: `Key ${apiKey}` } }) + const statusResponse = await fetchFalQueue(statusUrl, apiKey) if (!statusResponse.ok) { const body = await readResponseTextWithLimit(statusResponse, { maxBytes: DEFAULT_MAX_ERROR_BODY_BYTES, @@ -128,7 +223,7 @@ export async function runFalQueue( } const statusData = await readResponseJsonWithLimit(statusResponse, { - maxBytes: MAX_MEDIA_JSON_BYTES, + maxBytes: MAX_FAL_QUEUE_JSON_BYTES, label: 'Fal.ai status response', }) if (!isRecordLike(statusData)) throw new Error('Invalid Fal.ai status response') @@ -138,9 +233,10 @@ export async function runFalQueue( if (statusData.error) { throw new Error(`Fal.ai generation failed: ${falErrorMessage(statusData.error)}`) } - const resultResponse = await fetch(getStringProp(statusData, 'response_url') || responseUrl, { - headers: { Authorization: `Key ${apiKey}` }, - }) + const resultResponse = await fetchFalQueue( + resolveFalQueueUrl(getStringProp(statusData, 'response_url'), responseUrl), + apiKey + ) if (!resultResponse.ok) { const body = await readResponseTextWithLimit(resultResponse, { maxBytes: DEFAULT_MAX_ERROR_BODY_BYTES, @@ -151,7 +247,7 @@ export async function runFalQueue( ) } const resultData = await readResponseJsonWithLimit(resultResponse, { - maxBytes: MAX_MEDIA_JSON_BYTES, + maxBytes: MAX_FAL_QUEUE_JSON_BYTES, label: 'Fal.ai result response', }) if (!isRecordLike(resultData)) throw new Error('Invalid Fal.ai result response') diff --git a/apps/sim/lib/media/ffmpeg.test.ts b/apps/sim/lib/media/ffmpeg.test.ts new file mode 100644 index 00000000000..01178a290d1 --- /dev/null +++ b/apps/sim/lib/media/ffmpeg.test.ts @@ -0,0 +1,209 @@ +/** + * @vitest-environment node + */ +import { execFileSync } from 'node:child_process' +import fs from 'node:fs/promises' +import os from 'node:os' +import path from 'node:path' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' +import { buildDrawtextFilter, escapeFilterValue } from '@/lib/media/ffmpeg' + +/** Metacharacters that must survive both filtergraph tokenizer passes. */ +const METACHARACTER_CASES: Array<[string, string]> = [ + ['a:b', 'a\\\\\\:b'], + ["a'b", "a\\\\\\'b"], + ['a\\b', 'a\\\\\\\\b'], + ['a,b', 'a\\\\\\,b'], + ['a;b', 'a\\\\\\;b'], + ['a[b]', 'a\\\\\\[b\\\\\\]'], + ['a=b', 'a\\\\\\=b'], +] + +/** + * FFmpeg is not available in CI, so these assert the generated filtergraph + * string and the on-disk text file rather than the rendered output. The + * `runs through real ffmpeg` block below covers the grammar end to end where a + * binary exists. + */ +describe('escapeFilterValue', () => { + it.concurrent.each(METACHARACTER_CASES)('escapes %j at both levels', (input, expected) => { + expect(escapeFilterValue(input)).toBe(expected) + }) + + it.concurrent('leaves ordinary paths and newlines untouched', () => { + expect(escapeFilterValue('/tmp/media-ffmpeg-abc123/drawtext.txt')).toBe( + '/tmp/media-ffmpeg-abc123/drawtext.txt' + ) + expect(escapeFilterValue('a\nb')).toBe('a\nb') + }) + + it.concurrent('never leaves a bare quote that could open a quoted section', () => { + expect(escapeFilterValue("x':y=1")).not.toMatch(/(^|[^\\])'/) + }) +}) + +describe('buildDrawtextFilter', () => { + let dir: string + + beforeAll(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'ffmpeg-test-')) + }) + + afterAll(async () => { + await fs.rm(dir, { recursive: true, force: true }) + }) + + it('routes the text through textfile and never inlines it', async () => { + const { filter, textPath } = await buildDrawtextFilter(dir, 'Hello world', 'bottom') + + expect(filter.startsWith('drawtext=')).toBe(true) + expect(filter).toContain(`textfile=${textPath}`) + expect(filter).not.toContain('Hello world') + expect(filter).not.toContain(':text=') + expect(await fs.readFile(textPath, 'utf-8')).toBe('Hello world') + }) + + it('writes a distinct text file per call so two overlays cannot clobber each other', async () => { + const first = await buildDrawtextFilter(dir, 'first overlay', 'bottom') + const second = await buildDrawtextFilter(dir, 'second overlay', 'bottom') + + expect(second.textPath).not.toBe(first.textPath) + expect(await fs.readFile(first.textPath, 'utf-8')).toBe('first overlay') + expect(await fs.readFile(second.textPath, 'utf-8')).toBe('second overlay') + }) + + it('disables %{} expansion so text renders literally', async () => { + const { filter, textPath } = await buildDrawtextFilter(dir, '100% of %{pts}', 'bottom') + + expect(filter).toContain('expansion=none') + expect(await fs.readFile(textPath, 'utf-8')).toBe('100% of %{pts}') + }) + + it('cannot introduce a new filter option from the text value', async () => { + const injection = "a':x=90:fontcolor=red,drawbox=0:0:100:100:red\\:,[in]scale=2[out];" + const { filter, textPath } = await buildDrawtextFilter(dir, injection, 'bottom') + + const options = filter.replace(/^drawtext=/, '').split(':') + const optionNames = options.map((option) => option.split('=')[0]) + + expect(optionNames).toEqual([ + 'textfile', + 'expansion', + 'fontcolor', + 'fontsize', + 'box', + 'boxcolor', + 'boxborderw', + 'x', + 'y', + ]) + expect(filter).not.toContain('drawbox') + expect(filter).not.toContain('fontcolor=red') + expect(filter).toContain('fontcolor=white') + expect(filter).not.toContain('x=90') + expect(await fs.readFile(textPath, 'utf-8')).toBe(injection) + }) + + it.each([ + ['single quote', "it's here"], + ['colon', 'ratio 16:9'], + ['backslash', 'C:\\path\\to'], + ['comma', 'a, b, c'], + ['newline', 'line one\nline two'], + ['semicolon and brackets', 'a;b[c]d'], + ['textfile option injection', 'x:textfile=/etc/passwd'], + ['fontfile option injection', 'x:fontfile=/etc/shadow'], + ])('writes %s verbatim to the text file', async (_label, text) => { + const { filter, textPath } = await buildDrawtextFilter(dir, text, 'center') + + expect(await fs.readFile(textPath, 'utf-8')).toBe(text) + expect(filter.replace(/^drawtext=/, '').split(':').length).toBe(9) + expect(filter).toContain('x=(w-text_w)/2') + expect(filter).toContain('y=(h-text_h)/2') + expect(filter.match(/textfile=/g)?.length).toBe(1) + }) + + it('escapes a temp dir path containing filtergraph metacharacters', async () => { + const trickyDir = path.join(dir, "a:b'c[d]") + await fs.mkdir(trickyDir, { recursive: true }) + + const { filter, textPath } = await buildDrawtextFilter(trickyDir, 'text', 'top') + + // The literal expected escaping, not a restatement of the implementation: + // each metacharacter survives as three backslashes plus itself. + const escapedSegment = String.raw`a\\\:b\\\'c\\\[d\\\]` + expect(filter).toContain(`textfile=${dir}/${escapedSegment}/${path.basename(textPath)}`) + expect(filter).not.toContain(`textfile=${trickyDir}/`) + }) + + it('falls back to the bottom position for an unknown position', async () => { + const { filter } = await buildDrawtextFilter(dir, 'text', 'nowhere') + + expect(filter).toContain('y=h*0.86') + }) +}) + +function hasFfmpeg(): boolean { + try { + execFileSync('ffmpeg', ['-version'], { stdio: 'ignore' }) + return true + } catch { + return false + } +} + +/** + * Renders one frame against the real binary so the escaping is validated by + * FFmpeg's own parser rather than by a restatement of it. + */ +describe.skipIf(!hasFfmpeg())('runs through real ffmpeg', () => { + let base: string + + beforeAll(async () => { + base = await fs.mkdtemp(path.join(os.tmpdir(), 'ffmpeg-real-')) + }) + + afterAll(async () => { + await fs.rm(base, { recursive: true, force: true }) + }) + + it.each([ + ['plain', 'plain'], + ['quote', "a'b"], + ['colon', 'a:b'], + ['backslash', 'a\\b'], + ['comma', 'a,b'], + ['semicolon', 'a;b'], + ['equals', 'a=b'], + ['brackets', 'a[b]c'], + ['newline', 'a\nb'], + ['every metacharacter', "all'-:,;=[]\\-\nmix"], + ])('reads the text file from a dir containing %s', async (_label, name) => { + const dir = path.join(base, name) + await fs.mkdir(dir, { recursive: true }) + const { filter } = await buildDrawtextFilter(dir, 'hello', 'bottom') + + expect(() => + execFileSync( + 'ffmpeg', + [ + '-hide_banner', + '-loglevel', + 'error', + '-f', + 'lavfi', + '-i', + 'color=c=black:s=64x64:d=0.04', + '-vf', + filter, + '-frames:v', + '1', + '-f', + 'null', + '-', + ], + { stdio: ['ignore', 'pipe', 'pipe'] } + ) + ).not.toThrow() + }) +}) diff --git a/apps/sim/lib/media/ffmpeg.ts b/apps/sim/lib/media/ffmpeg.ts index bcfa0b6adf0..a07b32b2b19 100644 --- a/apps/sim/lib/media/ffmpeg.ts +++ b/apps/sim/lib/media/ffmpeg.ts @@ -3,6 +3,7 @@ import fs from 'node:fs/promises' import os from 'node:os' import path from 'node:path' import { createLogger } from '@sim/logger' +import { generateShortId } from '@sim/utils/id' import ffmpeg from 'fluent-ffmpeg' const logger = createLogger('MediaFfmpeg') @@ -169,8 +170,64 @@ const TEXT_POSITION: Record = { 'bottom-right': { x: 'w*0.95-text_w', y: 'h*0.86' }, } -function escapeDrawtext(text: string): string { - return text.replace(/\\/g, '\\\\').replace(/:/g, '\\:').replace(/'/g, "\\'").replace(/%/g, '\\%') +const FILTER_METACHARACTERS = /[\\':,;[\]=]/g + +const escapeFilterLevel = (value: string) => value.replace(FILTER_METACHARACTERS, (c) => `\\${c}`) + +/** + * Escapes a value for use inside an FFmpeg filtergraph option, outside quotes. + * + * A filtergraph option value is tokenized twice — once when the graph + * description is split into filters and their argument lists, and again when a + * filter's arguments are split into `name=value` pairs — and each pass consumes + * one level of backslash escapes. A value must therefore be escaped twice. + * + * Escaping `'` once is actively wrong: the surviving `'` opens a quoted section + * and swallows every following option into the value. Quoting the value instead + * is no defence either, since FFmpeg honours no escapes inside `'…'`. + * + * @see https://ffmpeg.org/ffmpeg-filters.html#Notes-on-filtergraph-escaping + */ +function escapeFilterValue(value: string): string { + return escapeFilterLevel(escapeFilterLevel(value)) +} + +/** + * Writes caller-supplied overlay text to a file inside the operation's temp dir + * and returns the `drawtext` filter string that reads it, plus the path written. + * + * Routing the text through `textfile=` keeps it out of the filter-option string + * entirely, so no value in `text` can introduce or alter a filter option. Only + * the temp-dir path — generated by `fs.mkdtemp`, never caller-controlled — + * reaches the filtergraph, and it is escaped for filesystems whose paths can + * contain filtergraph metacharacters. `expansion=none` disables `%{…}` text + * expansion so the content renders literally. + * + * The filename is derived per call so two overlays built in the same temp dir + * cannot clobber each other's text. + */ +async function buildDrawtextFilter( + dir: string, + text: string, + position: string | undefined +): Promise<{ filter: string; textPath: string }> { + const pos = TEXT_POSITION[position || 'bottom'] || TEXT_POSITION.bottom + const textPath = path.join(dir, `drawtext-${generateShortId(8)}.txt`) + await fs.writeFile(textPath, text, 'utf-8') + + const options = [ + `textfile=${escapeFilterValue(textPath)}`, + 'expansion=none', + 'fontcolor=white', + 'fontsize=h/18', + 'box=1', + 'boxcolor=black@0.5', + 'boxborderw=20', + `x=${pos.x}`, + `y=${pos.y}`, + ].join(':') + + return { filter: `drawtext=${options}`, textPath } } async function withTempDir(fn: (dir: string) => Promise): Promise { @@ -479,21 +536,9 @@ async function addText( options: FfmpegOptions ): Promise { if (!options.text) throw new Error('add_text requires text') - const pos = TEXT_POSITION[options.position || 'bottom'] || TEXT_POSITION.bottom - const drawtext = [ - `text='${escapeDrawtext(options.text)}'`, - 'fontcolor=white', - 'fontsize=h/18', - 'box=1', - 'boxcolor=black@0.5', - 'boxborderw=20', - `x=${pos.x}`, - `y=${pos.y}`, - ].join(':') + const { filter } = await buildDrawtextFilter(dir, options.text, options.position) const outputPath = path.join(dir, 'out.mp4') - const command = ffmpeg(inputPath) - .videoFilters(`drawtext=${drawtext}`) - .outputOptions(['-c:a', 'copy']) + const command = ffmpeg(inputPath).videoFilters(filter).outputOptions(['-c:a', 'copy']) await runCommand(command, outputPath) return readOut(outputPath, 'mp4') } @@ -557,4 +602,4 @@ async function thumbnail( return readOut(outputPath, 'jpg') } -export { extFromMime, mimeFromExt } +export { buildDrawtextFilter, escapeFilterValue, extFromMime, mimeFromExt }