Refactor Pretty control sequence escaping

tracing-subscriber/src/fmt/format/escape.rs

@@ -2,50 +2,68 @@
 
 use std::fmt::{self, Write};
 
-/// A wrapper that implements `fmt::Debug` and `fmt::Display` and escapes ANSI sequences on-the-fly.
+/// A wrapper that implements `fmt::Debug` and escapes control sequences on-the-fly.
 /// This avoids creating intermediate strings while providing security against terminal injection.
 pub(super) struct Escape<T>(pub(super) T);
 
+impl<T: fmt::Debug> fmt::Debug for Escape<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut escaping_writer = EscapingWriter {
+            inner: f,
+            skip_csi_codes: false,
+        };
+        write!(escaping_writer, "{:?}", self.0)
+    }
+}
+
+/// A wrapper that implements `fmt::Debug` and removes control sequences on-the-fly.
+/// This avoids creating intermediate strings while providing security against terminal injection.
+pub(super) struct EscapeSkip<T>(pub(super) T);
+
+impl<T: fmt::Debug> fmt::Debug for EscapeSkip<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut escaping_writer = EscapingWriter {
+            inner: f,
+            skip_csi_codes: true,
+        };
+        write!(escaping_writer, "{:?}", self.0)
+    }
+}
+
 /// Helper struct that escapes ANSI sequences as characters are written
 struct EscapingWriter<'a, 'b> {
     inner: &'a mut fmt::Formatter<'b>,
+    skip_csi_codes: bool,
 }
 
 impl<'a, 'b> fmt::Write for EscapingWriter<'a, 'b> {
     fn write_str(&mut self, s: &str) -> fmt::Result {
-        // Stream the string character by character, escaping ANSI and C1 control sequences
-        for ch in s.chars() {
-            match ch {
-                // C0 control characters that can be used in terminal escape sequences
-                '\x1b' => self.inner.write_str("\\x1b")?,  // ESC
-                '\x07' => self.inner.write_str("\\x07")?,  // BEL
-                '\x08' => self.inner.write_str("\\x08")?,  // BS
-                '\x0c' => self.inner.write_str("\\x0c")?,  // FF
-                '\x7f' => self.inner.write_str("\\x7f")?,  // DEL
-
-                // C1 control characters (\x80-\x9f) - 8-bit control codes
-                // These can be used as alternative escape sequences in some terminals
-                ch if ch as u32 >= 0x80 && ch as u32 <= 0x9f => {
+        // Stream the string character by character, escaping all control sequences
+        let mut chars = s.chars().peekable();
+        while let Some(ch) = chars.next() {
+            // Recognize and remove ECMA-48 CSI codes
+            // This is a best effort to clean up colour codes in the message field
+            if ch == '\x1b' && self.skip_csi_codes {
+                if chars.next_if_eq(&'[').is_some() {
+                    // Remove parameter and intermediate bytes
+                    while chars.next_if(|x| matches!(x, '\x20'..='\x3f')).is_some() {}
+                    // Remove final byte
+                    chars.next_if(|x| matches!(x, '\x40'..='\x7E'));
+                    continue;
+                }
+            }
+
+            // ESC BEL BS FF DEL
+            if matches!(ch, '\x1b' | '\x07' | '\x08' | '\x0c' | '\x7f'..='\u{9f}') {
+                if ch.is_ascii() {
+                    write!(self.inner, "\\x{:02x}", ch as u32)?
+                } else {
                     write!(self.inner, "\\u{{{:x}}}", ch as u32)?
-                },
-                
-                _ => self.inner.write_char(ch)?,
+                }
+            } else {
+                self.inner.write_char(ch)?;
             }
         }
         Ok(())
     }
 }
-
-impl<T: fmt::Debug> fmt::Debug for Escape<T> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let mut escaping_writer = EscapingWriter { inner: f };
-        write!(escaping_writer, "{:?}", self.0)
-    }
-}
-
-impl<T: fmt::Display> fmt::Display for Escape<T> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let mut escaping_writer = EscapingWriter { inner: f };
-        write!(escaping_writer, "{}", self.0)
-    }
-}

tracing-subscriber/src/fmt/format/mod.rs

@@ -400,7 +400,7 @@ pub struct Full;
 /// span context, but other information is abbreviated. The [`Pretty`] logging
 /// format is an extra-verbose, multi-line human-readable logging format
 /// intended for use in development.
-/// 
+///
 /// [`FmtSubscriber`]: super::Subscriber
 #[derive(Debug, Clone)]
 pub struct Format<F = Full, T = SystemTime> {
@@ -1262,7 +1262,7 @@ impl field::Visit for DefaultVisitor<'_> {
             self.record_debug(
                 field,
                 &format_args!(
-                    "{} {}{}{}{}",
+                    "{:?} {}{}{}{}",
                     Escape(&format_args!("{}", value)),
                     italic.paint(field.name()),
                     italic.paint(".sources"),
@@ -1271,10 +1271,7 @@ impl field::Visit for DefaultVisitor<'_> {
                 ),
             )
         } else {
-            self.record_debug(
-                field,
-                &format_args!("{}", Escape(&format_args!("{}", value))),
-            )
+            self.record_debug(field, &Escape(&format_args!("{}", value)))
         }
     }
 
@@ -1601,7 +1598,7 @@ impl<F> fmt::Debug for FieldFnVisitor<'_, F> {
 /// Configures what points in the span lifecycle are logged as events.
 ///
 /// See also [`with_span_events`].
-/// 
+///
 /// [`with_span_events`]: super::SubscriberBuilder::with_span_events
 #[derive(Clone, Eq, PartialEq, Ord, PartialOrd)]
 pub struct FmtSpan(u8);

tracing-subscriber/src/fmt/format/pretty.rs

@@ -1,7 +1,10 @@
 use super::*;
 use crate::{
     field::{VisitFmt, VisitOutput},
-    fmt::fmt_layer::{FmtContext, FormattedFields},
+    fmt::{
+        fmt_layer::{FmtContext, FormattedFields},
+        format::escape::EscapeSkip,
+    },
     registry::LookupSpan,
 };
 
@@ -419,6 +422,9 @@ impl<'a> PrettyVisitor<'a> {
     }
 
     fn write_padded(&mut self, value: &impl fmt::Debug) {
+        if self.result.is_err() {
+            return;
+        }
         let padding = if self.is_empty {
             self.is_empty = false;
             ""
@@ -435,14 +441,24 @@ impl<'a> PrettyVisitor<'a> {
             Style::new()
         }
     }
+
+    fn write_field(&mut self, mut name: &str, value: &dyn fmt::Debug) {
+        if name.starts_with("r#") {
+            name = &name[2..];
+        }
+        let bold = self.bold();
+        self.write_padded(&format_args!(
+            "{}{}{}: {:?}",
+            bold.prefix(),
+            name,
+            bold.infix(self.style),
+            Escape(value)
+        ))
+    }
 }
 
 impl field::Visit for PrettyVisitor<'_> {
     fn record_str(&mut self, field: &Field, value: &str) {
-        if self.result.is_err() {
-            return;
-        }
-
         if field.name() == "message" {
             self.record_debug(field, &format_args!("{}", value))
         } else {
@@ -451,51 +467,30 @@ impl field::Visit for PrettyVisitor<'_> {
     }
 
     fn record_error(&mut self, field: &Field, value: &(dyn std::error::Error + 'static)) {
+        self.record_debug(field, &format_args!("{}", value));
+
         if let Some(source) = value.source() {
-            let bold = self.bold();
-            self.record_debug(
-                field,
-                &format_args!(
-                    "{}, {}{}.sources{}: {}",
-                    Escape(&format_args!("{}", value)),
-                    bold.prefix(),
-                    field,
-                    bold.infix(self.style),
-                    ErrorSourceList(source),
-                ),
-            )
-        } else {
-            self.record_debug(field, &Escape(&format_args!("{}", value)))
+            self.write_field(
+                &std::format!("{field}.sources"),
+                &format_args!("{}", ErrorSourceList(source)),
+            );
         }
     }
 
     fn record_debug(&mut self, field: &Field, value: &dyn fmt::Debug) {
-        if self.result.is_err() {
-            return;
-        }
-        let bold = self.bold();
         match field.name() {
             "message" => {
                 // Escape ANSI characters to prevent malicious patterns (e.g., terminal injection attacks)
-                self.write_padded(&format_args!("{}{:?}", self.style.prefix(), Escape(value)))
-            },
+                self.write_padded(&format_args!(
+                    "{}{:?}",
+                    self.style.prefix(),
+                    EscapeSkip(value)
+                ))
+            }
             // Skip fields that are actually log metadata that have already been handled
             #[cfg(feature = "tracing-log")]
             name if name.starts_with("log.") => self.result = Ok(()),
-            name if name.starts_with("r#") => self.write_padded(&format_args!(
-                "{}{}{}: {:?}",
-                bold.prefix(),
-                &name[2..],
-                bold.infix(self.style),
-                value
-            )),
-            name => self.write_padded(&format_args!(
-                "{}{}{}: {:?}",
-                bold.prefix(),
-                name,
-                bold.infix(self.style),
-                value
-            )),
+            name => self.write_field(name, value),
         };
     }
 }

tracing-subscriber/tests/ansi_escaping.rs

@@ -145,7 +145,7 @@ fn test_json_ansi_escaping() {
     );
 }
 
-/// Test that pretty formatter properly escapes ANSI sequences  
+/// Test that pretty formatter properly escapes ANSI sequences
 #[cfg(feature = "ansi")]
 #[test]
 fn test_pretty_ansi_escaping() {
@@ -160,9 +160,12 @@ fn test_pretty_ansi_escaping() {
 
     tracing::subscriber::with_default(subscriber, || {
         let malicious_input = "\x1b]0;PWNED\x07\x1b[2J";
+        let colourful = "[\u{1b}[1m\u{1b}[38;5;9merror\u{1b}[0m\u{1b}[1m: Invalid JSON\u{1b}[0m]";
 
         // Pretty formatter should escape ANSI sequences
         tracing::info!("Testing: {}", malicious_input);
+        tracing::info!(user_input = %malicious_input, "Field test");
+        tracing::info!("Colour test: {}", colourful);
     });
 
     let output = writer.get_output();
@@ -176,6 +179,10 @@ fn test_pretty_ansi_escaping() {
         !output.contains('\x07'),
         "Pretty output should not contain raw BEL characters"
     );
+    assert!(
+        output.contains("[error: Invalid JSON]"),
+        "ansi escape code should be removed from message entirely"
+    )
 }
 
 /// Comprehensive test for ANSI sanitization that prevents injection attacks