From 99873a7b1234d176b4511f264cb74dc6456d5633 Mon Sep 17 00:00:00 2001 From: Bohdan Triapitsyn Date: Tue, 18 Aug 2026 23:16:46 +0300 Subject: [PATCH] fix(files): harden drag-and-drop uploads Co-authored-by: Serhii Dziupin Co-authored-by: Alan Chen <2144783+alanzchen@users.noreply.github.com> --- CHANGELOG.md | 1 + .../components/layout/SidebarFilesTree.tsx | 10 ++ .../ui/src/components/views/FilesView.tsx | 32 +++- .../src/lib/fileContentInvalidation.test.ts | 38 ++++ .../ui/src/lib/fileContentInvalidation.ts | 25 +++ packages/web/README.md | 1 + packages/web/server/lib/fs/DOCUMENTATION.md | 2 +- packages/web/server/lib/fs/routes.js | 99 ++++++++--- packages/web/server/lib/fs/routes.test.js | 162 +++++++++++++----- 9 files changed, 297 insertions(+), 73 deletions(-) create mode 100644 packages/ui/src/lib/fileContentInvalidation.test.ts create mode 100644 packages/ui/src/lib/fileContentInvalidation.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 349fb84d..ceaf6d51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ All notable changes to this project will be documented in this file. - **Settings/Integrations:** a new Integrations settings page lists Claude Code, Command Code, and Cursor plugins with install, update, setup, and remove actions, plus Discord and Telegram Coming soon placeholders. - **Chat:** an open conversation no longer keeps re-coloring the same code blocks in the background, so browsing files with a chat open stops pinning a CPU core and spinning up the fans (thanks to @makeittech). - **Stability/Proxy:** the local server now reuses its connection to OpenCode instead of opening a new one for every API request. Under sustained traffic the old behavior could use up every outgoing network port on the machine, at which point nothing on the computer could open a new connection until the traffic stopped and the ports were released (thanks to @alohaninja). +- Files: drag files onto the Files sidebar to upload them into the project or a specific folder; existing files require confirmation before replacement, and open previews refresh after an upload (thanks to @makeittech, @alanzchen). - Usage/Claude: Claude plan limits now work when you are signed in through Claude Code, without also signing into Anthropic in OpenCode; the account is read from Claude Code's own login on macOS, Linux, and WSL. The page shows your session and weekly limits again, adds per-model weekly limits and extra usage spending, and names your plan. Limits are kept on screen instead of disappearing when Anthropic temporarily blocks refreshes. - Git: the pull request panel now follows the branch's current open PR, and an open PR always wins over an older merged or closed one. After a PR is merged or closed the panel keeps showing it as the branch's last PR and offers creating the next one right below it (thanks to @makeittech). - Chat: new chats no longer start against a deleted last worktree directory; they fall back to the active project instead of saving the first message and never starting. diff --git a/packages/ui/src/components/layout/SidebarFilesTree.tsx b/packages/ui/src/components/layout/SidebarFilesTree.tsx index 8d56c831..624083c2 100644 --- a/packages/ui/src/components/layout/SidebarFilesTree.tsx +++ b/packages/ui/src/components/layout/SidebarFilesTree.tsx @@ -44,6 +44,7 @@ import { FileTypeIcon } from '@/components/icons/FileTypeIcon'; import { Icon } from "@/components/icon/Icon"; import { getContextFileOpenFailureMessage, validateContextFileOpen } from '@/lib/contextFileOpenGuard'; import { isFilesystemError } from '@/lib/api/files-errors'; +import { notifyFileContentInvalidated } from '@/lib/fileContentInvalidation'; import { isBrowserClientRuntime } from '@/lib/desktop'; import { useI18n } from '@/lib/i18n'; @@ -1044,9 +1045,18 @@ export const SidebarFilesTree: React.FC = () => { const uploadedCount = outcomes.filter((outcome) => outcome === 'uploaded').length; const failedCount = outcomes.filter((outcome) => outcome === 'failed').length; const conflictingFiles = droppedFiles.filter((_, index) => outcomes[index] === 'conflict'); + const uploadedPaths = droppedFiles.flatMap((file, index) => { + const name = getUploadName(file); + return outcomes[index] === 'uploaded' && name + ? [normalizePath(`${directory}/${name}`)] + : []; + }); const isCurrentDestination = rootRef.current === operationRoot && getRuntimeKey() === operationRuntime; try { + if (uploadedPaths.length > 0) { + notifyFileContentInvalidated({ runtimeKey: operationRuntime, paths: uploadedPaths }); + } if (uploadedCount > 0 && isCurrentDestination) { await refreshDirectory(directory); } diff --git a/packages/ui/src/components/views/FilesView.tsx b/packages/ui/src/components/views/FilesView.tsx index 6452099d..23e73735 100644 --- a/packages/ui/src/components/views/FilesView.tsx +++ b/packages/ui/src/components/views/FilesView.tsx @@ -48,8 +48,9 @@ import { getLanguageFromExtension, getImageMimeType, isBinaryFile, isDrawioFile, import { shouldAllowFileDraftSave, shouldScheduleFileAutosave } from '@/lib/fileEditorAutosave'; import { getRuntimeUrlResolver } from '@/lib/runtime-url'; import { acquireRuntimeUrlAuthToken, refreshRuntimeUrlAuthToken, subscribeRuntimeUrlAuthToken } from '@/lib/runtime-auth'; -import { getRuntimeApiBaseUrl } from '@/lib/runtime-switch'; +import { getRuntimeApiBaseUrl, getRuntimeKey } from '@/lib/runtime-switch'; import { getOutsideFileGrant } from '@/lib/outsideFileGrants'; +import { subscribeToFileContentInvalidation } from '@/lib/fileContentInvalidation'; import { DiagramEditor } from '@/components/diagram'; import { useRuntimeAPIs } from '@/hooks/useRuntimeAPIs'; import { EditorView } from '@codemirror/view'; @@ -920,6 +921,7 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { const lastLoadedFileStatRef = React.useRef(null); const activeFileLoadIdRef = React.useRef(0); const loadingFilePathRef = React.useRef(null); + const [fileContentRevision, setFileContentRevision] = React.useState(0); const [autoSaveStatus, setAutoSaveStatus] = React.useState<'idle' | 'saved'>('idle'); const [diagramSaved, setDiagramSaved] = React.useState(false); const [contentDetectedBinary, setContentDetectedBinary] = React.useState(false); @@ -2046,13 +2048,33 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { loadingFilePathRef.current = null; } }); - }, [loadSelectedFile, loadedFilePath, selectedFile]); + }, [fileContentRevision, loadSelectedFile, loadedFilePath, selectedFile]); // Sync isDirty to a ref so the polling interval can read the latest value // without isDirty in its dependency array (avoids interval restart on every edit/save). const isDirtyRef = React.useRef(isDirty); isDirtyRef.current = isDirty; + React.useEffect(() => subscribeToFileContentInvalidation(({ runtimeKey, paths }) => { + const selectedPath = selectedFile?.path; + if ( + runtimeKey !== getRuntimeKey() + || !selectedPath + || isDirtyRef.current + || !paths.includes(normalizePath(selectedPath)) + ) { + return; + } + + activeFileLoadIdRef.current += 1; + loadingFilePathRef.current = null; + lastLoadedFileStatRef.current = null; + setDesktopImageSrc(''); + setFileError(null); + setLoadedFilePath(null); + setFileContentRevision((revision) => revision + 1); + }), [selectedFile?.path]); + // Poll open file for external changes. // When a change is detected, reset loadedFilePath so the effect above // triggers a single reload — no double-load. @@ -3006,11 +3028,11 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { ); const pdfAssetAuthKey = selectedFile?.path && isSelectedPdf - ? `${selectedFile.path}|${selectedFileReadOptions.allowOutsideWorkspace ? 'outside' : 'workspace'}|${selectedFileReadOptions.outsideFileGrant ?? ''}` + ? `${selectedFile.path}|${selectedFileReadOptions.allowOutsideWorkspace ? 'outside' : 'workspace'}|${selectedFileReadOptions.outsideFileGrant ?? ''}|${fileContentRevision}` : ''; const htmlAssetAuthKey = selectedFile?.path && isHtml && htmlViewMode === 'preview' && !runtime.isVSCode - ? selectedFile.path + ? `${selectedFile.path}|${fileContentRevision}` : ''; const assetAuthErrorFallback = t('filesView.error.readFileFailed'); @@ -3113,7 +3135,7 @@ export const FilesView: React.FC = ({ mode = 'full' }) => { URL.revokeObjectURL(objectUrl); } }; - }, [files, isSelectedImage, isSelectedSvg, root, selectedFile?.path, selectedFileReadOptions, t]); + }, [fileContentRevision, files, isSelectedImage, isSelectedSvg, root, selectedFile?.path, selectedFileReadOptions, t]); const handleCloseDialog = React.useCallback(() => setActiveDialog(null), []); diff --git a/packages/ui/src/lib/fileContentInvalidation.test.ts b/packages/ui/src/lib/fileContentInvalidation.test.ts new file mode 100644 index 00000000..ed05e4b8 --- /dev/null +++ b/packages/ui/src/lib/fileContentInvalidation.test.ts @@ -0,0 +1,38 @@ +import { describe, expect, test } from 'bun:test'; + +import { + notifyFileContentInvalidated, + subscribeToFileContentInvalidation, +} from './fileContentInvalidation'; + +describe('fileContentInvalidation', () => { + test('publishes normalized paths within the captured runtime', () => { + const received: Array<{ runtimeKey: string; paths: readonly string[] }> = []; + const unsubscribe = subscribeToFileContentInvalidation((invalidation) => { + received.push(invalidation); + }); + + notifyFileContentInvalidated({ + runtimeKey: ' runtime-a ', + paths: [' /repo/a.txt ', '/repo/a.txt', '', '/repo/b.txt'], + }); + unsubscribe(); + + expect(received).toEqual([{ + runtimeKey: 'runtime-a', + paths: ['/repo/a.txt', '/repo/b.txt'], + }]); + }); + + test('stops publishing after unsubscribe', () => { + let calls = 0; + const unsubscribe = subscribeToFileContentInvalidation(() => { + calls += 1; + }); + unsubscribe(); + + notifyFileContentInvalidated({ runtimeKey: 'runtime-a', paths: ['/repo/a.txt'] }); + + expect(calls).toBe(0); + }); +}); diff --git a/packages/ui/src/lib/fileContentInvalidation.ts b/packages/ui/src/lib/fileContentInvalidation.ts new file mode 100644 index 00000000..5cddd169 --- /dev/null +++ b/packages/ui/src/lib/fileContentInvalidation.ts @@ -0,0 +1,25 @@ +type FileContentInvalidation = { + runtimeKey: string; + paths: readonly string[]; +}; + +type FileContentInvalidationListener = (invalidation: FileContentInvalidation) => void; + +const listeners = new Set(); + +export const notifyFileContentInvalidated = (invalidation: FileContentInvalidation): void => { + const runtimeKey = invalidation.runtimeKey.trim(); + const paths = Array.from(new Set(invalidation.paths.map((path) => path.trim()).filter(Boolean))); + if (!runtimeKey || paths.length === 0) return; + + for (const listener of listeners) { + listener({ runtimeKey, paths }); + } +}; + +export const subscribeToFileContentInvalidation = ( + listener: FileContentInvalidationListener, +): (() => void) => { + listeners.add(listener); + return () => listeners.delete(listener); +}; diff --git a/packages/web/README.md b/packages/web/README.md index 03e9e752..da27a1ef 100644 --- a/packages/web/README.md +++ b/packages/web/README.md @@ -115,6 +115,7 @@ OPENCODE_HOST=https://myhost:4096 OPENCODE_SKIP_START=true openchamber | `OPENCHAMBER_VERBOSE_REQUEST_LOGS` | Set to `true` to log every HTTP request; disabled by default to keep user logs small | | `OPENCHAMBER_SKIP_API_COMPRESSION` | Set to `true` to disable gzip compression for `/api/*` responses | | `OPENCHAMBER_COMPRESS_API` | Set to `true` to force `/api/*` compression, or `false` to disable it. Desktop runtime disables API compression by default to reduce local sidecar CPU use | +| `OPENCHAMBER_FS_UPLOAD_MAX_BYTES` | Maximum file upload size in bytes (default: 100 MiB) | | `OPENCHAMBER_TERMINAL_SHELL` | Preferred terminal shell executable used by the `Auto` setting before platform defaults | diff --git a/packages/web/server/lib/fs/DOCUMENTATION.md b/packages/web/server/lib/fs/DOCUMENTATION.md index cdf2fd62..07b166fd 100644 --- a/packages/web/server/lib/fs/DOCUMENTATION.md +++ b/packages/web/server/lib/fs/DOCUMENTATION.md @@ -39,4 +39,4 @@ Own filesystem API behavior for the web server runtime, including workspace-boun - Read-only routes authorize the requested path against the workspace before resolving symlinks. A symlink reached through the workspace may therefore target a file outside it, while a directly requested outside path still requires an exact-path grant. Write routes keep canonical-target boundary checks. - If adding new `/api/fs/*` endpoints, add them in `routes.js` and extend this document. - `GET /api/fs/list` may resolve symlinks with `realpath` to read directory contents, but the response `path` and each entry `path` must stay in the caller's requested path space (`path.join(requestedPath, name)`). Returning real paths breaks file-tree expansion for directories reached through workspace symlinks. -- `POST /api/fs/upload` accepts one `application/octet-stream` body (up to 100 MB) with `path` and optional `overwrite=true` query parameters. It rejects existing files with `409` unless overwrite is explicit, and resolves the destination parent before writing so uploads cannot escape through workspace symlinks. +- `POST /api/fs/upload` accepts one `application/octet-stream` body with `path` and optional `overwrite=true` query parameters. The body streams into a same-directory temp file with a 100 MiB default cap configurable through `OPENCHAMBER_FS_UPLOAD_MAX_BYTES`; failed and oversized uploads clean up that temp file. New files commit through an atomic no-replace link, existing files return `409` unless overwrite is explicit, directory targets are rejected, and the destination parent resolves before writing so uploads cannot escape through workspace symlinks. diff --git a/packages/web/server/lib/fs/routes.js b/packages/web/server/lib/fs/routes.js index 7888742c..96f07fda 100644 --- a/packages/web/server/lib/fs/routes.js +++ b/packages/web/server/lib/fs/routes.js @@ -108,6 +108,12 @@ const createGitCheckIgnoreTimeoutMs = () => { return 2500; }; +const createUploadMaxBytes = () => { + const raw = Number(process.env.OPENCHAMBER_FS_UPLOAD_MAX_BYTES); + if (Number.isFinite(raw) && raw > 0) return Math.floor(raw); + return 100 * 1024 * 1024; +}; + const FILE_MIME_MAP = Object.freeze({ '.html': 'text/html', '.htm': 'text/html', @@ -139,28 +145,26 @@ const FILE_MIME_MAP = Object.freeze({ }); const MAX_SERVE_BYTES = 100 * 1024 * 1024; -const MAX_UPLOAD_BYTES = 100 * 1024 * 1024; -const readUploadBody = async (req) => { - const declaredSize = Number.parseInt(req.headers?.['content-length'] || '0', 10); - if (Number.isFinite(declaredSize) && declaredSize > MAX_UPLOAD_BYTES) { - req.resume?.(); - return null; - } - - const chunks = []; - let size = 0; +const streamUploadBody = async (req, handle, maxBytes) => { + let received = 0; for await (const chunk of req) { const buffer = Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk); - size += buffer.length; - if (size > MAX_UPLOAD_BYTES) { + received += buffer.length; + if (received > maxBytes) { req.resume?.(); - return null; + throw Object.assign(new Error('Upload exceeds the maximum allowed size'), { uploadTooLarge: true }); } - chunks.push(buffer); - } - return Buffer.concat(chunks, size); + let offset = 0; + while (offset < buffer.length) { + const { bytesWritten } = await handle.write(buffer, offset, buffer.length - offset, null); + if (!Number.isFinite(bytesWritten) || bytesWritten <= 0) { + throw new Error('Failed to write upload'); + } + offset += bytesWritten; + } + } }; // Only deterministic, side-effect-free git plumbing path queries are cacheable. @@ -1074,6 +1078,13 @@ export const registerFsRoutes = (app, dependencies) => { return res.status(415).json({ error: 'Content-Type must be application/octet-stream' }); } + const maxUploadBytes = createUploadMaxBytes(); + const declaredSize = Number(req.headers?.['content-length']); + if (Number.isFinite(declaredSize) && declaredSize > maxUploadBytes) { + req.resume?.(); + return res.status(413).json({ error: `File exceeds maximum size of ${maxUploadBytes} bytes` }); + } + try { const resolved = await resolveWorkspacePathFromContext({ req, @@ -1106,22 +1117,50 @@ export const registerFsRoutes = (app, dependencies) => { return res.status(403).json({ error: 'Access denied' }); } - const body = await readUploadBody(req); - if (!body) { - return res.status(413).json({ error: `File exceeds maximum size of ${MAX_UPLOAD_BYTES} bytes` }); + if (existingPath) { + const stats = await fsPromises.stat(existingPath); + if (stats.isDirectory()) { + return res.status(400).json({ error: 'Specified path is a directory' }); + } + if (!overwrite) { + req.resume?.(); + return res.status(409).json({ error: 'File already exists', reason: 'already-exists' }); + } } - if (!overwrite) { - await fsPromises.writeFile(writePath, body, { flag: 'wx' }); - } else { - const tmp = `${writePath}.tmp-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const tmp = `${writePath}.upload-${crypto.randomUUID()}`; + let tempExists = false; + try { + const handle = await fsPromises.open(tmp, 'wx'); + tempExists = true; + let streamError = null; try { - await fsPromises.writeFile(tmp, body, { flag: 'wx' }); - await fsPromises.rename(tmp, writePath); + await streamUploadBody(req, handle, maxUploadBytes); } catch (error) { - await fsPromises.unlink(tmp).catch(() => {}); - throw error; + streamError = error; } + try { + await handle.close(); + } catch (error) { + if (!streamError) throw error; + } + if (streamError) throw streamError; + + if (overwrite) { + await fsPromises.rename(tmp, writePath); + } else { + // A same-directory hard link commits without replacing a target that + // appeared after the existence check. The temp file is already fully + // flushed, so readers never observe a partial upload. + await fsPromises.link(tmp, writePath); + await fsPromises.unlink(tmp).catch(() => {}); + } + tempExists = false; + } catch (error) { + if (tempExists) { + await fsPromises.unlink(tmp).catch(() => {}); + } + throw error; } return res.json({ success: true, path: resolved.resolved }); @@ -1133,6 +1172,12 @@ export const registerFsRoutes = (app, dependencies) => { if (err && typeof err === 'object' && err.code === 'ENOENT') { return res.status(404).json({ error: 'Destination directory not found', reason: 'not-found' }); } + if (err && typeof err === 'object' && err.uploadTooLarge) { + return res.status(413).json({ error: `File exceeds maximum size of ${maxUploadBytes} bytes` }); + } + if (err && typeof err === 'object' && (err.code === 'EISDIR' || err.code === 'ENOTDIR')) { + return res.status(400).json({ error: 'Specified path is a directory' }); + } if (isOsPermissionError(err)) { return sendOsPermissionDenied(res, 'Access denied'); } diff --git a/packages/web/server/lib/fs/routes.test.js b/packages/web/server/lib/fs/routes.test.js index dbda0a27..64467af7 100644 --- a/packages/web/server/lib/fs/routes.test.js +++ b/packages/web/server/lib/fs/routes.test.js @@ -146,7 +146,11 @@ const registerUpload = (fsPromises) => { os: { homedir: () => '/home/user' }, path: path.posix, fsPromises: { - realpath: async (targetPath) => targetPath, + realpath: async (targetPath) => { + if (targetPath === '/repo') return targetPath; + throw Object.assign(new Error('not found'), { code: 'ENOENT' }); + }, + stat: async () => ({ isDirectory: () => false }), ...fsPromises, }, spawn: vi.fn(), @@ -253,16 +257,22 @@ const callWrite = async (handler, body) => { return res; }; -const callUpload = async (handler, { body = Buffer.from('upload'), path: filePath = '/repo/file.bin', overwrite = false } = {}) => { +const callUpload = async (handler, { + body = Buffer.from('upload'), + chunks, + includeContentLength = true, + path: filePath = '/repo/file.bin', + overwrite = false, +} = {}) => { const res = createMockResponse(); + const uploadChunks = chunks ?? [body]; + const headers = { 'content-type': 'application/octet-stream' }; + if (includeContentLength) headers['content-length'] = String(body.length); const req = { - headers: { - 'content-type': 'application/octet-stream', - 'content-length': String(body.length), - }, + headers, query: { path: filePath, overwrite: overwrite ? 'true' : undefined }, async *[Symbol.asyncIterator]() { - yield body; + yield* uploadChunks; }, }; await handler(req, res); @@ -377,29 +387,40 @@ describe('fs write', () => { }); describe('fs upload', () => { - it('creates a binary file without overwriting existing content', async () => { + it('streams a binary file to temp storage before committing it without overwrite', async () => { + const write = vi.fn(async (_buffer, _offset, length) => ({ bytesWritten: length })); + const close = vi.fn(async () => undefined); const fsPromises = { - writeFile: vi.fn(async () => undefined), + open: vi.fn(async () => ({ write, close })), + link: vi.fn(async () => undefined), rename: vi.fn(async () => undefined), unlink: vi.fn(async () => undefined), }; const handler = registerUpload(fsPromises); - const res = await callUpload(handler, { body: Buffer.from([0, 1, 2, 255]) }); + const body = Buffer.from([0, 1, 2, 255]); + const res = await callUpload(handler, { + body, + chunks: [body.subarray(0, 2), body.subarray(2)], + }); expect(res.body).toEqual({ success: true, path: '/repo/file.bin' }); - expect(fsPromises.writeFile).toHaveBeenCalledWith( - '/repo/file.bin', - Buffer.from([0, 1, 2, 255]), - { flag: 'wx' }, - ); + const tmp = fsPromises.open.mock.calls[0][0]; + expect(tmp).toMatch(/^\/repo\/file\.bin\.upload-/); + expect(fsPromises.open).toHaveBeenCalledWith(tmp, 'wx'); + expect(write).toHaveBeenNthCalledWith(1, Buffer.from([0, 1]), 0, 2, null); + expect(write).toHaveBeenNthCalledWith(2, Buffer.from([2, 255]), 0, 2, null); + expect(close).toHaveBeenCalledTimes(1); + expect(fsPromises.link).toHaveBeenCalledWith(tmp, '/repo/file.bin'); + expect(fsPromises.unlink).toHaveBeenCalledWith(tmp); expect(fsPromises.rename).not.toHaveBeenCalled(); }); it('returns a conflict instead of silently replacing an existing file', async () => { - const error = Object.assign(new Error('exists'), { code: 'EEXIST' }); const fsPromises = { - writeFile: vi.fn(async () => { throw error; }), + realpath: vi.fn(async (targetPath) => targetPath), + stat: vi.fn(async () => ({ isDirectory: () => false })), + open: vi.fn(async () => ({ write: vi.fn(), close: vi.fn() })), }; const handler = registerUpload(fsPromises); @@ -407,11 +428,15 @@ describe('fs upload', () => { expect(res.statusCode).toBe(409); expect(res.body).toEqual({ error: 'File already exists', reason: 'already-exists' }); + expect(fsPromises.open).not.toHaveBeenCalled(); }); it('atomically replaces a file only when overwrite is explicit', async () => { + const write = vi.fn(async (_buffer, _offset, length) => ({ bytesWritten: length })); const fsPromises = { - writeFile: vi.fn(async () => undefined), + realpath: vi.fn(async (targetPath) => targetPath), + stat: vi.fn(async () => ({ isDirectory: () => false })), + open: vi.fn(async () => ({ write, close: vi.fn(async () => undefined) })), rename: vi.fn(async () => undefined), unlink: vi.fn(async () => undefined), }; @@ -420,16 +445,31 @@ describe('fs upload', () => { const res = await callUpload(handler, { overwrite: true }); expect(res.body).toEqual({ success: true, path: '/repo/file.bin' }); - const tmp = fsPromises.writeFile.mock.calls[0][0]; - expect(tmp).toMatch(/^\/repo\/file\.bin\.tmp-/); - expect(fsPromises.writeFile).toHaveBeenCalledWith(tmp, Buffer.from('upload'), { flag: 'wx' }); + const tmp = fsPromises.open.mock.calls[0][0]; + expect(tmp).toMatch(/^\/repo\/file\.bin\.upload-/); + expect(write).toHaveBeenCalledWith(Buffer.from('upload'), 0, 6, null); expect(fsPromises.rename).toHaveBeenCalledWith(tmp, '/repo/file.bin'); }); + it('rejects an existing directory before reading the upload body', async () => { + const fsPromises = { + realpath: vi.fn(async (targetPath) => targetPath), + stat: vi.fn(async () => ({ isDirectory: () => true })), + open: vi.fn(async () => ({ write: vi.fn(), close: vi.fn() })), + }; + const handler = registerUpload(fsPromises); + + const res = await callUpload(handler); + + expect(res.statusCode).toBe(400); + expect(res.body).toEqual({ error: 'Specified path is a directory' }); + expect(fsPromises.open).not.toHaveBeenCalled(); + }); + it('rejects a destination parent that resolves outside the workspace', async () => { const fsPromises = { realpath: vi.fn(async (targetPath) => targetPath === '/repo/link' ? '/outside' : targetPath), - writeFile: vi.fn(async () => undefined), + open: vi.fn(async () => ({ write: vi.fn(), close: vi.fn() })), }; const handler = registerUpload(fsPromises); @@ -437,31 +477,73 @@ describe('fs upload', () => { expect(res.statusCode).toBe(403); expect(res.body).toEqual({ error: 'Access denied' }); - expect(fsPromises.writeFile).not.toHaveBeenCalled(); + expect(fsPromises.open).not.toHaveBeenCalled(); }); - it('rejects streamed bodies larger than 100 MB', async () => { + it('cleans up a partial temp file when the configured streaming limit is exceeded', async () => { + const previous = process.env.OPENCHAMBER_FS_UPLOAD_MAX_BYTES; + process.env.OPENCHAMBER_FS_UPLOAD_MAX_BYTES = '5'; + const write = vi.fn(async (_buffer, _offset, length) => ({ bytesWritten: length })); const fsPromises = { - writeFile: vi.fn(async () => undefined), + open: vi.fn(async () => ({ write, close: vi.fn(async () => undefined) })), + link: vi.fn(async () => undefined), + unlink: vi.fn(async () => undefined), + }; + try { + const handler = registerUpload(fsPromises); + const res = await callUpload(handler, { + body: Buffer.from('123456'), + chunks: [Buffer.from('123'), Buffer.from('456')], + includeContentLength: false, + }); + + expect(res.statusCode).toBe(413); + expect(res.body).toEqual({ error: 'File exceeds maximum size of 5 bytes' }); + expect(write).toHaveBeenCalledWith(Buffer.from('123'), 0, 3, null); + expect(fsPromises.link).not.toHaveBeenCalled(); + expect(fsPromises.unlink).toHaveBeenCalledWith(expect.stringMatching(/^\/repo\/file\.bin\.upload-/)); + } finally { + if (previous === undefined) delete process.env.OPENCHAMBER_FS_UPLOAD_MAX_BYTES; + else process.env.OPENCHAMBER_FS_UPLOAD_MAX_BYTES = previous; + } + }); + + it('rejects a declared oversized upload before opening a temp file', async () => { + const previous = process.env.OPENCHAMBER_FS_UPLOAD_MAX_BYTES; + process.env.OPENCHAMBER_FS_UPLOAD_MAX_BYTES = '5'; + const fsPromises = { + open: vi.fn(async () => ({ write: vi.fn(), close: vi.fn() })), + }; + try { + const handler = registerUpload(fsPromises); + const res = await callUpload(handler, { body: Buffer.from('123456') }); + + expect(res.statusCode).toBe(413); + expect(res.body).toEqual({ error: 'File exceeds maximum size of 5 bytes' }); + expect(fsPromises.open).not.toHaveBeenCalled(); + } finally { + if (previous === undefined) delete process.env.OPENCHAMBER_FS_UPLOAD_MAX_BYTES; + else process.env.OPENCHAMBER_FS_UPLOAD_MAX_BYTES = previous; + } + }); + + it('keeps the existing file when a target appears before the atomic commit', async () => { + const error = Object.assign(new Error('exists'), { code: 'EEXIST' }); + const fsPromises = { + open: vi.fn(async () => ({ + write: vi.fn(async (_buffer, _offset, length) => ({ bytesWritten: length })), + close: vi.fn(async () => undefined), + })), + link: vi.fn(async () => { throw error; }), + unlink: vi.fn(async () => undefined), }; const handler = registerUpload(fsPromises); - const chunk = Buffer.alloc(1024 * 1024); - const req = { - headers: { 'content-type': 'application/octet-stream' }, - query: { path: '/repo/file.bin' }, - async *[Symbol.asyncIterator]() { - for (let index = 0; index < 101; index += 1) { - yield chunk; - } - }, - }; - const res = createMockResponse(); - await handler(req, res); + const res = await callUpload(handler); - expect(res.statusCode).toBe(413); - expect(res.body).toEqual({ error: `File exceeds maximum size of ${100 * 1024 * 1024} bytes` }); - expect(fsPromises.writeFile).not.toHaveBeenCalled(); + expect(res.statusCode).toBe(409); + expect(res.body).toEqual({ error: 'File already exists', reason: 'already-exists' }); + expect(fsPromises.unlink).toHaveBeenCalledWith(expect.stringMatching(/^\/repo\/file\.bin\.upload-/)); }); });