improve/simplify trusted dir handling

.github/workflows/test.yml

@@ -29,3 +29,7 @@ jobs:
 
       # Includes the interop tests against the runner's git.
       - run: go test ./...
+
+      # The trust tests give files to another user, which needs root.
+      - if: runner.os != 'Windows'
+        run: sudo -E env "PATH=$PATH" go test -count=1 -run Trust ./...

discover.go

@@ -3,6 +3,7 @@ package gitconfig
 import (
 	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -24,7 +25,7 @@ func get(env environ, key string) string {
 	return v
 }
 
-func discover(gitDir string, env environ) (Env, error) {
+func discover(dir string, env environ) (Env, error) {
 	var e Env
 
 	e.Home = get(env, "HOME")
@@ -77,23 +78,74 @@ func discover(gitDir string, env environ) (Env, error) {
 	}
 	e.Command = append(cmd, params...)
 
-	if gitDir != "" {
-		e.GitDir, err = resolveGitDir(gitDir)
-		if err != nil {
+	if dir != "" {
+		var owned []string
+		if e.GitDir, owned, err = findRepo(dir); err != nil {
+			return Env{}, err
+		}
+		if err := e.checkTrust(owned, get(env, "SUDO_UID")); err != nil {
 			return Env{}, err
 		}
 	}
 	return e, nil
 }
 
-// resolveGitDir makes gitDir absolute and follows a "gitdir: <path>" file.
-func resolveGitDir(gitDir string) (string, error) {
-	gitDir, err := filepath.Abs(gitDir)
-	if err != nil {
-		return "", err
+// findRepo returns the git directory of the repository at dir, as git finds
+// it when started there, and the paths whose owner git checks. In a working
+// tree (dir holds .git, or is a "gitdir:" file), they are the working tree,
+// its .git and the git directory; elsewhere, dir alone. The first is the
+// path safe.directory lists.
+func findRepo(dir string) (gitDir string, owned []string, err error) {
+	if dir, err = filepath.Abs(dir); err != nil {
+		return "", nil, err
+	}
+	dot := filepath.Join(dir, ".git")
+	if fi, err := os.Stat(dir); err == nil && fi.Mode().IsRegular() {
+		dir, dot = filepath.Dir(dir), dir
+	} else if _, err := os.Stat(dot); errors.Is(err, fs.ErrNotExist) {
+		// Also a directory that does not exist (yet): loading finds no
+		// files in it.
+		return dir, []string{dir}, nil
+	} else if err != nil {
+		return "", nil, &FileError{Path: dot, Err: err}
 	}
-	// A directory is the git directory itself. One that does not exist (yet)
-	// is not an error here: loading finds no files in it.
+	gitDir, err = resolveGitDir(dot)
+	return gitDir, []string{dir, dot, gitDir}, err
+}
+
+// checkTrust fails with ErrUntrusted unless the current user owns every
+// path of owned that exists, or safe.directory lists the first.
+func (e Env) checkTrust(owned []string, sudoUID string) error {
+	for _, p := range owned {
+		ok, err := ownedByCurrentUser(p, sudoUID)
+		if errors.Is(err, fs.ErrNotExist) {
+			continue
+		}
+		if err != nil {
+			return &FileError{Path: p, Err: err}
+		}
+		if ok {
+			continue
+		}
+		// Without the repository, as git: its HEAD and path must not
+		// match an includeIf that lists it.
+		p := e
+		p.GitDir = ""
+		c, err := p.LoadProtected()
+		if err != nil {
+			return err
+		}
+		if !safeDirectory(c, owned[0], e.Home) {
+			return &FileError{Path: owned[0], Err: ErrUntrusted}
+		}
+		return nil
+	}
+	return nil
+}
+
+// resolveGitDir follows a "gitdir: <path>" file; any other path is the git
+// directory itself. gitDir is absolute.
+func resolveGitDir(gitDir string) (string, error) {
 	fi, err := os.Stat(gitDir)
 	if err != nil || !fi.Mode().IsRegular() {
 		return gitDir, nil

doc.go

@@ -13,8 +13,8 @@
 //
 // Reading the effective configuration of a repository and changing a value:
 //
-//	env, err := gitconfig.Discover(".git")
-//	if err != nil { ... }
+//	env, err := gitconfig.Discover(".") // the working tree
+//	if err != nil { ... }               // ErrUntrusted, as git refuses it
 //
 //	cfg, err := env.Load()
 //	if err != nil { ... }

docs/COMPATIBILITY.md

@@ -73,7 +73,12 @@ in [GIT-BEHAVIOUR.md](GIT-BEHAVIOUR.md).
 | System file | set at build time (`$(prefix)/etc/gitconfig`) | `/etc/gitconfig` on Unix, none elsewhere | a git built with another prefix reads another file; callers can set `SystemConfig` |
 | `safe.directory` entry that does not resolve | skipped | the same | 1 |
 | `safe.directory = .` | trusted at the top of a repository | skipped | needs the working directory — *safe default* |
-| Repository ownership check | done by git | left to the caller | platform specific (X5) |
+| Repository ownership check | done when git finds the repository, not for `GIT_DIR` | done by `Discover`, not for an `Env` filled by hand | 1 (X5) |
+| What is checked, and listed in `safe.directory` | depends on where git starts: a working tree, or a git directory | the same, from the path given to `Discover` | 1 — the caller should pass the working tree, which users list |
+| Untrusted repository | `git config` silently skips the local scope; other commands fail | `Discover` fails with `ErrUntrusted` | a silently missing scope surprises; reading less is not safer than refusing — *safe default* |
+| `SUDO_UID` = `' 1000'`, `+1000`, or beyond 32 bits | accepted; a value beyond 32 bits wraps around (2³² + 1000 is 1000) | ignored: digits only, at most 32 bits | the wrap-around trusts a user nobody named — *safe default* |
+| Administrators group on Windows | trusted for an administrator (Git for Windows commit messages, 2022 and 2025) | the same | 1 — not observed: no Windows git to run but CI's |
+| UNC paths on Windows | must be listed in `safe.directory` since Git for Windows 2.54 (release notes) | owner checked like any path | the release note is not precise enough to follow; not observed |
 | Paths on Windows | some written with `/` (`HOME`), some with `\` | the caller's separators, kept | matching git's spelling serves no one |
 
 ## Editing

docs/GIT-BEHAVIOUR.md

@@ -239,3 +239,64 @@ the same results.
 | `hasconfig:remote.*.url:https://example.com/**` | no until a matching remote is configured, then yes |
 | `git -c 'includeIf.gitdir:./.path=…'` | `error: relative config include conditionals must come from files`, then git goes on without the include, exit 0 *(2.47.3)* — this library fails with `ErrNotAbsolute`, having no one to warn |
 | `gitdir:C:/x/link/`, `GIT_DIR=C:\x\link\proj\.git` with `link` a symbolic link to `work` (Windows) | **no**, while `gitdir:C:/x/work/` (the real path) matches *(2.55.0.windows.5)* — this library compares both paths with `/` and matches |
+
+## Ownership and `safe.directory`
+
+Observed with **git 2.56.0** (`alpine:edge`, Linux), as user `alice` (uid
+1000) unless stated, `bob` being uid 1001. "Refused" means `fatal: detected
+dubious ownership in repository at '<path>'`, exit 128. The repository is
+`/r/w` (working tree) with `.git` inside, all owned by `alice` except the
+path named.
+
+| Owned by `bob` | Result |
+|---|---|
+| nothing | accepted |
+| `/r/w` (top of the working tree) | refused, at `/r/w` — also when run from a subdirectory owned by `alice` |
+| `/r/w/.git` (directory) | refused, at `/r/w` |
+| `/r/w/.git` as a `gitdir:` file | refused, at `/r/w` |
+| the directory a `gitdir:` file points to | refused, at `/r/w` |
+| `/r/w/.git/config` | accepted: files inside the git directory are not checked |
+| `/r` (parent of the working tree) | accepted |
+| a symbolic link to `/r/w` (`chown -h`), used as `-C` | accepted: only the target counts |
+| a bare repository `/r/b.git` | refused, at `/r/b.git` |
+| `/r/b.git/config` in a bare repository | accepted |
+| linked worktree `/r/lw`: `/r/w/.git/worktrees/lw` | refused, at `/r/lw` |
+| linked worktree `/r/lw`: `/r/w/.git` or `/r/w` | accepted: the main repository is not checked |
+| `/r/w`, mode 777, or group `alice` | refused: only the owner uid counts |
+
+| Setup | Result |
+|---|---|
+| `/r/w/.git` owned by `bob`, `-c safe.directory=/r/w` | accepted |
+| `/r/w/.git` owned by `bob`, `-c safe.directory=/r/w/.git` | refused: the entry must name the working tree, whichever path failed |
+| `gitdir:` target owned by `bob`, `safe.directory` = the working tree / the target | accepted / refused |
+| bare `/r/b.git` owned by `bob`, `safe.directory=/r/b.git` | accepted |
+| `/r/w` owned by `bob`, `GIT_DIR=/r/w/.git` (with or without `GIT_WORK_TREE`) | accepted: nothing is checked when the repository is given rather than discovered |
+| `/r/w` owned by `bob`, `git config --show-scope --get-regexp foo` (`foo.bar` in `.git/config`) | exit 1, no output: the repository is treated as absent, its configuration silently skipped |
+| the same, `git config --local --list` | `fatal: --local can only be used inside a git repository`, exit 128 |
+| the same, `git rev-parse --show-toplevel` | refused |
+| as root, repository owned by `alice`, no `SUDO_UID` | refused |
+| as root, owned by `alice`, `SUDO_UID=1000` | accepted |
+| as root, owned by `alice`, `SUDO_UID=1001` or `SUDO_UID=abc` | refused |
+| as root, owned by root, `SUDO_UID=1000` | accepted: root is still trusted |
+| as `alice`, owned by `bob`, `SUDO_UID=1001` | refused: `SUDO_UID` counts only for root |
+| as root, owned by `alice`, `SUDO_UID` = `' 1000'`, `+1000`, `01000` | accepted |
+| as root, owned by `alice`, `SUDO_UID` = `'1000 '`, `1000x` | refused |
+| as root, owned by `alice`, `SUDO_UID=4294968296` (2³² + 1000) | accepted: the value wraps around to 1000 |
+
+What is checked depends on where git starts, not on the layout of the
+repository: started in a working tree, git checks the working tree, its
+`.git` and the git directory, and `safe.directory` must name the working
+tree; started in a git directory, it checks that directory alone, and
+`safe.directory` must name it.
+
+| Setup (`bob` owns the path named) | Result |
+|---|---|
+| normal repository, `git -C /r/w/.git`, `/r/w` owned by `bob` | accepted: the working tree is not checked |
+| normal repository, `git -C /r/w/.git`, `.git` owned by `bob`, `safe.directory=/r/w` / `=/r/w/.git` | refused, at `/r/w/.git` / accepted |
+| bare repository named `.git` at `/r/p/.git`, `/r/p` owned by `bob`, `git -C /r/p/.git` | accepted |
+| the same, `git -C /r/p` | refused, at `/r/p` (git looks for `/r/p/.git` from there) |
+| bare `/r/p/.git` owned by `bob`, `safe.directory=/r/p/.git` / `=/r/p` | accepted / refused, at `/r/p/.git` |
+| bare `/r/dots` with `core.worktree=/r/home` (and `core.bare=false`), `/r/home` owned by `bob` | accepted: `core.worktree` is not checked |
+| the same, `/r/dots` owned by `bob`, `safe.directory=/r/dots` / `=/r/home` | accepted / refused, at `/r/dots` |
+| `--separate-git-dir=/r/sep/x` for `/r/w`, `/r/w` owned by `bob`, `git -C /r/sep/x` | accepted |
+| linked worktree `/r/lw` owned by `bob`, `git -C /r/w/.git/worktrees/lw` / `git -C /r/lw` | accepted / refused, at `/r/lw` |

docs/INTEGRATION.md

@@ -25,9 +25,11 @@ Usage outside `repository/`: `ReadString` ×5, `RemoveAll` ×4, `ReadAll` ×3,
 
 ### Mapping
 
-`env, err := gitconfig.Discover(gitDir)` once per repository (a `.git`
-file of a linked worktree is followed); the `Env` is a plain value and can be
-kept.
+`env, err := gitconfig.Discover(workTree)` once per repository (the
+working tree, or the git directory of a bare repository; a `.git` file of a
+linked worktree is followed); the `Env` is a plain value and can be kept.
+`Discover` refuses a repository git would refuse (`ErrUntrusted`): git-bug
+gets git's `safe.directory` protection without code of its own.
 
 | git-bug | gitconfig |
 |---|---|
@@ -119,5 +121,6 @@ Adoption does not need the full RFC first:
 
 go-git-specific needs to keep in mind: `url.insteadOf`/`pushInsteadOf`
 (go-git#865, #844), `http.<url>.*` matching (go-git#474), `safe.directory`
-(`Env.SafeDirectory`, plus an ownership check go-git must do itself), `core.sharedRepository` permissions on written files
+and the ownership check (done by `Discover`; go-git must pass the working
+tree, and not build the `Env` by hand for a repository it found), `core.sharedRepository` permissions on written files
 (go-git#843, #1572) — set by go-git's own write path.

docs/REQUIREMENTS.md

@@ -92,7 +92,7 @@ and callers must be able to tell whether to trust it.
 | 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 and 64 MiB 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*); 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 |
-| X5 | Trust is decidable before the repository is read: `Env.LoadProtected` loads only the protected scopes (system, global, command; never `Parse` content or an explicit file) and `Env.SafeDirectory` applies git's `safe.directory` matching to them, without reading the repository's configuration. Each entry's scope is known. The ownership check and the policy belong to the caller. | 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 |
 
 ## 7. Won't

env.go

@@ -55,9 +55,10 @@ func (s Scope) String() string {
 //
 // Configuration can come from someone else: a repository on a shared disk,
 // an unpacked archive, a file reached through include.path. Loading such a
-// file is safe (bounded, see Load), but its values are not to be trusted:
-// check SafeDirectory before loading or editing a repository's
-// configuration, as git does.
+// file is safe (bounded, see Load), but its values are not to be trusted.
+// Discover refuses a repository git would refuse (ErrUntrusted); an Env
+// filled by hand is not checked, as git does not check a repository named
+// by GIT_DIR.
 type Env struct {
 	// Home is the user's home directory. Load uses it to expand "~/" in
 	// include paths; pass it to Config.Path and Entry.Path for values.
@@ -104,13 +105,20 @@ type CommandEntry struct {
 // XDG_CONFIG_HOME, …), the user's home directory, the working directory and
 // the platform's system configuration file.
 //
-// gitDir is the repository's git directory, such as ".git", or the
-// repository itself for a bare one; it is made absolute. If gitDir is a file
-// holding "gitdir: <path>", as .git is in a linked worktree or a submodule,
-// the path it names is used. Finding the repository is left to the caller:
-// Discover does not search parent directories and does not read GIT_DIR.
-// Pass "" when not in a repository: only the system, global and command-line
-// scopes exist then.
+// dir is where the repository is found, as git finds it when started there
+// (git -C dir): a working tree (a directory holding .git), its .git, or a
+// git directory, such as a bare repository; it is made absolute. A .git
+// file holding "gitdir: <path>", as in a linked worktree or a submodule, is
+// followed. Finding the repository is left to the caller: Discover does not
+// search parent directories and does not read GIT_DIR. Pass "" when not in
+// a repository: only the system, global and command-line scopes exist then.
+//
+// The repository must be trusted, as git requires: owned by the current
+// user (on Unix, also the SUDO_UID user for root; on Windows, the
+// Administrators group for an administrator), or listed in safe.directory.
+// Otherwise Discover fails with ErrUntrusted, in a *FileError naming the
+// path to list. As in git, that path is the working tree when dir is one,
+// dir itself otherwise: pass the working tree when there is one.
 //
 // Without a home directory, Home is "" and the global files are those git
 // reads then: the XDG file if XDG_CONFIG_HOME is set, no ~/.gitconfig.
@@ -125,8 +133,8 @@ type CommandEntry struct {
 //
 // It is the only function of this package that reads the process
 // environment.
-func Discover(gitDir string) (Env, error) {
-	return discover(gitDir, osEnviron{})
+func Discover(dir string) (Env, error) {
+	return discover(dir, osEnviron{})
 }
 
 // Load returns the effective configuration: system, global, local, worktree
@@ -201,7 +209,7 @@ func (l *loader) global() error {
 // includes followed: system, global and command line, in that order. This is
 // what git uses for keys a repository must not be able to set, such as
 // safe.directory or safe.bareRepository. The repository's configuration is
-// not read, so this is safe to call before SafeDirectory.
+// not read.
 func (e Env) LoadProtected() (*Config, error) {
 	l, err := e.newLoader()
 	if err != nil {
@@ -219,21 +227,11 @@ func (e Env) LoadProtected() (*Config, error) {
 	return l.config(), nil
 }
 
-// SafeDirectory reports whether dir is allowed by the safe.directory values
-// of LoadProtected, with git's matching rules ("*", a trailing "/*", an empty
-// value resetting the list). dir is the repository's working tree, or its
-// git directory for a bare repository, as git checks. It may be relative to
-// the working directory; symbolic links are resolved on both sides.
-//
-// The repository's configuration is not read. It does not check who owns
-// dir: git trusts a repository owned by the current user without consulting
-// safe.directory, and that check is platform specific and left to the
-// caller. A false result therefore means "not listed", not "unsafe".
-func (e Env) SafeDirectory(dir string) (bool, error) {
-	c, err := e.LoadProtected()
-	if err != nil {
-		return false, err
-	}
+// safeDirectory reports whether dir is allowed by the safe.directory values
+// of c, with git's matching rules ("*", a trailing "/*", an empty value
+// resetting the list). dir may be relative to the working directory;
+// symbolic links are resolved on both sides.
+func safeDirectory(c *Config, dir, home string) bool {
 	dir = realPath(dir)
 	if !filepath.IsAbs(dir) {
 		// The entries that count are absolute. Joining the real working
@@ -250,7 +248,7 @@ func (e Env) SafeDirectory(dir string) (bool, error) {
 		case v == "*":
 			safe = true
 		case strings.HasSuffix(v, "/*"):
-			prefix, err := expandPath(strings.TrimSuffix(v, "*"), e.Home)
+			prefix, err := expandPath(strings.TrimSuffix(v, "*"), home)
 			if err != nil || !filepath.IsAbs(prefix) {
 				// As for exact entries, relative ones are skipped.
 				continue
@@ -261,7 +259,7 @@ func (e Env) SafeDirectory(dir string) (bool, error) {
 				}
 			}
 		default:
-			p, err := expandPath(v, e.Home)
+			p, err := expandPath(v, home)
 			if err != nil || !filepath.IsAbs(p) {
 				// "." and unexpandable values are skipped.
 				continue
@@ -271,7 +269,7 @@ func (e Env) SafeDirectory(dir string) (bool, error) {
 			}
 		}
 	}
-	return safe, nil
+	return safe
 }
 
 // realPath returns path with symbolic links resolved, or cleaned if it
@@ -446,7 +444,7 @@ func (e Env) UpdateScope(s Scope, fn func(*File) error) error {
 // Following the symbolic link in step 1 is what git does, and what dotfile
 // managers rely on. It also means that whoever controls the directory of
 // path controls which file is written: do not edit the configuration of a
-// repository you have not checked with SafeDirectory and an ownership check.
+// repository that Discover has not accepted.
 func (e Env) UpdateFile(path string, fn func(*File) error) error {
 	return updateFile(path, ScopeFile, fn)
 }