diff --git a/.changeset/stderr-output-hygiene.md b/.changeset/stderr-output-hygiene.md new file mode 100644 index 00000000..9aacb7af --- /dev/null +++ b/.changeset/stderr-output-hygiene.md @@ -0,0 +1,10 @@ +--- +"@googleworkspace/cli": patch +--- + +Stderr/output hygiene rollup: route diagnostics to stderr, add colored error labels, propagate auth errors. + +- **triage.rs**: "No messages found" sent to stderr so stdout stays valid JSON for pipes +- **modelarmor.rs**: response body printed only on success; error message now includes body for diagnostics +- **error.rs**: colored `error[variant]:` labels on stderr (respects `NO_COLOR` env var), `hint:` prefix for accessNotConfigured guidance +- **calendar, chat, docs, drive, script, sheets**: auth failures now propagate as `GwsError::Auth` instead of silently proceeding unauthenticated (dry-run still works without auth) diff --git a/src/credential_store.rs b/src/credential_store.rs index 075a9765..7770fe0e 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -14,6 +14,8 @@ use std::path::PathBuf; +use crate::error::sanitize_for_terminal; + use aes_gcm::aead::{Aead, KeyInit, OsRng}; use aes_gcm::{AeadCore, Aes256Gcm, Nonce}; @@ -31,7 +33,10 @@ fn ensure_key_dir(path: &std::path::Path) -> std::io::Result<()> { use std::os::unix::fs::PermissionsExt; if let Err(e) = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)) { - eprintln!("Warning: failed to set secure permissions on key directory: {e}"); + eprintln!( + "Warning: failed to set secure permissions on key directory: {}", + sanitize_for_terminal(&e.to_string()) + ); } } } @@ -251,7 +256,10 @@ fn resolve_key( } } Err(e) => { - eprintln!("Warning: keyring access failed, falling back to file storage: {e}"); + eprintln!( + "Warning: keyring access failed, falling back to file storage: {}", + sanitize_for_terminal(&e.to_string()) + ); } } } diff --git a/src/error.rs b/src/error.rs index bb7c086e..2fd4d795 100644 --- a/src/error.rs +++ b/src/error.rs @@ -148,11 +148,54 @@ impl GwsError { } } +/// Returns true when stderr is connected to an interactive terminal, +/// meaning ANSI color codes will be visible to the user. +fn stderr_supports_color() -> bool { + use std::io::IsTerminal; + std::io::stderr().is_terminal() && std::env::var_os("NO_COLOR").is_none() +} + +/// Wrap `text` in ANSI bold + the given color code, resetting afterwards. +/// Returns the plain text unchanged when stderr is not a TTY. +fn colorize(text: &str, ansi_color: &str) -> String { + if stderr_supports_color() { + format!("\x1b[1;{ansi_color}m{text}\x1b[0m") + } else { + text.to_string() + } +} + +/// Strip terminal control characters from `text` to prevent escape-sequence +/// injection when printing untrusted content (API responses, user input) to +/// stderr. Preserves newlines and tabs for readability. +pub(crate) fn sanitize_for_terminal(text: &str) -> String { + text.chars() + .filter(|&c| { + if c == '\n' || c == '\t' { + return true; + } + !c.is_control() + }) + .collect() +} + +/// Format a colored error label for the given error variant. +fn error_label(err: &GwsError) -> String { + match err { + GwsError::Api { .. } => colorize("error[api]:", "31"), // red + GwsError::Auth(_) => colorize("error[auth]:", "31"), // red + GwsError::Validation(_) => colorize("error[validation]:", "33"), // yellow + GwsError::Discovery(_) => colorize("error[discovery]:", "31"), // red + GwsError::Other(_) => colorize("error:", "31"), // red + } +} + /// Formats any error as a JSON object and prints to stdout. /// -/// For `accessNotConfigured` errors (HTTP 403, reason `accessNotConfigured`), -/// additional human-readable guidance is printed to stderr explaining how to -/// enable the API in GCP. The JSON output on stdout is unchanged (machine-readable). +/// A human-readable colored label is printed to stderr when connected to a +/// TTY. For `accessNotConfigured` errors (HTTP 403, reason +/// `accessNotConfigured`), additional guidance is printed to stderr. +/// The JSON output on stdout is unchanged (machine-readable). pub fn print_error_json(err: &GwsError) { let json = err.to_json(); println!( @@ -160,22 +203,34 @@ pub fn print_error_json(err: &GwsError) { serde_json::to_string_pretty(&json).unwrap_or_default() ); - // Print actionable guidance to stderr for accessNotConfigured errors + // Print a colored summary to stderr. For accessNotConfigured errors, + // print specialized guidance instead of the generic message to avoid + // redundant output (the full API error already appears in the JSON). if let GwsError::Api { reason, enable_url, .. } = err { if reason == "accessNotConfigured" { eprintln!(); - eprintln!("💡 API not enabled for your GCP project."); + let hint = colorize("hint:", "36"); // cyan + eprintln!( + "{} {hint} API not enabled for your GCP project.", + error_label(err) + ); if let Some(url) = enable_url { - eprintln!(" Enable it at: {url}"); + eprintln!(" Enable it at: {url}"); } else { - eprintln!(" Visit the GCP Console → APIs & Services → Library to enable the required API."); + eprintln!(" Visit the GCP Console → APIs & Services → Library to enable the required API."); } - eprintln!(" After enabling, wait a few seconds and retry your command."); + eprintln!(" After enabling, wait a few seconds and retry your command."); + return; } } + eprintln!( + "{} {}", + error_label(err), + sanitize_for_terminal(&err.to_string()) + ); } #[cfg(test)] @@ -327,4 +382,58 @@ mod tests { // enable_url key should not appear in JSON when None assert!(json["error"]["enable_url"].is_null()); } + + // --- colored output tests --- + + #[test] + #[serial_test::serial] + fn test_colorize_respects_no_color_env() { + // NO_COLOR is the de-facto standard for disabling colors. + // When set, colorize() should return the plain text. + std::env::set_var("NO_COLOR", "1"); + let result = colorize("hello", "31"); + std::env::remove_var("NO_COLOR"); + assert_eq!(result, "hello"); + } + + #[test] + fn test_error_label_contains_variant_name() { + let api_err = GwsError::Api { + code: 400, + message: "bad".to_string(), + reason: "r".to_string(), + enable_url: None, + }; + let label = error_label(&api_err); + assert!(label.contains("error[api]:")); + + let auth_err = GwsError::Auth("fail".to_string()); + assert!(error_label(&auth_err).contains("error[auth]:")); + + let val_err = GwsError::Validation("bad input".to_string()); + assert!(error_label(&val_err).contains("error[validation]:")); + + let disc_err = GwsError::Discovery("missing".to_string()); + assert!(error_label(&disc_err).contains("error[discovery]:")); + + let other_err = GwsError::Other(anyhow::anyhow!("oops")); + assert!(error_label(&other_err).contains("error:")); + } + + #[test] + fn test_sanitize_for_terminal_strips_control_chars() { + // ANSI escape sequence should be stripped + let input = "normal \x1b[31mred text\x1b[0m end"; + let sanitized = sanitize_for_terminal(input); + assert_eq!(sanitized, "normal [31mred text[0m end"); + assert!(!sanitized.contains('\x1b')); + + // Newlines and tabs preserved + let input2 = "line1\nline2\ttab"; + assert_eq!(sanitize_for_terminal(input2), "line1\nline2\ttab"); + + // Other control characters stripped + let input3 = "hello\x07bell\x08backspace"; + assert_eq!(sanitize_for_terminal(input3), "hellobellbackspace"); + } } diff --git a/src/executor.rs b/src/executor.rs index a83919b2..11de8bd9 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -28,7 +28,7 @@ use serde_json::{json, Map, Value}; use tokio::io::AsyncWriteExt; use crate::discovery::{RestDescription, RestMethod}; -use crate::error::GwsError; +use crate::error::{sanitize_for_terminal, GwsError}; /// Tracks what authentication method was used for the request. #[derive(Debug, Clone, PartialEq)] @@ -261,7 +261,10 @@ async fn handle_json_response( } } Err(e) => { - eprintln!("⚠️ Model Armor sanitization failed: {e}"); + eprintln!( + "⚠️ Model Armor sanitization failed: {}", + sanitize_for_terminal(&e.to_string()) + ); } } } diff --git a/src/generate_skills.rs b/src/generate_skills.rs index ba2496b5..07cf41fa 100644 --- a/src/generate_skills.rs +++ b/src/generate_skills.rs @@ -18,7 +18,7 @@ use crate::commands; use crate::discovery; -use crate::error::GwsError; +use crate::error::{sanitize_for_terminal, GwsError}; use crate::services; use clap::Command; use std::path::Path; @@ -123,7 +123,10 @@ pub async fn handle_generate_skills(args: &[String]) -> Result<(), GwsError> { match discovery::fetch_discovery_document(entry.api_name, entry.version).await { Ok(d) => d, Err(e) => { - eprintln!(" WARNING: Failed to fetch discovery doc for {alias}: {e}"); + eprintln!( + " WARNING: Failed to fetch discovery doc for {alias}: {}", + sanitize_for_terminal(&e.to_string()) + ); continue; } } diff --git a/src/helpers/calendar.rs b/src/helpers/calendar.rs index ff8ae432..5a48233d 100644 --- a/src/helpers/calendar.rs +++ b/src/helpers/calendar.rs @@ -167,7 +167,8 @@ TIPS: let scopes_str: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect(); let (token, auth_method) = match auth::get_token(&scopes_str).await { Ok(t) => (Some(t), executor::AuthMethod::OAuth), - Err(_) => (None, executor::AuthMethod::None), + Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None), + Err(e) => return Err(GwsError::Auth(format!("Calendar auth failed: {e}"))), }; let events_res = doc.resources.get("events").ok_or_else(|| { diff --git a/src/helpers/chat.rs b/src/helpers/chat.rs index 94493e53..e95022d4 100644 --- a/src/helpers/chat.rs +++ b/src/helpers/chat.rs @@ -80,7 +80,8 @@ TIPS: let scope_strs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect(); let (token, auth_method) = match auth::get_token(&scope_strs).await { Ok(t) => (Some(t), executor::AuthMethod::OAuth), - Err(_) => (None, executor::AuthMethod::None), + Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None), + Err(e) => return Err(GwsError::Auth(format!("Chat auth failed: {e}"))), }; // Method: spaces.messages.create diff --git a/src/helpers/docs.rs b/src/helpers/docs.rs index 3f6b3896..672916ef 100644 --- a/src/helpers/docs.rs +++ b/src/helpers/docs.rs @@ -72,7 +72,8 @@ TIPS: let scope_strs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect(); let (token, auth_method) = match auth::get_token(&scope_strs).await { Ok(t) => (Some(t), executor::AuthMethod::OAuth), - Err(_) => (None, executor::AuthMethod::None), + Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None), + Err(e) => return Err(GwsError::Auth(format!("Docs auth failed: {e}"))), }; // Method: documents.batchUpdate diff --git a/src/helpers/drive.rs b/src/helpers/drive.rs index 393e0fde..04d4057e 100644 --- a/src/helpers/drive.rs +++ b/src/helpers/drive.rs @@ -98,7 +98,8 @@ TIPS: let scopes: Vec<&str> = create_method.scopes.iter().map(|s| s.as_str()).collect(); let (token, auth_method) = match auth::get_token(&scopes).await { Ok(t) => (Some(t), executor::AuthMethod::OAuth), - Err(_) => (None, executor::AuthMethod::None), + Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None), + Err(e) => return Err(GwsError::Auth(format!("Drive auth failed: {e}"))), }; executor::execute_method( diff --git a/src/helpers/events/subscribe.rs b/src/helpers/events/subscribe.rs index edbfb4dc..3a1c8e13 100644 --- a/src/helpers/events/subscribe.rs +++ b/src/helpers/events/subscribe.rs @@ -1,5 +1,6 @@ use super::*; use crate::auth::AccessTokenProvider; +use crate::error::sanitize_for_terminal; use crate::helpers::PUBSUB_API_BASE; use std::path::PathBuf; @@ -375,7 +376,11 @@ async fn pull_loop( .unwrap_or(0); let path = dir.join(format!("{ts}_{file_counter}.json")); if let Err(e) = std::fs::write(&path, &json_str) { - eprintln!("Warning: failed to write {}: {e}", path.display()); + eprintln!( + "Warning: failed to write {}: {}", + path.display(), + sanitize_for_terminal(&e.to_string()) + ); } else { eprintln!("Wrote {}", path.display()); } diff --git a/src/helpers/gmail/mod.rs b/src/helpers/gmail/mod.rs index 999d65ce..162ca841 100644 --- a/src/helpers/gmail/mod.rs +++ b/src/helpers/gmail/mod.rs @@ -26,6 +26,7 @@ use triage::handle_triage; use watch::handle_watch; pub(super) use crate::auth; +use crate::error::sanitize_for_terminal; pub(super) use crate::error::GwsError; pub(super) use crate::executor; pub(super) use anyhow::Context; @@ -223,11 +224,17 @@ fn extract_body_by_mime(payload: &Value, target_mime: &str) -> Option { Ok(decoded) => match String::from_utf8(decoded) { Ok(s) => return Some(s), Err(e) => { - eprintln!("Warning: {target_mime} body is not valid UTF-8: {e}"); + eprintln!( + "Warning: {target_mime} body is not valid UTF-8: {}", + sanitize_for_terminal(&e.to_string()) + ); } }, Err(e) => { - eprintln!("Warning: {target_mime} body has invalid base64: {e}"); + eprintln!( + "Warning: {target_mime} body has invalid base64: {}", + sanitize_for_terminal(&e.to_string()) + ); } } } @@ -389,7 +396,10 @@ pub(super) fn format_date_for_attribution(raw_date: &str) -> String { chrono::DateTime::parse_from_rfc2822(raw_date) .map(|dt| dt.format("%a, %b %-d, %Y at %-I:%M\u{202f}%p").to_string()) .unwrap_or_else(|e| { - eprintln!("Note: could not parse date as RFC 2822 ({e}); using raw value."); + eprintln!( + "Note: could not parse date as RFC 2822 ({}); using raw value.", + sanitize_for_terminal(&e.to_string()) + ); html_escape(raw_date) }) } diff --git a/src/helpers/gmail/triage.rs b/src/helpers/gmail/triage.rs index ec23bfba..8bc58804 100644 --- a/src/helpers/gmail/triage.rs +++ b/src/helpers/gmail/triage.rs @@ -76,13 +76,13 @@ pub async fn handle_triage(matches: &ArgMatches) -> Result<(), GwsError> { let messages = match list_json.get("messages").and_then(|m| m.as_array()) { Some(m) => m, None => { - println!("No messages found matching query: {query}"); + eprintln!("{}", no_messages_msg(query)); return Ok(()); } }; if messages.is_empty() { - println!("No messages found matching query: {query}"); + eprintln!("{}", no_messages_msg(query)); return Ok(()); } @@ -178,8 +178,15 @@ pub async fn handle_triage(matches: &ArgMatches) -> Result<(), GwsError> { Ok(()) } +/// Returns the human-readable "no messages" diagnostic string. +/// Extracted so the test can reference the exact same message without duplication. +fn no_messages_msg(query: &str) -> String { + format!("No messages found matching query: {query}") +} + #[cfg(test)] mod tests { + use super::no_messages_msg; use clap::{Arg, ArgAction, Command}; /// Build a clap command matching the +triage definition so we can @@ -283,4 +290,13 @@ mod tests { .unwrap_or(crate::formatter::OutputFormat::Table); assert!(matches!(fmt, crate::formatter::OutputFormat::Json)); } + + #[test] + fn empty_result_message_is_not_json() { + // Verify that no_messages_msg() produces a human-readable string that + // belongs on stderr, not stdout. If it were valid JSON it could corrupt + // pipe workflows like `gws gmail +triage | jq`. + let msg = no_messages_msg("label:inbox"); + assert!(serde_json::from_str::(&msg).is_err()); + } } diff --git a/src/helpers/gmail/watch.rs b/src/helpers/gmail/watch.rs index 15bc9889..5785fa7a 100644 --- a/src/helpers/gmail/watch.rs +++ b/src/helpers/gmail/watch.rs @@ -1,5 +1,6 @@ use super::*; use crate::auth::AccessTokenProvider; +use crate::error::sanitize_for_terminal; use crate::helpers::PUBSUB_API_BASE; const GMAIL_API_BASE: &str = "https://gmail.googleapis.com/gmail/v1"; @@ -453,7 +454,7 @@ async fn fetch_and_output_messages( } } Err(e) => { - eprintln!("\x1b[33m[WARNING]\x1b[0m Model Armor sanitization failed for message {msg_id}: {e}"); + eprintln!("\x1b[33m[WARNING]\x1b[0m Model Armor sanitization failed for message {msg_id}: {}", sanitize_for_terminal(&e.to_string())); } } } @@ -466,7 +467,11 @@ async fn fetch_and_output_messages( crate::validate::encode_path_segment(&msg_id) )); if let Err(e) = std::fs::write(&path, &json_str) { - eprintln!("Warning: failed to write {}: {e}", path.display()); + eprintln!( + "Warning: failed to write {}: {}", + path.display(), + sanitize_for_terminal(&e.to_string()) + ); } else { eprintln!("Wrote {}", path.display()); } diff --git a/src/helpers/modelarmor.rs b/src/helpers/modelarmor.rs index 8ac9fc1c..e1551dd1 100644 --- a/src/helpers/modelarmor.rs +++ b/src/helpers/modelarmor.rs @@ -298,14 +298,14 @@ async fn model_armor_post(url: &str, body: &str) -> Result<(), GwsError> { let status = resp.status(); let text = resp.text().await.context("Failed to read response")?; - println!("{text}"); - if !status.is_success() { return Err(GwsError::Other(anyhow::anyhow!( - "API returned status {status}" + "API returned status {status}: {text}" ))); } + println!("{text}"); + Ok(()) } diff --git a/src/helpers/script.rs b/src/helpers/script.rs index b0ad3497..bdef0a02 100644 --- a/src/helpers/script.rs +++ b/src/helpers/script.rs @@ -105,7 +105,8 @@ TIPS: let scopes: Vec<&str> = update_method.scopes.iter().map(|s| s.as_str()).collect(); let (token, auth_method) = match auth::get_token(&scopes).await { Ok(t) => (Some(t), executor::AuthMethod::OAuth), - Err(_) => (None, executor::AuthMethod::None), + Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None), + Err(e) => return Err(GwsError::Auth(format!("Script auth failed: {e}"))), }; let params = json!({ diff --git a/src/helpers/sheets.rs b/src/helpers/sheets.rs index 76f36ab2..e246f0d1 100644 --- a/src/helpers/sheets.rs +++ b/src/helpers/sheets.rs @@ -108,7 +108,8 @@ TIPS: let scope_strs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect(); let (token, auth_method) = match auth::get_token(&scope_strs).await { Ok(t) => (Some(t), executor::AuthMethod::OAuth), - Err(_) => (None, executor::AuthMethod::None), + Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None), + Err(e) => return Err(GwsError::Auth(format!("Sheets auth failed: {e}"))), }; let spreadsheets_res = doc.resources.get("spreadsheets").ok_or_else(|| { @@ -167,7 +168,8 @@ TIPS: let scope_strs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect(); let (token, auth_method) = match auth::get_token(&scope_strs).await { Ok(t) => (Some(t), executor::AuthMethod::OAuth), - Err(_) => (None, executor::AuthMethod::None), + Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None), + Err(e) => return Err(GwsError::Auth(format!("Sheets auth failed: {e}"))), }; executor::execute_method( diff --git a/src/helpers/workflows.rs b/src/helpers/workflows.rs index 48d21784..6831f889 100644 --- a/src/helpers/workflows.rs +++ b/src/helpers/workflows.rs @@ -17,7 +17,7 @@ use super::Helper; use crate::auth; -use crate::error::GwsError; +use crate::error::{sanitize_for_terminal, GwsError}; use clap::{Arg, ArgMatches, Command}; use serde_json::{json, Value}; use std::future::Future; @@ -297,9 +297,11 @@ async fn handle_standup_report(matches: &ArgMatches) -> Result<(), GwsError> { ], ) .await - .map_err(|e| { - eprintln!("Warning: Failed to fetch calendar events: {e}"); - e + .inspect_err(|e| { + eprintln!( + "Warning: Failed to fetch calendar events: {}", + sanitize_for_terminal(&e.to_string()) + ); }) .unwrap_or(json!({})); let events = events_json @@ -327,9 +329,11 @@ async fn handle_standup_report(matches: &ArgMatches) -> Result<(), GwsError> { &[("showCompleted", "false"), ("maxResults", "20")], ) .await - .map_err(|e| { - eprintln!("Warning: Failed to fetch tasks: {e}"); - e + .inspect_err(|e| { + eprintln!( + "Warning: Failed to fetch tasks: {}", + sanitize_for_terminal(&e.to_string()) + ); }) .unwrap_or(json!({})); let tasks = tasks_json @@ -557,9 +561,11 @@ async fn handle_weekly_digest(matches: &ArgMatches) -> Result<(), GwsError> { ], ) .await - .map_err(|e| { - eprintln!("Warning: Failed to fetch calendar events: {e}"); - e + .inspect_err(|e| { + eprintln!( + "Warning: Failed to fetch calendar events: {}", + sanitize_for_terminal(&e.to_string()) + ); }) .unwrap_or(json!({})); let events = events_json @@ -586,9 +592,11 @@ async fn handle_weekly_digest(matches: &ArgMatches) -> Result<(), GwsError> { &[("q", "is:unread"), ("maxResults", "1")], ) .await - .map_err(|e| { - eprintln!("Warning: Failed to fetch unread email count: {e}"); - e + .inspect_err(|e| { + eprintln!( + "Warning: Failed to fetch unread email count: {}", + sanitize_for_terminal(&e.to_string()) + ); }) .unwrap_or(json!({})); let unread_estimate = gmail_json diff --git a/src/setup.rs b/src/setup.rs index 10aebe1b..c3302c78 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -22,7 +22,7 @@ use std::process::Command; use serde_json::json; -use crate::error::GwsError; +use crate::error::{sanitize_for_terminal, GwsError}; use crate::setup_tui::{PickerResult, SelectItem, SetupWizard, StepStatus}; @@ -1371,7 +1371,7 @@ async fn stage_enable_apis(ctx: &mut SetupContext) -> Result j, Err(e) => { - eprintln!("warning: token cache contains invalid UTF-8: {e}"); + eprintln!( + "warning: token cache contains invalid UTF-8: {}", + sanitize_for_terminal(&e.to_string()) + ); return HashMap::new(); } }; @@ -63,7 +68,10 @@ impl EncryptedTokenStorage { match serde_json::from_str(&json) { Ok(map) => map, Err(e) => { - eprintln!("warning: failed to parse token cache JSON: {e}"); + eprintln!( + "warning: failed to parse token cache JSON: {}", + sanitize_for_terminal(&e.to_string()) + ); HashMap::new() } }