diff --git a/TECHNICAL_REPORTS/pr-2282-native-video-temp-hardening.en.md b/TECHNICAL_REPORTS/pr-2282-native-video-temp-hardening.en.md new file mode 100644 index 000000000..e491917d0 --- /dev/null +++ b/TECHNICAL_REPORTS/pr-2282-native-video-temp-hardening.en.md @@ -0,0 +1,52 @@ +# Technical Report: PR #2282 - Native-Video Temporary File Hardening + +**Date**: 2026-10-13 +**Status**: Ready for review +**Languages**: Rust, Markdown +**Risk Level**: Medium + +## Executive Summary + +Native-video data URI and HTTP payloads are now stored as create-new 0600 files inside per-request 0700 directories. The directory guard is created inside the blocking closure before any byte is written, so request cancellation and partial-write failures remove the complete private directory after the writer finishes. + +Native-video request preparation now receives the same immutable `MLXCEL_VIDEO_DIR_ALLOWLIST` snapshot as the frame fallback. Single-server, router-loaded model, and in-process startup paths all install this snapshot, and operator documentation states that environment or directory changes require a restart. + +## 1. Problem Statement + +The native-video resolver previously wrote directly under the system temporary directory with default permissions and created its cleanup guard only after a successful asynchronous write. A dropped request could therefore leave a readable clip behind, while a failed write could leave partial content. It also re-read and canonicalized the directory allowlist per request, producing behavior different from the frame fallback. + +## 2. Technical Review + +The implementation reuses `PrivateTempDir`, which creates its directory with mode 0700 and files with mode 0600 and `create_new`. The blocking closure owns the guard before invoking the writer; if its awaiting future is aborted, the closure completes and its unclaimed guarded result is dropped. Owned payload buffers move into the closure without an additional payload-sized copy. + +The startup allowlist is passed explicitly into request preparation. A review found that the public in-process server installed media capability flags without the allowlist; a shared startup helper now installs both together, and a constructor regression test covers that integration. + +## 3. Technical Decisions + +### Keep synchronous file creation on the blocking pool + +`PrivateTempDir` uses synchronous filesystem operations to guarantee exclusive creation and fixed Unix modes. Running the entire guard-create/write/result sequence in `spawn_blocking` avoids blocking Tokio workers while preserving one ownership boundary for cleanup. + +### Treat the allowlist as startup configuration + +All native and frame-fallback requests use the canonical directory list captured during model startup. A directory added later remains unavailable until restart, so request behavior no longer depends on when a path happens to appear. + +## 7. Change Summary + +| Category | Count | Summary | +|---|---:|---| +| Security | 2 | Private temporary storage and cancellation-safe cleanup | +| Integration | 1 | Startup allowlist propagated through every production server constructor | +| Tests | 18 | Temp lifecycle, startup snapshot, in-process constructor, and existing resolver coverage | +| Documentation | 2 | Restart requirements for native and fallback video paths | + +## Appendix: Validation Evidence + +- Implementation source commit: `790d981647901fdb690e223749a0162ed8f632f6`. +- Candidate `mlxcel-server` SHA-256: `fc7c3ebc3f1948fcd2dd9d535428123535a9cc12fd88b2139f576f87f635f9b4`. +- Focused CUDA-feature library tests passed: 3 temp lifecycle tests, 1 request allowlist snapshot test, 13 existing resolver tests, and 1 in-process startup test. +- A Qwen2.5-VL 3B 4-bit native data-video request answered `The main color in the video is red.` with 83 prompt tokens and 9 completion tokens. The private temp root was empty before and after the request, matching the baseline answer and counts. +- Contract gates passed with 123 Python checks and 49 WebUI checks. Formatting and diff checks passed. +- Independent implementation, security, and finalization reviews reported no remaining findings after the in-process constructor fix. + +The end-to-end model run used Linux CUDA. macOS Metal inference was not run locally. diff --git a/TECHNICAL_REPORTS/pr-2282-native-video-temp-hardening.ko.md b/TECHNICAL_REPORTS/pr-2282-native-video-temp-hardening.ko.md new file mode 100644 index 000000000..4ffd4b918 --- /dev/null +++ b/TECHNICAL_REPORTS/pr-2282-native-video-temp-hardening.ko.md @@ -0,0 +1,52 @@ +# 기술 보고서: PR #2282 - Native-Video 임시 파일 보안 강화 + +**작성일**: 2026-10-13 +**상태**: 리뷰 준비 완료 +**언어**: Rust, Markdown +**위험도**: 중간 + +## 요약 + +Native-video data URI와 HTTP payload를 요청별 0700 디렉터리 안의 create-new 0600 파일에 저장합니다. Blocking closure가 어떤 바이트도 쓰기 전에 디렉터리 guard를 생성하므로, 요청 취소와 부분 쓰기 실패가 발생해도 writer가 끝난 뒤 private 디렉터리 전체가 제거됩니다. + +Native-video 요청 준비는 이제 frame fallback과 동일한 불변 `MLXCEL_VIDEO_DIR_ALLOWLIST` snapshot을 받습니다. 단일 서버, router가 로드한 모델, in-process startup 경로가 모두 이 snapshot을 설치하며, 환경 변수나 디렉터리를 변경한 뒤에는 재시작해야 한다고 운영 문서에 명시했습니다. + +## 1. 문제 정의 + +기존 native-video resolver는 system temporary directory 바로 아래에 기본 권한으로 파일을 쓰고, 비동기 쓰기가 성공한 뒤에야 cleanup guard를 만들었습니다. 따라서 요청이 중단되면 읽을 수 있는 clip이 남을 수 있었고, 쓰기가 실패하면 일부 내용이 남을 수 있었습니다. 또한 요청마다 directory allowlist를 다시 읽고 canonicalize하여 frame fallback과 다른 동작을 보였습니다. + +## 2. 기술적 검토 사항 + +구현은 디렉터리를 0700으로, 파일을 0600과 `create_new`로 만드는 `PrivateTempDir`를 재사용합니다. Blocking closure는 writer를 호출하기 전에 guard를 소유합니다. 대기 중인 future가 중단되면 closure가 완료된 뒤 수신되지 않은 guarded result가 drop됩니다. Payload buffer는 payload 크기만큼 다시 복사하지 않고 closure로 이동합니다. + +Startup allowlist를 request preparation에 명시적으로 전달합니다. 리뷰에서 public in-process server가 media capability만 설치하고 allowlist는 설치하지 않는 통합 누락을 발견했으며, 이제 공통 startup helper가 둘을 함께 설치하고 constructor 회귀 테스트가 이 경로를 검증합니다. + +## 3. 기술적 선택과 그 이유 + +### 동기 파일 생성을 blocking pool에서 수행 + +`PrivateTempDir`는 배타적 생성과 고정된 Unix mode를 보장하기 위해 동기 파일 시스템 연산을 사용합니다. Guard 생성, 쓰기, 결과 반환 전체를 `spawn_blocking`에서 실행하여 Tokio worker를 막지 않으면서 cleanup을 위한 하나의 ownership 경계를 유지합니다. + +### Allowlist를 startup 설정으로 취급 + +Native 경로와 frame fallback의 모든 요청은 모델 startup 중 canonicalize한 directory 목록을 사용합니다. 이후 추가된 디렉터리는 재시작 전까지 허용되지 않으므로, 요청 동작이 경로 생성 시점에 따라 달라지지 않습니다. + +## 7. 변경 요약 + +| 분류 | 수 | 요약 | +|---|---:|---| +| 보안 | 2 | Private 임시 저장소와 취소 안전 cleanup | +| 통합 | 1 | 모든 production server constructor에 startup allowlist 전달 | +| 테스트 | 18 | Temp lifecycle, startup snapshot, in-process constructor, 기존 resolver 검증 | +| 문서 | 2 | Native 및 fallback video 경로의 재시작 요구사항 | + +## 부록: 검증 증거 + +- 구현 source commit: `790d981647901fdb690e223749a0162ed8f632f6`. +- Candidate `mlxcel-server` SHA-256: `fc7c3ebc3f1948fcd2dd9d535428123535a9cc12fd88b2139f576f87f635f9b4`. +- CUDA feature를 사용한 집중 library test가 통과했습니다. Temp lifecycle 3개, request allowlist snapshot 1개, 기존 resolver 13개, in-process startup 1개입니다. +- Qwen2.5-VL 3B 4-bit native data-video 요청이 `The main color in the video is red.`라고 답했습니다. Prompt token은 83개, completion token은 9개였고 private temp root는 요청 전후 모두 비어 있었습니다. Baseline의 답변과 token 수가 같습니다. +- Contract gate는 Python 123개와 WebUI 49개 검증을 통과했습니다. Formatting 및 diff 검사도 통과했습니다. +- 독립 구현, 보안, finalization 리뷰는 in-process constructor 수정을 반영한 뒤 남은 문제를 보고하지 않았습니다. + +End-to-end 모델 실행은 Linux CUDA에서 수행했습니다. macOS Metal inference는 로컬에서 실행하지 않았습니다. diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 24321e3bc..30f2d9ad7 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -687,7 +687,7 @@ both be on `PATH` and must be **ffmpeg 5.0 (2022) or newer**. See | Variable | Values | Default | Notes | |----------|--------|---------|-------| -| `MLXCEL_VIDEO_DIR_ALLOWLIST` | comma-separated directories | unset | Local `video_url` file paths are rejected unless they resolve under one of these canonicalized directories. Keep directories owner-writable only; group/world-writable entries warn at startup. The video-frames fallback resolves the list once when the server starts (once per model the router loads), so an entry that does not exist yet at that point is dropped with a warning until the next restart; the native-video families still read it per request. | +| `MLXCEL_VIDEO_DIR_ALLOWLIST` | comma-separated directories | unset | Local `video_url` file paths are rejected unless they resolve under one of these canonicalized directories. Keep directories owner-writable only; group/world-writable entries warn at startup. Both native-video families and the video-frames fallback resolve the list once when the server starts (once per model the router loads), so an entry that does not exist yet at that point is dropped with a warning until the next restart. Restart the server after changing the variable or creating a listed directory. | | `MLXCEL_VIDEO_MAX_PIXELS` | unsigned integer | `16777216` | Rejects source videos whose `width × height` exceeds the cap. | | `MLXCEL_VIDEO_MAX_DURATION_SEC` | float seconds | `600` | Rejects source videos longer than the cap. | | `MLXCEL_VIDEO_MAX_PNG_FRAME_BYTES` | unsigned integer bytes | `268435456` | Per-frame cap for the ffmpeg PNG stream splitter. | diff --git a/docs/llama-server-compat.md b/docs/llama-server-compat.md index 63dd294ca..cba5f8c9a 100644 --- a/docs/llama-server-compat.md +++ b/docs/llama-server-compat.md @@ -478,7 +478,7 @@ model gemma-3-4b-it-4bit has no native video path; sending 8 of 8 sampled frames That `info` line is the only signal that the substitution happened; `GET /props` reports `video: true` either way, because either way the block is accepted. The families with a native path (Gemma 4 VL, Gemma 4 Unified, Inkling, Kimi-VL / Kimi-VL 2.5, and the Qwen-VL generations) are untouched by this and never log it. Muse Glimmer is excluded on purpose: the CLI refuses `--video` for that family by name, and admitting the clip on the HTTP boundary alone would leave the two fronts disagreeing about one checkpoint. A checkpoint with no vision tower keeps its 501. -Two limits are worth knowing. The frames spend the ordinary per-request image budget, so `--video-max-frames` above `--max-images` (default 16 for both) is refused with a message naming the frame count, the caller's own image count and the limit rather than with a bare image-count error. And the substitution carries no temporal information: the model receives ordered pictures and the sentence saying they are one clip, not frame timestamps or motion, so a reply can describe what changes across the frames but cannot be asked about speed or duration. A local `file://` path still has to sit inside a directory named by `MLXCEL_VIDEO_DIR_ALLOWLIST`, which is fail-closed and resolved once at startup. A client that disconnects stops the substitution before the next clip is resolved and before a queued decode starts; a clip already decoding runs to completion. +Two limits are worth knowing. The frames spend the ordinary per-request image budget, so `--video-max-frames` above `--max-images` (default 16 for both) is refused with a message naming the frame count, the caller's own image count and the limit rather than with a bare image-count error. And the substitution carries no temporal information: the model receives ordered pictures and the sentence saying they are one clip, not frame timestamps or motion, so a reply can describe what changes across the frames but cannot be asked about speed or duration. A local `file://` path still has to sit inside a directory named by `MLXCEL_VIDEO_DIR_ALLOWLIST`, which is fail-closed and resolved once at startup for both native-video families and this fallback. Restart the server after changing the variable or creating a listed directory; request handling does not re-read it. A client that disconnects stops the substitution before the next clip is resolved and before a queued decode starts; a clip already decoding runs to completion. ### `--image-min-tokens` / `--image-max-tokens` move real pixel bounds diff --git a/src/multimodal/video.rs b/src/multimodal/video.rs index 1348b6d78..6a1822f8c 100644 --- a/src/multimodal/video.rs +++ b/src/multimodal/video.rs @@ -532,7 +532,8 @@ impl Drop for TempFile { /// Built on `std` rather than the `tempfile` crate, which is a dev-dependency /// only. /// -/// Used by: `commands::generate::CliVideoFrames` (CLI video-frames fallback). +/// Used by: `commands::generate::CliVideoFrames` (CLI video-frames fallback) +/// and `server::media` (native-video data URI and HTTP materialization). #[derive(Debug)] pub struct PrivateTempDir { path: PathBuf, diff --git a/src/server/chat_request.rs b/src/server/chat_request.rs index f8f039c93..ba963d8e9 100644 --- a/src/server/chat_request.rs +++ b/src/server/chat_request.rs @@ -89,8 +89,8 @@ use super::chat_template_kwargs::{ strip_think_block, }; use super::media::{ - MediaRequestMetadata, ResolvedVideo, extract_chat_video_paths, try_extract_chat_audio_data, - try_extract_chat_image_data, + MediaRequestMetadata, ResolvedVideo, extract_chat_video_paths_with_allowlist, + try_extract_chat_audio_data, try_extract_chat_image_data, }; use super::prompt_cache::key::resolve_session_key; use super::types::request::{ @@ -768,7 +768,7 @@ pub(crate) async fn prepare_chat_request( request: &ChatCompletionRequest, server_default_kwargs: Option<&ChatTemplateKwargs>, ) -> Result { - prepare_chat_request_with_cache( + prepare_chat_request_with_cache_and_video_allowlist( processor, request, server_default_kwargs, @@ -776,6 +776,34 @@ pub(crate) async fn prepare_chat_request( false, false, &crate::tokenizer::ThinkingMarkers::default(), + &[], + ) + .await +} + +/// Test-only compatibility entry point for focused renderer tests that do not +/// construct [`super::state::AppState`]. Production routes must call +/// [`prepare_chat_request_with_cache_and_video_allowlist`] with the immutable +/// startup snapshot. +#[cfg(test)] +pub(crate) async fn prepare_chat_request_with_cache( + processor: &ChatTemplateProcessor, + request: &ChatCompletionRequest, + server_default_kwargs: Option<&ChatTemplateKwargs>, + prompt_cache_enabled: bool, + snapshot_reuse_capable: bool, + prefill_assistant: bool, + thinking_markers: &crate::tokenizer::ThinkingMarkers, +) -> Result { + prepare_chat_request_with_cache_and_video_allowlist( + processor, + request, + server_default_kwargs, + prompt_cache_enabled, + snapshot_reuse_capable, + prefill_assistant, + thinking_markers, + &[], ) .await } @@ -792,7 +820,7 @@ pub(crate) async fn prepare_chat_request( /// /// When `prompt_cache_enabled` is `false` this is identical to the legacy /// [`prepare_chat_request`]. -pub(crate) async fn prepare_chat_request_with_cache( +pub(crate) async fn prepare_chat_request_with_cache_and_video_allowlist( processor: &ChatTemplateProcessor, request: &ChatCompletionRequest, server_default_kwargs: Option<&ChatTemplateKwargs>, @@ -800,6 +828,7 @@ pub(crate) async fn prepare_chat_request_with_cache( snapshot_reuse_capable: bool, prefill_assistant: bool, thinking_markers: &crate::tokenizer::ThinkingMarkers, + video_dir_allowlist: &[PathBuf], ) -> Result { // Refuse rather than fall back to the generic template when the loaded // tokenizer's vocabulary family is Kimi K3 but no native renderer could be @@ -1070,7 +1099,7 @@ pub(crate) async fn prepare_chat_request_with_cache( let (image_data, audio_data, videos) = tokio::join!( try_extract_chat_image_data(request), try_extract_chat_audio_data(request), - extract_chat_video_paths(request), + extract_chat_video_paths_with_allowlist(request, video_dir_allowlist), ); let image_data = image_data?; let audio_data = audio_data?; @@ -2175,3 +2204,7 @@ mod tests; #[cfg(test)] #[path = "chat_request_reasoning_content_tests.rs"] mod reasoning_content_tests; + +#[cfg(test)] +#[path = "chat_request_video_allowlist_tests.rs"] +mod video_allowlist_tests; diff --git a/src/server/chat_request_video_allowlist_tests.rs b/src/server/chat_request_video_allowlist_tests.rs new file mode 100644 index 000000000..305d9b1dc --- /dev/null +++ b/src/server/chat_request_video_allowlist_tests.rs @@ -0,0 +1,112 @@ +// Copyright 2025-2026 Lablup Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::fs; + +use super::prepare_chat_request_with_cache_and_video_allowlist; +use crate::server::chat_template::ChatTemplateProcessor; +use crate::server::types::request::VideoUrl; +use crate::server::types::{ + ChatCompletionRequest, ContentPart, Message, MessageContent, Role, SamplingParams, +}; + +fn video_request(path: &std::path::Path) -> ChatCompletionRequest { + ChatCompletionRequest { + model: "test-model".to_string(), + messages: vec![Message { + role: Role::User, + content: MessageContent::Parts(vec![ContentPart::VideoUrl { + video_url: VideoUrl { + url: path.to_string_lossy().into_owned(), + fps: None, + }, + }]), + name: None, + tool_call_id: None, + reasoning: None, + tool_calls: None, + }], + stream: false, + stream_options: None, + logprobs: None, + top_logprobs: None, + tools: None, + tool_choice: None, + parallel_tool_calls: None, + chat_template_kwargs: None, + extra_body: None, + prompt_cache_key: None, + cache_prompt: None, + user: None, + reasoning_effort: None, + extra_body_fields: serde_json::Map::new(), + response_format: None, + params: SamplingParams::default(), + } +} + +#[tokio::test] +async fn native_video_preparation_uses_startup_allowlist_snapshot() { + let startup_dir = + std::env::temp_dir().join(format!("mlxcel-video-startup-{}", uuid::Uuid::new_v4())); + fs::create_dir(&startup_dir).unwrap(); + let startup_dir = fs::canonicalize(startup_dir).unwrap(); + let startup_video = startup_dir.join("startup.mp4"); + fs::write(&startup_video, b"startup video").unwrap(); + let startup_allowlist = vec![startup_dir.clone()]; + + let later_dir = + std::env::temp_dir().join(format!("mlxcel-video-later-{}", uuid::Uuid::new_v4())); + fs::create_dir(&later_dir).unwrap(); + let later_video = later_dir.join("later.mp4"); + fs::write(&later_video, b"later video").unwrap(); + + let processor = ChatTemplateProcessor::with_template("ok".to_string()); + let markers = crate::tokenizer::ThinkingMarkers::default(); + let prepared_at_startup = prepare_chat_request_with_cache_and_video_allowlist( + &processor, + &video_request(&startup_video), + None, + false, + false, + false, + &markers, + &startup_allowlist, + ) + .await + .unwrap(); + assert_eq!(prepared_at_startup.videos.len(), 1); + drop(prepared_at_startup); + + let prepared_after_new_directory = prepare_chat_request_with_cache_and_video_allowlist( + &processor, + &video_request(&later_video), + None, + false, + false, + false, + &markers, + &startup_allowlist, + ) + .await + .unwrap(); + assert!( + prepared_after_new_directory.videos.is_empty(), + "a directory created after startup is not accepted until restart" + ); + drop(prepared_after_new_directory); + + fs::remove_dir_all(startup_dir).unwrap(); + fs::remove_dir_all(later_dir).unwrap(); +} diff --git a/src/server/in_process/mod.rs b/src/server/in_process/mod.rs index d4d1aae92..df06af858 100644 --- a/src/server/in_process/mod.rs +++ b/src/server/in_process/mod.rs @@ -48,7 +48,7 @@ use anyhow::{Context, Result}; use crate::server::batch::BatchObservability; use crate::server::startup::{ build_server_config, detect_model_media_support, load_chat_front, resolve_prompt_cache_store, - run_startup_warmup, + resolve_video_request_inputs, run_startup_warmup, }; use crate::server::{ ApiKeys, AppState, BatchMetrics, ModelMediaSupport, ModelProvider, ServerConfig, @@ -107,16 +107,19 @@ impl InProcessServer { // warmup is logged, not fatal. run_startup_warmup(&model_path, &provider); } - let state = AppState::with_observability( - Arc::new(provider), - config, - chat_template, - tokenizer, - model_path.clone(), - batch_metrics, - observability.clone(), + let media_support = detect_model_media_support(&model_path); + let state = install_media_startup_inputs( + AppState::with_observability( + Arc::new(provider), + config, + chat_template, + tokenizer, + model_path.clone(), + batch_metrics, + observability.clone(), + ), + media_support, ) - .with_media_support(detect_model_media_support(&model_path)) .with_prompt_cache(store); Ok(Self { state, @@ -211,9 +214,25 @@ impl InProcessServer { } } +/// Install the same immutable media inputs as the HTTP startup paths. +/// +/// Keeping this in one helper makes it harder for the in-process front end to +/// install detected video support without also installing the startup-resolved +/// local-video allowlist used by request preparation. +fn install_media_startup_inputs(state: AppState, media_support: ModelMediaSupport) -> AppState { + let video_dir_allowlist = resolve_video_request_inputs(media_support); + state + .with_media_support(media_support) + .with_video_dir_allowlist(video_dir_allowlist) +} + impl Drop for InProcessServer { fn drop(&mut self) { // Idempotent: a worker that `shutdown` already stopped ignores it. self.state.model_provider.shutdown_worker(); } } + +#[cfg(test)] +#[path = "startup_state_tests.rs"] +mod tests; diff --git a/src/server/in_process/startup_state_tests.rs b/src/server/in_process/startup_state_tests.rs new file mode 100644 index 000000000..0fbf192b0 --- /dev/null +++ b/src/server/in_process/startup_state_tests.rs @@ -0,0 +1,80 @@ +// Copyright 2025-2026 Lablup Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::ffi::OsString; +use std::sync::{Arc, mpsc}; + +use super::install_media_startup_inputs; +use crate::server::media::VIDEO_DIR_ALLOWLIST_ENV; +use crate::server::{ + AppState, ChatTemplateProcessor, ModelMediaSupport, ModelProvider, ServerConfig, +}; +use crate::tokenizer::MlxcelTokenizer; + +struct EnvGuard { + previous: Option, +} + +impl EnvGuard { + fn set(value: &std::path::Path) -> Self { + let previous = std::env::var_os(VIDEO_DIR_ALLOWLIST_ENV); + // SAFETY: the test holds the crate-wide environment lock until this + // guard restores the previous value. + unsafe { std::env::set_var(VIDEO_DIR_ALLOWLIST_ENV, value) }; + Self { previous } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + // SAFETY: the crate-wide environment lock still outlives this guard. + unsafe { + match &self.previous { + Some(value) => std::env::set_var(VIDEO_DIR_ALLOWLIST_ENV, value), + None => std::env::remove_var(VIDEO_DIR_ALLOWLIST_ENV), + } + } + } +} + +fn test_state() -> AppState { + let (options_tx, _options_rx) = mpsc::channel(); + let provider = Arc::new(ModelProvider::recording_for_route_tests(options_tx)); + let batch_metrics = provider.batch_metrics().clone(); + AppState::new( + provider, + ServerConfig::default(), + ChatTemplateProcessor::with_template("ok".to_string()), + MlxcelTokenizer::stub(), + "in-process-startup-state-test-model".into(), + batch_metrics, + ) +} + +#[test] +fn installs_video_allowlist_with_media_support() { + let _env_lock = crate::test_support::env_lock::env_lock(); + let allowlist_dir = tempfile::tempdir().unwrap(); + let canonical = std::fs::canonicalize(allowlist_dir.path()).unwrap(); + let _env = EnvGuard::set(&canonical); + let support = ModelMediaSupport { + video_native: true, + ..ModelMediaSupport::default() + }; + + let state = install_media_startup_inputs(test_state(), support); + + assert_eq!(state.media_support, support); + assert_eq!(state.video_dir_allowlist.as_slice(), &[canonical]); +} diff --git a/src/server/media.rs b/src/server/media.rs index 10883058b..e46b95d73 100644 --- a/src/server/media.rs +++ b/src/server/media.rs @@ -22,12 +22,13 @@ use anyhow::{Context, Result, anyhow, bail}; use base64::Engine; use futures::StreamExt; use std::{ + io, path::{Path, PathBuf}, sync::atomic::{AtomicU32, AtomicU64, AtomicUsize, Ordering}, time::Duration, }; -use crate::multimodal::video::{TempFile, VideoSource, is_video_file}; +use crate::multimodal::video::{PrivateTempDir, VideoSource, is_video_file}; use super::media_net; use super::media_root; @@ -72,7 +73,7 @@ struct MediaAcquisitionCancelled { /// and ffmpeg's `open`; /// * the optional per-video FPS override from /// [`crate::server::types::request::VideoUrl::fps`]; -/// * an optional [`TempFile`] guard that owns the cleanup of any +/// * an optional [`PrivateTempDir`] guard that owns the cleanup of any /// server-allocated temporary file (data-URI decode, HTTP fetch). The /// guard is held until the response handler drops `ResolvedVideo`. /// @@ -98,15 +99,16 @@ pub(crate) struct ResolvedVideo { /// Per-video FPS override from /// [`crate::server::types::request::VideoUrl::fps`]. pub(crate) fps: Option, - /// `Some(TempFile)` when the resolver allocated a server-owned - /// temp file (data URI / HTTP fetch). The guard is dropped — and the - /// file unlinked — when this struct drops. + /// `Some(PrivateTempDir)` when the resolver allocated a server-owned + /// private directory and temp file (data URI / HTTP fetch). The guard is + /// dropped — and the directory removed recursively — when this struct + /// drops. /// /// `dead_code` suppression is intentional: the guard is read only for /// its `Drop` impl. Any other access path would defeat the point of /// the guard. #[allow(dead_code)] - pub(crate) temp_guard: Option, + pub(crate) temp_guard: Option, } impl ResolvedVideo { @@ -538,32 +540,11 @@ pub(crate) const VIDEO_DIR_ALLOWLIST_ENV: &str = "MLXCEL_VIDEO_DIR_ALLOWLIST"; /// Stash the returned vector in a struct that lives for the full duration /// of the request handler — when that struct drops, every fd is closed /// and every temp file is removed automatically. -pub(crate) async fn extract_chat_video_paths( - request: &ChatCompletionRequest, -) -> Vec { - // `prepare_chat_request_with_cache` calls this for every request, so a - // body with no clip, which includes every body the video-frames fallback - // has already rewritten into images, returns before reading the - // environment or touching the filesystem (issue #1766). `has_video_urls` - // rather than `video_urls().is_empty()`, which would clone every inline - // `data:video/` payload just to count them. - if !request.has_video_urls() { - return Vec::new(); - } - // The native-video path still reads the allowlist per request, because - // request preparation does not see `AppState`; the canonicalization at - // least runs on the blocking pool rather than on this Tokio worker. A - // failed task resolves to the empty, fail-closed list. - let allowlist = tokio::task::spawn_blocking(video_dir_allowlist_from_env) - .await - .unwrap_or_default(); - extract_chat_video_paths_with_allowlist(request, &allowlist).await -} - -/// Test/internal-friendly variant of [`extract_chat_video_paths`] that -/// accepts the allowlist directly. Production code reads the env var via -/// the public wrapper above; tests inject a controlled allowlist so they -/// don't depend on process-wide state. +/// Resolve video inputs against the allowlist captured at server startup. +/// +/// The caller passes [`crate::server::state::AppState::video_dir_allowlist`] +/// so native-video requests and the video-frames fallback share one immutable +/// view. Request handling never re-reads or re-canonicalizes the environment. /// /// Returns a vector of [`ResolvedVideo`] records — one per `video_url` /// content block that resolved successfully. Failed resolutions are @@ -574,6 +555,12 @@ pub(crate) async fn extract_chat_video_paths_with_allowlist( request: &ChatCompletionRequest, allowlist: &[PathBuf], ) -> Vec { + // This runs for every prepared request. Use `has_video_urls` rather than + // `video_urls().is_empty()`, which would clone every inline data URI just + // to discover that there is no work. + if !request.has_video_urls() { + return Vec::new(); + } let mut resolved = Vec::new(); for video in request.video_urls() { if let Some(item) = resolve_video_url(&video, allowlist).await { @@ -594,9 +581,8 @@ pub(crate) async fn extract_chat_video_paths_with_allowlist( /// Synchronous, and it calls the blocking `std::fs::canonicalize`, so keep it /// off Tokio workers. Startup resolves it once per model load /// ([`crate::server::startup::resolve_video_request_inputs`]) into -/// `AppState`, which is what the video-frames fallback reads (issue #1766); -/// the native-video path in [`extract_chat_video_paths`] still calls it per -/// request that carries a clip, through `spawn_blocking`. +/// `AppState`, which both native-video resolution and the video-frames +/// fallback read (issues #1766 and #1787). /// /// Deliberately not memoized: tests set the variable and read it back, and /// the `_with_allowlist` entry points exist so the rest of the tests can @@ -873,7 +859,7 @@ async fn resolve_local_video_path(raw: &str, allowlist: &[PathBuf]) -> Option Option<(PathBuf, TempFile)> { +async fn decode_video_data_uri(url: &str) -> Option<(PathBuf, PrivateTempDir)> { let Some((metadata, encoded_data)) = url.split_once(',') else { tracing::warn!("Invalid video data URI format"); return None; @@ -897,7 +883,7 @@ async fn decode_video_data_uri(url: &str) -> Option<(PathBuf, TempFile)> { ); return None; } - write_video_temp_file(&bytes, infer_video_extension(metadata)).await + write_video_temp_file(bytes, infer_video_extension(metadata)).await } /// Fetch a remote video URL with size enforcement applied **incrementally** @@ -916,7 +902,7 @@ async fn decode_video_data_uri(url: &str) -> Option<(PathBuf, TempFile)> { /// /// Connect timeout, total request timeout, and redirect cap are configured /// on the shared client (see [`http_image_client`]). -async fn fetch_remote_video(url: &str) -> Option<(PathBuf, TempFile)> { +async fn fetch_remote_video(url: &str) -> Option<(PathBuf, PrivateTempDir)> { let client = match http_image_client().await { Ok(client) => client, Err(err) => { @@ -985,7 +971,7 @@ async fn fetch_remote_video(url: &str) -> Option<(PathBuf, TempFile)> { .rsplit_once('.') .map(|(_, ext)| ext.split('?').next().unwrap_or(ext).to_string()) .unwrap_or_else(|| "mp4".to_string()); - write_video_temp_file(&accumulated, sanitize_video_extension(&ext)).await + write_video_temp_file(accumulated, sanitize_video_extension(&ext)).await } fn infer_video_extension(metadata: &str) -> &str { @@ -1002,37 +988,61 @@ fn infer_video_extension(metadata: &str) -> &str { } } -/// Write `bytes` to a fresh temp file and wrap the path in a [`TempFile`] -/// drop guard. The guard's `Drop` impl removes the file when the caller -/// drops it, so callers MUST keep the guard alive for as long as the file -/// must remain on disk (e.g., until ffmpeg has finished probing it). +/// Materialize `bytes` in a private 0700 directory as a create-new 0600 file. /// -/// Returning the guard alongside the path is the fix for the temp-file leak -/// reported during review: every previous version returned a bare -/// `PathBuf` and relied on no-one to call cleanup explicitly. With the guard -/// in place a panic, an early-return error path, or a normal completion all -/// converge on the same cleanup behaviour. -async fn write_video_temp_file(bytes: &[u8], ext: &str) -> Option<(PathBuf, TempFile)> { - let dir = std::env::temp_dir(); - let unique = uuid::Uuid::new_v4(); - let ext = sanitize_video_extension(ext); - let path = dir.join(format!("mlxcel-video-{unique}.{ext}")); - match tokio::fs::write(&path, bytes).await { - Ok(()) => { - let guard = TempFile::new(path.clone()); - Some((path, guard)) +/// The blocking closure owns the guard before it writes the first byte. Tokio +/// cannot cancel an already-running `spawn_blocking` task, so this ownership +/// order is what makes handler cancellation safe: if the awaiting request is +/// dropped, the closure finishes and its unclaimed return value immediately +/// drops the guard. Write errors drop it inside the closure as well. +async fn write_video_temp_file(bytes: Vec, ext: &str) -> Option<(PathBuf, PrivateTempDir)> { + write_video_temp_file_with_writer(bytes, sanitize_video_extension(ext), |dir, name, bytes| { + dir.write_file(name, bytes) + }) + .await +} + +async fn write_video_temp_file_with_writer( + bytes: Vec, + ext: &'static str, + writer: F, +) -> Option<(PathBuf, PrivateTempDir)> +where + F: FnOnce(&PrivateTempDir, &str, &[u8]) -> io::Result + Send + 'static, +{ + let result = tokio::task::spawn_blocking(move || { + let guard = PrivateTempDir::create("mlxcel-video")?; + let name = format!("clip.{ext}"); + let path = writer(&guard, &name, &bytes)?; + Ok::<_, io::Error>((path, guard)) + }) + .await; + + match result { + Ok(Ok(materialized)) => Some(materialized), + Ok(Err(err)) => { + tracing::warn!("Failed to write private video temp file: {err}"); + None } Err(err) => { - tracing::warn!( - "Failed to write video temp file {}: {}", - path.display(), - err - ); + tracing::warn!("Video temp-file task failed: {err}"); None } } } +#[cfg(test)] +pub(crate) async fn write_video_temp_file_for_test( + bytes: Vec, + ext: &'static str, + writer: F, +) -> Option<(PathBuf, PrivateTempDir)> +where + F: FnOnce(&PrivateTempDir, &str, &[u8]) -> io::Result + Send + 'static, +{ + write_video_temp_file_with_writer(bytes, sanitize_video_extension(ext), writer).await +} + fn sanitize_video_extension(ext: &str) -> &'static str { match ext.trim_start_matches('.').to_ascii_lowercase().as_str() { "mp4" => "mp4", @@ -1666,3 +1676,7 @@ async fn http_image_client() -> Result<&'static reqwest::Client> { #[cfg(test)] #[path = "media_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "media_temp_tests.rs"] +mod temp_tests; diff --git a/src/server/media_temp_tests.rs b/src/server/media_temp_tests.rs new file mode 100644 index 000000000..369a92e68 --- /dev/null +++ b/src/server/media_temp_tests.rs @@ -0,0 +1,127 @@ +// Copyright 2025-2026 Lablup Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::fs::OpenOptions; +use std::io::{self, Write as _}; +use std::path::PathBuf; +use std::sync::{Arc, Mutex, mpsc}; +use std::time::Duration; + +use super::{write_video_temp_file, write_video_temp_file_for_test}; + +#[cfg(unix)] +#[tokio::test] +async fn native_video_temp_file_is_private_and_owned_by_directory_guard() { + use std::os::unix::fs::PermissionsExt as _; + + let (path, guard) = write_video_temp_file(b"private video".to_vec(), "mp4") + .await + .expect("temp materialization succeeds"); + let directory = guard.path().to_path_buf(); + + assert_eq!( + std::fs::metadata(&directory).unwrap().permissions().mode() & 0o777, + 0o700 + ); + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); + + drop(guard); + assert!( + !directory.exists(), + "dropping the guard removes the directory" + ); +} + +#[tokio::test] +async fn forced_write_error_removes_partial_private_directory() { + let observed = Arc::new(Mutex::new(None::)); + let observed_by_writer = Arc::clone(&observed); + + let result = write_video_temp_file_for_test( + b"partial video".to_vec(), + "mp4", + move |dir, name, bytes| { + *observed_by_writer.lock().unwrap() = Some(dir.path().to_path_buf()); + dir.write_file(name, &bytes[..4])?; + Err(io::Error::other("forced write failure")) + }, + ) + .await; + + assert!(result.is_none()); + let directory = observed.lock().unwrap().clone().unwrap(); + assert!( + !directory.exists(), + "a failed writer must drop the guard and remove partial output" + ); +} + +#[tokio::test] +async fn dropped_resolver_future_cleans_up_after_blocking_writer_finishes() { + let (entered_tx, entered_rx) = mpsc::channel(); + let (release_tx, release_rx) = mpsc::channel(); + let (finished_tx, finished_rx) = mpsc::channel(); + + let resolver = tokio::spawn(async move { + write_video_temp_file_for_test( + b"video bytes written in two pieces".to_vec(), + "mp4", + move |dir, name, bytes| { + let path = dir.path().join(name); + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(0o600); + } + let mut file = options.open(&path)?; + file.write_all(&bytes[..4])?; + entered_tx.send(dir.path().to_path_buf()).unwrap(); + release_rx.recv().unwrap(); + file.write_all(&bytes[4..])?; + finished_tx.send(()).unwrap(); + Ok(path) + }, + ) + .await + }); + + let directory = tokio::task::spawn_blocking(move || entered_rx.recv().unwrap()) + .await + .unwrap(); + assert!( + directory.exists(), + "writer must be paused with partial output" + ); + + resolver.abort(); + release_tx.send(()).unwrap(); + tokio::task::spawn_blocking(move || finished_rx.recv().unwrap()) + .await + .unwrap(); + assert!(resolver.await.unwrap_err().is_cancelled()); + + tokio::time::timeout(Duration::from_secs(2), async { + while directory.exists() { + tokio::task::yield_now().await; + } + }) + .await + .expect("blocking writer output must be dropped after request cancellation"); + assert!(!directory.exists()); +} diff --git a/src/server/model_provider.rs b/src/server/model_provider.rs index 7a9f09aad..6f2b7034f 100644 --- a/src/server/model_provider.rs +++ b/src/server/model_provider.rs @@ -1610,9 +1610,10 @@ impl ModelProvider { /// full result. /// /// Server video routes pass `videos` through here once the path-traversal - /// guard in [`crate::server::media::extract_chat_video_paths`] has cleared - /// each entry against `MLXCEL_VIDEO_DIR_ALLOWLIST`. Empty `videos` matches - /// earlier behavior bit-for-bit. + /// guard in + /// [`crate::server::media::extract_chat_video_paths_with_allowlist`] has + /// cleared each entry against `MLXCEL_VIDEO_DIR_ALLOWLIST`. Empty `videos` + /// matches earlier behavior bit-for-bit. /// /// Restricted to `pub(crate)` because the input type [`ResolvedVideo`] is /// crate-internal — it carries an [`crate::multimodal::video::VideoSource`] diff --git a/src/server/model_worker.rs b/src/server/model_worker.rs index d875f6b50..d2af44e34 100644 --- a/src/server/model_worker.rs +++ b/src/server/model_worker.rs @@ -3071,7 +3071,8 @@ fn prepare_nemotron_h_nano_omni_audio_embeddings( /// whose startup media support advertises video): /// * The model is one of the video-capable VLM families below. /// * The video paths have already been canonicalised and validated against -/// `MLXCEL_VIDEO_DIR_ALLOWLIST` by [`crate::server::media::extract_chat_video_paths`]. +/// `MLXCEL_VIDEO_DIR_ALLOWLIST` by +/// [`crate::server::media::extract_chat_video_paths_with_allowlist`]. /// /// Defence-in-depth: this function still rejects non-video models with a clean /// error so a future caller that bypasses the route guard cannot silently diff --git a/src/server/router_front.rs b/src/server/router_front.rs index 3bb209ec7..9a7e62630 100644 --- a/src/server/router_front.rs +++ b/src/server/router_front.rs @@ -914,7 +914,7 @@ async fn route_chat( // Render the chat template. let thinking_markers = state.tokenizer.infer_thinking_markers(); - let prepared = match super::chat_request::prepare_chat_request_with_cache( + let prepared = match super::chat_request::prepare_chat_request_with_cache_and_video_allowlist( &state.chat_template, &request, live.chat_template_kwargs.as_ref(), @@ -922,6 +922,9 @@ async fn route_chat( false, !state.config.no_prefill_assistant, &thinking_markers, + // The disaggregated router rejects every declared media input before + // preparation, so it has no video allowlist to consult. + &[], ) .await { diff --git a/src/server/routes/anthropic.rs b/src/server/routes/anthropic.rs index bd7c697fd..9d059d452 100644 --- a/src/server/routes/anthropic.rs +++ b/src/server/routes/anthropic.rs @@ -42,7 +42,9 @@ use crate::server::anthropic_translator::{ AnthropicTranslated, anthropic_request_to_chat, anthropic_stop_reason, apply_stop_sequences, build_content_blocks, parsed_call_to_tool_use, short_uuid, thinking_enabled, }; -use crate::server::chat_request::{prepare_chat_request_with_cache, request_has_effective_input}; +use crate::server::chat_request::{ + prepare_chat_request_with_cache_and_video_allowlist, request_has_effective_input, +}; use crate::server::config::ReasoningBudgetOverride; use crate::server::model_provider::{ChatWorkerGoneError, QueueFullError}; use crate::server::streaming::sse_response; @@ -276,7 +278,7 @@ async fn non_stream_messages( echo_scope.as_ref(), &translated.chat_request, ); - let mut prepared = match prepare_chat_request_with_cache( + let mut prepared = match prepare_chat_request_with_cache_and_video_allowlist( &state.chat_template, &render_request, live.chat_template_kwargs.as_ref(), @@ -284,6 +286,7 @@ async fn non_stream_messages( state.should_render_history_boundary_snapshot(), state.prefill_assistant(), &state.thinking_markers, + &state.video_dir_allowlist, ) .await { @@ -480,7 +483,7 @@ async fn stream_messages( echo_scope.as_ref(), &translated.chat_request, ); - let mut prepared = match prepare_chat_request_with_cache( + let mut prepared = match prepare_chat_request_with_cache_and_video_allowlist( &state.chat_template, &render_request, live.chat_template_kwargs.as_ref(), @@ -488,6 +491,7 @@ async fn stream_messages( state.should_render_history_boundary_snapshot(), state.prefill_assistant(), &state.thinking_markers, + &state.video_dir_allowlist, ) .await { @@ -855,7 +859,7 @@ pub async fn anthropic_count_tokens( } let prompt_cache_enabled = state.prompt_cache.is_some(); - let prepared = match prepare_chat_request_with_cache( + let prepared = match prepare_chat_request_with_cache_and_video_allowlist( &state.chat_template, &translated.chat_request, live.chat_template_kwargs.as_ref(), @@ -863,6 +867,7 @@ pub async fn anthropic_count_tokens( state.should_render_history_boundary_snapshot(), state.prefill_assistant(), &state.thinking_markers, + &state.video_dir_allowlist, ) .await { diff --git a/src/server/routes/chat.rs b/src/server/routes/chat.rs index 814506579..84444768d 100644 --- a/src/server/routes/chat.rs +++ b/src/server/routes/chat.rs @@ -30,7 +30,9 @@ use axum::{ use mlxcel_core::sampling::TokenLogprobData; use crate::server::batch::RequestPriority; -use crate::server::chat_request::{prepare_chat_request_with_cache, resolve_effective_kwargs}; +use crate::server::chat_request::{ + prepare_chat_request_with_cache_and_video_allowlist, resolve_effective_kwargs, +}; use crate::server::chat_template::ChatTemplateProcessor; use crate::server::config::{PromptCacheRequestContext, ReasoningBudgetOverride}; use crate::server::prompt_cache::key::{ @@ -1092,7 +1094,7 @@ pub(crate) async fn stream_asr_completion( } let prompt_cache_enabled = state.prompt_cache.is_some(); - let mut prepared = match prepare_chat_request_with_cache( + let mut prepared = match prepare_chat_request_with_cache_and_video_allowlist( &state.chat_template, &request, live.chat_template_kwargs.as_ref(), @@ -1100,6 +1102,7 @@ pub(crate) async fn stream_asr_completion( state.should_render_history_boundary_snapshot(), state.prefill_assistant(), &state.thinking_markers, + &state.video_dir_allowlist, ) .await { diff --git a/src/server/routes/chat_generation.rs b/src/server/routes/chat_generation.rs index 3d923f179..db9032df0 100644 --- a/src/server/routes/chat_generation.rs +++ b/src/server/routes/chat_generation.rs @@ -37,7 +37,8 @@ use super::chat::{ }; use crate::server::batch::RequestPriority; use crate::server::chat_request::{ - PreparedChatRequest, prepare_chat_request_with_cache, request_has_effective_input, + PreparedChatRequest, prepare_chat_request_with_cache_and_video_allowlist, + request_has_effective_input, }; use crate::server::config::{PromptCacheRequestContext, ReasoningBudgetOverride}; use crate::server::reasoning_echo::ReasoningEchoScope; @@ -190,7 +191,7 @@ pub(crate) async fn prepare_chat_generation<'r>( let echo_scope = crate::server::reasoning_echo::chat_scope(state, &live, request); let render_request = crate::server::reasoning_echo::render_request(state, echo_scope.as_ref(), request); - let mut prepared = prepare_chat_request_with_cache( + let mut prepared = prepare_chat_request_with_cache_and_video_allowlist( &state.chat_template, &render_request, live.chat_template_kwargs.as_ref(), @@ -198,6 +199,7 @@ pub(crate) async fn prepare_chat_generation<'r>( state.should_render_history_boundary_snapshot(), state.prefill_assistant(), &state.thinking_markers, + &state.video_dir_allowlist, ) .await .map_err(|err| ErrorResponse::new(err.to_string(), "invalid_request_error"))?; diff --git a/src/server/routes/prompt_inspection.rs b/src/server/routes/prompt_inspection.rs index 633b02e68..cb84dbe29 100644 --- a/src/server/routes/prompt_inspection.rs +++ b/src/server/routes/prompt_inspection.rs @@ -44,7 +44,7 @@ use axum::{ response::{IntoResponse, Response}, }; -use crate::server::chat_request::prepare_chat_request_with_cache; +use crate::server::chat_request::prepare_chat_request_with_cache_and_video_allowlist; use crate::server::responses_translator::responses_request_to_chat; use crate::server::types::{ChatCompletionRequest, CreateResponseRequest, ErrorResponse}; use crate::server::{AppState, LiveSettings}; @@ -120,7 +120,7 @@ async fn render_chat_prompt( .await .map_err(crate::server::chat_request::VideoFramesError::into_error_response)?; let prompt_cache_enabled = state.prompt_cache.is_some(); - prepare_chat_request_with_cache( + prepare_chat_request_with_cache_and_video_allowlist( &state.chat_template, &request, live.chat_template_kwargs.as_ref(), @@ -128,6 +128,7 @@ async fn render_chat_prompt( state.should_render_history_boundary_snapshot(), state.prefill_assistant(), &state.thinking_markers, + &state.video_dir_allowlist, ) .await .map(|prepared| RenderedPrompt { diff --git a/src/server/routes/responses.rs b/src/server/routes/responses.rs index 9d3f6f545..fa7293bb6 100644 --- a/src/server/routes/responses.rs +++ b/src/server/routes/responses.rs @@ -35,7 +35,9 @@ use axum::{ response::{IntoResponse, Response}, }; -use crate::server::chat_request::{prepare_chat_request_with_cache, request_has_effective_input}; +use crate::server::chat_request::{ + prepare_chat_request_with_cache_and_video_allowlist, request_has_effective_input, +}; use crate::server::config::ReasoningBudgetOverride; use crate::server::conversation_store::ConversationItem; use crate::server::model_provider::QueueFullError; @@ -255,7 +257,7 @@ async fn non_stream_create_response( echo_scope.as_ref(), &translated.chat_request, ); - let prepared = prepare_chat_request_with_cache( + let prepared = prepare_chat_request_with_cache_and_video_allowlist( &state.chat_template, &render_request, live.chat_template_kwargs.as_ref(), @@ -263,6 +265,7 @@ async fn non_stream_create_response( state.should_render_history_boundary_snapshot(), state.prefill_assistant(), &state.thinking_markers, + &state.video_dir_allowlist, ) .await; let mut prepared = match prepared { @@ -436,7 +439,7 @@ async fn stream_create_response( echo_scope.as_ref(), &translated.chat_request, ); - let prepared = prepare_chat_request_with_cache( + let prepared = prepare_chat_request_with_cache_and_video_allowlist( &state.chat_template, &render_request, live.chat_template_kwargs.as_ref(), @@ -444,6 +447,7 @@ async fn stream_create_response( state.should_render_history_boundary_snapshot(), state.prefill_assistant(), &state.thinking_markers, + &state.video_dir_allowlist, ) .await; let mut prepared = match prepared { diff --git a/src/server/state.rs b/src/server/state.rs index 3e4db81bf..d4ab4f86d 100644 --- a/src/server/state.rs +++ b/src/server/state.rs @@ -508,11 +508,12 @@ pub struct AppState { /// `MLXCEL_VIDEO_DIR_ALLOWLIST`, canonicalized once at startup (issue /// #1766). /// - /// The video-frames fallback resolves local clips against this rather than - /// re-reading the environment and calling `std::fs::canonicalize` on each - /// entry per request, which ran on a Tokio worker. Empty by default, which - /// is the same fail-closed answer an unset variable gives: no local path - /// resolves until the startup pipeline installs the operator's list. + /// Native-video resolution and the video-frames fallback both resolve + /// local clips against this rather than re-reading the environment and + /// calling `std::fs::canonicalize` on each entry per request. Empty by + /// default, which is the same fail-closed answer an unset variable gives: + /// no local path resolves until the startup pipeline installs the + /// operator's list. pub(crate) video_dir_allowlist: Arc>, /// Batch-level metrics (active sequences, queue depth) for admission /// control and status reporting.