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..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,13 +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 + .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; } @@ -48,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::*; @@ -56,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 49101ece..51960009 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.is_control()) + .collect(); // Build multipart/related body let metadata_json = metadata @@ -1218,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 {