PERF/SECURITY: make uploads streaming/bounded instead of allowing multi-GB blocking and in-memory expansion #41

New issue

Open

opened 2026-08-24 01:01:00 +00:00 by lost-rob0t · 0 comments

lost-rob0t commented

2026-08-24 01:01:00 +00:00

(Migrated from github.com)

Finding

The UI server defaults both MAX_CONTENT_LENGTH and MAX_FORM_MEMORY_SIZE to 5 GiB. The upload paths do not consistently impose a smaller effective file limit or keep large file work off the async request path.

Concrete paths:

api/upload.py accepts every extension (allowed_file() immediately returns True) and calls FileStorage.save() synchronously from an async handler.
api/upload_work_dir_files.py production mode calls synchronous FileBrowser.save_files() from an async handler.
development mode is worse: it performs file.stream.read() for the complete upload, base64-encodes the whole byte string (adding ~33% size overhead plus copies), then transports that string through the development-function bridge.
FileBrowser.MAX_FILE_SIZE = 100 MiB and _check_file_size() exist, but _is_allowed_file() simply returns True and never invokes the size check, so that apparent safety limit is not enforced by save_files().

A large authenticated upload can therefore monopolize memory/event-loop time, and development mode can expand a large request into multiple simultaneous full-size buffers.

Direction

Define one upload policy and enforce it before/while streaming:

explicit per-file and per-request limits (configurable where large artifacts are intentional)
stream/spool file bodies rather than read() + base64 whole-file expansion
move blocking disk I/O off the main async loop or use a bounded async/worker pipeline
keep development transport chunked/streamed instead of base64-ing entire files
reject oversize input before expensive copies where possible
distinguish ordinary attachments from intentional large artifact-transfer endpoints

Acceptance

FileBrowser.MAX_FILE_SIZE is either actually enforced or removed/replaced by the authoritative limit; there is no decorative dead limit.
Development upload path never reads/base64-encodes an entire large file in memory.
Production upload path does not perform multi-second blocking file copies on the main async loop.
Per-file/request limits are explicit, documented and tested at boundary + 1 byte.
Large-artifact use cases, if required, use a deliberate streaming endpoint rather than a 5 GiB generic form-memory allowance.
Multiple concurrent uploads have bounded memory/concurrency.
Tests cover aborted uploads, disk-full/write failure, multiple files and oversized multipart fields.
Benchmark RSS/event-loop lag for 10 MiB, 100 MiB and configured maximum uploads.

## Finding The UI server defaults both `MAX_CONTENT_LENGTH` and `MAX_FORM_MEMORY_SIZE` to **5 GiB**. The upload paths do not consistently impose a smaller effective file limit or keep large file work off the async request path. Concrete paths: - `api/upload.py` accepts every extension (`allowed_file()` immediately returns `True`) and calls `FileStorage.save()` synchronously from an async handler. - `api/upload_work_dir_files.py` production mode calls synchronous `FileBrowser.save_files()` from an async handler. - development mode is worse: it performs `file.stream.read()` for the complete upload, base64-encodes the whole byte string (adding ~33% size overhead plus copies), then transports that string through the development-function bridge. - `FileBrowser.MAX_FILE_SIZE = 100 MiB` and `_check_file_size()` exist, but `_is_allowed_file()` simply returns `True` and never invokes the size check, so that apparent safety limit is not enforced by `save_files()`. A large authenticated upload can therefore monopolize memory/event-loop time, and development mode can expand a large request into multiple simultaneous full-size buffers. ## Direction Define one upload policy and enforce it before/while streaming: - explicit per-file and per-request limits (configurable where large artifacts are intentional) - stream/spool file bodies rather than `read()` + base64 whole-file expansion - move blocking disk I/O off the main async loop or use a bounded async/worker pipeline - keep development transport chunked/streamed instead of base64-ing entire files - reject oversize input before expensive copies where possible - distinguish ordinary attachments from intentional large artifact-transfer endpoints ## Acceptance - [ ] `FileBrowser.MAX_FILE_SIZE` is either actually enforced or removed/replaced by the authoritative limit; there is no decorative dead limit. - [ ] Development upload path never reads/base64-encodes an entire large file in memory. - [ ] Production upload path does not perform multi-second blocking file copies on the main async loop. - [ ] Per-file/request limits are explicit, documented and tested at boundary + 1 byte. - [ ] Large-artifact use cases, if required, use a deliberate streaming endpoint rather than a 5 GiB generic form-memory allowance. - [ ] Multiple concurrent uploads have bounded memory/concurrency. - [ ] Tests cover aborted uploads, disk-full/write failure, multiple files and oversized multipart fields. - [ ] Benchmark RSS/event-loop lag for 10 MiB, 100 MiB and configured maximum uploads.