fix(server): harden native-video temp files and resolve allowlist once
- Dominant language
- Rust
- Stars
- 467
- Forks
- 54
- Avg merge
- 4h 25m
- Merged PRs (30d)
- 310
Description
## Problem
Found during review of PR #1777, which fixed these for the video-frames fallback and left the native-video path (`data:video/` and `http(s)://` clips) as a follow-up.
1. `write_video_temp_file` writes the client's clip into `std::env::temp_dir()` (`/tmp` on Linux, or wherever `TMPDIR` points) with `tokio::fs::write`, so the file gets the default mode (0644 under a 022 umask) and other local users can read it while it exists.
2. The `TempFile` guard is built only after the write returns `Ok`. A client disconnect drops request preparation as a unit (`src/server/chat_request.rs:1050-1052`), the blocking write runs on, and nothing owns the path, so the file stays on disk. A write that fails partway also leaves its partial file.
3. The native-video path reads and canonicalizes `MLXCEL_VIDEO_DIR_ALLOWLIST` per request, while the fallback uses the list resolved at startup, so a directory created after startup is accepted by one path and refused by the other.
## Evidence
- `src/server/media.rs:1015-1034` `write_video_temp_file` (guard at `:1022`, after the write); callers `:900` and `:988`; the guard is held in `ResolvedVideo::temp_guard: Option` (`:109`).
- `src/server/media.rs:553-560`: per-request `spawn_blocking(video_dir_allowlist_from_env)`, whose comment says request preparation does not see `AppState`; called from `prepare_chat_request_with_cache` (`src/server/chat_request.rs:1058`).
- Startup: `resolve_video_request_inputs` (`src/server/startup.rs:1072`) fills `AppState.video_dir_allowlist` (`src/server/state.rs:475`), read by the fallback at `chat_request.rs:441`.
- To reuse: `PrivateTempDir` (`src/multimodal/video.rs:537-603`), a 0700 directory whose `write_file` creates 0600 `create_new` files, removed on drop.
## Proposed fix
- Write the clip through `PrivateTempDir` and have `ResolvedVideo` own that guard instead of `TempFile`; its calls are synchronous `std` I/O, so keep them on the blocking pool.
- Create the guard before the first byte is written.
- Pass the startup-resolved allowlist into request preparation and drop the per-request read.
## Acceptance criteria
- [ ] The temp file is 0600 inside a 0700 directory (Unix test).
- [ ] Dropping the resolver future mid-write, and a forced write error, leave nothing on disk (tests).
- [ ] No request path calls `video_dir_allowlist_from_env`.
- [ ] `docs/environment-variables.md:416` and `docs/llama-server-compat.md` state the restart requirement for both video paths.
- [ ] A `data:video/` request on a native-video checkpoint still answers end to end.
## Verification
```bash
cargo test --workspace --profile test-fast --features metal,accelerate
cargo clippy --workspace --all-targets -- -D warnings
cargo fmt --all -- --check
```
Each new test must fail with its fix reverted.
Contributor guide
Research direction
Start with write_video_temp_file and ResolvedVideo in src/server/media.rs, then trace request preparation in src/server/chat_request.rs and startup state in src/server/startup.rs and src/server/state.rs. Reuse the PrivateTempDir behavior in src/multimodal/video.rs and review the two documentation locations named in the acceptance criteria. Done means the listed security, cleanup, allowlist, documentation, and end-to-end criteria pass, along with the provided Cargo checks.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- backend, documentation, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 57/100