From 3ed03374f50cb3efc9bcaa32c2d59ae84d6fa40c Mon Sep 17 00:00:00 2001 From: TheSecMaven Date: Sun, 23 Aug 2026 12:09:48 -0500 Subject: [PATCH] fix(executor): short-circuit 204 No Content before binary response routing A 204 No Content response that still carries a non-empty Content-Type header (text/html on calendar.events.delete and drive.files.delete) was being routed to the binary-file handler purely on that header, which wrote a stray zero-byte download.html into the caller's cwd and printed a spurious success status for commands with no --output flag and nothing to do with downloading. Introduce classify_response_body(status, content_type) so a 204 is always treated as having no body to read or write, independent of whatever Content-Type the endpoint happens to attach to it. --- .changeset/fix-204-no-content-misrouting.md | 15 ++ crates/google-workspace-cli/src/executor.rs | 163 ++++++++++++++++---- 2 files changed, 145 insertions(+), 33 deletions(-) create mode 100644 .changeset/fix-204-no-content-misrouting.md diff --git a/.changeset/fix-204-no-content-misrouting.md b/.changeset/fix-204-no-content-misrouting.md new file mode 100644 index 000000000..7a66c4173 --- /dev/null +++ b/.changeset/fix-204-no-content-misrouting.md @@ -0,0 +1,15 @@ +--- +"@googleworkspace/cli": patch +--- + +Fix HTTP 204 No Content responses being mis-routed to the binary-file download path. + +Some endpoints (observed on `calendar.events.delete` and `drive.files.delete`) return an +empty 204 body that still carries a non-empty `Content-Type` header such as `text/html`. +Response routing decided JSON-vs-binary purely from that header, so these 204s fell +through to the binary handler, which created a zero-byte `download.html` in the current +working directory and printed a spurious "success" status — even for commands that never +took a `--output` flag and have nothing to do with downloading a file. + +Response handling now classifies on status first: a 204 is always treated as having no +body, regardless of `Content-Type`, so no file is read or written for it. diff --git a/crates/google-workspace-cli/src/executor.rs b/crates/google-workspace-cli/src/executor.rs index 46f31ac4b..3a5fb6f2c 100644 --- a/crates/google-workspace-cli/src/executor.rs +++ b/crates/google-workspace-cli/src/executor.rs @@ -336,6 +336,37 @@ async fn handle_json_response( Ok(false) } +/// How the body of an API response should be handled. +#[derive(Debug, PartialEq, Eq)] +enum ResponseBodyKind { + /// No body was sent (HTTP 204 No Content). Some endpoints — observed on + /// `calendar.events.delete` and `drive.files.delete` — still attach a + /// non-empty `Content-Type` header (commonly `text/html`) to an otherwise + /// empty 204 response. That header must never be used to route the + /// (nonexistent) body into the binary-file path, or a stray zero-byte + /// `download.html` gets written into the caller's working directory. + NoContent, + Json, + Binary, +} + +/// Decides how to handle a response body from its status and `Content-Type`, +/// independent of any actual bytes. Kept pure and separate from +/// `execute_method` so the 204-vs-content-type interaction can be unit +/// tested without a live HTTP round trip. +fn classify_response_body(status: reqwest::StatusCode, content_type: &str) -> ResponseBodyKind { + if status == reqwest::StatusCode::NO_CONTENT { + return ResponseBodyKind::NoContent; + } + + let is_json = content_type.contains("application/json") || content_type.contains("text/json"); + if is_json || content_type.is_empty() { + ResponseBodyKind::Json + } else { + ResponseBodyKind::Binary + } +} + /// Handle a binary response by streaming it to a file. async fn handle_binary_response( response: reqwest::Response, @@ -486,41 +517,48 @@ pub async fn execute_method( "API request" ); - let is_json = - content_type.contains("application/json") || content_type.contains("text/json"); - - if is_json || content_type.is_empty() { - let body_text = response - .text() - .await - .context("Failed to read response body")?; - - let should_continue = handle_json_response( - &body_text, - pagination, - sanitize_template, - sanitize_mode, - output_format, - &mut pages_fetched, - &mut page_token, - capture_output, - &mut captured_values, - ) - .await?; + match classify_response_body(status, &content_type) { + ResponseBodyKind::NoContent => { + // Nothing was sent and nothing should be read or written — + // do not fall through to the binary-file path just because + // this 204 happened to carry a non-empty Content-Type. + } + ResponseBodyKind::Json => { + let body_text = response + .text() + .await + .context("Failed to read response body")?; + + let should_continue = handle_json_response( + &body_text, + pagination, + sanitize_template, + sanitize_mode, + output_format, + &mut pages_fetched, + &mut page_token, + capture_output, + &mut captured_values, + ) + .await?; - if should_continue { - continue; + if should_continue { + continue; + } + } + ResponseBodyKind::Binary => { + if let Some(res) = handle_binary_response( + response, + &content_type, + output_path, + output_format, + capture_output, + ) + .await? + { + captured_values.push(res); + } } - } else if let Some(res) = handle_binary_response( - response, - &content_type, - output_path, - output_format, - capture_output, - ) - .await? - { - captured_values.push(res); } break; @@ -1202,6 +1240,65 @@ mod tests { assert_eq!(config.page_delay_ms, 100); } + #[test] + fn test_classify_response_body_204_is_never_binary_even_with_binary_content_type() { + // Reproduces `calendar events delete` / `drive files delete`: a 204 No + // Content response that still carries a `text/html` Content-Type header. + // Before this fix, this combination fell through to the binary-file + // path and wrote a stray, zero-byte `download.html` into the caller's + // working directory even though no `--output` flag was given and the + // command was not a download at all. + assert_eq!( + classify_response_body(reqwest::StatusCode::NO_CONTENT, "text/html"), + ResponseBodyKind::NoContent + ); + } + + #[test] + fn test_classify_response_body_204_with_no_content_type() { + assert_eq!( + classify_response_body(reqwest::StatusCode::NO_CONTENT, ""), + ResponseBodyKind::NoContent + ); + } + + #[test] + fn test_classify_response_body_204_with_json_content_type() { + // A 204 is still "no content" even if some endpoint bafflingly labels + // it application/json — the status code always wins. + assert_eq!( + classify_response_body(reqwest::StatusCode::NO_CONTENT, "application/json"), + ResponseBodyKind::NoContent + ); + } + + #[test] + fn test_classify_response_body_200_json() { + assert_eq!( + classify_response_body(reqwest::StatusCode::OK, "application/json; charset=utf-8"), + ResponseBodyKind::Json + ); + } + + #[test] + fn test_classify_response_body_200_empty_content_type_is_json() { + assert_eq!( + classify_response_body(reqwest::StatusCode::OK, ""), + ResponseBodyKind::Json + ); + } + + #[test] + fn test_classify_response_body_200_binary() { + assert_eq!( + classify_response_body( + reqwest::StatusCode::OK, + "application/vnd.openxmlformats-officedocument.presentationml.presentation" + ), + ResponseBodyKind::Binary + ); + } + #[test] fn test_auth_method_equality() { assert_eq!(AuthMethod::OAuth, AuthMethod::OAuth);