diff --git a/src/openhuman/tools/impl/filesystem/git_operations.rs b/src/openhuman/tools/impl/filesystem/git_operations.rs index 920d824418..b81b1e147d 100644 --- a/src/openhuman/tools/impl/filesystem/git_operations.rs +++ b/src/openhuman/tools/impl/filesystem/git_operations.rs @@ -90,11 +90,22 @@ impl GitOperationsTool { } async fn run_git_command_in(&self, cwd: &Path, args: &[&str]) -> anyhow::Result { - let output = tokio::process::Command::new("git") - .args(args) - .current_dir(cwd) - .output() - .await?; + if let Some(key) = first_disallowed_repo_config_key(cwd).await? { + tracing::debug!( + "[git_operations] refusing to run git: dir={}, disallowed_config_key={key}", + cwd.display() + ); + anyhow::bail!( + "refusing to run git in {}: its repository config sets `{key}`, which is \ + not on the allowlist of configuration this tool will run under. \ + Several git config keys name a command git then executes, and this \ + directory is agent-writable, so unrecognised configuration is treated \ + as untrusted rather than honoured.", + cwd.display() + ) + } + + let output = hardened_git(cwd).args(args).output().await?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); @@ -423,6 +434,275 @@ impl GitOperationsTool { } } +/// Repository config keys this tool will run `git` under. +/// +/// This is an allowlist, and the direction is the whole point. Several git +/// config keys name a command that git then executes — `core.fsmonitor` is +/// run by `git status` (used by the `status` operation here), `core.sshCommand` +/// by anything that reaches a remote, `diff.external` by `diff`, `core.pager` +/// and `core.editor` by the commands that use them, and the `filter.*.process` +/// / `*.clean` / `*.smudge` and `diff.*.textconv` families by content +/// operations. Enumerating *those* and clearing them is the obvious fix and it +/// is the wrong shape: the list is only correct until git adds a key, and a +/// denylist that has gone stale reads as protection while providing none. +/// +/// An allowlist ages the other way. A key nobody here has heard of is refused, +/// so a new git release makes this tool fail closed and loud rather than +/// silently regain the hole. +/// +/// The entries are `section.key`, lowercased, with any subsection elided — +/// `remote.origin.url` is checked as `remote.url`. +const ALLOWED_REPO_CONFIG: &[&str] = &[ + // What `git init` and `git clone` write, and nothing else. + // + // `core.worktree` is deliberately absent, unlike the read-only sibling + // this list started from. It redirects the working-tree root Git + // operates against — including a linked-worktree-shaped redirect this + // tool does not otherwise offer — and this tool runs *write* operations + // (`checkout`, `add`, `commit`, `stash`) whose target directory is + // supposed to be `action_dir`/the resolved workspace, never something a + // repository's own config gets to name. Worktree isolation is already + // handled through `WorkspaceDescriptor` (`effective_action_dir_for_context`), + // a trusted, in-process mechanism — nothing here needs the config key. + "core.repositoryformatversion", + "core.filemode", + "core.bare", + "core.logallrefupdates", + "core.ignorecase", + "core.precomposeunicode", + "core.symlinks", + "remote.url", + "remote.fetch", + "remote.pushurl", + "remote.mirror", + "branch.remote", + "branch.merge", + "branch.rebase", + "submodule.active", + "submodule.url", + "user.name", + "user.email", + "pull.rebase", + "push.default", + "init.defaultbranch", + // Inert settings ordinary repositories carry that a first-draft allowlist + // would refuse, making the tool useless on a large class of real + // workspaces. Each is a value git *interprets*; none names a program git + // runs. + "core.autocrlf", + "core.eol", + "core.untrackedcache", + "core.longpaths", + "core.fscache", + "core.hidedotfiles", + "core.sparsecheckout", + "core.sparsecheckoutcone", + "commit.gpgsign", + "tag.gpgsign", + "remote.tagopt", + "remote.prune", + "remote.partialclonefilter", + "remote.promisor", + "branch.vscodemerge", + "gc.auto", + "fetch.prune", + // SHA-256 repositories and worktree-scoped config. Only these two + // `extensions.*` keys — the namespace as a whole is where git puts + // repository-format switches, and a blanket allow would admit whatever it + // adds next. + "extensions.objectformat", + "extensions.worktreeconfig", + // `filter..required` is a boolean. The driver's actual programs — + // `clean`, `smudge`, `process` — are NOT here and must not be; see the LFS + // note on `NEUTRALISED_CONFIG`. + "filter.required", + "lfs.repositoryformatversion", +]; + +/// Command-valued keys cleared on the command line as a second layer. +/// +/// Command-line `-c` outranks every config file, so this genuinely +/// neutralises these keys even when a repository sets them — and it does so +/// at the moment `git` actually runs, not at the moment +/// [`first_disallowed_repo_config_key`] happened to inspect the config a +/// moment earlier. That distinction matters: the inspection and the real +/// command are two separate `git` invocations, so a key set in the gap +/// between them (a second concurrent writer, or a worktree-scoped write) is +/// invisible to the inspection but still reaches here — where it is +/// neutralised regardless of timing. It is a denylist and therefore cannot be +/// the *only* guarantee — [`ALLOWED_REPO_CONFIG`] is the one that fails +/// closed on a key nobody anticipated — but this layer is what actually holds +/// under a race, not the inspection step. +/// +/// `credential.helper` is command-valued — a value beginning `!` is run as a +/// shell command — so it belongs nowhere near [`ALLOWED_REPO_CONFIG`] despite +/// reading like a mere preference; it is refused there instead, since none of +/// this tool's operations reach a remote and so have no legitimate use for it +/// to preserve. +/// +/// A `git lfs install` clone is refused, deliberately: `filter.lfs.clean`, +/// `.smudge` and `.process` each name a program, so an LFS working copy +/// cannot be read by this tool. That is the fail-closed answer and it is the +/// intended one. Only `filter..required`, a boolean, is allowed. +/// +/// `core.hooksPath` and `commit.gpgSign` are handled separately, in +/// [`hardened_git`] — see there for why. +const NEUTRALISED_CONFIG: &[&str] = &[ + "core.fsmonitor=", + "core.sshCommand=", + "core.pager=cat", + "core.editor=false", + "diff.external=", + "sequence.editor=false", + "uploadpack.packObjectsHook=", +]; + +/// A path git will read as an empty config file. +/// +/// `GIT_CONFIG_GLOBAL` must name something readable-and-empty rather than be +/// unset — unsetting it lets git fall back to `~/.gitconfig`, which is the +/// thing being suppressed. `/dev/null` is not a path on Windows; `NUL` is. +const NULL_CONFIG_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; + +/// Close the system/global config files and the command-valued `GIT_*` env +/// vars on a `git` invocation, without touching anything about how it reads +/// the repository's own config. +/// +/// `GIT_CONFIG_NOSYSTEM` and `GIT_CONFIG_GLOBAL` close the system and global +/// config files. Note what they do *not* close: the repository's own local +/// and worktree-scoped config, which is what an agent-writable workspace +/// actually lets an attacker author. That is handled by +/// [`first_disallowed_repo_config_key`] — a separate step, precisely because +/// [`hardened_git`]'s `-c` layer below must not be present while that step is +/// reading what the repository itself set (see its own doc comment). +fn suppress_ambient_git_config(cmd: &mut tokio::process::Command) -> &mut tokio::process::Command { + cmd.env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_GLOBAL", NULL_CONFIG_PATH) + // `git` consults these before it reads any config file. + .env_remove("GIT_EXTERNAL_DIFF") + .env_remove("GIT_PAGER") + .env_remove("GIT_EDITOR") + .env_remove("GIT_SSH") + .env_remove("GIT_SSH_COMMAND") + .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES") + .env("GIT_TERMINAL_PROMPT", "0") +} + +/// Build the `git` invocation actually used to run a requested operation. +/// +/// Layers [`suppress_ambient_git_config`] under the [`NEUTRALISED_CONFIG`] +/// `-c` overrides, which outrank every config file the repository itself +/// could set — including one set in the gap between +/// [`first_disallowed_repo_config_key`]'s inspection and this invocation. +/// +/// Two more `-c` overrides are added here rather than in the static +/// [`NEUTRALISED_CONFIG`] list, because their safe value is not a fixed +/// literal: +/// +/// - `core.hooksPath=` pointed at [`NULL_CONFIG_PATH`]. `commit` and +/// `checkout` are the two operations this tool exposes that run hooks +/// (`pre-commit`/`commit-msg`/`post-commit`, `post-checkout`), and a +/// repository-writable `core.hooksPath` naming a directory with an +/// executable `pre-commit` in it is exactly the shape of the worktree-scoped +/// bypass this hardening exists to close — verified directly: pointing +/// `core.hooksPath` at a directory containing a `pre-commit` that touches a +/// marker file, then running with `-c core.hooksPath=`, leaves +/// the marker untouched. A previous version of this comment claimed there +/// was no portable value that meant "nowhere"; there is — the same one +/// [`suppress_ambient_git_config`] already uses for `GIT_CONFIG_GLOBAL`, +/// since a location with nothing at it is exactly what git needs it to be. +/// - `commit.gpgSign=false`. `commit.gpgsign` is on [`ALLOWED_REPO_CONFIG`] as +/// an ordinary boolean, but a repository could still set it to force every +/// commit through this tool to be GPG-signed — with whatever real signing +/// key the *host* happens to have configured, silently attributing a +/// cryptographic signature to a commit the operator did not ask this tool to +/// sign. Overriding it here removes that decision from the repository +/// entirely, the same way `core.editor=false` removes commit message +/// editing from it in [`NEUTRALISED_CONFIG`] above. +fn hardened_git(dir: &Path) -> tokio::process::Command { + let mut cmd = tokio::process::Command::new("git"); + suppress_ambient_git_config(&mut cmd).current_dir(dir); + for kv in NEUTRALISED_CONFIG { + cmd.arg("-c").arg(kv); + } + cmd.arg("-c") + .arg(format!("core.hooksPath={NULL_CONFIG_PATH}")) + .arg("-c") + .arg("commit.gpgSign=false"); + cmd +} + +/// Normalise a `git config --list` key to the `section.key` form +/// [`ALLOWED_REPO_CONFIG`] uses, dropping any subsection. +/// +/// `remote.origin.url` → `remote.url`; `core.filemode` → `core.filemode`. A +/// subsection may itself contain dots (`includeIf.gitdir:~/x.y/.path`), so the +/// first and last components are the reliable ones. +fn normalise_config_key(key: &str) -> String { + let key = key.to_ascii_lowercase(); + match (key.find('.'), key.rfind('.')) { + (Some(first), Some(last)) if first != last => { + format!("{}.{}", &key[..first], &key[last + 1..]) + } + _ => key, + } +} + +/// Returns the first repository config key at `dir` that is not on +/// [`ALLOWED_REPO_CONFIG`], or `None` if every key it sets is recognised. +/// +/// Deliberately **not** `--local`: when `extensions.worktreeConfig` is set — +/// itself on [`ALLOWED_REPO_CONFIG`] as an ordinary, non-command-valued +/// setting — git additionally reads `config.worktree`, a second file `--local` +/// does not cover. A `core.hooksPath` set with `git config --worktree ...` +/// lands there, is invisible to `--local`, and is exactly the kind of key this +/// check exists to catch: `commit` runs the hook it names. Bare `--list` +/// returns the same merged view `git` itself consults — local *and* +/// worktree-scoped — with system and global excluded by +/// [`suppress_ambient_git_config`] instead of by a location flag. Verified +/// directly: `git config --worktree core.hooksPath ...` followed by +/// `--local --null` omits it; the same followed by a bare `--list --null` +/// (system/global suppressed) reports it. +/// +/// This step must run through [`suppress_ambient_git_config`] alone, **not** +/// [`hardened_git`]: the latter's `-c` layer would inject the very keys this +/// check inspects for (`core.fsmonitor=`, `core.pager=cat`, …), none of which +/// are themselves on [`ALLOWED_REPO_CONFIG`], and every invocation would +/// refuse itself. Reading the config this way also does not consult +/// `core.fsmonitor` or spawn a pager when its output is captured, so this +/// inspection step does not have the property it is checking for. +/// +/// A non-zero exit here is treated as a refusal, not as "nothing to +/// distrust": by the time this runs, the caller (`execute_in_context`) has +/// already confirmed `dir` — or one of its parents — contains a `.git`, so +/// `git config --list` failing means the config could not be read, not that +/// there is none. Proceeding to run the real command against config this step +/// never actually inspected would defeat the point of inspecting it first. +async fn first_disallowed_repo_config_key(dir: &Path) -> anyhow::Result> { + let mut cmd = tokio::process::Command::new("git"); + suppress_ambient_git_config(&mut cmd).current_dir(dir); + let output = cmd.args(["config", "--list", "--null"]).output().await?; + + if !output.status.success() { + anyhow::bail!( + "refusing to run git in {}: could not inspect its repository config \ + ({}), so whether it is safe to run under could not be determined", + dir.display(), + String::from_utf8_lossy(&output.stderr).trim() + ); + } + + let listing = String::from_utf8_lossy(&output.stdout); + for entry in listing.split('\0').filter(|e| !e.is_empty()) { + // `--null` separates entries with NUL and key from value with LF. + let key = entry.split('\n').next().unwrap_or(entry); + if !ALLOWED_REPO_CONFIG.contains(&normalise_config_key(key).as_str()) { + return Ok(Some(key.to_string())); + } + } + Ok(None) +} + #[async_trait] impl Tool for GitOperationsTool { fn name(&self) -> &str { diff --git a/src/openhuman/tools/impl/filesystem/git_operations_tests.rs b/src/openhuman/tools/impl/filesystem/git_operations_tests.rs index abf1bd619b..c04a77deea 100644 --- a/src/openhuman/tools/impl/filesystem/git_operations_tests.rs +++ b/src/openhuman/tools/impl/filesystem/git_operations_tests.rs @@ -368,15 +368,28 @@ async fn not_in_git_repo_returns_error() { assert!(result.output().contains("Not in a git repository")); } +/// Suppress the developer's own system/global git config on a raw +/// `std::process::Command`, so a machine-local `init.templateDir` or similar +/// cannot write extra keys into a test repository's `.git/config` and make +/// these tests depend on ambient environment. Mirrors [`hardened_git`]'s two +/// env vars; the production code under test applies its own suppression when +/// it later reads this same config, so this only affects setup. +fn hermetic(cmd: &mut std::process::Command) -> &mut std::process::Command { + cmd.env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_CONFIG_GLOBAL", NULL_CONFIG_PATH) +} + /// Initialise a git repo at `path` and fail the test if `git init` /// itself didn't succeed (so we don't misread later assertion failures /// as product bugs when the real problem is a missing/broken git). fn init_git_repo(path: &std::path::Path) { - let output = std::process::Command::new("git") - .args(["init"]) - .current_dir(path) - .output() - .expect("failed to spawn `git init`"); + let output = hermetic( + std::process::Command::new("git") + .args(["init"]) + .current_dir(path), + ) + .output() + .expect("failed to spawn `git init`"); assert!( output.status.success(), "`git init` failed: {}", @@ -466,3 +479,450 @@ async fn add_missing_paths_returns_error() { "expected missing-paths error, got: {msg}" ); } + +// ── run_git_command_in: repository config hardening (issue #5494) ───────── +// +// `run_git_command_in` backs every operation this tool exposes, including +// `status`, which — like `read_workspace_state`'s `run_git` before #5493 — +// invokes `core.fsmonitor` from the repository's own `.git/config`. That file +// lives in `action_dir`, which `file_write` and `git_operations` itself +// (`add`, `commit`, `checkout`) can write to, so it is attacker-controlled +// input, not trusted configuration. + +/// Write a `core.fsmonitor` hook into `dir`'s repository config that creates a +/// marker file when git runs it, and return the marker's path. +/// +/// Runs the hook once up front and asserts the marker appears, then removes +/// it — so a later absent marker means the hardening refused the hook, not +/// that the hook itself was silently broken (e.g. by `{:?}`-escaping a path +/// the shell would quote differently than Rust's `Debug` does). +#[cfg(unix)] +fn plant_fsmonitor_hook(dir: &std::path::Path) -> std::path::PathBuf { + let hook = dir.join("hook.sh"); + let marker = dir.join("COMMAND_RAN"); + std::fs::write( + &hook, + format!("#!/bin/sh\ntouch {:?}\nexit 1\n", marker.to_string_lossy()), + ) + .unwrap(); + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&hook, std::fs::Permissions::from_mode(0o755)).unwrap(); + + std::process::Command::new(&hook).status().unwrap(); + assert!(marker.exists(), "the planted hook does not run at all"); + std::fs::remove_file(&marker).unwrap(); + + // Written with `git config` rather than by appending to the file: + // appending only lands in `[core]` while `[core]` happens to be the last + // section, which is true of a fresh `git init` and is not a property + // worth depending on. + let ok = hermetic( + std::process::Command::new("git") + .args(["config", "core.fsmonitor"]) + .arg(&hook) + .current_dir(dir), + ) + .status() + .unwrap() + .success(); + assert!(ok, "failed to plant the hook in the repository config"); + marker +} + +/// Set a repository config key with `git config`, asserting it took. +fn set_config(dir: &std::path::Path, key: &str, value: &str) { + let ok = hermetic( + std::process::Command::new("git") + .args(["config", key, value]) + .current_dir(dir), + ) + .status() + .unwrap() + .success(); + assert!(ok, "failed to set {key} in the test workspace"); +} + +/// Issue #5494. `git status` executes the command named by the workspace's +/// own repository config unless `run_git_command_in` refuses to run under it. +/// Revert the hardening and this test fails by finding the marker — verified, +/// not assumed. +#[cfg(unix)] +#[tokio::test] +async fn repository_config_naming_a_command_does_not_get_to_run_it() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + let marker = plant_fsmonitor_hook(tmp.path()); + + let tool = test_tool(tmp.path()); + let result = tool.execute(json!({"operation": "status"})).await; + let msg = error_text(&result); + + assert!( + !marker.exists(), + "`git status` executed the command named by the workspace's own \ + repository config — this tool is a code-execution primitive" + ); + assert!( + msg.contains("fsmonitor"), + "the refusal should name the key that caused it, got: {msg}" + ); +} + +/// The allowlist has to leave an ordinary repository working, or the fix is +/// just a different way of breaking the tool. +#[tokio::test] +async fn an_ordinary_repository_still_reports_status() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + std::fs::write(tmp.path().join("tracked.txt"), "hi").unwrap(); + + let tool = test_tool(tmp.path()); + let result = tool.execute(json!({"operation": "status"})).await.unwrap(); + + assert!(!result.is_error, "got: {}", result.output()); + assert!( + result.output().contains("tracked.txt"), + "a plain `git init` workspace must still report status, got: {}", + result.output() + ); +} + +/// A first-draft allowlist that refused any repository carrying an ordinary +/// setting like `core.autocrlf` would report nothing useful for a large class +/// of real workspaces. +#[tokio::test] +async fn an_inert_setting_an_ordinary_repository_carries_is_allowed() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + set_config(tmp.path(), "core.autocrlf", "input"); + set_config(tmp.path(), "gc.auto", "0"); + set_config(tmp.path(), "remote.origin.prune", "true"); + + let tool = test_tool(tmp.path()); + let result = tool.execute(json!({"operation": "status"})).await.unwrap(); + + assert!( + !result.is_error && !result.output().contains("not on the allowlist"), + "an ordinary repository must still report status, got: {}", + result.output() + ); +} + +/// The other half of the same question, and the answer is the opposite one. +/// `filter.lfs.clean` names a program, so an LFS working copy is refused — +/// fail-closed, and intended rather than an oversight. +#[tokio::test] +async fn an_lfs_clone_is_refused_because_its_filter_names_a_program() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + // What `git lfs install` writes. `required` is inert and allowed; the + // three programs are not. + set_config(tmp.path(), "filter.lfs.required", "true"); + set_config(tmp.path(), "filter.lfs.clean", "git-lfs clean -- %f"); + + let tool = test_tool(tmp.path()); + let result = tool.execute(json!({"operation": "status"})).await; + let msg = error_text(&result); + + assert!( + msg.contains("filter.lfs.clean"), + "the refusal must name the key that caused it, got: {msg}" + ); +} + +/// `credential.helper` reads like a preference and is command-valued: a value +/// beginning `!` is run as a shell command. It must be refused however inert +/// it reads. +#[tokio::test] +async fn credential_helper_is_refused_despite_looking_like_a_preference() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + set_config(tmp.path(), "credential.helper", "!echo pwned"); + + let tool = test_tool(tmp.path()); + let result = tool.execute(json!({"operation": "status"})).await; + let msg = error_text(&result); + + assert!( + msg.contains("credential.helper"), + "a command-valued key must be refused however inert it reads, got: {msg}" + ); +} + +/// The refusal must hold for a write operation too, not just `status` — the +/// same repository config runs under `commit`/`add`/`checkout`/`stash`. +#[tokio::test] +async fn refusal_also_covers_write_operations() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + set_config(tmp.path(), "credential.helper", "!echo pwned"); + + let tool = test_tool(tmp.path()); + let result = tool + .execute(json!({"operation": "commit", "message": "test"})) + .await + .unwrap(); + + assert!( + result.is_error && result.output().contains("credential.helper"), + "write operations must be refused under untrusted repo config too, got: {}", + result.output() + ); +} + +/// `core.worktree` redirects the working-tree root every write operation +/// here (`checkout`, `add`, `commit`, `stash`) targets. Left on the +/// allowlist, a repository config could point that root outside +/// `action_dir` and turn a supposedly sandboxed write into one against an +/// arbitrary directory. Nothing this tool does needs the key — worktree +/// isolation goes through `WorkspaceDescriptor` instead. +#[tokio::test] +async fn core_worktree_is_refused_because_it_can_redirect_writes_outside_the_sandbox() { + let tmp = TempDir::new().unwrap(); + let elsewhere = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + set_config( + tmp.path(), + "core.worktree", + &elsewhere.path().to_string_lossy(), + ); + + let tool = test_tool(tmp.path()); + let result = tool.execute(json!({"operation": "status"})).await; + let msg = error_text(&result); + + assert!( + msg.contains("core.worktree"), + "the refusal must name the key that caused it, got: {msg}" + ); +} + +/// `extensions.worktreeConfig` is itself allowlisted as an ordinary setting, +/// but turning it on makes git additionally read `config.worktree` — a +/// second file `--local` alone does not see. A `core.hooksPath` set there is +/// invisible to a `--local`-only inspection and would still run on the next +/// `commit`. The inspection step must read the same merged view git does. +#[tokio::test] +async fn a_hookspath_hidden_in_worktree_scoped_config_is_still_refused() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + set_config(tmp.path(), "extensions.worktreeConfig", "true"); + // What `git config --worktree core.hooksPath ` writes; `set_config` + // only reaches `--local`, so this key is planted directly the same way + // the production inspection step reads it — via a real `git config + // --worktree` invocation — to prove the bypass is closed, not just that + // `set_config` happens to skip it. + let ok = hermetic( + std::process::Command::new("git") + .args(["config", "--worktree", "core.hooksPath"]) + .arg(tmp.path()) + .current_dir(tmp.path()), + ) + .status() + .unwrap() + .success(); + assert!(ok, "failed to set core.hooksPath in worktree-scoped config"); + + let tool = test_tool(tmp.path()); + let result = tool.execute(json!({"operation": "status"})).await; + let msg = error_text(&result); + + assert!( + msg.contains("core.hookspath"), + "a hookspath hidden in worktree-scoped config must still be refused, got: {msg}" + ); +} + +/// The allowlist inspection and the real command are two separate `git` +/// invocations, so a config change landing in the gap between them would be +/// invisible to the first and still reach the second. This test calls +/// `hardened_git` directly — skipping `first_disallowed_repo_config_key` +/// entirely, standing in for that gap — to prove the second invocation does +/// not depend on the first having caught anything: `core.hooksPath` is +/// neutralised at the point of execution regardless of what any inspection +/// saw or missed. +#[cfg(unix)] +#[tokio::test] +async fn hardened_git_neutralises_hookspath_even_if_the_allowlist_check_never_ran() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + set_config(tmp.path(), "user.email", "test@example.com"); + set_config(tmp.path(), "user.name", "Test"); + + let hooks_dir = tmp.path().join("evil-hooks"); + std::fs::create_dir(&hooks_dir).unwrap(); + let marker = tmp.path().join("HOOK_RAN"); + std::fs::write( + hooks_dir.join("pre-commit"), + format!("#!/bin/sh\ntouch {:?}\nexit 0\n", marker.to_string_lossy()), + ) + .unwrap(); + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions( + hooks_dir.join("pre-commit"), + std::fs::Permissions::from_mode(0o755), + ) + .unwrap(); + } + set_config(tmp.path(), "core.hooksPath", &hooks_dir.to_string_lossy()); + + std::fs::write(tmp.path().join("f.txt"), "hi").unwrap(); + let staged = hermetic( + std::process::Command::new("git") + .args(["add", "f.txt"]) + .current_dir(tmp.path()), + ) + .status() + .unwrap() + .success(); + assert!(staged, "failed to stage the test file"); + + let output = super::hardened_git(tmp.path()) + .args(["commit", "-m", "msg"]) + .output() + .await + .unwrap(); + + assert!( + output.status.success(), + "commit should succeed: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert!( + !marker.exists(), + "hardened_git ran the repository-configured pre-commit hook — the \ + override that is supposed to hold even without the allowlist check \ + did not" + ); +} + +/// `commit.gpgsign` is on `ALLOWED_REPO_CONFIG` as an ordinary boolean, but +/// left un-neutralised it would let a repository force every commit through +/// this tool to be signed. `output.status.success()` alone does not prove +/// that: a repository that also configures a *working* `gpg.program` would +/// make a signed commit succeed too, so this plants a fake one that always +/// signs successfully and then inspects the commit object itself for a +/// `gpgsig` header — the only assertion that actually distinguishes "signing +/// was skipped" from "signing was attempted and happened to work". +#[cfg(unix)] +#[tokio::test] +async fn hardened_git_neutralises_forced_commit_signing() { + use std::os::unix::fs::PermissionsExt; + + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + set_config(tmp.path(), "user.email", "test@example.com"); + set_config(tmp.path(), "user.name", "Test"); + set_config(tmp.path(), "commit.gpgsign", "true"); + + // A `gpg.program` that always "signs" successfully, so a regression here + // fails by finding a signature, not by the commit merely erroring out — + // the same distinction CodeRabbit's review raised. + let fake_gpg = tmp.path().join("fake-gpg.sh"); + std::fs::write( + &fake_gpg, + "#!/bin/sh\n\ + printf '%s\\n' '[GNUPG:] BEGIN_SIGNING H10' >&2\n\ + cat >/dev/null\n\ + printf -- '-----BEGIN PGP SIGNATURE-----\\n\\nZmFrZQ==\\n-----END PGP SIGNATURE-----\\n'\n\ + printf '%s\\n' '[GNUPG:] SIG_CREATED D 1 10 00 0 0123456789ABCDEF' >&2\n", + ) + .unwrap(); + std::fs::set_permissions(&fake_gpg, std::fs::Permissions::from_mode(0o755)).unwrap(); + set_config(tmp.path(), "gpg.program", &fake_gpg.to_string_lossy()); + + std::fs::write(tmp.path().join("f.txt"), "hi").unwrap(); + let staged = hermetic( + std::process::Command::new("git") + .args(["add", "f.txt"]) + .current_dir(tmp.path()), + ) + .status() + .unwrap() + .success(); + assert!(staged, "failed to stage the test file"); + + let output = super::hardened_git(tmp.path()) + .args(["commit", "-m", "msg"]) + .output() + .await + .unwrap(); + assert!( + output.status.success(), + "commit failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let commit_object = super::hardened_git(tmp.path()) + .args(["cat-file", "-p", "HEAD"]) + .output() + .await + .unwrap(); + assert!(commit_object.status.success()); + let commit_object = String::from_utf8_lossy(&commit_object.stdout); + assert!( + !commit_object.lines().any(|l| l.starts_with("gpgsig ")), + "commit.gpgsign=true must not be honoured, but HEAD carries a \ + signature: {commit_object}" + ); +} + +/// The config-inspection step must fail closed: if `git config --list +/// --local` cannot be read, that is not the same as "nothing to distrust", +/// and running the real command anyway would skip the check entirely. +#[cfg(unix)] +#[tokio::test] +async fn unreadable_repo_config_fails_closed_rather_than_running_anyway() { + use std::os::unix::fs::PermissionsExt; + + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + let config_path = tmp.path().join(".git").join("config"); + std::fs::set_permissions(&config_path, std::fs::Permissions::from_mode(0o000)).unwrap(); + + // Root (and some CI containers run as root) ignores this permission bit + // entirely, which would make the assertion below meaningless rather than + // wrong. Detect that up front instead of failing on an unrelated cause. + let permission_enforced = std::fs::File::open(&config_path).is_err(); + + let result = if permission_enforced { + let tool = test_tool(tmp.path()); + Some(tool.execute(json!({"operation": "status"})).await) + } else { + None + }; + + // Restore permissions before the TempDir is dropped, so cleanup doesn't + // fail on an unreadable file. + std::fs::set_permissions(&config_path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + let Some(result) = result else { + eprintln!( + "skipping: file permissions are not enforced against this process (running as root?)" + ); + return; + }; + let msg = error_text(&result); + + assert!( + msg.contains("could not inspect its repository config"), + "an unreadable repo config must refuse, not silently proceed, got: {msg}" + ); +} + +#[test] +fn a_subsection_is_elided_so_one_entry_covers_every_remote() { + assert_eq!(normalise_config_key("remote.origin.url"), "remote.url"); + assert_eq!(normalise_config_key("remote.a.b.c.url"), "remote.url"); + assert_eq!(normalise_config_key("core.fileMode"), "core.filemode"); + assert_eq!(normalise_config_key("core.fsmonitor"), "core.fsmonitor"); + // The subsection itself contains dots; the first and last components + // remain the reliable ones. + assert_eq!( + normalise_config_key("includeIf.gitdir:~/x.y/.path"), + "includeif.path" + ); + // A key with no dot at all is returned unchanged rather than panicking. + assert_eq!(normalise_config_key("bare"), "bare"); +}