BuzzbeeSCD/openchamber
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(files): harden drag-and-drop uploads

Browse Source 
Co-authored-by: Serhii Dziupin <serkraser@gmail.com>

Co-authored-by: Alan Chen <2144783+alanzchen@users.noreply.github.com>
This commit is contained in:
Bohdan Triapitsyn
2026-08-18 23:16:46 +03:00
co-authored by Serhii Dziupin Alan Chen
parent 0c5e183c62
commit 99873a7b12
9 changed files with 297 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/web/server/lib/fs/DOCUMENTATION.md
+1 -1
View File
@@ -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.
packages/web/server/lib/fs/routes.js
+72 -27
View File
@@ -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');
}
packages/web/server/lib/fs/routes.test.js
+122 -40
View File
@@ -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-/));
});
});