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

.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

skills/gws-calendar-insert/SKILL.md

@@ -33,18 +33,20 @@ gws calendar +insert --summary <TEXT> --start <TIME> --end <TIME>
 | `--location` | — | — | Event location |
 | `--description` | — | — | Event description/body |
 | `--attendee` | — | — | Attendee email (can be used multiple times) |
+| `--meet` | — | — | Add a Google Meet video conference link |
 
 ## Examples
 
 ```bash
 gws calendar +insert --summary 'Standup' --start '2026-06-17T09:00:00-07:00' --end '2026-06-17T09:30:00-07:00'
 gws calendar +insert --summary 'Review' --start ... --end ... --attendee alice@example.com
+gws calendar +insert --summary 'Meet' --start ... --end ... --meet
 ```
 
 ## Tips
 
 - Use RFC3339 format for times (e.g. 2026-06-17T09:00:00-07:00).
-- For recurring events or conference links, use the raw API instead.
+- The --meet flag automatically adds a Google Meet link to the event.
 
 > [!CAUTION]
 > This is a **write** command — confirm with the user before executing.

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);
+    }
 }

src/executor.rs

@@ -798,30 +798,45 @@ 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 {
+    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();

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

src/validate.rs

@@ -137,6 +137,51 @@ 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.
+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, 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");
+        }
+    }
 }