fix(security): cap Retry-After sleep, sanitize upload mimeType, and validate --upload/--output paths (#523)

jpoehnelt · jpoehnelt-bot · googleworkspace-bot · web-flow · commit b241a5be928f · 2026-03-17T13:18:06.000-06:00
* 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. * chore: regenerate skills [skip ci] * fix: resolve mimeType sanitization bypass and document TOCTOU caveat - Restructure resolve_upload_mime() using or_else chain so all code paths go through control-char stripping (early returns were bypassing it) - Document TOCTOU limitation in validate_safe_file_path() as known caveat * fix: use canonicalized paths for I/O and normalize .. in non-existent suffix Address review comments: - main.rs: use canonicalized path from validate_safe_file_path for I/O instead of discarding it (closes TOCTOU gap) - validate.rs: add normalize_dotdot() to resolve .. components in non-existent suffix (prevents traversal via doesnt_exist/../../etc/passwd) - Add regression test for non-existent prefix traversal bypass --------- Co-authored-by: jpoehnelt-bot <jpoehnelt-bot@users.noreply.github.com> Co-authored-by: googleworkspace-bot <googleworkspace-bot@users.noreply.github.com>
diff --git a/.changeset/security-hardening-rollup.md 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
@@ -20,6 +20,9 @@ pub fn build_client() -> Result<reqwest::Client, crate::error::GwsError> {
 }
 
 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::<u64>().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::<u64>().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
@@ -798,30 +798,37 @@ fn handle_error_response<T>(
 /// 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<Value>,
 ) -> String {
-    if let Some(mime) = explicit {
-        return mime.to_string();
-    }
-
-    if let Some(path) = upload_path {
-        if let Some(detected) = mime_from_extension(path) {
-            return detected.to_string();
-        }
-    }
+    let raw = explicit
+        .map(|s| s.to_string())
+        .or_else(|| {
+            upload_path
+                .and_then(mime_from_extension)
+                .map(|s| s.to_string())
+        })
+        .or_else(|| {
+            metadata
+                .as_ref()
+                .and_then(|m| m.get("mimeType"))
+                .and_then(|v| v.as_str())
+                .map(|s| s.to_string())
+        })
+        .unwrap_or_else(|| "application/octet-stream".to_string());
 
-    if let Some(mime) = metadata
-        .as_ref()
-        .and_then(|m| m.get("mimeType"))
-        .and_then(|v| v.as_str())
-    {
-        return mime.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
     }
-
-    "application/octet-stream".to_string()
 }
 
 /// Infers a MIME type from a file path's extension.
@@ -1459,6 +1466,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
@@ -227,6 +227,22 @@ async fn run() -> Result<(), GwsError> {
         .flatten()
         .map(|s| s.as_str());
 
+    // Validate file paths against traversal before any I/O.
+    // Use the returned canonical paths so the validated path is the one
+    // actually used for I/O (closes TOCTOU gap).
+    let upload_path_buf = if let Some(p) = upload_path {
+        Some(crate::validate::validate_safe_file_path(p, "--upload")?)
+    } else {
+        None
+    };
+    let output_path_buf = if let Some(p) = output_path {
+        Some(crate::validate::validate_safe_file_path(p, "--output")?)
+    } else {
+        None
+    };
+    let upload_path = upload_path_buf.as_deref().and_then(|p| p.to_str());
+    let output_path = output_path_buf.as_deref().and_then(|p| p.to_str());
+
     let dry_run = matched_args.get_flag("dry-run");
 
     // Build pagination config from flags
diff --git a/src/validate.rs b/src/validate.rs
@@ -137,6 +137,79 @@ pub fn validate_safe_dir_path(dir: &str) -> Result<PathBuf, GwsError> {
     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.
+///
+/// # TOCTOU caveat
+///
+/// This is a best-effort defence-in-depth check. A local attacker with
+/// write access to a parent directory could replace a path component
+/// between this validation and the subsequent I/O. Fully eliminating
+/// TOCTOU would require `openat(O_NOFOLLOW)` on each path component,
+/// which is tracked as a follow-up for Unix platforms.
+pub fn validate_safe_file_path(path_str: &str, flag_name: &str) -> Result<PathBuf, GwsError> {
+    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, get the prefix canonicalized then normalize
+    // the remaining components to resolve any `..` or `.` segments.
+    let canonical = if resolved.exists() {
+        resolved.canonicalize().map_err(|e| {
+            GwsError::Validation(format!("Failed to resolve {flag_name} '{}': {e}", path_str))
+        })?
+    } else {
+        let raw = normalize_non_existing(&resolved)?;
+        // normalize_non_existing does NOT resolve `..` in the non-existent
+        // suffix. We must resolve them here to prevent bypass via paths like
+        // `non_existent/../../etc/passwd`.
+        normalize_dotdot(&raw)
+    };
+
+    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)
+}
+
+/// Resolve `.` and `..` components in a path without touching the filesystem.
+fn normalize_dotdot(path: &Path) -> PathBuf {
+    let mut out = PathBuf::new();
+    for component in path.components() {
+        match component {
+            std::path::Component::ParentDir => {
+                out.pop();
+            }
+            std::path::Component::CurDir => {}
+            c => out.push(c),
+        }
+    }
+    out
+}
+
 /// 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 +774,90 @@ 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");
+        }
+    }
+
+    #[test]
+    #[serial]
+    fn test_file_path_rejects_traversal_via_nonexistent_prefix() {
+        // Regression: non_existent/../../etc/passwd could bypass starts_with
+        // because normalize_non_existing preserves ".." in the non-existent
+        // suffix. The normalize_dotdot fix resolves this.
+        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("doesnt_exist/../../etc/passwd", "--output");
+        std::env::set_current_dir(&saved_cwd).unwrap();
+
+        assert!(
+            result.is_err(),
+            "traversal via non-existent prefix should be rejected"
+        );
+    }
 }

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"@googleworkspace/cli": patch
 +---
++
 +fix(security): cap Retry-After sleep, sanitize upload mimeType, and validate --upload/--output paths