diff --git a/.changeset/path-hardening-upload-output.md b/.changeset/path-hardening-upload-output.md new file mode 100644 index 0000000..0c5dd0d --- /dev/null +++ b/.changeset/path-hardening-upload-output.md @@ -0,0 +1,5 @@ +--- +"@googleworkspace/cli": patch +--- + +Hardened runtime file path handling for dynamic `--upload` and `--output` flags by enforcing safe relative paths within the current working directory. diff --git a/Cargo.lock b/Cargo.lock index e40df1c..15c5951 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -860,6 +860,7 @@ dependencies = [ "futures-util", "hostname", "keyring", + "libc", "percent-encoding", "rand 0.8.5", "ratatui", diff --git a/Cargo.toml b/Cargo.toml index 5babca8..a3f3bba 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,6 +56,7 @@ keyring = "3.6.3" async-trait = "0.1.89" serde_yaml = "0.9.34" percent-encoding = "2.3.2" +libc = "0.2" zeroize = { version = "1.8.2", features = ["derive"] } diff --git a/src/executor.rs b/src/executor.rs index 49101ec..15e977b 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -75,7 +75,7 @@ fn parse_and_validate_inputs( method: &RestMethod, params_json: Option<&str>, body_json: Option<&str>, - upload_path: Option<&str>, + has_upload: bool, ) -> Result { let params: Map = if let Some(p) = params_json { serde_json::from_str(p) @@ -122,8 +122,8 @@ fn parse_and_validate_inputs( } } - let (full_url, query_params) = build_url(doc, method, ¶ms, upload_path.is_some())?; - let is_upload = upload_path.is_some() && method.supports_media_upload; + let (full_url, query_params) = build_url(doc, method, ¶ms, has_upload)?; + let is_upload = has_upload && method.supports_media_upload; Ok(ExecutionInput { params, @@ -144,7 +144,7 @@ async fn build_http_request( auth_method: &AuthMethod, page_token: Option<&str>, pages_fetched: u32, - upload_path: Option<&str>, + upload_bytes: Option<&[u8]>, ) -> Result { let mut request = match method.http_method.as_str() { "GET" => client.get(&input.full_url), @@ -180,17 +180,11 @@ async fn build_http_request( if pages_fetched == 0 { if input.is_upload { - let upload_path = upload_path.expect("upload_path must be Some when is_upload is true"); - - let file_bytes = tokio::fs::read(upload_path).await.map_err(|e| { - GwsError::Validation(format!( - "Failed to read upload file '{}': {}", - upload_path, e - )) - })?; + let file_bytes = + upload_bytes.expect("upload bytes must be Some when is_upload is true"); request = request.query(&[("uploadType", "multipart")]); - let (multipart_body, content_type) = build_multipart_body(&input.body, &file_bytes)?; + let (multipart_body, content_type) = build_multipart_body(&input.body, file_bytes)?; request = request.header("Content-Type", content_type); request = request.body(multipart_body); } else if let Some(ref body_val) = input.body { @@ -307,17 +301,27 @@ async fn handle_binary_response( output_format: &crate::formatter::OutputFormat, capture_output: bool, ) -> Result, GwsError> { - let file_path = if let Some(p) = output_path { - PathBuf::from(p) + let (mut file, file_path) = if let Some(path) = output_path { + let path_owned = path.to_string(); + let (file, canonical_path) = tokio::task::spawn_blocking(move || { + crate::validate::open_safe_output_file(&path_owned) + }) + .await + .map_err(|e| { + GwsError::Other(anyhow::anyhow!( + "Output file open/validation task failed: {e}" + )) + })??; + (tokio::fs::File::from_std(file), canonical_path) } else { let ext = mime_to_extension(content_type); - PathBuf::from(format!("download.{ext}")) + let file_path = PathBuf::from(format!("download.{ext}")); + let file = tokio::fs::File::create(&file_path) + .await + .context("Failed to create output file")?; + (file, file_path) }; - let mut file = tokio::fs::File::create(&file_path) - .await - .context("Failed to create output file")?; - let mut stream = response.bytes_stream(); let mut total_bytes: u64 = 0; @@ -347,6 +351,37 @@ async fn handle_binary_response( Ok(None) } +async fn validate_execution_paths( + output_path: Option<&str>, + upload_path: Option<&str>, +) -> Result<(Option, Option), GwsError> { + let safe_output_path = if let Some(path) = output_path { + let path_owned = path.to_string(); + tokio::task::spawn_blocking(move || { + crate::validate::validate_safe_output_file_path(&path_owned) + }) + .await + .map_err(|e| GwsError::Other(anyhow::anyhow!("Path validation task failed: {e}")))??; + Some(path.to_string()) + } else { + None + }; + + let safe_upload_path = if let Some(path) = upload_path { + let path_owned = path.to_string(); + tokio::task::spawn_blocking(move || { + crate::validate::validate_safe_input_file_path(&path_owned) + }) + .await + .map_err(|e| GwsError::Other(anyhow::anyhow!("Path validation task failed: {e}")))??; + Some(path.to_string()) + } else { + None + }; + + Ok((safe_output_path, safe_upload_path)) +} + /// Executes an API method call. /// /// This is the core function of the CLI that handles: @@ -374,7 +409,15 @@ pub async fn execute_method( output_format: &crate::formatter::OutputFormat, capture_output: bool, ) -> Result, GwsError> { - let input = parse_and_validate_inputs(doc, method, params_json, body_json, upload_path)?; + let (safe_output_path, safe_upload_path) = + validate_execution_paths(output_path, upload_path).await?; + let input = parse_and_validate_inputs( + doc, + method, + params_json, + body_json, + safe_upload_path.is_some(), + )?; if dry_run { let dry_run_info = json!({ @@ -395,6 +438,21 @@ pub async fn execute_method( return Ok(None); } + let upload_bytes = if input.is_upload { + let upload_path = safe_upload_path + .as_deref() + .expect("safe_upload_path must be Some when is_upload is true") + .to_string(); + let bytes = tokio::task::spawn_blocking(move || { + crate::validate::read_safe_upload_file(&upload_path) + }) + .await + .map_err(|e| GwsError::Other(anyhow::anyhow!("Upload file read task failed: {e}")))??; + Some(bytes) + } else { + None + }; + let mut page_token: Option = None; let mut pages_fetched: u32 = 0; let mut captured_values = Vec::new(); @@ -409,7 +467,7 @@ pub async fn execute_method( &auth_method, page_token.as_deref(), pages_fetched, - upload_path, + upload_bytes.as_deref(), ) .await?; @@ -456,7 +514,7 @@ pub async fn execute_method( } else if let Some(res) = handle_binary_response( response, &content_type, - output_path, + safe_output_path.as_deref(), output_format, capture_output, ) @@ -1727,6 +1785,229 @@ async fn test_execute_method_missing_path_param() { .contains("Required path parameter")); } +#[tokio::test] +async fn test_execute_method_dry_run_rejects_absolute_upload_path() { + let doc = RestDescription::default(); + let method = RestMethod { + http_method: "GET".to_string(), + path: "files".to_string(), + media_upload: Some(crate::discovery::MediaUpload { + protocols: Some(crate::discovery::MediaUploadProtocols { + simple: Some(crate::discovery::MediaUploadProtocol { + path: "/upload/files".to_string(), + multipart: Some(true), + }), + }), + ..Default::default() + }), + supports_media_upload: true, + ..Default::default() + }; + + let sanitize_mode = crate::helpers::modelarmor::SanitizeMode::Warn; + let result = execute_method( + &doc, + &method, + None, + None, + None, + AuthMethod::None, + None, + Some("/etc/hosts"), + true, + &PaginationConfig::default(), + None, + &sanitize_mode, + &crate::formatter::OutputFormat::default(), + false, + ) + .await; + + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("--upload must be a relative path")); +} + +#[tokio::test] +async fn test_execute_method_dry_run_rejects_absolute_output_path() { + let doc = RestDescription::default(); + let method = RestMethod { + http_method: "GET".to_string(), + path: "files".to_string(), + ..Default::default() + }; + + let sanitize_mode = crate::helpers::modelarmor::SanitizeMode::Warn; + let result = execute_method( + &doc, + &method, + None, + None, + None, + AuthMethod::None, + Some("/tmp/out.bin"), + None, + true, + &PaginationConfig::default(), + None, + &sanitize_mode, + &crate::formatter::OutputFormat::default(), + false, + ) + .await; + + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("--output must be a relative path")); +} + +#[tokio::test] +async fn test_execute_method_dry_run_accepts_safe_relative_paths() { + let method = RestMethod { + http_method: "GET".to_string(), + path: "files".to_string(), + media_upload: Some(crate::discovery::MediaUpload { + protocols: Some(crate::discovery::MediaUploadProtocols { + simple: Some(crate::discovery::MediaUploadProtocol { + path: "/upload/files".to_string(), + multipart: Some(true), + }), + }), + ..Default::default() + }), + supports_media_upload: true, + ..Default::default() + }; + let sanitize_mode = crate::helpers::modelarmor::SanitizeMode::Warn; + let result = execute_method( + &RestDescription::default(), + &method, + None, + None, + None, + AuthMethod::None, + Some("target/path-hardening-upload-output/out.bin"), + Some("Cargo.toml"), + true, + &PaginationConfig::default(), + None, + &sanitize_mode, + &crate::formatter::OutputFormat::default(), + true, + ) + .await; + + assert!(result.is_ok(), "expected Ok, got: {result:?}"); + let val = result.unwrap().expect("expected dry-run JSON"); + assert_eq!(val["dry_run"], true); +} + +#[tokio::test] +async fn test_execute_method_rejects_unsafe_upload_path() { + let doc = RestDescription::default(); + let method = RestMethod { + http_method: "GET".to_string(), + path: "files".to_string(), + flat_path: Some("files".to_string()), + ..Default::default() + }; + + let result = execute_method( + &doc, + &method, + None, + None, + None, + AuthMethod::None, + None, + Some("bad\0path"), + true, + &PaginationConfig::default(), + None, + &crate::helpers::modelarmor::SanitizeMode::Warn, + &crate::formatter::OutputFormat::default(), + false, + ) + .await; + + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("--upload")); +} + +#[tokio::test] +async fn test_execute_method_rejects_unsafe_output_path() { + let doc = RestDescription::default(); + let method = RestMethod { + http_method: "GET".to_string(), + path: "files".to_string(), + flat_path: Some("files".to_string()), + ..Default::default() + }; + + let result = execute_method( + &doc, + &method, + None, + None, + None, + AuthMethod::None, + Some("bad\0path"), + None, + true, + &PaginationConfig::default(), + None, + &crate::helpers::modelarmor::SanitizeMode::Warn, + &crate::formatter::OutputFormat::default(), + false, + ) + .await; + + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("--output")); +} + +#[tokio::test] +async fn test_build_http_request_upload_uses_provided_bytes() { + let client = reqwest::Client::new(); + let method = RestMethod { + http_method: "POST".to_string(), + path: "files".to_string(), + ..Default::default() + }; + let input = ExecutionInput { + full_url: "https://example.com/files".to_string(), + body: None, + params: Map::new(), + query_params: HashMap::new(), + is_upload: true, + }; + + let request = build_http_request( + &client, + &method, + &input, + None, + &AuthMethod::None, + None, + 0, + Some(b"hello"), + ) + .await + .unwrap(); + + let built = request.build().unwrap(); + assert_eq!(built.url().query(), Some("uploadType=multipart")); + assert!(built + .headers() + .get("Content-Type") + .and_then(|v| v.to_str().ok()) + .is_some_and(|value| value.starts_with("multipart/related; boundary="))); +} + #[test] fn test_handle_error_response_non_json() { let err = handle_error_response::<()>( diff --git a/src/validate.rs b/src/validate.rs index cfefd60..21d749c 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -19,6 +19,9 @@ //! LLM agent rather than a human operator. use crate::error::GwsError; +#[cfg(not(unix))] +use std::fs::OpenOptions; +use std::io::Read; use std::path::{Path, PathBuf}; /// Validates that `dir` is a safe output directory. @@ -118,6 +121,428 @@ pub fn validate_safe_dir_path(dir: &str) -> Result { Ok(canonical) } +/// Validates that `path` is a safe input file path for reading (e.g. `--upload`). +/// +/// The path must be relative to CWD, must resolve within CWD, and must point +/// to an existing regular file. +pub fn validate_safe_input_file_path(path: &str) -> Result { + validate_safe_read_file_path(path, "--upload") +} + +/// Validates that `path` is a safe output file path for writing (e.g. `--output`). +/// +/// The path must be relative to CWD and must resolve within CWD. Existing +/// directories are rejected. +pub fn validate_safe_output_file_path(path: &str) -> Result { + validate_safe_write_file_path(path, "--output") +} + +/// Validate and read an upload file in one operation. +/// +/// This reduces the race window between path validation and file consumption by +/// reading from the validated path immediately. +pub fn read_safe_upload_file(path: &str) -> Result, GwsError> { + let canonical = validate_safe_input_file_path(path)?; + + #[cfg(unix)] + { + let parent = canonical.parent().ok_or_else(|| { + GwsError::Validation(format!( + "Failed to resolve parent directory for --upload '{}'", + path + )) + })?; + let leaf = canonical.file_name().ok_or_else(|| { + GwsError::Validation(format!( + "Failed to resolve file name for --upload '{}'", + path + )) + })?; + + let parent_dir = open_parent_dir_under_cwd(parent, "--upload", path)?; + let mut file = open_child_no_follow( + &parent_dir, + leaf, + libc::O_RDONLY | libc::O_NOFOLLOW, + 0, + "--upload", + path, + )?; + + let meta = file.metadata().map_err(|e| { + GwsError::Validation(format!( + "Failed to inspect upload file '{}': {}", + canonical.display(), + e + )) + })?; + if !meta.is_file() { + return Err(GwsError::Validation(format!( + "--upload '{}' must reference a regular file", + path + ))); + } + + let mut bytes = Vec::new(); + file.read_to_end(&mut bytes).map_err(|e| { + GwsError::Validation(format!( + "Failed to read upload file '{}': {}", + canonical.display(), + e + )) + })?; + Ok(bytes) + } + + #[cfg(not(unix))] + { + std::fs::read(&canonical).map_err(|e| { + GwsError::Validation(format!( + "Failed to read upload file '{}': {}", + canonical.display(), + e + )) + }) + } +} + +/// Validate and open an output file in one operation. +/// +/// Returns a trusted writable file handle and the canonical path used for +/// writing. Existing files are opened without truncation first so post-open +/// safety checks still run before bytes are written. +pub fn open_safe_output_file(path: &str) -> Result<(std::fs::File, PathBuf), GwsError> { + let canonical = validate_safe_output_file_path(path)?; + let existed_before_open = std::fs::symlink_metadata(&canonical).is_ok(); + + #[cfg(unix)] + { + let parent = canonical.parent().ok_or_else(|| { + GwsError::Validation(format!( + "Failed to resolve parent directory for --output '{}'", + path + )) + })?; + let leaf = canonical.file_name().ok_or_else(|| { + GwsError::Validation(format!( + "Failed to resolve file name for --output '{}'", + path + )) + })?; + + let parent_dir = open_parent_dir_under_cwd(parent, "--output", path)?; + let flags = if existed_before_open { + libc::O_WRONLY | libc::O_NOFOLLOW + } else { + libc::O_WRONLY | libc::O_CREAT | libc::O_EXCL | libc::O_NOFOLLOW + }; + let file = open_child_no_follow(&parent_dir, leaf, flags, 0o600, "--output", path)?; + + let meta = file.metadata().map_err(|e| { + GwsError::Validation(format!( + "Failed to inspect output file '{}': {}", + canonical.display(), + e + )) + })?; + if !meta.is_file() { + return Err(GwsError::Validation(format!( + "--output '{}' is not a regular file", + path + ))); + } + + if existed_before_open { + file.set_len(0).map_err(|e| { + GwsError::Validation(format!( + "Failed to truncate output file '{}': {}", + canonical.display(), + e + )) + })?; + } + + Ok((file, canonical)) + } + + #[cfg(not(unix))] + { + let mut opts = OpenOptions::new(); + opts.write(true); + if existed_before_open { + opts.create(false); + } else { + opts.create_new(true); + } + + let file = opts.open(&canonical).map_err(|e| { + GwsError::Validation(format!( + "Failed to open output file '{}': {}", + canonical.display(), + e + )) + })?; + + let canonical_after_open = canonical.canonicalize().map_err(|e| { + GwsError::Validation(format!( + "Failed to resolve output file after open '{}': {}", + canonical.display(), + e + )) + })?; + let cwd = std::env::current_dir().map_err(|e| { + GwsError::Validation(format!("Failed to determine current directory: {e}")) + })?; + let canonical_cwd = cwd.canonicalize().map_err(|e| { + GwsError::Validation(format!("Failed to canonicalize current directory: {e}")) + })?; + if !canonical_after_open.starts_with(&canonical_cwd) { + return Err(GwsError::Validation(format!( + "--output '{}' resolves outside current directory after open: '{}'", + path, + canonical_after_open.display() + ))); + } + + let meta = file.metadata().map_err(|e| { + GwsError::Validation(format!( + "Failed to inspect output file '{}': {}", + canonical_after_open.display(), + e + )) + })?; + if !meta.is_file() { + return Err(GwsError::Validation(format!( + "--output '{}' is not a regular file", + path + ))); + } + + if existed_before_open { + file.set_len(0).map_err(|e| { + GwsError::Validation(format!( + "Failed to truncate output file '{}': {}", + canonical_after_open.display(), + e + )) + })?; + } + + Ok((file, canonical_after_open)) + } +} + +#[cfg(unix)] +fn open_parent_dir_under_cwd( + parent: &Path, + flag_name: &str, + original_path: &str, +) -> Result { + use std::os::unix::fs::MetadataExt; + + let dir = std::fs::File::open(parent).map_err(|e| { + GwsError::Validation(format!( + "Failed to open parent directory for {flag_name} '{}': {}", + original_path, e + )) + })?; + + let dir_meta = dir.metadata().map_err(|e| { + GwsError::Validation(format!( + "Failed to inspect parent directory for {flag_name} '{}': {}", + original_path, e + )) + })?; + if !dir_meta.is_dir() { + return Err(GwsError::Validation(format!( + "Parent path for {flag_name} '{}' is not a directory", + original_path + ))); + } + + let cwd = std::env::current_dir() + .map_err(|e| GwsError::Validation(format!("Failed to determine current directory: {e}")))?; + let canonical_cwd = cwd.canonicalize().map_err(|e| { + GwsError::Validation(format!("Failed to canonicalize current directory: {e}")) + })?; + let canonical_parent = parent.canonicalize().map_err(|e| { + GwsError::Validation(format!( + "Failed to resolve parent directory for {flag_name} '{}': {}", + original_path, e + )) + })?; + if !canonical_parent.starts_with(&canonical_cwd) { + return Err(GwsError::Validation(format!( + "{flag_name} '{}' resolves outside current directory", + original_path + ))); + } + + let parent_meta = std::fs::metadata(&canonical_parent).map_err(|e| { + GwsError::Validation(format!( + "Failed to inspect resolved parent directory for {flag_name} '{}': {}", + original_path, e + )) + })?; + if dir_meta.dev() != parent_meta.dev() || dir_meta.ino() != parent_meta.ino() { + return Err(GwsError::Validation(format!( + "Detected concurrent filesystem change while resolving {flag_name} '{}'", + original_path + ))); + } + + Ok(dir) +} + +#[cfg(unix)] +fn open_child_no_follow( + parent_dir: &std::fs::File, + leaf: &std::ffi::OsStr, + flags: libc::c_int, + mode: libc::mode_t, + flag_name: &str, + original_path: &str, +) -> Result { + use std::ffi::CString; + use std::os::fd::{AsRawFd, FromRawFd}; + use std::os::unix::ffi::OsStrExt; + + let leaf_c = CString::new(leaf.as_bytes()).map_err(|_| { + GwsError::Validation(format!( + "{flag_name} '{}' contains an invalid path segment", + original_path + )) + })?; + + let fd = unsafe { + libc::openat( + parent_dir.as_raw_fd(), + leaf_c.as_ptr(), + flags, + mode as libc::c_uint, + ) + }; + if fd < 0 { + return Err(GwsError::Validation(format!( + "Failed to open {flag_name} '{}': {}", + original_path, + std::io::Error::last_os_error() + ))); + } + + Ok(unsafe { std::fs::File::from_raw_fd(fd) }) +} + +fn validate_safe_read_file_path(path: &str, flag_name: &str) -> Result { + reject_control_chars(path, flag_name)?; + + let input_path = Path::new(path); + if input_path.as_os_str().is_empty() { + return Err(GwsError::Validation(format!( + "{flag_name} must not be empty" + ))); + } + if input_path.is_absolute() { + return Err(GwsError::Validation(format!( + "{flag_name} must be a relative path, got absolute path '{}'", + path + ))); + } + if input_path + .components() + .any(|c| c == std::path::Component::ParentDir) + { + return Err(GwsError::Validation(format!( + "{flag_name} must not contain path traversal ('..'): {}", + path + ))); + } + + let cwd = std::env::current_dir() + .map_err(|e| GwsError::Validation(format!("Failed to determine current directory: {e}")))?; + let resolved = cwd.join(input_path); + let canonical = resolved.canonicalize().map_err(|e| { + GwsError::Validation(format!("Failed to resolve {flag_name} '{}': {e}", path)) + })?; + + let canonical_cwd = cwd.canonicalize().map_err(|e| { + GwsError::Validation(format!("Failed to canonicalize current directory: {e}")) + })?; + if !canonical.starts_with(&canonical_cwd) { + return Err(GwsError::Validation(format!( + "{flag_name} '{}' resolves to '{}' which is outside the current directory", + path, + canonical.display() + ))); + } + + if !canonical.is_file() { + return Err(GwsError::Validation(format!( + "{flag_name} '{}' must reference a file", + path + ))); + } + + Ok(canonical) +} + +fn validate_safe_write_file_path(path: &str, flag_name: &str) -> Result { + reject_control_chars(path, flag_name)?; + + let output_path = Path::new(path); + if output_path.as_os_str().is_empty() { + return Err(GwsError::Validation(format!( + "{flag_name} must not be empty" + ))); + } + if output_path.is_absolute() { + return Err(GwsError::Validation(format!( + "{flag_name} must be a relative path, got absolute path '{}'", + path + ))); + } + if output_path + .components() + .any(|c| c == std::path::Component::ParentDir) + { + return Err(GwsError::Validation(format!( + "{flag_name} must not contain path traversal ('..'): {}", + path + ))); + } + + let cwd = std::env::current_dir() + .map_err(|e| GwsError::Validation(format!("Failed to determine current directory: {e}")))?; + let resolved = cwd.join(output_path); + let canonical = if std::fs::symlink_metadata(&resolved).is_ok() { + resolved.canonicalize().map_err(|e| { + GwsError::Validation(format!("Failed to resolve {flag_name} '{}': {e}", path)) + })? + } else { + normalize_non_existing(&resolved)? + }; + + let canonical_cwd = cwd.canonicalize().map_err(|e| { + GwsError::Validation(format!("Failed to canonicalize current directory: {e}")) + })?; + if !canonical.starts_with(&canonical_cwd) { + return Err(GwsError::Validation(format!( + "{flag_name} '{}' resolves to '{}' which is outside the current directory", + path, + canonical.display() + ))); + } + + if canonical.exists() && !canonical.is_file() { + return Err(GwsError::Validation(format!( + "{flag_name} '{}' must reference a regular file path", + path + ))); + } + + Ok(canonical) +} + /// Rejects strings containing null bytes or ASCII control characters /// (including DEL, 0x7F). fn reject_control_chars(value: &str, flag_name: &str) -> Result<(), GwsError> { @@ -129,8 +554,8 @@ fn reject_control_chars(value: &str, flag_name: &str) -> Result<(), GwsError> { Ok(()) } -/// Resolves a path that may not exist yet by canonicalizing the existing -/// prefix and appending remaining components. +/// Resolves a path that may not exist yet by canonicalizing the longest +/// existing prefix (including symlinks) and appending the remaining components. fn normalize_non_existing(path: &Path) -> Result { let mut resolved = PathBuf::new(); let mut remaining = Vec::new(); @@ -138,10 +563,13 @@ fn normalize_non_existing(path: &Path) -> Result { // Walk backwards until we find a component that exists let mut current = path.to_path_buf(); loop { - if current.exists() { - resolved = current - .canonicalize() - .map_err(|e| GwsError::Validation(format!("Failed to canonicalize path: {e}")))?; + if std::fs::symlink_metadata(¤t).is_ok() { + resolved = current.canonicalize().map_err(|e| { + GwsError::Validation(format!( + "Failed to canonicalize path '{}': {e}", + current.display() + )) + })?; break; } if let Some(name) = current.file_name() { @@ -372,6 +800,232 @@ mod tests { assert!(validate_safe_dir_path("/usr/local").is_err()); } + // --- validate_safe_input_file_path --- + + #[test] + #[serial] + fn test_input_file_path_valid_relative_file() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + fs::create_dir_all(canonical_dir.join("sub")).unwrap(); + fs::write(canonical_dir.join("sub").join("file.txt"), b"ok").unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + let result = validate_safe_input_file_path("sub/file.txt"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_ok(), "expected Ok, got: {result:?}"); + } + + #[test] + fn test_input_file_path_rejects_absolute() { + assert!(validate_safe_input_file_path("/tmp/evil").is_err()); + } + + #[test] + #[serial] + fn test_input_file_path_rejects_traversal() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + let result = validate_safe_input_file_path("../secret.txt"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("path traversal")); + } + + #[test] + #[serial] + fn test_input_file_path_rejects_symlink_escape() { + let cwd_dir = tempdir().unwrap(); + let outside_dir = tempdir().unwrap(); + let canonical_cwd = cwd_dir.path().canonicalize().unwrap(); + let canonical_outside = outside_dir.path().canonicalize().unwrap(); + fs::write(canonical_outside.join("secret.txt"), b"secret").unwrap(); + + #[cfg(unix)] + std::os::unix::fs::symlink(&canonical_outside, canonical_cwd.join("sneaky")).unwrap(); + #[cfg(windows)] + return; + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_cwd).unwrap(); + let result = validate_safe_input_file_path("sneaky/secret.txt"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("outside the current directory")); + } + + #[test] + #[serial] + fn test_read_safe_upload_file_reads_valid_file() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + fs::write(canonical_dir.join("input.txt"), b"hello").unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + let result = read_safe_upload_file("input.txt"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert_eq!(result.unwrap(), b"hello"); + } + + // --- validate_safe_output_file_path --- + + #[test] + #[serial] + fn test_output_file_path_valid_nested_under_cwd() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + let result = validate_safe_output_file_path("downloads/out.bin"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_ok(), "expected Ok, got: {result:?}"); + } + + #[test] + fn test_output_file_path_rejects_absolute() { + assert!(validate_safe_output_file_path("/tmp/out.bin").is_err()); + } + + #[test] + #[serial] + fn test_output_file_path_rejects_traversal() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + let result = validate_safe_output_file_path("../out.bin"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("path traversal")); + } + + #[test] + #[serial] + fn test_output_file_path_rejects_symlink_escape_parent() { + let cwd_dir = tempdir().unwrap(); + let outside_dir = tempdir().unwrap(); + let canonical_cwd = cwd_dir.path().canonicalize().unwrap(); + let canonical_outside = outside_dir.path().canonicalize().unwrap(); + + #[cfg(unix)] + std::os::unix::fs::symlink(&canonical_outside, canonical_cwd.join("sneaky_out")).unwrap(); + #[cfg(windows)] + return; + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_cwd).unwrap(); + let result = validate_safe_output_file_path("sneaky_out/out.bin"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("outside the current directory")); + } + + #[test] + #[serial] + fn test_output_file_path_rejects_broken_symlink_prefix() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + + #[cfg(unix)] + std::os::unix::fs::symlink( + "/tmp/definitely-missing-target", + canonical_dir.join("broken"), + ) + .unwrap(); + #[cfg(windows)] + return; + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + let result = validate_safe_output_file_path("broken/out.bin"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_err()); + } + + #[test] + #[serial] + #[cfg(unix)] + fn test_output_file_path_rejects_fifo_target() { + use std::ffi::CString; + use std::os::unix::ffi::OsStrExt; + + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + let fifo = canonical_dir.join("out.fifo"); + let fifo_c = CString::new(fifo.as_os_str().as_bytes()).unwrap(); + + let status = unsafe { libc::mkfifo(fifo_c.as_ptr(), 0o600) }; + assert_eq!( + status, + 0, + "mkfifo failed: {}", + std::io::Error::last_os_error() + ); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + let result = validate_safe_output_file_path("out.fifo"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_err()); + } + + #[test] + #[serial] + fn test_open_safe_output_file_creates_new_file() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + let (mut file, path) = open_safe_output_file("created.bin").unwrap(); + std::io::Write::write_all(&mut file, b"abc").unwrap(); + drop(file); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(path.ends_with("created.bin")); + assert_eq!(fs::read(path).unwrap(), b"abc"); + } + + #[test] + #[serial] + fn test_open_safe_output_file_truncates_existing_file() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + fs::write(canonical_dir.join("existing.bin"), b"old").unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + let (mut file, path) = open_safe_output_file("existing.bin").unwrap(); + std::io::Write::write_all(&mut file, b"new").unwrap(); + drop(file); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(path.ends_with("existing.bin")); + assert_eq!(fs::read(path).unwrap(), b"new"); + } + // --- reject_control_chars --- #[test]