Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-output-json-error.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@googleworkspace/cli": patch
---

Return an error when `--output` is used with JSON responses instead of silently succeeding without writing a file.
77 changes: 77 additions & 0 deletions crates/google-workspace-cli/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ async fn build_http_request(
#[allow(clippy::too_many_arguments)]
async fn handle_json_response(
body_text: &str,
output_path: Option<&str>,
pagination: &PaginationConfig,
sanitize_template: Option<&str>,
sanitize_mode: &crate::helpers::modelarmor::SanitizeMode,
Expand All @@ -253,6 +254,12 @@ async fn handle_json_response(
captured: &mut Vec<Value>,
) -> Result<bool, GwsError> {
if let Ok(mut json_val) = serde_json::from_str::<Value>(body_text) {
if output_path.is_some() {
return Err(GwsError::Validation(
"--output only supports raw binary responses; this endpoint returned JSON instead. Use jq/base64 to extract the payload or rerun without --output.".to_string(),
));
}
Comment on lines +257 to +261
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This check occurs after the entire response body has been read into memory and parsed as JSON. This is inefficient for large JSON responses when the Content-Type header already indicates it's JSON.

Furthermore, if the response is NOT valid JSON (e.g., plain text or binary with a missing Content-Type), the function falls through to the else block (line 336) and prints to stdout. If --output was specified, this results in the output being dumped to the terminal instead of the file, which is likely not what the user intended.

Consider refactoring execute_method to fail early if is_json && output_path.is_some(), and to use handle_binary_response if content_type.is_empty() && output_path.is_some(). Please ensure these changes are focused on the primary goal to avoid scope creep, and acknowledge potential TOCTOU race conditions in file path validation as a known limitation.

References
  1. Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
  2. When implementing file path validation, acknowledge and document potential Time-of-check to time-of-use (TOCTOU) race conditions as a known limitation if a full mitigation (e.g., using openat(O_NOFOLLOW)) is considered out of scope.


*pages_fetched += 1;

// Run Model Armor sanitization if --sanitize is enabled
Expand Down Expand Up @@ -497,6 +504,7 @@ pub async fn execute_method(

let should_continue = handle_json_response(
&body_text,
output_path,
pagination,
sanitize_template,
sanitize_mode,
Expand Down Expand Up @@ -2150,6 +2158,75 @@ async fn test_execute_method_missing_path_param() {
.contains("Required path parameter"));
}

#[cfg(test)]
fn spawn_single_response_server(body: &'static str, content_type: &'static str) -> String {
let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
let addr = listener.local_addr().unwrap();
let body = body.to_string();
let content_type = content_type.to_string();

std::thread::spawn(move || {
let (mut stream, _) = listener.accept().unwrap();
let mut request_buf = [0_u8; 1024];
let _ = std::io::Read::read(&mut stream, &mut request_buf);

let response = format!(
"HTTP/1.1 200 OK\r\nContent-Type: {content_type}\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}",
body.len()
);
std::io::Write::write_all(&mut stream, response.as_bytes()).unwrap();
});

format!("http://{addr}/")
}

#[cfg(test)]
#[tokio::test]
async fn test_execute_method_errors_when_output_targets_json_response() {
let root_url =
spawn_single_response_server(r#"{"data":"aGVsbG8","size":5}"#, "application/json");
let tempdir = tempfile::tempdir().unwrap();
let output_path = tempdir.path().join("attachment.bin");

let doc = RestDescription {
root_url,
service_path: "".to_string(),
..Default::default()
};
let method = RestMethod {
http_method: "GET".to_string(),
id: Some("gmail.users.messages.attachments.get".to_string()),
path: "users/me/messages/1/attachments/2".to_string(),
..Default::default()
};

let sanitize_mode = crate::helpers::modelarmor::SanitizeMode::Warn;
let result = execute_method(
&doc,
&method,
None,
None,
None,
AuthMethod::None,
Some(output_path.to_str().unwrap()),
None,
false,
&PaginationConfig::default(),
None,
&sanitize_mode,
&crate::formatter::OutputFormat::default(),
false,
)
.await;

assert!(result.is_err());
assert!(result
.unwrap_err()
.to_string()
.contains("--output only supports raw binary responses"));
assert!(!output_path.exists());
}

#[test]
fn test_handle_error_response_non_json() {
let err = handle_error_response::<()>(
Expand Down
Loading