prevent path traversal via log_file_index in log endpoints (#9569)

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

backend/windmill-api-jobs/src/jobs_export.rs

@@ -16,7 +16,7 @@ use uuid::Uuid;
 use windmill_common::{
     db::UserDB,
     error,
-    jobs::{JobKind, JobStatus, JobTriggerKind},
+    jobs::{is_safe_log_file_path, JobKind, JobStatus, JobTriggerKind},
     scripts::ScriptLang,
     utils::{paginate, paginate_without_limits, require_admin, Pagination},
 };
@@ -328,6 +328,20 @@ pub async fn import_completed_jobs(
 ) -> error::Result<String> {
     require_admin(authed.is_admin, &authed.username)?;
 
+    // log_file_index is read back by the log endpoints as paths under the windmill
+    // log directory; an attacker-supplied traversal here would become an arbitrary
+    // file read. Reject anything that could escape the log directory at ingestion.
+    for job in &jobs {
+        if let Some(file_index) = &job.log_file_index {
+            if file_index.iter().any(|p| !is_safe_log_file_path(p)) {
+                return Err(error::Error::BadRequest(format!(
+                    "Invalid log_file_index for job {}: entries must be relative paths without '..'",
+                    job.id
+                )));
+            }
+        }
+    }
+
     let mut tx = user_db.begin(&authed).await?;
 
     for job in jobs {

backend/windmill-api/src/jobs.rs

@@ -34,8 +34,8 @@ use windmill_common::db::UserDbWithAuthed;
 use windmill_common::error::JsonResult;
 use windmill_common::flow_status::{JobResult, RestartedFrom};
 use windmill_common::jobs::{
-    format_completed_job_result, format_result, is_valid_entrypoint_name, DynamicInput,
-    ENTRYPOINT_OVERRIDE,
+    format_completed_job_result, format_result, is_safe_log_file_path, is_valid_entrypoint_name,
+    DynamicInput, ENTRYPOINT_OVERRIDE,
 };
 #[cfg(feature = "run_inline")]
 use windmill_common::jobs::{
@@ -1912,12 +1912,17 @@ async fn get_logs_from_disk(
     if log_offset > 0 {
         if let Some(file_index) = log_file_index.clone() {
             for file_p in &file_index {
-                if !tokio::fs::metadata(format!("{}/{file_p}", *WINDMILL_DIR))
-                    .await
-                    .is_ok()
-                {
+                if !is_safe_log_file_path(file_p) {
                     return None;
                 }
+                let local_file = format!("{}/{file_p}", *WINDMILL_DIR);
+                // Defense in depth: refuse to read through a symlink so a planted
+                // symlink under the log directory cannot exfiltrate arbitrary files.
+                match tokio::fs::symlink_metadata(&local_file).await {
+                    Ok(meta) if meta.file_type().is_symlink() => return None,
+                    Ok(_) => {}
+                    Err(_) => return None,
+                }
             }
 
             let logs = logs.to_string();

backend/windmill-common/src/jobs.rs

@@ -283,6 +283,19 @@ pub fn format_completed_job_result(mut cj: CompletedJob) -> CompletedJob {
     cj
 }
 
+/// `log_file_index` is normally written by the worker as job-id-scoped relative
+/// paths under the windmill log directory. Any code path that lets a request
+/// control this value (e.g. job import) must reject entries that could escape
+/// that directory, otherwise the log-reading endpoints become an arbitrary file
+/// read primitive. Rejects path traversal (`..`) and absolute paths; on-disk
+/// readers additionally refuse symlinks (see `get_logs_from_disk`).
+pub fn is_safe_log_file_path(file_p: &str) -> bool {
+    !file_p.is_empty()
+        && !file_p.starts_with('/')
+        && !file_p.starts_with('\\')
+        && !file_p.split(['/', '\\']).any(|c| c == "..")
+}
+
 pub async fn get_logs_from_disk(
     log_offset: i32,
     logs: &str,
@@ -291,12 +304,17 @@ pub async fn get_logs_from_disk(
     if log_offset > 0 {
         if let Some(file_index) = log_file_index.clone() {
             for file_p in &file_index {
-                if !tokio::fs::metadata(format!("{}/{file_p}", *WINDMILL_DIR))
-                    .await
-                    .is_ok()
-                {
+                if !is_safe_log_file_path(file_p) {
                     return None;
                 }
+                let local_file = format!("{}/{file_p}", *WINDMILL_DIR);
+                // Defense in depth: refuse to read through a symlink so a planted
+                // symlink under the log directory cannot exfiltrate arbitrary files.
+                match tokio::fs::symlink_metadata(&local_file).await {
+                    Ok(meta) if meta.file_type().is_symlink() => return None,
+                    Ok(_) => {}
+                    Err(_) => return None,
+                }
             }
 
             let logs = logs.to_string();
@@ -439,3 +457,30 @@ pub struct WorkerInternalServerInlineUtils {
 // The server cannot call the worker functions directly because they are independent crates
 pub static WORKER_INTERNAL_SERVER_INLINE_UTILS: OnceCell<WorkerInternalServerInlineUtils> =
     OnceCell::new();
+
+#[cfg(test)]
+mod tests {
+    use super::is_safe_log_file_path;
+
+    #[test]
+    fn safe_log_file_paths_are_accepted() {
+        // Legit worker-written entries are job-id-scoped relative paths.
+        assert!(is_safe_log_file_path(
+            "0190d3e2-0000-7000-8000-000000000000/0.txt"
+        ));
+        assert!(is_safe_log_file_path("logs/abc/chunk1.log"));
+        assert!(is_safe_log_file_path("file..with..dots.txt"));
+    }
+
+    #[test]
+    fn traversal_and_absolute_paths_are_rejected() {
+        assert!(!is_safe_log_file_path(""));
+        assert!(!is_safe_log_file_path("../../../../etc/passwd"));
+        assert!(!is_safe_log_file_path("a/../../etc/passwd"));
+        assert!(!is_safe_log_file_path(".."));
+        assert!(!is_safe_log_file_path("/etc/passwd"));
+        assert!(!is_safe_log_file_path("/proc/self/environ"));
+        assert!(!is_safe_log_file_path("\\windows\\path"));
+        assert!(!is_safe_log_file_path("a\\..\\..\\b"));
+    }
+}

backend/windmill-object-store/src/lib.rs

@@ -52,6 +52,7 @@ use tokio::task;
 #[cfg(feature = "parquet")]
 use windmill_common::error::to_anyhow;
 #[cfg(feature = "parquet")]
+use windmill_common::jobs::is_safe_log_file_path;
 use windmill_common::utils::rd_string;
 #[cfg(all(feature = "parquet", feature = "private"))]
 pub mod job_s3_helpers_ee;
@@ -1296,6 +1297,9 @@ pub async fn get_logs_from_store(
 ) -> Option<impl futures::Stream<Item = Result<bytes::Bytes, object_store::Error>>> {
     if log_offset > 0 {
         if let Some(file_index) = log_file_index.clone() {
+            if file_index.iter().any(|p| !is_safe_log_file_path(p)) {
+                return None;
+            }
             if let Some(os) = get_object_store().await {
                 let logs = logs.to_string();
                 let stream = async_stream::stream! {