@ 261104.1

260915.1 · 24198d904a074f2943b2a3be925e53134c994734

fix: Better sanitizing

This refactors the sanitization code somewhat, to provide a couple benefits:

- Ensure we sanitize output in error messages (#2119), while not
escaping newlines from error messages.
- Avoids some unnecessary allocations and string copies
- Makes `print_error` a little nicer to use. As a macro, it avoids
needing a separate call to `format!`.

The main downside is added complexity.

Fixes: #2119

reading declarations

Changes to src/sanitize.rs. All files

src/sanitize.rs

@@ -1,7 +1,6 @@ //! TTY-output sanitization to prevent terminal escape injection via filenames. -use std::borrow::Cow;-use std::fmt::Write;+use std::fmt::{Display, Formatter, Write};  /// True for any char that is neither printable nor permitted whitespace (only HT). /// Covers C0/C1/DEL, bidi overrides, zero-width and format chars, and tag chars.@@ -23,34 +22,51 @@ fn needs_escape(c: char) -> bool {         ) } -/// Returns a `Cow<str>` borrowing `s` when no escaping is needed, otherwise an owned-/// escaped copy. Use this when an owned `&str`/`String` is required (e.g. ANSI paint).-pub fn sanitize_for_terminal(s: &str) -> Cow<'_, str> {-    if !s.chars().any(needs_escape) {-        return Cow::Borrowed(s);-    }-    let mut out = String::with_capacity(s.len());-    for c in s.chars() {+/// Write the sanitized contents of `raw` to `f`.+pub fn write_sanitized(f: &mut std::fmt::Formatter<'_>, raw: &str) -> std::fmt::Result {+    // Would it be faster to do a pass to see if we don't need an escape and write+    // the whole string first? Maybe with a faster check just for non-control ASCII?+    for c in raw.chars() {         if needs_escape(c) {             let v = c as u32;             if v <= 0xFF {-                let _ = write!(out, "\\x{v:02X}");+                write!(f, "\\x{v:02X}")?;             } else {-                let _ = write!(out, "\\u{{{v:04X}}}");+                write!(f, "\\u{{{v:04X}}}")?;             }         } else {-            out.push(c);+            f.write_char(c)?;+        }+    }+    Ok(())+}++// TODO: add something to sanitize paths directly instead of as strings.++/// A wrapper type that sanitizes the output to Display+pub(crate) struct SanitizedStr<'a> {+    raw: &'a str,+    /// If true, the content is safe to output without sanitizing,+    /// either because it is trusted, or because it isn't going to a terminal.+    is_safe: bool,+}++impl<'a> Display for SanitizedStr<'a> {+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {+        if self.is_safe {+            // We don't need to sanitize anything, just forward+            self.raw.fmt(f)+        } else {+            write_sanitized(f, self.raw)         }     }-    Cow::Owned(out) }  /// Sanitize for terminal output only; raw bytes pass through on pipes/files.-pub fn maybe_sanitize<'a>(s: &'a str, is_terminal: bool) -> Cow<'a, str> {-    if is_terminal {-        sanitize_for_terminal(s)-    } else {-        Cow::Borrowed(s)+pub fn sanitize_for_term(raw: &str, is_terminal: bool) -> SanitizedStr<'_> {+    SanitizedStr {+        raw,+        is_safe: !is_terminal,     } } @@ -58,6 +74,14 @@ pub fn maybe_sanitize<'a>(s: &'a str, is_terminal: bool) -> Cow<'a, str> { mod tests {     use super::*; +    fn sanitize_string(s: &str) -> String {+        SanitizedStr {+            raw: s,+            is_safe: false,+        }+        .to_string()+    }+     #[test]     fn preserves_safe_content() {         for s in [@@ -68,75 +92,63 @@ mod tests {             "a\tb",             "a\u{FFFD}b",         ] {-            assert!(-                matches!(sanitize_for_terminal(s), Cow::Borrowed(_)),-                "{s:?}"-            );-            assert_eq!(sanitize_for_terminal(s), s);+            assert_eq!(sanitize_string(s), s);         }     }      #[test]     fn strips_osc52_clipboard_payload() {         let attack = "innocent\x1b]52;c;cHduZWQ=\x1b\\.txt";-        let safe = sanitize_for_terminal(attack);+        let safe = sanitize_string(attack);         assert!(!safe.contains('\x1b'));         assert_eq!(safe, "innocent\\x1B]52;c;cHduZWQ=\\x1B\\.txt");     }      #[test]     fn strips_cr_output_forgery() {-        assert_eq!(sanitize_for_terminal("A\rFAKE OUTPUT"), "A\\x0DFAKE OUTPUT");+        assert_eq!(sanitize_string("A\rFAKE OUTPUT"), "A\\x0DFAKE OUTPUT");     }      #[test]     fn strips_osc8_hyperlink_injection() {         let attack = "phish\x1b]8;;https:evil.example\x1b\\phony.txt";-        assert!(!sanitize_for_terminal(attack).contains('\x1b'));+        assert!(!sanitize_string(attack).contains('\x1b'));     }      #[test]     fn strips_del() {-        assert_eq!(sanitize_for_terminal("a\x7fb"), "a\\x7Fb");+        assert_eq!(sanitize_string("a\x7fb"), "a\\x7Fb");     }      #[test]     fn strips_bel_and_null() {-        assert_eq!(sanitize_for_terminal("a\x07b"), "a\\x07b");-        assert_eq!(sanitize_for_terminal("a\0b"), "a\\x00b");-    }--    #[test]-    fn strips_newline() {-        assert_eq!(sanitize_for_terminal("a\nb"), "a\\x0Ab");+        assert_eq!(sanitize_string("a\x07b"), "a\\x07b");+        assert_eq!(sanitize_string("a\0b"), "a\\x00b");     }      #[test]     fn escape_preserves_information() {         let s = "name\x1bX\x07Y.txt";-        assert_eq!(sanitize_for_terminal(s), "name\\x1BX\\x07Y.txt");+        assert_eq!(sanitize_string(s), "name\\x1BX\\x07Y.txt");     }      #[test]     fn strips_c1_csi_and_osc_initiators() {         // U+009B is CSI, U+009D is OSC; dangerous on 8-bit-control terminals.-        assert_eq!(sanitize_for_terminal("\u{9b}31m"), "\\x9B31m");-        assert_eq!(-            sanitize_for_terminal("\u{9d}0;pwned\u{9c}"),-            "\\x9D0;pwned\\x9C"-        );+        assert_eq!(sanitize_string("\u{9b}31m"), "\\x9B31m");+        assert_eq!(sanitize_string("\u{9d}0;pwned\u{9c}"), "\\x9D0;pwned\\x9C");     }      #[test]     fn strips_bidi_overrides_and_zero_width() {         // Trojan-Source style RLO/LRO that flip rendered order of filename text.         assert_eq!(-            sanitize_for_terminal("safe\u{202E}fil\u{202D}gnp.exe"),+            sanitize_string("safe\u{202E}fil\u{202D}gnp.exe"),             "safe\\u{202E}fil\\u{202D}gnp.exe"         );         // Zero-width space and BOM are also format chars used to disguise filenames.-        assert_eq!(sanitize_for_terminal("a\u{200B}b"), "a\\u{200B}b");-        assert_eq!(sanitize_for_terminal("\u{FEFF}name"), "\\u{FEFF}name");+        assert_eq!(sanitize_string("a\u{200B}b"), "a\\u{200B}b");+        assert_eq!(sanitize_string("\u{FEFF}name"), "\\u{FEFF}name");     }      #[test]@@ -148,10 +160,7 @@ mod tests {             "icon\u{E000}.cfg",             "cjk\u{6F22}\u{E0101}.txt",         ] {-            assert!(-                matches!(sanitize_for_terminal(s), Cow::Borrowed(_)),-                "{s:?}"-            );+            assert_eq!(sanitize_string(s), s);         }     } @@ -159,10 +168,9 @@ mod tests {     fn maybe_sanitize_passthrough_when_not_terminal() {         let attack = "x\x1by";         // Pipe context: bytes pass through unchanged (zero-copy).-        let out = maybe_sanitize(attack, false);-        assert!(matches!(out, Cow::Borrowed(_)));+        let out = sanitize_for_term(attack, false).to_string();         assert_eq!(out, attack);         // TTY context: escapes apply.-        assert_eq!(maybe_sanitize(attack, true), "x\\x1By");+        assert_eq!(sanitize_for_term(attack, true).to_string(), "x\\x1By");     } }