From 3cc015acd265d0e4e4c9d0a73bf7e5d5a5e66eef Mon Sep 17 00:00:00 2001 From: Connor Carpenter Date: Thu, 27 Aug 2026 19:00:12 -0400 Subject: [PATCH] [Rust Frontend][gRPC] Enforce LoRA path validation across transports (#53756) Signed-off-by: Connor Carpenter Co-authored-by: Bugen Zhao --- rust/src/server/src/grpc/control.rs | 6 +- rust/src/server/src/grpc/tests.rs | 12 ++ rust/src/server/src/lora.rs | 172 ++++++++++++++++++++++++++ rust/src/server/src/routes/lora.rs | 182 ++-------------------------- 4 files changed, 197 insertions(+), 175 deletions(-) diff --git a/rust/src/server/src/grpc/control.rs b/rust/src/server/src/grpc/control.rs index 99b74ded82..987414b963 100644 --- a/rust/src/server/src/grpc/control.rs +++ b/rust/src/server/src/grpc/control.rs @@ -13,7 +13,7 @@ use vllm_engine_core_client::protocol::lora::LoraRequest; use vllm_engine_core_client::protocol::utility::PauseMode as EnginePauseMode; use super::{ControlServer, pb}; -use crate::lora::{LoadLoraError, LoraDisabledError, UnloadLoraError}; +use crate::lora::{LoadLoraError, LoraDisabledError, LoraPathAccessError, UnloadLoraError}; use crate::state::AppState; pub(crate) type ControlGrpcService = ControlServer; @@ -148,6 +148,10 @@ fn load_lora_status(error: LoadLoraError) -> Status { let code = match &error { LoadLoraError::Disabled(_) => Code::FailedPrecondition, LoadLoraError::InvalidRequest(_) => Code::InvalidArgument, + LoadLoraError::PathAccess(LoraPathAccessError::InvalidPath { .. }) => Code::InvalidArgument, + LoadLoraError::PathAccess(LoraPathAccessError::InvalidConfiguration { .. }) => { + Code::Internal + } LoadLoraError::AlreadyLoaded { .. } | LoadLoraError::BaseModelName { .. } => { Code::AlreadyExists } diff --git a/rust/src/server/src/grpc/tests.rs b/rust/src/server/src/grpc/tests.rs index 0db360f14b..34667c31ea 100644 --- a/rust/src/server/src/grpc/tests.rs +++ b/rust/src/server/src/grpc/tests.rs @@ -1632,6 +1632,18 @@ async fn control_lora_lifecycle_selects_adapter_for_generation() { .await; let mut control_client = ControlClient::new(channel.clone()); let mut inference_client = InferenceClient::new(channel); + + let denied_source_path = + std::env::temp_dir().join(format!("vllm-grpc-lora-denied-{}", std::process::id())); + let denied = control_client + .load_lora(pb::LoadLoraRequest { + lora_name: "denied-adapter".to_string(), + source_path: denied_source_path.to_string_lossy().into_owned(), + }) + .await + .expect_err("local LoRA path should be validated before engine load"); + assert_eq!(denied.code(), tonic::Code::InvalidArgument); + let loaded = control_client .load_lora(pb::LoadLoraRequest { lora_name: "adapter".to_string(), diff --git a/rust/src/server/src/lora.rs b/rust/src/server/src/lora.rs index 5cca58c57d..b339688578 100644 --- a/rust/src/server/src/lora.rs +++ b/rust/src/server/src/lora.rs @@ -1,14 +1,86 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright contributors to the vLLM project +use std::path::{Component, Path, PathBuf}; use std::sync::atomic::{AtomicU64, Ordering}; use indexmap::IndexMap; use thiserror::Error; +use thiserror_ext::Macro; use tokio::sync::{Mutex, RwLock}; use vllm_engine_core_client::EngineCoreClient; use vllm_engine_core_client::protocol::lora::{LoraRequest, LoraRequestError}; +const RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV: &str = "VLLM_RUNTIME_LORA_ALLOWED_PATH_PREFIXES"; + +#[derive(Debug, Error, Macro)] +pub(crate) enum LoraPathAccessError { + #[error("{message}")] + InvalidPath { message: String }, + #[error("{message}")] + InvalidConfiguration { message: String }, +} + +fn runtime_lora_allowed_path_prefixes() -> Option> { + let prefixes = std::env::var_os(RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV)?; + let prefixes: Vec<_> = std::env::split_paths(&prefixes) + .filter(|path| !path.as_os_str().is_empty()) + .collect(); + (!prefixes.is_empty()).then_some(prefixes) +} + +fn looks_like_local_lora_path(lora_path: &str) -> bool { + let path = Path::new(lora_path); + path.is_absolute() + || lora_path.starts_with('~') + || lora_path.starts_with('.') + || path.components().any(|component| matches!(component, Component::ParentDir)) +} + +fn validate_lora_path_access( + lora_path: &str, + allowed_prefixes: Option<&[PathBuf]>, +) -> Result, LoraPathAccessError> { + let path = Path::new(lora_path); + if !looks_like_local_lora_path(lora_path) && !path.exists() { + return Ok(None); + } + + let Some(allowed_prefixes) = allowed_prefixes else { + return Err(invalid_path!( + "Local LoRA adapter paths require {RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV} to be configured." + )); + }; + + if !path.is_absolute() { + return Err(invalid_path!( + "Local LoRA adapter paths must be absolute and under one of the prefixes configured by {RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV}." + )); + } + + let canonical_path = path + .canonicalize() + .map_err(|_| invalid_path!("Local LoRA adapter path must exist and be accessible."))?; + let canonical_prefixes = allowed_prefixes + .iter() + .map(|prefix| { + prefix.canonicalize().map_err(|_| { + invalid_configuration!( + "configured {RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV} path prefix must exist and be accessible" + ) + }) + }) + .collect::, _>>()?; + + if !canonical_prefixes.iter().any(|prefix| canonical_path.starts_with(prefix)) { + return Err(invalid_path!( + "Local LoRA adapter path is outside the configured allowed prefixes." + )); + } + + Ok(Some(canonical_path.to_string_lossy().into_owned())) +} + /// Snapshot of the currently served model names plus the requested LoRA, if /// the model name resolves to a dynamic adapter. #[derive(Debug, Clone)] @@ -37,6 +109,8 @@ pub(crate) enum LoadLoraError { Disabled(#[from] LoraDisabledError), #[error(transparent)] InvalidRequest(#[from] LoraRequestError), + #[error(transparent)] + PathAccess(#[from] LoraPathAccessError), #[error("LoRA adapter `{lora_name}` is already loaded")] AlreadyLoaded { lora_name: String }, #[error("LoRA adapter `{lora_name}` conflicts with a served base model")] @@ -117,6 +191,9 @@ impl LoraManager { load_inplace: bool, is_3d_lora_weight: bool, ) -> Result { + let allowed_prefixes = runtime_lora_allowed_path_prefixes(); + let lora_path = validate_lora_path_access(&lora_path, allowed_prefixes.as_deref())? + .unwrap_or(lora_path); let _guard = self.update_lock.lock().await; if base_model_names.iter().any(|name| name == &lora_name) { return Err(LoadLoraError::BaseModelName { lora_name }); @@ -195,3 +272,98 @@ impl LoraManager { Ok(self.requests.write().await.shift_remove(lora_name).unwrap_or(lora_request)) } } + +#[cfg(test)] +mod tests { + use std::fs; + use std::path::PathBuf; + use std::time::{SystemTime, UNIX_EPOCH}; + + use super::validate_lora_path_access; + + fn temp_lora_dir(test_name: &str) -> PathBuf { + let suffix = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("clock should be after unix epoch") + .as_nanos(); + let path = std::env::temp_dir().join(format!( + "vllm-lora-{test_name}-{}-{suffix}", + std::process::id() + )); + fs::create_dir_all(&path).expect("create temp lora dir"); + path + } + + #[test] + fn lora_path_allows_hf_repo_ids_without_prefixes() { + assert_eq!( + validate_lora_path_access("org/adapter-a", None).expect("hf repo id should be allowed"), + None + ); + } + + #[test] + fn lora_path_rejects_local_paths_without_prefixes() { + assert!(validate_lora_path_access("/tmp/adapter-a", None).is_err()); + assert!(validate_lora_path_access("./adapter-a", None).is_err()); + assert!(validate_lora_path_access("~/adapter-a", None).is_err()); + assert!(validate_lora_path_access("subdir/../../../etc/sensitive", None).is_err()); + } + + #[test] + fn lora_path_rejects_existing_bare_relative_paths_without_prefixes() { + let root = + PathBuf::from("target").join(format!("vllm-lora-relative-{}", std::process::id())); + let adapter = root.join("adapter-a"); + fs::create_dir_all(&adapter).expect("create relative adapter dir"); + + assert!( + validate_lora_path_access(adapter.to_str().expect("utf-8 temp path"), None).is_err() + ); + + fs::remove_dir_all(root).ok(); + } + + #[test] + fn lora_path_allows_absolute_paths_under_configured_prefixes() { + let root = temp_lora_dir("allowed-prefix"); + let allowed = root.join("allowed"); + let adapter = allowed.join("adapter-a"); + fs::create_dir_all(&adapter).expect("create adapter dir"); + + let prefixes = [allowed]; + let resolved = + validate_lora_path_access(adapter.to_str().expect("utf-8 temp path"), Some(&prefixes)) + .expect("path under configured prefix should be allowed"); + assert_eq!( + resolved.as_deref(), + Some( + adapter + .canonicalize() + .expect("canonical adapter") + .to_str() + .expect("utf-8 temp path") + ) + ); + + fs::remove_dir_all(root).ok(); + } + + #[test] + fn lora_path_rejects_parent_escape_from_configured_prefixes() { + let root = temp_lora_dir("parent-escape"); + let allowed = root.join("allowed"); + let private_adapter = root.join("private").join("adapter-a"); + fs::create_dir_all(&allowed).expect("create allowed dir"); + fs::create_dir_all(&private_adapter).expect("create private adapter dir"); + + let escaped = allowed.join("../private/adapter-a"); + let prefixes = [allowed]; + assert!( + validate_lora_path_access(escaped.to_str().expect("utf-8 temp path"), Some(&prefixes)) + .is_err() + ); + + fs::remove_dir_all(root).ok(); + } +} diff --git a/rust/src/server/src/routes/lora.rs b/rust/src/server/src/routes/lora.rs index c434e9fd6f..a9bd7d3f30 100644 --- a/rust/src/server/src/routes/lora.rs +++ b/rust/src/server/src/routes/lora.rs @@ -1,7 +1,6 @@ // SPDX-License-Identifier: Apache-2.0 // SPDX-FileCopyrightText: Copyright contributors to the vLLM project -use std::path::{Component, Path, PathBuf}; use std::sync::Arc; use axum::extract::State; @@ -10,13 +9,11 @@ use thiserror_ext::AsReport; use validator::Validate; use crate::error::ApiError; -use crate::lora::{LoadLoraError, UnloadLoraError}; +use crate::lora::{LoadLoraError, LoraPathAccessError, UnloadLoraError}; use crate::routes::openai::utils::types::Normalizable; use crate::routes::openai::utils::validated_json::ValidatedJson; use crate::state::AppState; -const RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV: &str = "VLLM_RUNTIME_LORA_ALLOWED_PATH_PREFIXES"; - #[derive(Debug, Deserialize, Validate)] pub(crate) struct LoadLoraAdapterRequest { lora_name: String, @@ -38,81 +35,17 @@ pub(crate) struct UnloadLoraAdapterRequest { impl Normalizable for UnloadLoraAdapterRequest {} -fn runtime_lora_allowed_path_prefixes() -> Option> { - let prefixes = std::env::var_os(RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV)?; - let prefixes: Vec<_> = std::env::split_paths(&prefixes) - .filter(|path| !path.as_os_str().is_empty()) - .collect(); - (!prefixes.is_empty()).then_some(prefixes) -} - -fn looks_like_local_lora_path(lora_path: &str) -> bool { - let path = Path::new(lora_path); - path.is_absolute() - || lora_path.starts_with('~') - || lora_path.starts_with('.') - || path.components().any(|component| matches!(component, Component::ParentDir)) -} - -fn validate_lora_path_access( - lora_path: &str, - allowed_prefixes: Option<&[PathBuf]>, -) -> Result, ApiError> { - let path = Path::new(lora_path); - if !looks_like_local_lora_path(lora_path) && !path.exists() { - return Ok(None); - } - - let Some(allowed_prefixes) = allowed_prefixes else { - return Err(ApiError::invalid_request( - format!( - "Local LoRA adapter paths require {RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV} to be configured." - ), - Some("lora_path"), - )); - }; - - if !path.is_absolute() { - return Err(ApiError::invalid_request( - format!( - "Local LoRA adapter paths must be absolute and under one of the prefixes configured by {RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV}." - ), - Some("lora_path"), - )); - } - - let canonical_path = path.canonicalize().map_err(|_| { - ApiError::invalid_request( - "Local LoRA adapter path must exist and be accessible.".to_string(), - Some("lora_path"), - ) - })?; - let canonical_prefixes = allowed_prefixes - .iter() - .map(|prefix| { - prefix.canonicalize().map_err(|_| { - ApiError::server_error(format!( - "configured {RUNTIME_LORA_ALLOWED_PATH_PREFIXES_ENV} path prefix must exist and be accessible" - )) - }) - }) - .collect::, _>>()?; - - if !canonical_prefixes.iter().any(|prefix| canonical_path.starts_with(prefix)) { - return Err(ApiError::invalid_request( - "Local LoRA adapter path is outside the configured allowed prefixes.".to_string(), - Some("lora_path"), - )); - } - - Ok(Some(canonical_path.to_string_lossy().into_owned())) -} - fn load_lora_api_error(error: LoadLoraError) -> ApiError { let message = error.to_report_string(); match error { LoadLoraError::Disabled(_) => ApiError::invalid_request(message, None), LoadLoraError::InvalidRequest(_) => ApiError::invalid_request(message, None), + LoadLoraError::PathAccess(LoraPathAccessError::InvalidPath { .. }) => { + ApiError::invalid_request(message, Some("lora_path")) + } + LoadLoraError::PathAccess(LoraPathAccessError::InvalidConfiguration { .. }) => { + ApiError::server_error(message) + } LoadLoraError::AlreadyLoaded { .. } => ApiError::invalid_request( format!( "{message}. If you want to load the adapter in place, set 'load_inplace' to true." @@ -147,15 +80,11 @@ pub async fn load_lora_adapter( State(state): State>, ValidatedJson(request): ValidatedJson, ) -> Result { - let allowed_prefixes = runtime_lora_allowed_path_prefixes(); - let lora_path = validate_lora_path_access(&request.lora_path, allowed_prefixes.as_deref())? - .unwrap_or(request.lora_path); - let lora_name = request.lora_name; state .load_lora( lora_name.clone(), - lora_path, + request.lora_path, request.load_inplace, request.is_3d_lora_weight, ) @@ -189,98 +118,3 @@ pub async fn unload_lora_adapter( lora_request.lora_name )) } - -#[cfg(test)] -mod tests { - use std::fs; - use std::path::PathBuf; - use std::time::{SystemTime, UNIX_EPOCH}; - - use super::validate_lora_path_access; - - fn temp_lora_dir(test_name: &str) -> PathBuf { - let suffix = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("clock should be after unix epoch") - .as_nanos(); - let path = std::env::temp_dir().join(format!( - "vllm-lora-{test_name}-{}-{suffix}", - std::process::id() - )); - fs::create_dir_all(&path).expect("create temp lora dir"); - path - } - - #[test] - fn lora_path_allows_hf_repo_ids_without_prefixes() { - assert_eq!( - validate_lora_path_access("org/adapter-a", None).expect("hf repo id should be allowed"), - None - ); - } - - #[test] - fn lora_path_rejects_local_paths_without_prefixes() { - assert!(validate_lora_path_access("/tmp/adapter-a", None).is_err()); - assert!(validate_lora_path_access("./adapter-a", None).is_err()); - assert!(validate_lora_path_access("~/adapter-a", None).is_err()); - assert!(validate_lora_path_access("subdir/../../../etc/sensitive", None).is_err()); - } - - #[test] - fn lora_path_rejects_existing_bare_relative_paths_without_prefixes() { - let root = - PathBuf::from("target").join(format!("vllm-lora-relative-{}", std::process::id())); - let adapter = root.join("adapter-a"); - fs::create_dir_all(&adapter).expect("create relative adapter dir"); - - assert!( - validate_lora_path_access(adapter.to_str().expect("utf-8 temp path"), None).is_err() - ); - - fs::remove_dir_all(root).ok(); - } - - #[test] - fn lora_path_allows_absolute_paths_under_configured_prefixes() { - let root = temp_lora_dir("allowed-prefix"); - let allowed = root.join("allowed"); - let adapter = allowed.join("adapter-a"); - fs::create_dir_all(&adapter).expect("create adapter dir"); - - let prefixes = [allowed]; - let resolved = - validate_lora_path_access(adapter.to_str().expect("utf-8 temp path"), Some(&prefixes)) - .expect("path under configured prefix should be allowed"); - assert_eq!( - resolved.as_deref(), - Some( - adapter - .canonicalize() - .expect("canonical adapter") - .to_str() - .expect("utf-8 temp path") - ) - ); - - fs::remove_dir_all(root).ok(); - } - - #[test] - fn lora_path_rejects_parent_escape_from_configured_prefixes() { - let root = temp_lora_dir("parent-escape"); - let allowed = root.join("allowed"); - let private_adapter = root.join("private").join("adapter-a"); - fs::create_dir_all(&allowed).expect("create allowed dir"); - fs::create_dir_all(&private_adapter).expect("create private adapter dir"); - - let escaped = allowed.join("../private/adapter-a"); - let prefixes = [allowed]; - assert!( - validate_lora_path_access(escaped.to_str().expect("utf-8 temp path"), Some(&prefixes)) - .is_err() - ); - - fs::remove_dir_all(root).ok(); - } -}