sanatize errors from terminal control characters

docs/REQUIREMENTS.md

@@ -91,7 +91,7 @@ and callers must be able to tell whether to trust it.
 | X1 | No injection through edits: whatever key, subsection and value an edit is given, the edited file parses to the previous entries with exactly that change. A subsection containing a newline or NUL is rejected (as by git). Sections to remove or rename are found by parsing, where git scans lines: git takes a line starting with `[` inside a continued value for a header (removing the entries after it, or rewriting the value), and misses a header after a BOM or after another header on the same line. A file holding only a BOM keeps it first, where git writes the new section before it and the file no longer parses. *verified* Fuzzed: parse ∘ edit ∘ parse, for key and section edits. | MUST |
 | X2 | Parsing untrusted input never panics and is linear in time and memory. Parser and edits are fuzzed continuously. | MUST |
 | X3 | Bounded loads: git's include depth limit, plus at most 1000 files, 16 MiB and 2²⁰ entries and distinct section headers read by one load, counting every read (an include fan-out of 10 per level makes git read 10ⁿ files from a few KB, *verified*; an entry costs about a hundred times its size in memory, so the entry count, not the bytes, bounds memory); `Parse` and edits apply the same limits to their one file; configuration files and includes must be regular files (git hangs on a FIFO and fails on a directory, *verified*). A deliberate improvement over git, reached only by files built to reach it. | MUST |
-| X4 | Errors do not leak or spoof: a syntax error never quotes the file's content (an include may point at a secret file), values in messages are quoted with `%q` (no terminal escape sequences). | MUST |
+| X4 | Errors do not leak or spoof: a syntax error never quotes the file's content (an include may point at a secret file), values in messages are quoted with `%q`, and paths, which a configuration file can choose, have their control characters (newline and tab included), bidirectional controls and non-UTF-8 bytes replaced with `?`: stricter than git 2.47.3 and 2.56.0, which replace only C0 controls but tab and newline, and DEL (*verified*); no terminal escape sequences, forged lines or disguised paths. | MUST |
 | X5 | Trust is checked before the repository is read, as git does: `Discover` refuses (`ErrUntrusted`) a repository that is neither owned by the current user (on Unix, also the `SUDO_UID` user for root; on Windows, the Administrators group for an administrator) nor listed in `safe.directory`, with git's rules for which paths are checked and listed. `safe.directory` is read from the protected scopes only (`Env.LoadProtected`: system, global, command; never `Parse` content or an explicit file), never from the repository's configuration. An `Env` filled by hand is not checked, as git does not check `GIT_DIR`. Each entry's scope is known. *verified* | MUST |
 | X6 | Writes cannot be redirected through the lock file: it is created with `O_CREATE\|O_EXCL` and never through a symbolic link. Following a link at the config path itself is git's behaviour (W7) and is kept; the documentation says that only X5 protects against a hostile `.git` directory. | MUST |
 

errors.go

@@ -3,6 +3,9 @@ package gitconfig
 import (
 	"errors"
 	"fmt"
+	"strings"
+	"unicode"
+	"unicode/utf8"
 )
 
 // The sentinel errors below are returned wrapped in a typed error that says
@@ -20,6 +23,14 @@ import (
 //   - Discover also returns an *EnvError for an environment variable it
 //     cannot interpret, and ErrUntrusted in a *FileError for a repository
 //     git would refuse.
+//
+// Error messages name files, whose paths can come from a configuration
+// file someone else wrote. As in git, but stricter, control characters
+// (newline and tab included), bidirectional controls and bytes that are not
+// UTF-8 are replaced with '?', so that a path cannot drive the terminal it
+// is printed to, nor forge a line or disguise itself. The fields of the
+// errors, and Entry, Origin and Key, hold the raw data: quote them before
+// printing them yourself.
 var (
 	// ErrNotFound is returned when the section or subsection to rename is
 	// absent. Removing something absent is not an error.
@@ -84,7 +95,7 @@ type KeyError struct {
 }
 
 func (e *KeyError) Error() string {
-	return fmt.Sprintf("%v: %q", e.Err, e.Key)
+	return sanitize(fmt.Sprintf("%v: %q", e.Err, e.Key))
 }
 
 func (e *KeyError) Unwrap() error {
@@ -104,7 +115,8 @@ func (e *FileError) Error() string {
 	if se, ok := errors.AsType[*SyntaxError](e.Err); ok && se.File == e.Path {
 		return se.Error()
 	}
-	return fmt.Sprintf("gitconfig: %s: %v", e.Path, e.Err)
+	// The wrapped error may name a path too (*fs.PathError).
+	return sanitize(fmt.Sprintf("gitconfig: %s: %v", e.Path, e.Err))
 }
 
 func (e *FileError) Unwrap() error {
@@ -121,7 +133,7 @@ type EnvError struct {
 }
 
 func (e *EnvError) Error() string {
-	return fmt.Sprintf("gitconfig: %s: %v", e.Var, e.Err)
+	return sanitize(fmt.Sprintf("gitconfig: %s: %v", e.Var, e.Err))
 }
 
 func (e *EnvError) Unwrap() error {
@@ -138,7 +150,7 @@ type SyntaxError struct {
 }
 
 func (e *SyntaxError) Error() string {
-	return fmt.Sprintf("gitconfig: %s line %d in file %s", e.Msg, e.Line, e.File)
+	return sanitize(fmt.Sprintf("gitconfig: %s line %d in file %s", e.Msg, e.Line, e.File))
 }
 
 // ValueError reports a value that cannot be read as the requested type. Its
@@ -167,9 +179,34 @@ func (e *ValueError) Error() string {
 	if e.Err != nil {
 		msg += ": " + e.Err.Error()
 	}
-	return msg
+	return sanitize(msg)
 }
 
 func (e *ValueError) Unwrap() error {
 	return e.Err
 }
+
+// sanitize replaces with '?' what could drive a terminal or disguise a
+// path: control characters (C0, DEL, C1), bidirectional controls, and bytes
+// that are not UTF-8. git does this for C0 and DEL only, but keeps tab and
+// newline; no message of this package spans lines, and a newline in a path
+// could forge one.
+func sanitize(msg string) string {
+	i := 0
+	for i < len(msg) && ' ' <= msg[i] && msg[i] < 0x7f {
+		i++
+	}
+	if i == len(msg) {
+		return msg
+	}
+	var sb strings.Builder
+	sb.WriteString(msg[:i])
+	for _, r := range msg[i:] {
+		// An invalid byte reads as utf8.RuneError, which is printed as '?' too.
+		if r < ' ' || 0x7f <= r && r <= 0x9f || r == utf8.RuneError || unicode.Is(unicode.Bidi_Control, r) {
+			r = '?'
+		}
+		sb.WriteRune(r)
+	}
+	return sb.String()
+}

errors_test.go

@@ -1,6 +1,8 @@
 package gitconfig_test
 
 import (
+	"fmt"
+	"io/fs"
 	"path/filepath"
 	"testing"
 
@@ -58,6 +60,52 @@ func TestErrorMessagesEscapeControlCharacters(t *testing.T) {
 		assert.NotContains(t, err.Error(), "\r")
 	})
 
+	// Paths are not quoted: as in git, their control characters become '?',
+	// and also newline and tab, C1 and bidirectional controls, and bytes that
+	// are not UTF-8.
+	const evilPath = "p\x1b]52;c;cHduZWQ=\x07\x1b[2J\x7f\t\n\u009b\u202e\x9b é"
+	const shown = "p?]52;c;cHduZWQ=??[2J?????? é"
+
+	t.Run("SyntaxError", func(t *testing.T) {
+		_, err := gitconfig.Parse(evilPath, []byte("[a\n"))
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "in file "+shown)
+	})
+
+	t.Run("ValueError file", func(t *testing.T) {
+		e := gitconfig.Entry{Key: gitconfig.Key{Section: "a", Name: "k"}, Value: "x", Origin: gitconfig.Origin{File: evilPath}}
+		_, err := e.Bool()
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "in file "+shown)
+	})
+
+	t.Run("FileError", func(t *testing.T) {
+		err := error(&gitconfig.FileError{Path: evilPath, Err: gitconfig.ErrLocked})
+		assert.Equal(t, "gitconfig: "+shown+": "+gitconfig.ErrLocked.Error(), err.Error())
+
+		// A wrapped *fs.PathError names the path again. It is made by hand:
+		// opening such a name fails differently on each system.
+		err = &gitconfig.FileError{Path: "f", Err: &fs.PathError{Op: "open", Path: evilPath, Err: fs.ErrNotExist}}
+		require.ErrorIs(t, err, fs.ErrNotExist)
+		assert.NotContains(t, err.Error(), "\x1b")
+		assert.Contains(t, err.Error(), shown)
+	})
+
+	t.Run("include path", func(t *testing.T) {
+		// Only the deepest file includes the path, and the depth limit
+		// stops it there: it is never opened, which Windows would refuse.
+		dir := tempDir(t)
+		for i := range 10 {
+			writeFile(t, filepath.Join(dir, fmt.Sprintf("f%d", i)), fmt.Sprintf("[include]\n\tpath = f%d\n", i+1))
+		}
+		writeFile(t, filepath.Join(dir, "f10"), "[include]\n\tpath = \"x\x1b]52;c;cHduZWQ=\x07\"\n")
+		_, err := gitconfig.Env{GlobalConfig: []string{filepath.Join(dir, "f0")}}.Load()
+		require.ErrorIs(t, err, gitconfig.ErrIncludeDepth)
+		assert.NotContains(t, err.Error(), "\x1b")
+		assert.NotContains(t, err.Error(), "\x07")
+		assert.Contains(t, err.Error(), "x?]52;c;cHduZWQ=?")
+	})
+
 	t.Run("EnvError", func(t *testing.T) {
 		for _, name := range []string{"GIT_CONFIG_COUNT", "GIT_CONFIG_NOSYSTEM"} {
 			cleanGitEnv(t)