From 8abe347651b14d880ae9c15e2b289f91b56d8abf Mon Sep 17 00:00:00 2001 From: jpoehnelt-bot Date: Tue, 17 Mar 2026 12:20:17 -0600 Subject: [PATCH 1/4] fix(security): cap Retry-After sleep, sanitize upload mimeType, and validate --upload/--output paths - Cap Retry-After header at 60s to prevent hostile servers from hanging the CLI - Extract compute_retry_delay() with saturating_pow for safe exponential backoff - Sanitize mimeType by stripping control characters to prevent MIME header injection - Add validate_safe_file_path() for --upload and --output path validation - Gate --upload/--output through path validation in main.rs before any I/O Consolidates security fixes from PRs #448 and #447. --- .changeset/security-hardening-rollup.md | 5 ++ src/client.rs | 57 +++++++++--- src/executor.rs | 61 ++++++++++--- src/main.rs | 8 ++ src/validate.rs | 110 ++++++++++++++++++++++++ 5 files changed, 217 insertions(+), 24 deletions(-) create mode 100644 .changeset/security-hardening-rollup.md diff --git a/.changeset/security-hardening-rollup.md b/.changeset/security-hardening-rollup.md new file mode 100644 index 00000000..65a92fec --- /dev/null +++ b/.changeset/security-hardening-rollup.md @@ -0,0 +1,5 @@ +--- +"@googleworkspace/cli": patch +--- + +fix(security): cap Retry-After sleep, sanitize upload mimeType, and validate --upload/--output paths diff --git a/src/client.rs b/src/client.rs index 6c08318d..e0abe409 100644 --- a/src/client.rs +++ b/src/client.rs @@ -20,6 +20,9 @@ pub fn build_client() -> Result { } const MAX_RETRIES: u32 = 3; +/// Maximum seconds to sleep on a 429 Retry-After header. Prevents a hostile +/// or misconfigured server from hanging the process indefinitely. +const MAX_RETRY_DELAY_SECS: u64 = 60; /// Send an HTTP request with automatic retry on 429 (rate limit) responses. /// Respects the `Retry-After` header; falls back to exponential backoff (1s, 2s, 4s). @@ -33,20 +36,11 @@ pub async fn send_with_retry( return Ok(resp); } - // Parse Retry-After header (seconds), fall back to exponential backoff - let retry_after = resp + let header_value = resp .headers() .get("retry-after") - .and_then(|v| v.to_str().ok()) - .and_then(|s| s.parse::().ok()) - .unwrap_or(1 << attempt); // 1, 2, 4 seconds - - tracing::debug!( - attempt = attempt + 1, - max_retries = MAX_RETRIES, - retry_after_secs = retry_after, - "Rate limited, retrying" - ); + .and_then(|v| v.to_str().ok()); + let retry_after = compute_retry_delay(header_value, attempt); tokio::time::sleep(std::time::Duration::from_secs(retry_after)).await; } @@ -55,6 +49,16 @@ pub async fn send_with_retry( build_request().send().await } +/// Compute the retry delay from a Retry-After header value and attempt number. +/// Falls back to exponential backoff (1, 2, 4s) when the header is absent or +/// unparseable. Always caps the result at MAX_RETRY_DELAY_SECS. +fn compute_retry_delay(header_value: Option<&str>, attempt: u32) -> u64 { + header_value + .and_then(|s| s.parse::().ok()) + .unwrap_or(2u64.saturating_pow(attempt)) + .min(MAX_RETRY_DELAY_SECS) +} + #[cfg(test)] mod tests { use super::*; @@ -63,4 +67,33 @@ mod tests { fn build_client_succeeds() { assert!(build_client().is_ok()); } + + #[test] + fn retry_delay_caps_large_header_value() { + assert_eq!(compute_retry_delay(Some("999999"), 0), MAX_RETRY_DELAY_SECS); + } + + #[test] + fn retry_delay_passes_through_small_header_value() { + assert_eq!(compute_retry_delay(Some("5"), 0), 5); + } + + #[test] + fn retry_delay_falls_back_to_exponential_on_missing_header() { + assert_eq!(compute_retry_delay(None, 0), 1); // 2^0 + assert_eq!(compute_retry_delay(None, 1), 2); // 2^1 + assert_eq!(compute_retry_delay(None, 2), 4); // 2^2 + } + + #[test] + fn retry_delay_falls_back_on_unparseable_header() { + assert_eq!(compute_retry_delay(Some("not-a-number"), 1), 2); + assert_eq!(compute_retry_delay(Some(""), 0), 1); + } + + #[test] + fn retry_delay_caps_at_boundary() { + assert_eq!(compute_retry_delay(Some("60"), 0), 60); + assert_eq!(compute_retry_delay(Some("61"), 0), MAX_RETRY_DELAY_SECS); + } } diff --git a/src/executor.rs b/src/executor.rs index 73fd772f..2423e8b9 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -798,30 +798,45 @@ fn handle_error_response( /// represents the *source* type (what the bytes are). When a user uploads /// `notes.md` with `"mimeType":"application/vnd.google-apps.document"`, the /// media part should be `text/markdown`, not a Google Workspace MIME type. +/// +/// All returned MIME types have control characters stripped to prevent +/// MIME header injection via user-controlled metadata. fn resolve_upload_mime( explicit: Option<&str>, upload_path: Option<&str>, metadata: &Option, ) -> String { - if let Some(mime) = explicit { - return mime.to_string(); - } - - if let Some(path) = upload_path { + let raw = if let Some(mime) = explicit { + mime.to_string() + } else if let Some(path) = upload_path { if let Some(detected) = mime_from_extension(path) { - return detected.to_string(); + detected.to_string() + } else if let Some(mime) = metadata + .as_ref() + .and_then(|m| m.get("mimeType")) + .and_then(|v| v.as_str()) + { + mime.to_string() + } else { + return "application/octet-stream".to_string(); } - } - - if let Some(mime) = metadata + } else if let Some(mime) = metadata .as_ref() .and_then(|m| m.get("mimeType")) .and_then(|v| v.as_str()) { - return mime.to_string(); - } + mime.to_string() + } else { + return "application/octet-stream".to_string(); + }; - "application/octet-stream".to_string() + // Strip CR/LF and other control characters to prevent MIME header injection. + let sanitized: String = raw.chars().filter(|c| !c.is_control()).collect(); + if sanitized.is_empty() { + "application/octet-stream".to_string() + } else { + sanitized + } } /// Infers a MIME type from a file path's extension. @@ -1459,6 +1474,28 @@ mod tests { ); } + #[test] + fn test_resolve_upload_mime_sanitizes_crlf_injection() { + // A malicious mimeType with CRLF should be stripped to prevent + // MIME header injection in the multipart body. + let metadata = Some(json!({ + "mimeType": "text/plain\r\nX-Injected: malicious" + })); + let mime = resolve_upload_mime(None, None, &metadata); + assert!( + !mime.contains('\r') && !mime.contains('\n'), + "control characters must be stripped: got '{mime}'" + ); + assert_eq!(mime, "text/plainX-Injected: malicious"); + } + + #[test] + fn test_resolve_upload_mime_all_control_chars_fallback() { + let metadata = Some(json!({ "mimeType": "\r\n\t" })); + let mime = resolve_upload_mime(None, None, &metadata); + assert_eq!(mime, "application/octet-stream"); + } + #[tokio::test] async fn test_build_multipart_stream_content_length() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/main.rs b/src/main.rs index bd72c642..c0d66274 100644 --- a/src/main.rs +++ b/src/main.rs @@ -227,6 +227,14 @@ async fn run() -> Result<(), GwsError> { .flatten() .map(|s| s.as_str()); + // Validate file paths against traversal before any I/O. + if let Some(p) = upload_path { + crate::validate::validate_safe_file_path(p, "--upload")?; + } + if let Some(p) = output_path { + crate::validate::validate_safe_file_path(p, "--output")?; + } + let dry_run = matched_args.get_flag("dry-run"); // Build pagination config from flags diff --git a/src/validate.rs b/src/validate.rs index f3ffe69c..d0384d70 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -137,6 +137,51 @@ pub fn validate_safe_dir_path(dir: &str) -> Result { Ok(canonical) } +/// Validates that a file path (e.g. `--upload` or `--output`) is safe. +/// +/// Rejects paths that escape above CWD via `..` traversal, contain +/// control characters, or follow symlinks to locations outside CWD. +/// Absolute paths are allowed (reading an existing file from a known +/// location is legitimate) but the resolved target must still live +/// under CWD. +pub fn validate_safe_file_path(path_str: &str, flag_name: &str) -> Result { + reject_control_chars(path_str, flag_name)?; + + let path = Path::new(path_str); + let cwd = std::env::current_dir() + .map_err(|e| GwsError::Validation(format!("Failed to determine current directory: {e}")))?; + + let resolved = if path.is_absolute() { + path.to_path_buf() + } else { + cwd.join(path) + }; + + // For existing files, canonicalize to resolve symlinks. + // For non-existing files, normalize the path. + let canonical = if resolved.exists() { + resolved.canonicalize().map_err(|e| { + GwsError::Validation(format!("Failed to resolve {flag_name} '{}': {e}", path_str)) + })? + } 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_str, + canonical.display() + ))); + } + + Ok(canonical) +} + /// Rejects strings containing null bytes, ASCII control characters /// (including DEL, 0x7F), or dangerous Unicode characters such as /// zero-width chars, bidi overrides, and Unicode line/paragraph separators. @@ -701,4 +746,69 @@ mod tests { fn test_validate_api_identifier_empty() { assert!(validate_api_identifier("").is_err()); } + + // --- validate_safe_file_path --- + + #[test] + #[serial] + fn test_file_path_relative_is_ok() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + fs::write(canonical_dir.join("test.txt"), "data").unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + + let result = validate_safe_file_path("test.txt", "--upload"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_ok(), "expected Ok, got: {result:?}"); + } + + #[test] + #[serial] + fn test_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_file_path("../../etc/passwd", "--upload"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_err(), "path traversal should be rejected"); + assert!( + result.unwrap_err().to_string().contains("outside"), + "error should mention 'outside'" + ); + } + + #[test] + fn test_file_path_rejects_control_chars() { + let result = validate_safe_file_path("file\x00.txt", "--output"); + assert!(result.is_err(), "null bytes should be rejected"); + } + + #[test] + #[serial] + fn test_file_path_rejects_symlink_escape() { + let dir = tempdir().unwrap(); + let canonical_dir = dir.path().canonicalize().unwrap(); + + // Create a symlink that points outside the directory + #[cfg(unix)] + { + let link_path = canonical_dir.join("escape"); + std::os::unix::fs::symlink("/tmp", &link_path).unwrap(); + + let saved_cwd = std::env::current_dir().unwrap(); + std::env::set_current_dir(&canonical_dir).unwrap(); + + let result = validate_safe_file_path("escape/secret.txt", "--output"); + std::env::set_current_dir(&saved_cwd).unwrap(); + + assert!(result.is_err(), "symlink escape should be rejected"); + } + } } From bee95aca98ea50c574f21b3041100c00834e5ad7 Mon Sep 17 00:00:00 2001 From: googleworkspace-bot Date: Tue, 17 Mar 2026 18:22:24 +0000 Subject: [PATCH 2/4] chore: regenerate skills [skip ci] --- skills/gws-calendar-insert/SKILL.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/skills/gws-calendar-insert/SKILL.md b/skills/gws-calendar-insert/SKILL.md index bd0aeb2e..62ee143a 100644 --- a/skills/gws-calendar-insert/SKILL.md +++ b/skills/gws-calendar-insert/SKILL.md @@ -33,18 +33,20 @@ gws calendar +insert --summary --start