From 5b71e8c14d8843e725467174b2684debdf31ab58 Mon Sep 17 00:00:00 2001 From: proseer-lars Date: Mon, 28 Sep 2026 13:15:47 -0400 Subject: [PATCH] fix: deduplicate files by destination before sandbox validation (#247) * fix: deduplicate files by destination before sandbox validation Some callers (e.g. LibreChat) send the same file more than once per exec request when a user re-uploads a file in the same conversation. The sandbox's validateExecuteFiles() rejects duplicate destinations, which causes every tool call in that conversation to fail with: Error from sandbox: [bad_request] files contains duplicate destination "filename.pdf" This surfaces to the end user as a confusing "sandbox is down" error when the sandbox itself is fine - it is just rejecting the duplicate file list. Fix: add deduplicateFilesByDestination() and call it in getJob() before validateExecuteFiles(). Keeps the first file for each destination, silently drops subsequent duplicates, and logs a warning. * fix: preserve validated destinations when deduplicating input files * fix: protect submitted code from uploaded file collisions --------- Co-authored-by: Lia --- api/src/api/v2-files.test.ts | 103 ++++++++++++++++- api/src/api/v2-session-binding.test.ts | 86 ++++++++++++++ api/src/api/v2.ts | 151 ++++++++++++++++--------- 3 files changed, 284 insertions(+), 56 deletions(-) diff --git a/api/src/api/v2-files.test.ts b/api/src/api/v2-files.test.ts index 540b529b..83918884 100644 --- a/api/src/api/v2-files.test.ts +++ b/api/src/api/v2-files.test.ts @@ -1,7 +1,8 @@ import { describe, expect, test } from 'bun:test'; import { config } from '../config'; +import { collectExecuteRequestInputFiles } from '../execution-manifest-request'; import type { TFile } from '../job'; -import { validateExecuteArguments, validateExecuteFiles } from './v2'; +import { validateExecuteArguments, validateExecuteFiles, deduplicateFilesByDestination } from './v2'; function messageOf(fn: () => void): string { try { @@ -85,4 +86,104 @@ describe('execute file validation', () => { ]; expect(() => validateExecuteFiles(files)).not.toThrow(); }); + + test('deduplicateFilesByDestination keeps the latest upload in the original destination order', () => { + const files: TFile[] = [ + { name: 'data.csv', id: 'first', storage_session_id: 'uploads' }, + { name: 'data.csv', id: 'second', storage_session_id: 'uploads' }, + { name: 'other.csv', id: 'unique', storage_session_id: 'uploads' }, + { name: 'data.csv', id: 'third', storage_session_id: 'latest-uploads' }, + ]; + const result = deduplicateFilesByDestination(files); + expect(result.map(file => file.name)).toEqual(['data.csv', 'other.csv']); + expect(result[0].id).toBe('third'); + expect(result[0].storage_session_id).toBe('latest-uploads'); + }); + + test('keeps a by-reference program in the entrypoint position when its upload is refreshed', () => { + const files: TFile[] = [ + { name: 'run.py', id: 'old-script', storage_session_id: 'uploads' }, + { name: 'data.csv', id: 'data', storage_session_id: 'uploads' }, + { name: 'run.py', id: 'new-script', storage_session_id: 'uploads' }, + ]; + const result = deduplicateFilesByDestination(files); + expect(result.map(file => file.name)).toEqual(['run.py', 'data.csv']); + expect(result[0].id).toBe('new-script'); + expect(() => validateExecuteFiles(result)).not.toThrow(); + }); + + test('rejects an upload that targets inline source, regardless of file order', () => { + const source: TFile = { name: 'main.py', content: 'print("submitted program")' }; + const upload: TFile = { name: 'main.py', id: 'uploaded-source', storage_session_id: 'uploads' }; + for (const files of [[source, upload], [upload, source]]) { + const message = messageOf(() => deduplicateFilesByDestination(files)); + expect(message).toContain('duplicate destination "main.py"'); + expect(message).toContain('inline content'); + } + expect(messageOf(() => deduplicateFilesByDestination([ + { content: 'unnamed source' } as TFile, + { name: 'file0.code', id: 'uploaded-source', storage_session_id: 'uploads' }, + ]))).toContain('duplicate destination "file0.code"'); + expect(messageOf(() => deduplicateFilesByDestination([ + source, { name: 'main.py', content: 'different source' }, + ]))).toContain('inline content'); + }); + + test('deduplicateFilesByDestination returns the same array when there are no duplicates', () => { + const files: TFile[] = [ + { name: 'a.csv', content: 'a' }, + { name: 'b.csv', content: 'b' }, + ]; + const result = deduplicateFilesByDestination(files); + expect(result).toEqual(files); + }); + + test('preserves the original destination of an unnamed file reference after deduplication', () => { + const files: TFile[] = [ + { name: 'main.py', content: 'print(1)' }, + { name: 'data.csv', id: 'old', storage_session_id: 'storage-session' }, + { name: 'data.csv', id: 'new', storage_session_id: 'storage-session' }, + { id: 'file-ref', storage_session_id: 'storage-session' } as TFile, + ]; + const deduped = deduplicateFilesByDestination(files); + expect(deduped.map(file => file.name)).toEqual(['main.py', 'data.csv', 'file3.code']); + expect(deduped[1].id).toBe('new'); + const signedDestination = collectExecuteRequestInputFiles({ files }) + .find(file => file.id === 'file-ref'); + expect(collectExecuteRequestInputFiles({ files: deduped }) + .find(file => file.id === 'file-ref')).toEqual(signedDestination); + expect(() => validateExecuteFiles(deduped)).not.toThrow(); + }); + + test('rejects malformed files even if another entry owns their destination', () => { + expect(messageOf(() => deduplicateFilesByDestination([ + { name: 'file1.code', content: 'source' }, + null as unknown as TFile, + ]))).toContain('files[1] must be an object'); + expect(messageOf(() => deduplicateFilesByDestination([ + { name: 'data.csv', content: 'old', encoding: 'invalid' as TFile['encoding'] }, + { name: 'data.csv', content: 'new' }, + ]))).toContain('files[0].encoding'); + expect(messageOf(() => deduplicateFilesByDestination([ + { name: 'data.csv', content: 'old' }, + { name: 'data.csv', id: 'file-ref' }, + ]))).toContain('files[1].storage_session_id'); + }); + + test('caps raw input count before dropping duplicates', () => { + const files = Array.from({ length: config.max_input_files + 1 }, () => ({ + name: 'data.csv', content: 'duplicate', + })); + expect(messageOf(() => deduplicateFilesByDestination(files))).toContain('cannot contain more than'); + }); + + test('deduplicateFilesByDestination allows validateExecuteFiles to accept duplicate uploads', () => { + const files: TFile[] = [ + { name: 'data.csv', id: 'first', storage_session_id: 'uploads' }, + { name: 'data.csv', id: 'second', storage_session_id: 'uploads' }, + ]; + const deduped = deduplicateFilesByDestination(files); + expect(deduped[0].id).toBe('second'); + expect(() => validateExecuteFiles(deduped)).not.toThrow(); + }); }); diff --git a/api/src/api/v2-session-binding.test.ts b/api/src/api/v2-session-binding.test.ts index 32a245f6..0987a68a 100644 --- a/api/src/api/v2-session-binding.test.ts +++ b/api/src/api/v2-session-binding.test.ts @@ -281,6 +281,92 @@ describe('per-request session binding', () => { } }); + test('uses the latest upload without renumbering later unnamed files', async () => { + config.session_workspace_enabled = false; + config.require_execution_manifest = false; + + const originalPrime = Job.prototype.prime; + const originalExecute = Job.prototype.execute; + const originalCleanup = Job.prototype.cleanup; + + let primedFiles: Array<{ name: string; id?: string; content?: string }> = []; + Job.prototype.prime = async function captureFiles(): Promise { + primedFiles = this.files; + }; + Job.prototype.execute = async function executeWithoutSandbox() { + return {} as Awaited>; + }; + Job.prototype.cleanup = async function cleanupWithoutFilesystem(): Promise {}; + + try { + const response = await fetch(`${baseUrl}/api/v2/execute`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + language: testLanguage, + version: testVersion, + files: [ + { name: 'main.txt', content: 'source' }, + { name: 'data.csv', id: 'old-upload', storage_session_id: 'uploads' }, + { name: 'data.csv', id: 'new-upload', storage_session_id: 'uploads' }, + { content: 'unnamed source' }, + ], + }), + }); + + expect(response.status).toBe(200); + expect(primedFiles.map(file => file.name)).toEqual(['main.txt', 'data.csv', 'file3.code']); + expect(primedFiles[0].content).toBe('source'); + expect(primedFiles[1].id).toBe('new-upload'); + expect(primedFiles[2].content).toBe('unnamed source'); + } finally { + Job.prototype.prime = originalPrime; + Job.prototype.execute = originalExecute; + Job.prototype.cleanup = originalCleanup; + } + }); + + test('rejects an upload targeting the submitted source before binding or priming', async () => { + config.session_workspace_enabled = true; + config.require_execution_manifest = false; + + const originalPrime = Job.prototype.prime; + const originalExecute = Job.prototype.execute; + const originalCleanup = Job.prototype.cleanup; + let primed = false; + Job.prototype.prime = async function trackPrime(): Promise { primed = true; }; + Job.prototype.execute = async function executeWithoutSandbox() { + return {} as Awaited>; + }; + Job.prototype.cleanup = async function cleanupWithoutFilesystem(): Promise {}; + + try { + const response = await fetch(`${baseUrl}/api/v2/execute`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', 'X-Runtime-Session-Id': 'rt_source_collision' }, + body: JSON.stringify({ + language: testLanguage, + version: testVersion, + files: [ + { name: 'main.txt', content: 'submitted program' }, + { name: 'data.csv', id: 'data', storage_session_id: 'uploads' }, + { name: 'main.txt', id: 'uploaded-program', storage_session_id: 'uploads' }, + ], + }), + }); + + expect(response.status).toBe(400); + expect((await response.json() as { message: string }).message) + .toContain('duplicate destination "main.txt"'); + expect(primed).toBe(false); + expect(getBoundSessionWorkspace()).toBeUndefined(); + } finally { + Job.prototype.prime = originalPrime; + Job.prototype.execute = originalExecute; + Job.prototype.cleanup = originalCleanup; + } + }); + test('a post-prime failure still reports the workspace as dirty', async () => { config.session_workspace_enabled = true; config.require_execution_manifest = false; diff --git a/api/src/api/v2.ts b/api/src/api/v2.ts index 121cf2e3..3ace08cf 100644 --- a/api/src/api/v2.ts +++ b/api/src/api/v2.ts @@ -46,72 +46,111 @@ import { const router = express.Router(); const SYNTHETIC_PRINCIPAL_SOURCE = 'synthetic_test'; +/** + * Deduplicate files by destination name. Some callers (e.g. LibreChat) + * send the same file more than once per request when a user re-uploads a + * file in the same conversation. The sandbox rejects duplicate destinations, + * which surfaces to the end user as a confusing "sandbox is down" error. + * Keeps the latest uploaded file at each destination without moving its + * original position (the first runnable file is the program entrypoint). + * Never replace inline source code with an uploaded file. + */ +export function deduplicateFilesByDestination(files: TFile[]): TFile[] { + if (files.length > config.max_input_files) { + throw { message: `files cannot contain more than ${config.max_input_files} destinations` }; + } + const byDestination = new Map(); + for (const [i, file] of files.entries()) { + // Validate even entries that would otherwise be replaced by a later upload. + const destination = validateExecuteFile(file, i); + const previous = byDestination.get(destination); + if (previous && (typeof previous.content === 'string' || typeof file.content === 'string')) { + throw { message: `files contains duplicate destination "${destination}" involving inline content` }; + } + // Updating a Map value preserves its first position for Job.execute's entrypoint. + // Manifest claims use original indices; Job would otherwise renumber unnamed files. + byDestination.set(destination, file.name ? file : { ...file, name: destination }); + } + if (byDestination.size < files.length) { + logger.warn( + { original: files.length, deduped: byDestination.size }, + 'Deduplicated file list before validation', + ); + } + return [...byDestination.values()]; +} + function existingDestinationConflictMessage(existing: string, destination: string): string { return existing === destination ? `files contains duplicate destination "${destination}"` : `files contains conflicting destinations "${existing}" and "${destination}"`; } +function validateExecuteFile(value: TFile, i: number): string { + if (value == null || typeof value !== 'object' || Array.isArray(value)) { + throw { message: `files[${i}] must be an object` }; + } + const file = value as TFile; + const inline = typeof file.content === 'string'; + const byRef = typeof file.id === 'string' && file.id.length > 0; + if (inline === byRef) { + throw { + message: `files[${i}] must contain exactly one of non-empty id or string content`, + }; + } + if (file.id !== undefined && !byRef) { + throw { message: `files[${i}].id must be a non-empty string if provided` }; + } + if (byRef) { + if (typeof file.storage_session_id !== 'string' || file.storage_session_id.length === 0) { + throw { message: `files[${i}].storage_session_id is required as a non-empty string for file refs` }; + } + } else if (file.storage_session_id !== undefined || file.input_cache_key !== undefined) { + throw { + message: `files[${i}] inline content cannot include storage_session_id or input_cache_key`, + }; + } + if (file.name !== undefined && typeof file.name !== 'string') { + throw { message: `files[${i}].name must be a string if provided` }; + } + if ( + file.encoding !== undefined + && !(['base64', 'hex', 'utf8'] as const).includes(file.encoding) + ) { + throw { message: `files[${i}].encoding must be base64, hex, or utf8 if provided` }; + } + if (file.entity_id !== undefined && typeof file.entity_id !== 'string') { + throw { message: `files[${i}].entity_id must be a string if provided` }; + } + if ( + file.input_cache_key !== undefined && + ( + typeof file.input_cache_key !== 'string' || + !/^[0-9a-f]{64}$/.test(file.input_cache_key) + ) + ) { + throw { message: `files[${i}].input_cache_key must be a 64-character lowercase hex digest` }; + } + const destination = file.name || `file${i}.code`; + try { + validateFilePath(destination, '/tmp/codeapi-request-validation'); + } catch (error) { + throw { + message: error instanceof Error + ? `files[${i}].name is invalid: ${error.message}` + : `files[${i}].name is invalid`, + }; + } + return destination; +} + export function validateExecuteFiles(files: TFile[]): void { if (files.length > config.max_input_files) { throw { message: `files cannot contain more than ${config.max_input_files} destinations` }; } const destinations = new Set(); for (const [i, value] of files.entries()) { - if (value == null || typeof value !== 'object' || Array.isArray(value)) { - throw { message: `files[${i}] must be an object` }; - } - const file = value as TFile; - const inline = typeof file.content === 'string'; - const byRef = typeof file.id === 'string' && file.id.length > 0; - if (inline === byRef) { - throw { - message: `files[${i}] must contain exactly one of non-empty id or string content`, - }; - } - if (file.id !== undefined && !byRef) { - throw { message: `files[${i}].id must be a non-empty string if provided` }; - } - if (byRef) { - if (typeof file.storage_session_id !== 'string' || file.storage_session_id.length === 0) { - throw { message: `files[${i}].storage_session_id is required as a non-empty string for file refs` }; - } - } else if (file.storage_session_id !== undefined || file.input_cache_key !== undefined) { - throw { - message: `files[${i}] inline content cannot include storage_session_id or input_cache_key`, - }; - } - if (file.name !== undefined && typeof file.name !== 'string') { - throw { message: `files[${i}].name must be a string if provided` }; - } - if ( - file.encoding !== undefined - && !(['base64', 'hex', 'utf8'] as const).includes(file.encoding) - ) { - throw { message: `files[${i}].encoding must be base64, hex, or utf8 if provided` }; - } - if (file.entity_id !== undefined && typeof file.entity_id !== 'string') { - throw { message: `files[${i}].entity_id must be a string if provided` }; - } - if ( - file.input_cache_key !== undefined && - ( - typeof file.input_cache_key !== 'string' || - !/^[0-9a-f]{64}$/.test(file.input_cache_key) - ) - ) { - throw { message: `files[${i}].input_cache_key must be a 64-character lowercase hex digest` }; - } - const destination = file.name || `file${i}.code`; - try { - validateFilePath(destination, '/tmp/codeapi-request-validation'); - } catch (error) { - throw { - message: error instanceof Error - ? `files[${i}].name is invalid: ${error.message}` - : `files[${i}].name is invalid`, - }; - } + const destination = validateExecuteFile(value, i); const conflict = [...destinations].find( existing => existing === destination || @@ -251,12 +290,13 @@ function getJob( runtimeSessionHeader?: string | string[], ): Job { const { - session_id, language, version, args, stdin, files, + session_id, language, version, args, stdin, files: rawFiles, compile_memory_limit, run_memory_limit, compile_timeout, run_cpu_time, compile_cpu_time, env_vars, } = body; + let files = rawFiles; if (!language || typeof language !== 'string') { throw { message: 'language is required as a string' }; @@ -271,6 +311,7 @@ function getJob( throw { message: 'tool_call_socket must be a boolean if specified' }; } validateExecuteArguments(args, stdin); + files = deduplicateFilesByDestination(files); validateExecuteFiles(files); const rt = getLatestRuntimeMatchingLanguageVersion(language, version);