src/cli.rs

@@ -11,7 +11,6 @@ use clap::{ use clap_complete::Shell; use normpath::PathExt; -use crate::error::print_error; use crate::exec::CommandSet; use crate::filesystem; #[cfg(unix)]@@ -711,10 +710,12 @@ impl Opts {                 if filesystem::is_existing_directory(path) {                     Some(self.normalize_path(path))                 } else {-                    print_error(format!(-                        "Search path '{}' is not a directory.",-                        path.to_string_lossy()-                    ));+                    // We use debug for the path to make it more readable if it has special characters+                    // Should we use a more reliable escape?+                    print_error!(+                        "Search path {:?} is not a directory.",+                        path.to_string_lossy().as_ref()+                    );                     None                 }             })

src/error.rs

@@ -1,3 +1,51 @@-pub fn print_error(msg: impl std::fmt::Display) {-    eprintln!("[fd error]: {msg}");+use std::fmt::Display;+use std::io;++use crate::sanitize::write_sanitized;++/// Print an error message using a format string, sanitizing any arguments if necessary+macro_rules! print_error {+    ($fmt:literal, $($arg:expr),*) => {+        eprintln!(concat!("[fd error]: ", $fmt), $($crate::error::SanitizeErr::sanitize($arg)),*)+    }+}++/// Trait for specifying how to sanitize a type for error display, if needed.+pub(crate) trait SanitizeErr {+    type Sanitized: Display;++    fn sanitize(self) -> Self::Sanitized;+}++impl SanitizeErr for &str {+    type Sanitized = Self;++    fn sanitize(self) -> Self {+        self+    }+}++impl SanitizeErr for io::Error {+    type Sanitized = SanitizedError<io::Error>;++    fn sanitize(self) -> Self::Sanitized {+        SanitizedError(self)+    }+}++impl SanitizeErr for ignore::Error {+    type Sanitized = SanitizedError<ignore::Error>;++    fn sanitize(self) -> Self::Sanitized {+        SanitizedError(self)+    }+}++pub struct SanitizedError<E>(E);++impl<E: std::error::Error> Display for SanitizedError<E> {+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {+        let raw = self.0.to_string();+        write_sanitized(f, &raw)+    } }

src/exec/command.rs

@@ -3,7 +3,6 @@ use std::io::Write;  use argmax::Command; -use crate::error::print_error; use crate::exit_codes::ExitCode;  struct Outputs {@@ -101,14 +100,14 @@ pub fn execute_commands<I: Iterator<Item = io::Result<Command>>>( pub fn handle_cmd_error(cmd: Option<&Command>, err: io::Error) -> ExitCode {     match (cmd, err) {         (Some(cmd), err) if err.kind() == io::ErrorKind::NotFound => {-            print_error(format!(-                "Command not found: {}",-                cmd.get_program().to_string_lossy()-            ));+            print_error!(+                "Command not found: {:?}",+                cmd.get_program().to_string_lossy().as_ref()+            );             ExitCode::GeneralError         }         (_, err) => {-            print_error(format!("Problem while executing command: {err}"));+            print_error!("Problem while executing command: {}", err);             ExitCode::GeneralError         }     }

src/exec/job.rs

@@ -1,5 +1,4 @@ use crate::config::Config;-use crate::error::print_error; use crate::exit_codes::{ExitCode, merge_exitcodes}; use crate::walk::WorkerResult; @@ -24,7 +23,7 @@ pub fn job(             WorkerResult::Entry(dir_entry) => dir_entry,             WorkerResult::Error(err) => {                 if config.show_filesystem_errors {-                    print_error(err.to_string());+                    print_error!("{}", err);                 }                 continue;             }@@ -54,7 +53,7 @@ pub fn batch(             WorkerResult::Entry(dir_entry) => Some(dir_entry.into_stripped_path(config)),             WorkerResult::Error(err) => {                 if config.show_filesystem_errors {-                    print_error(err.to_string());+                    print_error!("{}", err);                 }                 None             }

src/main.rs

@@ -1,7 +1,10 @@+// needs to be first because it defines a macro+#[macro_use]+mod error;+ mod cli; mod config; mod dir_entry;-mod error; mod exec; mod exit_codes; mod filesystem;@@ -66,7 +69,9 @@ fn main() {             exit_code.exit();         }         Err(err) => {-            crate::error::print_error(format!("{err:#}"));+            // NB: we use eprintln directly instead of print_error!() because+            // we sanitize anyhow errors at the generation site+            eprintln!("[fd error]: {:#}", err);             ExitCode::GeneralError.exit();         }     }@@ -132,14 +137,14 @@ fn set_working_dir(opts: &Opts) -> Result<()> {     if let Some(ref base_directory) = opts.base_directory {         if !filesystem::is_existing_directory(base_directory) {             return Err(anyhow!(-                "The '--base-directory' path '{}' is not a directory.",-                base_directory.to_string_lossy()+                "The '--base-directory' path {:?} is not a directory.",+                base_directory.to_string_lossy().as_ref()             ));         }         env::set_current_dir(base_directory).with_context(|| {             format!(-                "Could not set '{}' as the current working directory",-                base_directory.to_string_lossy()+                "Could not set {:?} as the current working directory",+                base_directory.to_string_lossy().as_ref()             )         })?;     }

src/output.rs

@@ -7,7 +7,7 @@ use crate::config::Config; use crate::dir_entry::DirEntry; use crate::fmt::FormatTemplate; use crate::hyperlink::PathUrl;-use crate::sanitize::maybe_sanitize;+use crate::sanitize::sanitize_for_term;  fn replace_path_separator(path: &str, new_path_separator: &str) -> String {     path.replace(std::path::MAIN_SEPARATOR, new_path_separator)@@ -81,7 +81,7 @@ fn print_entry_format<W: Write>(     write!(         stdout,         "{}",-        maybe_sanitize(&s, config.interactive_terminal)+        sanitize_for_term(&s, config.interactive_terminal)     ) } @@ -117,16 +117,30 @@ fn print_entry_colorized<W: Write>(             .style_for_indicator(Indicator::Directory)             .map(Style::to_nu_ansi_term_style)             .unwrap_or_default();-        let safe_parent = maybe_sanitize(&parent_str, config.interactive_terminal);-        write!(stdout, "{}", style.paint(safe_parent.as_ref()))?;+        let safe_parent = sanitize_for_term(&parent_str, config.interactive_terminal);+        // We explicitly use prefix/suffix instead of paint so that we can use Sanitized+        write!(+            stdout,+            "{}{}{}",+            style.prefix(),+            safe_parent,+            style.suffix()+        )?;     }      let style = entry         .style(ls_colors)         .map(Style::to_nu_ansi_term_style)         .unwrap_or_default();-    let safe_basename = maybe_sanitize(&path_str[offset..], config.interactive_terminal);-    write!(stdout, "{}", style.paint(safe_basename.as_ref()))?;+    let safe_basename = sanitize_for_term(&path_str[offset..], config.interactive_terminal);+    // We explicitly use prefix/suffix instead of paint so that we can use Sanitized+    write!(+        stdout,+        "{}{}{}",+        style.prefix(),+        safe_basename,+        style.suffix()+    )?;      print_trailing_slash(         stdout,@@ -150,7 +164,7 @@ fn print_entry_uncolorized_base<W: Write>(     if let Some(ref separator) = config.path_separator {         *path_string.to_mut() = replace_path_separator(&path_string, separator);     }-    let safe = maybe_sanitize(&path_string, config.interactive_terminal);+    let safe = sanitize_for_term(&path_string, config.interactive_terminal);     write!(stdout, "{safe}")?;     print_trailing_slash(stdout, entry, config, None) }

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");     } }

src/walk.rs

@@ -17,7 +17,6 @@ use regex::bytes::Regex;  use crate::config::Config; use crate::dir_entry::DirEntry;-use crate::error::print_error; use crate::exec; use crate::exit_codes::{ExitCode, merge_exitcodes}; use crate::filesystem;@@ -226,7 +225,7 @@ impl<'a, W: Write> ReceiverBuffer<'a, W> {                         }                         WorkerResult::Error(err) => {                             if self.config.show_filesystem_errors {-                                print_error(err.to_string());+                                print_error!("{}", err);                             }                         }                     }@@ -253,7 +252,7 @@ impl<'a, W: Write> ReceiverBuffer<'a, W> {         if let Err(e) = output::print_entry(&mut self.stdout, entry, self.config)             && e.kind() != ::std::io::ErrorKind::BrokenPipe         {-            print_error(format!("Could not write to output: {e}"));+            print_error!("Could not write to output: {}", e);             return Err(ExitCode::GeneralError);         } @@ -377,7 +376,7 @@ impl WorkerState {                 match result {                     Some(ignore::Error::Partial(_)) => (),                     Some(err) => {-                        print_error(format!("Malformed pattern in global ignore file. {err}."));+                        print_error!("Malformed pattern in global ignore file. {}.", err);                     }                     None => (),                 }@@ -389,7 +388,7 @@ impl WorkerState {             match result {                 Some(ignore::Error::Partial(_)) => (),                 Some(err) => {-                    print_error(format!("Malformed pattern in custom ignore file. {err}."));+                    print_error!("Malformed pattern in custom ignore file. {}.", err);                 }                 None => (),             }

tests/testenv/mod.rs

@@ -196,6 +196,22 @@ impl TestEnv {         Ok(broken_symlink_link)     } +    pub fn create_symlink<P: AsRef<Path>>(+        &mut self,+        target: P,+        link_path: P,+    ) -> io::Result<PathBuf> {+        let root = self.test_root();+        let target_path = root.join(target);+        let link_path = root.join(link_path);+        #[cfg(unix)]+        unix::fs::symlink(&target_path, &link_path)?;+        #[cfg(windows)]+        windows::fs::symlink_file(&target_path, &link_path)?;++        Ok(link_path)+    }+     /// Get the root directory for the tests.     pub fn test_root(&self) -> PathBuf {         self.temp_dir.path().to_path_buf()

tests/tests.rs

@@ -345,7 +345,7 @@ fn test_multi_file_with_missing() {      te.assert_error(         &["a.foo", "real", "fake"],-        "[fd error]: Search path 'fake' is not a directory.",+        "[fd error]: Search path \"fake\" is not a directory.",     );      te.assert_output(@@ -362,14 +362,14 @@ fn test_multi_file_with_missing() {      te.assert_error(         &["", "real", "fake1", "fake2"],-        "[fd error]: Search path 'fake1' is not a directory.-        [fd error]: Search path 'fake2' is not a directory.",+        "[fd error]: Search path \"fake1\" is not a directory.+        [fd error]: Search path \"fake2\" is not a directory.",     );      te.assert_failure_with_error(         &["", "fake1", "fake2"],-        "[fd error]: Search path 'fake1' is not a directory.-        [fd error]: Search path 'fake2' is not a directory.+        "[fd error]: Search path \"fake1\" is not a directory.+        [fd error]: Search path \"fake2\" is not a directory.         [fd error]: No valid search paths given.",     ); }@@ -2887,3 +2887,33 @@ fn test_ignore_contain_precedence_over_root_check() {     let expected = "";     te.assert_output(&["--ignore-contain=CACHEDIR.TAG", "."], expected); }++// The error message is probably OS-specific.+// This is also somewhat fragile as it depends on the error message+// from creating an executable, and the debug formatting of strings+// could possibly change in the future.+#[cfg(unix)]+#[test]+fn test_sanitize_exec_error_msg() {+    let mut te = TestEnv::new(&[], &[]);+    te.create_broken_symlink("Hello\x1b\r World!\x7fwith\u{9b}\u{1F600}\u{200B}a\u{FEFF}b")+        .expect("failed to create symlink");++    te.assert_error(&["Hello", "--exec", "{}"], +        "[fd error]: Command not found: \"./Hello\\u{1b}\\r World!\\u{7f}with\\u{9b}\u{1F600}\\u{200b}a\\u{feff}b\""+        );+}++// Windows doesn't let us make files containing \x1b so only test on unix+#[cfg(unix)]+#[test]+fn test_sanitize_recursive_link_msg() {+    let mut te = TestEnv::new(&["loop"], &[]);+    te.create_symlink("loop", "loop/foo\x1bb")+        .expect("failed to create symlink");++    te.assert_error(+        &["--follow", "--show-errors", "foo"],+        "[fd error]: File system loop found: ./loop/foo\\x1Bb points to an ancestor ./loop",+    );+}