From 12f4e0745e30912484fe9ae94cab5af0db58d30d Mon Sep 17 00:00:00 2001 From: Anshul Garg Date: Fri, 13 Mar 2026 03:01:43 +0530 Subject: [PATCH 1/6] fix(security): cap Retry-After to 60s and sanitize mimeType header Two security fixes: 1. Cap Retry-After header value to 60 seconds. A hostile or compromised API server could send Retry-After: 4294967295 to hang the CLI process indefinitely. This is especially dangerous for AI agent workflows where the process runs unattended. 2. Strip CR/LF from user-supplied mimeType before embedding it in the multipart MIME header. A mimeType like "text/plain\r\nX-Evil: injected" could inject arbitrary MIME headers into the upload body. --- .../fix-retry-after-and-mime-injection.md | 5 +++++ src/client.rs | 20 +++++++++++++++++-- src/executor.rs | 9 +++++++-- 3 files changed, 30 insertions(+), 4 deletions(-) create mode 100644 .changeset/fix-retry-after-and-mime-injection.md diff --git a/.changeset/fix-retry-after-and-mime-injection.md b/.changeset/fix-retry-after-and-mime-injection.md new file mode 100644 index 00000000..49fe7536 --- /dev/null +++ b/.changeset/fix-retry-after-and-mime-injection.md @@ -0,0 +1,5 @@ +--- +"@googleworkspace/cli": patch +--- + +fix(security): cap Retry-After sleep to 60s and sanitize mimeType in multipart uploads diff --git a/src/client.rs b/src/client.rs index eb838852..c30c5627 100644 --- a/src/client.rs +++ b/src/client.rs @@ -33,13 +33,17 @@ pub async fn send_with_retry( return Ok(resp); } - // Parse Retry-After header (seconds), fall back to exponential backoff + // Parse Retry-After header (seconds), fall back to exponential backoff. + // Cap at 60 seconds to prevent a hostile server from hanging the process + // indefinitely — especially important when the CLI is invoked by AI agents. + const MAX_RETRY_DELAY_SECS: u64 = 60; let retry_after = 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 + .unwrap_or(1 << attempt) // 1, 2, 4 seconds + .min(MAX_RETRY_DELAY_SECS); tokio::time::sleep(std::time::Duration::from_secs(retry_after)).await; } @@ -56,4 +60,16 @@ mod tests { fn build_client_succeeds() { assert!(build_client().is_ok()); } + + #[test] + fn retry_after_cap_prevents_unbounded_sleep() { + // Verify the cap constant is reasonable + const MAX_RETRY_DELAY_SECS: u64 = 60; + // A server sending Retry-After: 999999 should be capped to 60 + let server_value: u64 = 999_999; + let capped = server_value.min(MAX_RETRY_DELAY_SECS); + assert_eq!(capped, 60); + // Normal values below the cap pass through unchanged + assert_eq!(5u64.min(MAX_RETRY_DELAY_SECS), 5); + } } diff --git a/src/executor.rs b/src/executor.rs index 49101ece..2362af3a 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -737,12 +737,17 @@ fn build_multipart_body( ) -> Result<(Vec, String), GwsError> { let boundary = format!("gws_boundary_{:016x}", rand::random::()); - // Determine the media MIME type from the metadata's mimeType field, or fall back - let media_mime = metadata + // Determine the media MIME type from the metadata's mimeType field, or fall back. + // Strip CR/LF to prevent MIME header injection via user-controlled mimeType. + let media_mime_raw = metadata .as_ref() .and_then(|m| m.get("mimeType")) .and_then(|v| v.as_str()) .unwrap_or("application/octet-stream"); + let media_mime: String = media_mime_raw + .chars() + .filter(|c| *c != '\r' && *c != '\n') + .collect(); // Build multipart/related body let metadata_json = metadata From 2a819845382f0c5e170e8ee49ce484d0e6477516 Mon Sep 17 00:00:00 2001 From: Anshul Garg Date: Fri, 13 Mar 2026 12:17:44 +0530 Subject: [PATCH 2/6] refactor: move MAX_RETRY_DELAY_SECS to module level Extract the constant from inside the function body to module level so it's shared between production code and tests. Use the constant in the test assertion instead of the magic number 60. --- src/client.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/client.rs b/src/client.rs index c30c5627..45f9c800 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). @@ -34,9 +37,7 @@ pub async fn send_with_retry( } // Parse Retry-After header (seconds), fall back to exponential backoff. - // Cap at 60 seconds to prevent a hostile server from hanging the process - // indefinitely — especially important when the CLI is invoked by AI agents. - const MAX_RETRY_DELAY_SECS: u64 = 60; + // Cap to MAX_RETRY_DELAY_SECS to prevent indefinite hangs. let retry_after = resp .headers() .get("retry-after") @@ -63,12 +64,10 @@ mod tests { #[test] fn retry_after_cap_prevents_unbounded_sleep() { - // Verify the cap constant is reasonable - const MAX_RETRY_DELAY_SECS: u64 = 60; - // A server sending Retry-After: 999999 should be capped to 60 + // A server sending Retry-After: 999999 should be capped let server_value: u64 = 999_999; let capped = server_value.min(MAX_RETRY_DELAY_SECS); - assert_eq!(capped, 60); + assert_eq!(capped, MAX_RETRY_DELAY_SECS); // Normal values below the cap pass through unchanged assert_eq!(5u64.min(MAX_RETRY_DELAY_SECS), 5); } From a4cd503e2fb63377f7ad738fb6c8750b5c3f4ed3 Mon Sep 17 00:00:00 2001 From: Anshul Garg Date: Fri, 13 Mar 2026 12:26:59 +0530 Subject: [PATCH 3/6] refactor: extract compute_retry_delay, add comprehensive tests Address Gemini review feedback: - Extract retry delay logic into a testable compute_retry_delay() function with 5 focused unit tests covering: large values, small values, missing header, unparseable header, and boundary values - Add unit test for mimeType CRLF sanitization in multipart uploads to verify header injection prevention All 590 tests pass locally. --- src/client.rs | 52 ++++++++++++++++++++++++++++++++++++------------- src/executor.rs | 25 ++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 13 deletions(-) diff --git a/src/client.rs b/src/client.rs index 45f9c800..8505f89e 100644 --- a/src/client.rs +++ b/src/client.rs @@ -36,15 +36,12 @@ pub async fn send_with_retry( return Ok(resp); } - // Parse Retry-After header (seconds), fall back to exponential backoff. - // Cap to MAX_RETRY_DELAY_SECS to prevent indefinite hangs. - 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 - .min(MAX_RETRY_DELAY_SECS); + .map(|s| s.to_string()); + let retry_after = compute_retry_delay(header_value.as_deref(), attempt); tokio::time::sleep(std::time::Duration::from_secs(retry_after)).await; } @@ -53,6 +50,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(1 << attempt) + .min(MAX_RETRY_DELAY_SECS) +} + #[cfg(test)] mod tests { use super::*; @@ -63,12 +70,31 @@ mod tests { } #[test] - fn retry_after_cap_prevents_unbounded_sleep() { - // A server sending Retry-After: 999999 should be capped - let server_value: u64 = 999_999; - let capped = server_value.min(MAX_RETRY_DELAY_SECS); - assert_eq!(capped, MAX_RETRY_DELAY_SECS); - // Normal values below the cap pass through unchanged - assert_eq!(5u64.min(MAX_RETRY_DELAY_SECS), 5); + 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 2362af3a..070304ef 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -1223,6 +1223,31 @@ mod tests { assert!(body_str.contains("Binary data")); } + #[tokio::test] + async fn test_build_multipart_body_sanitizes_mime_injection() { + // A malicious mimeType with CRLF should be stripped to prevent + // MIME header injection in the multipart body. + let metadata = Some(json!({ + "name": "evil.txt", + "mimeType": "text/plain\r\nX-Injected: malicious" + })); + let content = b"payload"; + + let (body, _) = build_multipart_body(&metadata, content).unwrap(); + let body_str = String::from_utf8(body).unwrap(); + + // After stripping CR/LF, the Content-Type line should NOT have a + // line break that would create a separate injected header. + // The CRLF between "text/plain" and "X-Injected" is removed, + // so the value is concatenated into a single line. + assert!( + !body_str.contains("Content-Type: text/plain\r\nX-Injected"), + "CRLF injection must not produce a separate header line" + ); + // Verify the sanitized mime type appears as one continuous string + assert!(body_str.contains("Content-Type: text/plainX-Injected: malicious")); + } + #[test] fn test_build_url_basic() { let doc = RestDescription { From c2fe2e6e2b7f9ea243591773e1034a211f2d44f9 Mon Sep 17 00:00:00 2001 From: Anshul Garg Date: Fri, 13 Mar 2026 12:36:22 +0530 Subject: [PATCH 4/6] fix: use u64 literal for shift and strip all control chars from mimeType - Use 1u64 << attempt to prevent potential i32 overflow on large attempt values - Strip all control characters (not just CR/LF) from mimeType to prevent any control char injection in MIME headers --- src/client.rs | 2 +- src/executor.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client.rs b/src/client.rs index 8505f89e..272ecac2 100644 --- a/src/client.rs +++ b/src/client.rs @@ -56,7 +56,7 @@ pub async fn send_with_retry( fn compute_retry_delay(header_value: Option<&str>, attempt: u32) -> u64 { header_value .and_then(|s| s.parse::().ok()) - .unwrap_or(1 << attempt) + .unwrap_or(1u64 << attempt) .min(MAX_RETRY_DELAY_SECS) } diff --git a/src/executor.rs b/src/executor.rs index 070304ef..51960009 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -746,7 +746,7 @@ fn build_multipart_body( .unwrap_or("application/octet-stream"); let media_mime: String = media_mime_raw .chars() - .filter(|c| *c != '\r' && *c != '\n') + .filter(|c| !c.is_control()) .collect(); // Build multipart/related body From e3a5f9c073301edefbbf2754e9d63a5e2ce6f2ba Mon Sep 17 00:00:00 2001 From: Anshul Garg Date: Fri, 13 Mar 2026 12:41:02 +0530 Subject: [PATCH 5/6] refactor: avoid unnecessary String allocation in retry header parsing Pass the &str directly from HeaderValue::to_str() to compute_retry_delay instead of cloning into an owned String. --- src/client.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/client.rs b/src/client.rs index 272ecac2..9dc17541 100644 --- a/src/client.rs +++ b/src/client.rs @@ -39,9 +39,8 @@ pub async fn send_with_retry( let header_value = resp .headers() .get("retry-after") - .and_then(|v| v.to_str().ok()) - .map(|s| s.to_string()); - let retry_after = compute_retry_delay(header_value.as_deref(), attempt); + .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; } From 5fd6c34d8fbdfa8461668c06da8ba534589f64fa Mon Sep 17 00:00:00 2001 From: Anshul Garg Date: Fri, 13 Mar 2026 12:51:09 +0530 Subject: [PATCH 6/6] fix: use saturating_pow for overflow-safe exponential backoff Replace 1u64 << attempt with 2u64.saturating_pow(attempt) to prevent panic if MAX_RETRIES is ever increased beyond 63. --- src/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client.rs b/src/client.rs index 9dc17541..e0abe409 100644 --- a/src/client.rs +++ b/src/client.rs @@ -55,7 +55,7 @@ pub async fn send_with_retry( fn compute_retry_delay(header_value: Option<&str>, attempt: u32) -> u64 { header_value .and_then(|s| s.parse::().ok()) - .unwrap_or(1u64 << attempt) + .unwrap_or(2u64.saturating_pow(attempt)) .min(MAX_RETRY_DELAY_SECS) }