From c8b095b88791424fac6c6955d7d3853ef59eb3e3 Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 24 May 2026 23:17:22 -0400 Subject: [PATCH] fix(stack-files): confirm before overwriting an existing upload target (#1204) * fix(stack-files): confirm before overwriting an existing upload target Same-name uploads previously truncated the existing file silently. A user dragging a file with a name that matched an in-place file destroyed the original with no warning and no undo. The upload route now reads ?overwrite=0|1. When the flag is not set and the target already exists, the server returns 409 FILE_EXISTS and the original file is untouched. The frontend opens a confirm dialog and retries with overwrite=1 on the user's approval; cancel keeps the original. A new pathExists helper on FileSystemService performs the existence check through the same path-resolution barrier as the write so a malicious relPath cannot bypass the conflict check. UploadConflictError is exported so callers can distinguish the conflict case from generic upload failures without parsing error strings. * fix(stack-files): distinct DIR_EXISTS code, drop INVALID_PATH swallow in existence check --- .../src/__tests__/stack-files-routes.test.ts | 64 ++++++++++++++- backend/src/routes/stacks.ts | 22 +++++- backend/src/services/FileSystemService.ts | 19 +++++ .../components/files/FileUploadDropzone.tsx | 49 +++++++++--- .../__tests__/FileUploadDropzone.test.tsx | 77 +++++++++++++++++-- frontend/src/lib/stackFilesApi.ts | 29 ++++++- 6 files changed, 240 insertions(+), 20 deletions(-) diff --git a/backend/src/__tests__/stack-files-routes.test.ts b/backend/src/__tests__/stack-files-routes.test.ts index 87703fc9..75230d21 100644 --- a/backend/src/__tests__/stack-files-routes.test.ts +++ b/backend/src/__tests__/stack-files-routes.test.ts @@ -320,6 +320,63 @@ describe('POST /api/stacks/:stackName/files/upload', () => { const content = await fs.readFile(path.join(stacksDir, STACK, 'subdir', 'sub.txt'), 'utf-8'); expect(content).toBe('subdir content'); }); + + it('returns 409 FILE_EXISTS when the target name already exists and overwrite is not set', async () => { + const target = path.join(stacksDir, STACK, 'existing.txt'); + await fs.writeFile(target, 'original'); + const res = await request(app) + .post(`/api/stacks/${STACK}/files/upload`) + .set('Cookie', adminCookie) + .attach('file', Buffer.from('replacement'), 'existing.txt'); + expect(res.status).toBe(409); + expect(res.body.code).toBe('FILE_EXISTS'); + // Original content must be preserved when the upload is rejected. + const after = await fs.readFile(target, 'utf-8'); + expect(after).toBe('original'); + await fs.unlink(target); + }); + + it('overwrites when ?overwrite=1 is set', async () => { + const target = path.join(stacksDir, STACK, 'replaceme.txt'); + await fs.writeFile(target, 'before'); + const res = await request(app) + .post(`/api/stacks/${STACK}/files/upload`) + .query({ overwrite: '1' }) + .set('Cookie', adminCookie) + .attach('file', Buffer.from('after'), 'replaceme.txt'); + expect(res.status).toBe(204); + const after = await fs.readFile(target, 'utf-8'); + expect(after).toBe('after'); + await fs.unlink(target); + }); + + it('returns 409 DIR_EXISTS when a directory occupies the upload target name', async () => { + const dir = path.join(stacksDir, STACK, 'collide-dir'); + await fs.mkdir(dir, { recursive: true }); + const res = await request(app) + .post(`/api/stacks/${STACK}/files/upload`) + .set('Cookie', adminCookie) + .attach('file', Buffer.from('whatever'), 'collide-dir'); + expect(res.status).toBe(409); + expect(res.body.code).toBe('DIR_EXISTS'); + await fs.rm(dir, { recursive: true, force: true }); + }); + + it('still returns 409 DIR_EXISTS even when ?overwrite=1 is set (directories are never replaced)', async () => { + const dir = path.join(stacksDir, STACK, 'collide-dir-2'); + await fs.mkdir(dir, { recursive: true }); + const res = await request(app) + .post(`/api/stacks/${STACK}/files/upload`) + .query({ overwrite: '1' }) + .set('Cookie', adminCookie) + .attach('file', Buffer.from('whatever'), 'collide-dir-2'); + expect(res.status).toBe(409); + expect(res.body.code).toBe('DIR_EXISTS'); + // The directory must still exist after the rejected upload. + const stat = await fs.stat(dir); + expect(stat.isDirectory()).toBe(true); + await fs.rm(dir, { recursive: true, force: true }); + }); }); // ── PUT /:stackName/files/content ───────────────────────────────────────────── @@ -702,10 +759,15 @@ describe('protected stack files', () => { expect(res.status).toBe(204); }); - it('POST /files/upload still succeeds when overwriting compose.yaml (legitimate replace)', async () => { + it('POST /files/upload still succeeds when overwriting compose.yaml with overwrite=1 (legitimate replace)', async () => { + // Combined semantics: same-name uploads need ?overwrite=1 to pass the + // upload-confirm gate. The protected-file enforcement deliberately does + // NOT block this path because replacing compose.yaml via upload is a + // legitimate user-driven action. const replacement = 'services:\n uploaded:\n image: busybox\n'; const res = await request(app) .post(`/api/stacks/${STACK}/files/upload`) + .query({ overwrite: '1' }) .set('Cookie', adminCookie) .attach('file', Buffer.from(replacement), 'compose.yaml'); expect(res.status).toBe(204); diff --git a/backend/src/routes/stacks.ts b/backend/src/routes/stacks.ts index e19a16ff..fdf06e5d 100644 --- a/backend/src/routes/stacks.ts +++ b/backend/src/routes/stacks.ts @@ -1310,6 +1310,8 @@ type FsErrorCode = | 'NOT_FOUND' | 'TOO_LARGE' | 'ALREADY_EXISTS' + | 'FILE_EXISTS' + | 'DIR_EXISTS' | 'PROTECTED_FILE'; function sendFsError( @@ -1477,11 +1479,27 @@ stacksRouter.post( return res.status(400).json({ error: 'Invalid filename' }); } const targetRelPath = relPath ? `${relPath}/${originalName}` : originalName; + const overwrite = String(req.query.overwrite) === '1'; const startedAt = Date.now(); - logFileDiag('upload start', { stackName, relPath: targetRelPath, nodeId: req.nodeId, size: req.file.size }); + logFileDiag('upload start', { stackName, relPath: targetRelPath, nodeId: req.nodeId, size: req.file.size, overwrite }); try { + const existing = await FileSystemService.getInstance(req.nodeId).pathKind(stackName, targetRelPath); + if (existing === 'directory') { + // A directory can never be replaced by an upload; surface a distinct code + // so the UI does not offer a useless "Replace" button. + return res.status(409).json({ + error: `A folder named ${originalName} already exists in this folder. Rename the upload or remove the folder first.`, + code: 'DIR_EXISTS', + }); + } + if (existing === 'file' && !overwrite) { + return res.status(409).json({ + error: `${originalName} already exists in this folder. Confirm to replace.`, + code: 'FILE_EXISTS', + }); + } await FileSystemService.getInstance(req.nodeId).writeStackFileBuffer(stackName, targetRelPath, req.file.buffer); - logFileOperation('info', 'upload complete', { nodeId: req.nodeId, size: req.file.size }); + logFileOperation('info', 'upload complete', { nodeId: req.nodeId, size: req.file.size, overwrite }); logFileDiag('upload timing', { stackName, relPath: targetRelPath, nodeId: req.nodeId, elapsedMs: Date.now() - startedAt }); return res.status(204).send(); } catch (err: unknown) { diff --git a/backend/src/services/FileSystemService.ts b/backend/src/services/FileSystemService.ts index d38d58e5..29b26732 100644 --- a/backend/src/services/FileSystemService.ts +++ b/backend/src/services/FileSystemService.ts @@ -711,6 +711,25 @@ export class FileSystemService { await fsPromises.writeFile(safePath, buffer); } + /** + * Returns 'file' or 'directory' if the resolved path exists, null if it + * does not. Path-resolution errors (INVALID_PATH, SYMLINK_ESCAPE) propagate + * so callers do not silently treat a malformed path as 'available for write'. + * Callers should validate inputs upstream before invoking this helper. + */ + async pathKind(stackName: string, relPath: string): Promise<'file' | 'directory' | null> { + const safePath = await this.resolveSafeStackPath(stackName, relPath); + try { + const stat = await fsPromises.lstat(safePath); + if (stat.isDirectory()) return 'directory'; + return 'file'; + } catch (err: unknown) { + const e = err as NodeJS.ErrnoException; + if (e.code === 'ENOENT') return null; + throw err; + } + } + async deleteStackPath(stackName: string, relPath: string, recursive: boolean = false): Promise { if (isProtectedRelPath(relPath)) throw protectedFileError(relPath); const safePath = await this.resolveSafeStackPath(stackName, relPath); diff --git a/frontend/src/components/files/FileUploadDropzone.tsx b/frontend/src/components/files/FileUploadDropzone.tsx index db67e0c3..c2d47e3b 100644 --- a/frontend/src/components/files/FileUploadDropzone.tsx +++ b/frontend/src/components/files/FileUploadDropzone.tsx @@ -1,7 +1,8 @@ -import { useRef } from 'react'; +import { useRef, useState } from 'react'; import { UploadCloud } from 'lucide-react'; +import { ConfirmModal } from '@/components/ui/modal'; import { toast } from '@/components/ui/toast-store'; -import { uploadStackFile } from '@/lib/stackFilesApi'; +import { uploadStackFile, UploadConflictError } from '@/lib/stackFilesApi'; const MAX_BYTES = 25 * 1024 * 1024; // 25 MB @@ -19,29 +20,38 @@ export function FileUploadDropzone({ onUploaded, }: FileUploadDropzoneProps) { const inputRef = useRef(null); + const [conflict, setConflict] = useState(null); if (!canEdit) return null; - const handleFile = async (file: File) => { - if (file.size > MAX_BYTES) { - toast.error('File exceeds 25 MB.'); - return; - } + const runUpload = async (file: File, overwrite: boolean): Promise => { const loadingId = toast.loading(`Uploading ${file.name}...`); try { - await uploadStackFile(stackName, currentDir, file); - toast.success('Uploaded.'); + await uploadStackFile(stackName, currentDir, file, { overwrite }); + toast.success(overwrite ? 'Replaced.' : 'Uploaded.'); onUploaded(); } catch (e: unknown) { + if (e instanceof UploadConflictError) { + setConflict(file); + return; + } toast.error(e instanceof Error ? e.message : 'Upload failed.'); } finally { toast.dismiss(loadingId); } }; + const handleFile = (file: File) => { + if (file.size > MAX_BYTES) { + toast.error('File exceeds 25 MB.'); + return; + } + void runUpload(file, false); + }; + const handleChange = (ev: React.ChangeEvent) => { const file = ev.target.files?.[0]; - if (file) void handleFile(file); + if (file) handleFile(file); ev.target.value = ''; }; @@ -69,6 +79,25 @@ export function FileUploadDropzone({ Upload file + + { if (!next) setConflict(null); }} + onCancel={() => setConflict(null)} + kicker="FILES · REPLACE EXISTING" + title="Replace existing file?" + description={conflict ? `${conflict.name} already exists in this folder.` : ''} + confirmLabel="Replace" + onConfirm={() => { + const file = conflict; + setConflict(null); + if (file) void runUpload(file, true); + }} + > +

+ {conflict?.name ?? 'The file'} already exists. Replacing it overwrites the current contents and cannot be undone. +

+ ); } diff --git a/frontend/src/components/files/__tests__/FileUploadDropzone.test.tsx b/frontend/src/components/files/__tests__/FileUploadDropzone.test.tsx index cd4df191..30499d3f 100644 --- a/frontend/src/components/files/__tests__/FileUploadDropzone.test.tsx +++ b/frontend/src/components/files/__tests__/FileUploadDropzone.test.tsx @@ -1,10 +1,22 @@ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { render, screen } from '@testing-library/react'; -import { FileUploadDropzone } from '../FileUploadDropzone'; +import userEvent from '@testing-library/user-event'; -vi.mock('@/lib/stackFilesApi', () => ({ - uploadStackFile: vi.fn(), -})); +// Mock the API module FIRST so the real class export is replaced before +// FileUploadDropzone imports it. +vi.mock('@/lib/stackFilesApi', () => { + class MockUploadConflictError extends Error { + readonly code = 'FILE_EXISTS' as const; + constructor(message: string) { + super(message); + this.name = 'UploadConflictError'; + } + } + return { + uploadStackFile: vi.fn(), + UploadConflictError: MockUploadConflictError, + }; +}); vi.mock('@/components/ui/toast-store', () => ({ toast: { @@ -15,6 +27,15 @@ vi.mock('@/components/ui/toast-store', () => ({ }, })); +import { FileUploadDropzone } from '../FileUploadDropzone'; +import { uploadStackFile, UploadConflictError } from '@/lib/stackFilesApi'; + +const mockUpload = uploadStackFile as unknown as ReturnType; + +beforeEach(() => { + mockUpload.mockReset(); +}); + describe('FileUploadDropzone', () => { it('renders upload control for users with stack edit permission', () => { render( @@ -41,4 +62,50 @@ describe('FileUploadDropzone', () => { expect(screen.queryByRole('button', { name: /upload file/i })).not.toBeInTheDocument(); }); + + it('opens the replace dialog on FILE_EXISTS and retries with overwrite on confirm', async () => { + const user = userEvent.setup(); + const onUploaded = vi.fn(); + mockUpload + .mockRejectedValueOnce(new UploadConflictError('foo.txt already exists.')) + .mockResolvedValueOnce(undefined); + + render( + , + ); + + const input = screen.getByLabelText(/upload file/i) as HTMLInputElement; + const file = new File(['payload'], 'foo.txt', { type: 'text/plain' }); + await user.upload(input, file); + + expect(await screen.findByText(/replace existing file/i)).toBeInTheDocument(); + expect(mockUpload).toHaveBeenCalledTimes(1); + expect(mockUpload).toHaveBeenNthCalledWith(1, 'app', '', file, { overwrite: false }); + + await user.click(screen.getByRole('button', { name: /^replace$/i })); + + expect(mockUpload).toHaveBeenCalledTimes(2); + expect(mockUpload).toHaveBeenNthCalledWith(2, 'app', '', file, { overwrite: true }); + expect(onUploaded).toHaveBeenCalledTimes(1); + }); + + it('leaves the original file untouched when the user cancels the replace dialog', async () => { + const user = userEvent.setup(); + const onUploaded = vi.fn(); + mockUpload.mockRejectedValueOnce(new UploadConflictError('foo.txt already exists.')); + + render( + , + ); + + const input = screen.getByLabelText(/upload file/i) as HTMLInputElement; + const file = new File(['payload'], 'foo.txt', { type: 'text/plain' }); + await user.upload(input, file); + + expect(await screen.findByText(/replace existing file/i)).toBeInTheDocument(); + await user.click(screen.getByRole('button', { name: /^cancel$/i })); + + expect(mockUpload).toHaveBeenCalledTimes(1); + expect(onUploaded).not.toHaveBeenCalled(); + }); }); diff --git a/frontend/src/lib/stackFilesApi.ts b/frontend/src/lib/stackFilesApi.ts index 79676379..487cc1db 100644 --- a/frontend/src/lib/stackFilesApi.ts +++ b/frontend/src/lib/stackFilesApi.ts @@ -84,11 +84,24 @@ export async function downloadStackFile( return apiFetch(stackFilesUrl(stackName, `/download?path=${encodeURIComponent(relPath)}`)); } +/** + * Thrown by uploadStackFile when the target filename already exists in the + * directory and the caller did not opt into overwrite. The FileUploadDropzone + * surfaces a confirm dialog on this signal and retries with overwrite=true. + */ +export class UploadConflictError extends Error { + readonly code = 'FILE_EXISTS' as const; + constructor(message: string) { + super(message); + this.name = 'UploadConflictError'; + } +} + export async function uploadStackFile( stackName: string, targetDir: string, file: File, - options?: { localOnly?: boolean } + options?: { localOnly?: boolean; overwrite?: boolean } ): Promise { assertSafeRelPath(targetDir, 'target directory'); const fd = new FormData(); @@ -100,11 +113,12 @@ export async function uploadStackFile( headers['x-node-id'] = activeNodeId; } + const overwriteSuffix = options?.overwrite ? '&overwrite=1' : ''; // Use fetch directly: apiFetch always sets Content-Type: application/json, // which breaks multipart boundary negotiation. The 401 side-effects are // replicated manually below. const res = await fetch( - `/api${stackFilesUrl(stackName, `/upload?path=${encodeURIComponent(targetDir)}`)}`, + `/api${stackFilesUrl(stackName, `/upload?path=${encodeURIComponent(targetDir)}${overwriteSuffix}`)}`, { method: 'POST', credentials: 'include', headers, body: fd } ); @@ -115,6 +129,17 @@ export async function uploadStackFile( throw new Error('Unauthorized'); } + if (res.status === 409) { + let body: { code?: string; error?: string } = {}; + try { body = await res.clone().json(); } catch { /* ignore */ } + if (body.code === 'FILE_EXISTS') { + throw new UploadConflictError(body.error ?? `${file.name} already exists.`); + } + // DIR_EXISTS and any other 409 fall through to the generic Error path so the + // dropzone surfaces the server message as a toast and does NOT offer a Replace + // confirmation (a directory cannot be replaced by a file upload). + } + if (!res.ok) { if (res.status === 404) { try {