Skip to content

Commit 6f92e5b

Browse files
jpoehneltjpoehnelt-bot
jpoehnelt
and
jpoehnelt-bot
authored
fix: stderr/output hygiene rollup (#525)
* fix: stderr/output hygiene rollup — diagnostics to stderr, colored labels, auth propagation Component 1 (PR #485): Route triage 'no messages' and modelarmor error bodies to stderr so stdout stays machine-readable. Component 2 (PR #466): Add colored error[variant]: labels to stderr on TTY, respecting NO_COLOR. Replace emoji hint with colorized text. Component 3 (PR #446): Propagate auth errors as GwsError::Auth in calendar, chat, docs, drive, script, sheets helpers instead of silently proceeding unauthenticated. dry-run bypass preserved. * fix: deduplicate accessNotConfigured stderr output Use if/else so that accessNotConfigured errors get the specialized hint guidance instead of redundantly printing both the generic summary and the hint. Non-accessNotConfigured Api errors and all other variants still get the generic error[variant]: summary line. * test: remove misleading model_armor_post error format test model_armor_post function. A proper integration test would require HTTP mocking (e.g. mockito/wiremock) which is out of scope for this PR. * refactor: deduplicate error printing else branches Use early return in accessNotConfigured branch so the generic eprintln! only appears once, eliminating the duplicated else blocks. * security: sanitize error messages before printing to stderr Add sanitize_for_terminal() to strip control characters (ANSI escape sequences, bell, backspace, etc.) from error messages before printing to stderr, preventing terminal escape injection from API responses. Newlines and tabs are preserved for readability. The function is pub(crate) so it can be reused by other modules that print untrusted content to stderr. * fix: sanitize all stderr error output across codebase Apply sanitize_for_terminal() to all 16 remaining eprintln sites that print unsanitized error strings to stderr. This prevents terminal escape sequence injection through error messages. Files updated: - workflows.rs (4 sites) - watch.rs (2 sites) - gmail/mod.rs (3 sites) - executor.rs (1 site) - subscribe.rs (1 site) - token_storage.rs (2 sites) - credential_store.rs (2 sites) - setup.rs (1 site) - generate_skills.rs (1 site) Also fixes clippy: map_err -> inspect_err where closure only logs. --------- Co-authored-by: jpoehnelt-bot <[email protected]>
1 parent b241a5b commit 6f92e5b

File tree

19 files changed

+241
-49
lines changed

19 files changed

+241
-49
lines changed

‎.changeset/stderr-output-hygiene.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
"@googleworkspace/cli": patch
3+
---
4+
5+
Stderr/output hygiene rollup: route diagnostics to stderr, add colored error labels, propagate auth errors.
6+
7+
- **triage.rs**: "No messages found" sent to stderr so stdout stays valid JSON for pipes
8+
- **modelarmor.rs**: response body printed only on success; error message now includes body for diagnostics
9+
- **error.rs**: colored `error[variant]:` labels on stderr (respects `NO_COLOR` env var), `hint:` prefix for accessNotConfigured guidance
10+
- **calendar, chat, docs, drive, script, sheets**: auth failures now propagate as `GwsError::Auth` instead of silently proceeding unauthenticated (dry-run still works without auth)

‎src/credential_store.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
use std::path::PathBuf;
1616

17+
use crate::error::sanitize_for_terminal;
18+
1719
use aes_gcm::aead::{Aead, KeyInit, OsRng};
1820
use aes_gcm::{AeadCore, Aes256Gcm, Nonce};
1921

@@ -31,7 +33,10 @@ fn ensure_key_dir(path: &std::path::Path) -> std::io::Result<()> {
3133
use std::os::unix::fs::PermissionsExt;
3234
if let Err(e) = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700))
3335
{
34-
eprintln!("Warning: failed to set secure permissions on key directory: {e}");
36+
eprintln!(
37+
"Warning: failed to set secure permissions on key directory: {}",
38+
sanitize_for_terminal(&e.to_string())
39+
);
3540
}
3641
}
3742
}
@@ -251,7 +256,10 @@ fn resolve_key(
251256
}
252257
}
253258
Err(e) => {
254-
eprintln!("Warning: keyring access failed, falling back to file storage: {e}");
259+
eprintln!(
260+
"Warning: keyring access failed, falling back to file storage: {}",
261+
sanitize_for_terminal(&e.to_string())
262+
);
255263
}
256264
}
257265
}

‎src/error.rs‎

Lines changed: 117 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -148,34 +148,89 @@ impl GwsError {
148148
}
149149
}
150150

151+
/// Returns true when stderr is connected to an interactive terminal,
152+
/// meaning ANSI color codes will be visible to the user.
153+
fn stderr_supports_color() -> bool {
154+
use std::io::IsTerminal;
155+
std::io::stderr().is_terminal() && std::env::var_os("NO_COLOR").is_none()
156+
}
157+
158+
/// Wrap `text` in ANSI bold + the given color code, resetting afterwards.
159+
/// Returns the plain text unchanged when stderr is not a TTY.
160+
fn colorize(text: &str, ansi_color: &str) -> String {
161+
if stderr_supports_color() {
162+
format!("\x1b[1;{ansi_color}m{text}\x1b[0m")
163+
} else {
164+
text.to_string()
165+
}
166+
}
167+
168+
/// Strip terminal control characters from `text` to prevent escape-sequence
169+
/// injection when printing untrusted content (API responses, user input) to
170+
/// stderr. Preserves newlines and tabs for readability.
171+
pub(crate) fn sanitize_for_terminal(text: &str) -> String {
172+
text.chars()
173+
.filter(|&c| {
174+
if c == '\n' || c == '\t' {
175+
return true;
176+
}
177+
!c.is_control()
178+
})
179+
.collect()
180+
}
181+
182+
/// Format a colored error label for the given error variant.
183+
fn error_label(err: &GwsError) -> String {
184+
match err {
185+
GwsError::Api { .. } => colorize("error[api]:", "31"), // red
186+
GwsError::Auth(_) => colorize("error[auth]:", "31"), // red
187+
GwsError::Validation(_) => colorize("error[validation]:", "33"), // yellow
188+
GwsError::Discovery(_) => colorize("error[discovery]:", "31"), // red
189+
GwsError::Other(_) => colorize("error:", "31"), // red
190+
}
191+
}
192+
151193
/// Formats any error as a JSON object and prints to stdout.
152194
///
153-
/// For `accessNotConfigured` errors (HTTP 403, reason `accessNotConfigured`),
154-
/// additional human-readable guidance is printed to stderr explaining how to
155-
/// enable the API in GCP. The JSON output on stdout is unchanged (machine-readable).
195+
/// A human-readable colored label is printed to stderr when connected to a
196+
/// TTY. For `accessNotConfigured` errors (HTTP 403, reason
197+
/// `accessNotConfigured`), additional guidance is printed to stderr.
198+
/// The JSON output on stdout is unchanged (machine-readable).
156199
pub fn print_error_json(err: &GwsError) {
157200
let json = err.to_json();
158201
println!(
159202
"{}",
160203
serde_json::to_string_pretty(&json).unwrap_or_default()
161204
);
162205

163-
// Print actionable guidance to stderr for accessNotConfigured errors
206+
// Print a colored summary to stderr. For accessNotConfigured errors,
207+
// print specialized guidance instead of the generic message to avoid
208+
// redundant output (the full API error already appears in the JSON).
164209
if let GwsError::Api {
165210
reason, enable_url, ..
166211
} = err
167212
{
168213
if reason == "accessNotConfigured" {
169214
eprintln!();
170-
eprintln!("💡 API not enabled for your GCP project.");
215+
let hint = colorize("hint:", "36"); // cyan
216+
eprintln!(
217+
"{} {hint} API not enabled for your GCP project.",
218+
error_label(err)
219+
);
171220
if let Some(url) = enable_url {
172-
eprintln!(" Enable it at: {url}");
221+
eprintln!(" Enable it at: {url}");
173222
} else {
174-
eprintln!(" Visit the GCP Console → APIs & Services → Library to enable the required API.");
223+
eprintln!(" Visit the GCP Console → APIs & Services → Library to enable the required API.");
175224
}
176-
eprintln!(" After enabling, wait a few seconds and retry your command.");
225+
eprintln!(" After enabling, wait a few seconds and retry your command.");
226+
return;
177227
}
178228
}
229+
eprintln!(
230+
"{} {}",
231+
error_label(err),
232+
sanitize_for_terminal(&err.to_string())
233+
);
179234
}
180235

181236
#[cfg(test)]
@@ -327,4 +382,58 @@ mod tests {
327382
// enable_url key should not appear in JSON when None
328383
assert!(json["error"]["enable_url"].is_null());
329384
}
385+
386+
// --- colored output tests ---
387+
388+
#[test]
389+
#[serial_test::serial]
390+
fn test_colorize_respects_no_color_env() {
391+
// NO_COLOR is the de-facto standard for disabling colors.
392+
// When set, colorize() should return the plain text.
393+
std::env::set_var("NO_COLOR", "1");
394+
let result = colorize("hello", "31");
395+
std::env::remove_var("NO_COLOR");
396+
assert_eq!(result, "hello");
397+
}
398+
399+
#[test]
400+
fn test_error_label_contains_variant_name() {
401+
let api_err = GwsError::Api {
402+
code: 400,
403+
message: "bad".to_string(),
404+
reason: "r".to_string(),
405+
enable_url: None,
406+
};
407+
let label = error_label(&api_err);
408+
assert!(label.contains("error[api]:"));
409+
410+
let auth_err = GwsError::Auth("fail".to_string());
411+
assert!(error_label(&auth_err).contains("error[auth]:"));
412+
413+
let val_err = GwsError::Validation("bad input".to_string());
414+
assert!(error_label(&val_err).contains("error[validation]:"));
415+
416+
let disc_err = GwsError::Discovery("missing".to_string());
417+
assert!(error_label(&disc_err).contains("error[discovery]:"));
418+
419+
let other_err = GwsError::Other(anyhow::anyhow!("oops"));
420+
assert!(error_label(&other_err).contains("error:"));
421+
}
422+
423+
#[test]
424+
fn test_sanitize_for_terminal_strips_control_chars() {
425+
// ANSI escape sequence should be stripped
426+
let input = "normal \x1b[31mred text\x1b[0m end";
427+
let sanitized = sanitize_for_terminal(input);
428+
assert_eq!(sanitized, "normal [31mred text[0m end");
429+
assert!(!sanitized.contains('\x1b'));
430+
431+
// Newlines and tabs preserved
432+
let input2 = "line1\nline2\ttab";
433+
assert_eq!(sanitize_for_terminal(input2), "line1\nline2\ttab");
434+
435+
// Other control characters stripped
436+
let input3 = "hello\x07bell\x08backspace";
437+
assert_eq!(sanitize_for_terminal(input3), "hellobellbackspace");
438+
}
330439
}

‎src/executor.rs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ use serde_json::{json, Map, Value};
2828
use tokio::io::AsyncWriteExt;
2929

3030
use crate::discovery::{RestDescription, RestMethod};
31-
use crate::error::GwsError;
31+
use crate::error::{sanitize_for_terminal, GwsError};
3232

3333
/// Tracks what authentication method was used for the request.
3434
#[derive(Debug, Clone, PartialEq)]
@@ -261,7 +261,10 @@ async fn handle_json_response(
261261
}
262262
}
263263
Err(e) => {
264-
eprintln!("⚠️ Model Armor sanitization failed: {e}");
264+
eprintln!(
265+
"⚠️ Model Armor sanitization failed: {}",
266+
sanitize_for_terminal(&e.to_string())
267+
);
265268
}
266269
}
267270
}

‎src/generate_skills.rs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
1919
use crate::commands;
2020
use crate::discovery;
21-
use crate::error::GwsError;
21+
use crate::error::{sanitize_for_terminal, GwsError};
2222
use crate::services;
2323
use clap::Command;
2424
use std::path::Path;
@@ -123,7 +123,10 @@ pub async fn handle_generate_skills(args: &[String]) -> Result<(), GwsError> {
123123
match discovery::fetch_discovery_document(entry.api_name, entry.version).await {
124124
Ok(d) => d,
125125
Err(e) => {
126-
eprintln!(" WARNING: Failed to fetch discovery doc for {alias}: {e}");
126+
eprintln!(
127+
" WARNING: Failed to fetch discovery doc for {alias}: {}",
128+
sanitize_for_terminal(&e.to_string())
129+
);
127130
continue;
128131
}
129132
}

‎src/helpers/calendar.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,8 @@ TIPS:
167167
let scopes_str: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect();
168168
let (token, auth_method) = match auth::get_token(&scopes_str).await {
169169
Ok(t) => (Some(t), executor::AuthMethod::OAuth),
170-
Err(_) => (None, executor::AuthMethod::None),
170+
Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None),
171+
Err(e) => return Err(GwsError::Auth(format!("Calendar auth failed: {e}"))),
171172
};
172173

173174
let events_res = doc.resources.get("events").ok_or_else(|| {

‎src/helpers/chat.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,8 @@ TIPS:
8080
let scope_strs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect();
8181
let (token, auth_method) = match auth::get_token(&scope_strs).await {
8282
Ok(t) => (Some(t), executor::AuthMethod::OAuth),
83-
Err(_) => (None, executor::AuthMethod::None),
83+
Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None),
84+
Err(e) => return Err(GwsError::Auth(format!("Chat auth failed: {e}"))),
8485
};
8586

8687
// Method: spaces.messages.create

‎src/helpers/docs.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,8 @@ TIPS:
7272
let scope_strs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect();
7373
let (token, auth_method) = match auth::get_token(&scope_strs).await {
7474
Ok(t) => (Some(t), executor::AuthMethod::OAuth),
75-
Err(_) => (None, executor::AuthMethod::None),
75+
Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None),
76+
Err(e) => return Err(GwsError::Auth(format!("Docs auth failed: {e}"))),
7677
};
7778

7879
// Method: documents.batchUpdate

‎src/helpers/drive.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,8 @@ TIPS:
9898
let scopes: Vec<&str> = create_method.scopes.iter().map(|s| s.as_str()).collect();
9999
let (token, auth_method) = match auth::get_token(&scopes).await {
100100
Ok(t) => (Some(t), executor::AuthMethod::OAuth),
101-
Err(_) => (None, executor::AuthMethod::None),
101+
Err(_) if matches.get_flag("dry-run") => (None, executor::AuthMethod::None),
102+
Err(e) => return Err(GwsError::Auth(format!("Drive auth failed: {e}"))),
102103
};
103104

104105
executor::execute_method(

‎src/helpers/events/subscribe.rs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use super::*;
22
use crate::auth::AccessTokenProvider;
3+
use crate::error::sanitize_for_terminal;
34
use crate::helpers::PUBSUB_API_BASE;
45
use std::path::PathBuf;
56

@@ -375,7 +376,11 @@ async fn pull_loop(
375376
.unwrap_or(0);
376377
let path = dir.join(format!("{ts}_{file_counter}.json"));
377378
if let Err(e) = std::fs::write(&path, &json_str) {
378-
eprintln!("Warning: failed to write {}: {e}", path.display());
379+
eprintln!(
380+
"Warning: failed to write {}: {}",
381+
path.display(),
382+
sanitize_for_terminal(&e.to_string())
383+
);
379384
} else {
380385
eprintln!("Wrote {}", path.display());
381386
}

0 commit comments

Comments
 (0)