diff --git a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java index a08938f..971eeb6 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -54,6 +54,56 @@ class GitWorktreesTest { /** A non-empty autoenv file — the form that would prompt for authorization in a worktree. */ private static final String AUTOENV_WITH_DIRECTIVE = "export HELLO=world\n"; + /** + * fleetd #369. A throwaway directory that lives for the whole class (JUnit 5.4+ supports a + * static {@code @TempDir} field, created once and removed once every test in this class has + * run) — backing every raw {@code git} subprocess's {@code XDG_CONFIG_HOME} below. It only + * ever needs to exist and be guaranteed free of a {@code git/ignore} file; nothing writes + * inside it. + */ + @TempDir + private static Path CLASS_TMP; + + /** + * fleetd #369 — the leak measured: {@code XDG_CONFIG_HOME= mvn test + * -Dtest=GitWorktreesTest} failed 56 of 59 tests on an unpatched checkout, because {@link + * #gitOutput} set {@code GIT_CONFIG_GLOBAL}/{@code GIT_CONFIG_SYSTEM}/{@code + * GIT_TERMINAL_PROMPT} but not {@code XDG_CONFIG_HOME}, and {@link #status}/{@link + * #fullStatus} (plus every other raw {@code git} subprocess this class started) set NOTHING at + * all — inheriting the JVM's whole real environment, including the operator's real {@code + * ~/.gitconfig} and real default excludes file ({@code $XDG_CONFIG_HOME/git/ignore} or {@code + * $HOME/.config/git/ignore}, applied by git with no {@code core.excludesFile} configured at + * all — see {@code gitignore(5)}). {@code GIT_CONFIG_GLOBAL=/dev/null} does not stop that + * default from applying; only setting {@code XDG_CONFIG_HOME} to a directory that provably + * carries no {@code git/ignore} does. + * + *

This is the same isolation {@link #hermeticGitEnv} already gives {@link + * #seedingGitWorktrees}'s production {@link GitWorktrees} instances (fleetd #362 review fix, + * finding 2), reused here for every subprocess the TEST ITSELF starts to drive and inspect + * those fixture repos. + */ + private static Map hermeticEnv() { + return hermeticGitEnv(CLASS_TMP); + } + + /** + * The one seam every git subprocess in this class is built through — see criterion 4's + * self-check, {@link #everyGitSubprocessGoesThroughTheHermeticFactory}, which fails the moment + * a future helper builds its own {@code git} subprocess directly instead of calling this, so + * the omission that caused fleetd #369 gets caught by name rather than rediscovered by a + * poisoned machine. The one deliberate exception is {@link + * #worktreeCredentialHelperCompletesWithoutUsingAnInheritedHelper}, which needs a + * non-hermetic, test-controlled global config to prove the credential helper ignores it — see + * the comment on that test. + */ + private static ProcessBuilder gitProcessBuilder(Path cwd, String... args) { + List cmd = new java.util.ArrayList<>(List.of("git")); + cmd.addAll(List.of(args)); + ProcessBuilder pb = new ProcessBuilder(cmd).directory(cwd.toFile()).redirectErrorStream(true); + pb.environment().putAll(hermeticEnv()); + return pb; + } + private static Path initRepo(Path dir) throws Exception { Files.createDirectories(dir); git(dir, "init", "-q", "-b", "main"); @@ -71,23 +121,16 @@ class GitWorktreesTest { } private static String gitOutput(Path cwd, String... args) throws Exception { - List cmd = new java.util.ArrayList<>(List.of("git")); - cmd.addAll(List.of(args)); - ProcessBuilder pb = new ProcessBuilder(cmd).directory(cwd.toFile()).redirectErrorStream(true); - pb.environment().put("GIT_CONFIG_GLOBAL", "/dev/null"); - pb.environment().put("GIT_CONFIG_SYSTEM", "/dev/null"); - pb.environment().put("GIT_TERMINAL_PROMPT", "0"); - Process p = pb.start(); + Process p = gitProcessBuilder(cwd, args).start(); String out = new String(p.getInputStream().readAllBytes()); - assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git timed out: " + String.join(" ", cmd)); + assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git timed out: git " + String.join(" ", args)); assertEquals(0, p.exitValue(), "git " + String.join(" ", args) + " failed:\n" + out); return out; } /** Pending changes to {@code file} in {@code cwd}, empty when git considers it unmodified. */ private static String status(Path cwd, String file) throws Exception { - Process p = new ProcessBuilder("git", "status", "--porcelain", "--", file) - .directory(cwd.toFile()).redirectErrorStream(true).start(); + Process p = gitProcessBuilder(cwd, "status", "--porcelain", "--", file).start(); String out = new String(p.getInputStream().readAllBytes()); assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git status timed out"); return out; @@ -95,16 +138,14 @@ class GitWorktreesTest { /** Every pending change in {@code cwd} — the whole-tree porcelain status, unlike {@link #status}. */ private static String fullStatus(Path cwd) throws Exception { - Process p = new ProcessBuilder("git", "status", "--porcelain") - .directory(cwd.toFile()).redirectErrorStream(true).start(); + Process p = gitProcessBuilder(cwd, "status", "--porcelain").start(); String out = new String(p.getInputStream().readAllBytes()); assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git status timed out"); return out; } private static String revParse(Path cwd, String ref) throws Exception { - Process p = new ProcessBuilder("git", "-C", cwd.toString(), "rev-parse", ref) - .redirectErrorStream(true).start(); + Process p = gitProcessBuilder(cwd, "rev-parse", ref).start(); String out = new String(p.getInputStream().readAllBytes()).trim(); assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git rev-parse timed out"); assertEquals(0, p.exitValue(), "git rev-parse " + ref + " failed:\n" + out); @@ -113,8 +154,7 @@ class GitWorktreesTest { /** The recursive file list of a commit's tree — used to check what a snapshot actually committed. */ private static String lsTree(Path cwd, String ref) throws Exception { - Process p = new ProcessBuilder("git", "-C", cwd.toString(), "ls-tree", "-r", "--name-only", ref) - .redirectErrorStream(true).start(); + Process p = gitProcessBuilder(cwd, "ls-tree", "-r", "--name-only", ref).start(); String out = new String(p.getInputStream().readAllBytes()); assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git ls-tree timed out"); assertEquals(0, p.exitValue(), "git ls-tree " + ref + " failed:\n" + out); @@ -125,8 +165,7 @@ class GitWorktreesTest { * snapshot's tree changed relative to its parent, the same shape {@code git status --porcelain} * reports for the worktree it was taken from. */ private static Set diffNameOnly(Path cwd, String from, String to) throws Exception { - Process p = new ProcessBuilder("git", "-C", cwd.toString(), "diff", "--name-only", from, to) - .redirectErrorStream(true).start(); + Process p = gitProcessBuilder(cwd, "diff", "--name-only", from, to).start(); String out = new String(p.getInputStream().readAllBytes()); assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git diff timed out"); assertEquals(0, p.exitValue(), "git diff " + from + ".." + to + " failed:\n" + out); @@ -152,8 +191,7 @@ class GitWorktreesTest { } private static String forEachRef(Path cwd, String pattern) throws Exception { - Process p = new ProcessBuilder("git", "-C", cwd.toString(), "for-each-ref", pattern) - .redirectErrorStream(true).start(); + Process p = gitProcessBuilder(cwd, "for-each-ref", pattern).start(); String out = new String(p.getInputStream().readAllBytes()); assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git for-each-ref timed out"); assertEquals(0, p.exitValue(), "git for-each-ref " + pattern + " failed:\n" + out); @@ -162,8 +200,7 @@ class GitWorktreesTest { /** Write {@code content} as a blob into the object database; returns its sha. */ private static String blobOf(Path cwd, String content) throws Exception { - Process p = new ProcessBuilder("git", "-C", cwd.toString(), "hash-object", "-w", "--stdin") - .redirectErrorStream(true).start(); + Process p = gitProcessBuilder(cwd, "hash-object", "-w", "--stdin").start(); p.getOutputStream().write(content.getBytes(StandardCharsets.UTF_8)); p.getOutputStream().close(); String out = new String(p.getInputStream().readAllBytes()).trim(); @@ -174,8 +211,7 @@ class GitWorktreesTest { /** Build a single-file tree object from {@code blob}; returns the tree's sha. */ private static String treeOf(Path cwd, String path, String blob) throws Exception { - Process p = new ProcessBuilder("git", "-C", cwd.toString(), "mktree") - .redirectErrorStream(true).start(); + Process p = gitProcessBuilder(cwd, "mktree").start(); p.getOutputStream().write(("100644 blob " + blob + "\t" + path + "\n").getBytes(StandardCharsets.UTF_8)); p.getOutputStream().close(); String out = new String(p.getInputStream().readAllBytes()).trim(); @@ -187,10 +223,9 @@ class GitWorktreesTest { /** {@code git commit-tree} rooted at {@code tree} with a chosen committer date; returns the sha. */ private static String commitTree(Path cwd, String tree, String parent, String committerDate, String message) throws Exception { - ProcessBuilder pb = new ProcessBuilder("git", "-C", cwd.toString(), "commit-tree", - tree, "-p", parent, "-m", message); + ProcessBuilder pb = gitProcessBuilder(cwd, "commit-tree", tree, "-p", parent, "-m", message); pb.environment().put("GIT_COMMITTER_DATE", committerDate); - Process p = pb.redirectErrorStream(true).start(); + Process p = pb.start(); String out = new String(p.getInputStream().readAllBytes()).trim(); assertTrue(p.waitFor(30, TimeUnit.SECONDS), "git commit-tree timed out"); assertEquals(0, p.exitValue(), "git commit-tree failed:\n" + out); @@ -263,6 +298,11 @@ class GitWorktreesTest { helper = !f() { printf 'username=%s\\npassword=%s\\n\\n' operator operator-secret; }; f """); + // fleetd #369: the one deliberate exception to gitProcessBuilder. This test's whole point is + // that git must resolve `globalConfig` (a synthetic "operator's global config", never the + // real machine's) and then IGNORE it — so it cannot use the shared hermetic env, which would + // point GIT_CONFIG_GLOBAL at /dev/null and defeat the very thing under test. It never runs + // `git status`, so it does not need XDG_CONFIG_HOME isolation either. ProcessBuilder pb = new ProcessBuilder("git", "credential", "fill") .directory(Path.of(wt).toFile()).redirectErrorStream(true); pb.environment().put("GIT_CONFIG_GLOBAL", globalConfig.toString()); @@ -329,20 +369,16 @@ class GitWorktreesTest { Path worktree = Path.of(wt); assertEquals("https://git.ltms.dev/akb/kb.git", gitOutput(worktree, "remote", "get-url", "origin").trim()); - assertEquals(1, exitCode("git", "-C", wt, "config", "--worktree", "--get-regexp", "^url\\."), + assertEquals(1, gitExitCode(worktree, "config", "--worktree", "--get-regexp", "^url\\."), "no url.*.insteadOf rewrite should be added for an already-HTTPS origin"); } /** Test-local exit-code probe, mirroring {@link GitWorktrees#exitCode} for an assertion the * production class does not expose. */ - private static int exitCode(String... command) throws Exception { - ProcessBuilder pb = new ProcessBuilder(command).redirectErrorStream(true); - pb.environment().put("GIT_CONFIG_GLOBAL", "/dev/null"); - pb.environment().put("GIT_CONFIG_SYSTEM", "/dev/null"); - pb.environment().put("GIT_TERMINAL_PROMPT", "0"); - Process p = pb.start(); + private static int gitExitCode(Path cwd, String... args) throws Exception { + Process p = gitProcessBuilder(cwd, args).start(); p.getInputStream().readAllBytes(); - assertTrue(p.waitFor(30, TimeUnit.SECONDS), "command timed out: " + String.join(" ", command)); + assertTrue(p.waitFor(30, TimeUnit.SECONDS), "command timed out: git " + String.join(" ", args)); return p.exitValue(); } @@ -1739,4 +1775,77 @@ class GitWorktreesTest { "the XDG default excludesFile pattern ('xdg-fallback-marker') must still apply " + "after skill seeding ran — got:\n" + porcelain); } + + /** + * fleetd #369, acceptance criterion 4 — make the fix hard to undo by accident. Every git + * subprocess this class starts is required to go through {@link #gitProcessBuilder}, the one + * place {@link #hermeticEnv} is applied; a helper built directly, the way the original leak in + * {@link #status}/{@link #fullStatus} was, is now a source-level fact this test can catch by + * name instead of a machine-dependent failure someone has to rediscover. + * + *

This counts a literal marker in this very file's own source, split into three + * concatenated pieces below so the count is not thrown off by this method's own text — a + * plain, unsplit occurrence of the marker anywhere in this file (a helper's construction, or a + * comment that happens to spell it out contiguously) adds to the count the same way. Today + * there are exactly two: the factory itself, and the one documented exception in {@link + * #worktreeCredentialHelperCompletesWithoutUsingAnInheritedHelper}, which needs a + * non-hermetic, test-controlled global config to prove the credential helper ignores it. A + * third means a new helper was added the old, leak-prone way — route it through {@link + * #gitProcessBuilder} instead, or explain the new exception here and bump this number. + */ + @Test + void everyGitSubprocessGoesThroughTheHermeticFactory() throws Exception { + Path source = Path.of("src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java"); + String text = Files.readString(source); + String marker = "new " + "ProcessBuilder" + "("; + int count = 0; + for (int from = text.indexOf(marker); from >= 0; from = text.indexOf(marker, from + marker.length())) { + count++; + } + assertEquals(2, count, + "expected exactly 2 direct git-subprocess constructions in this file (the " + + "gitProcessBuilder factory itself, plus the one documented exception in " + + "worktreeCredentialHelperCompletesWithoutUsingAnInheritedHelper) — a " + + "different count means a helper now bypasses the hermetic factory; route " + + "it through gitProcessBuilder or document the new exception here"); + } + + /** + * fleetd #369 review round 2. {@link #everyGitSubprocessGoesThroughTheHermeticFactory} counts + * call sites, not behaviour — it catches a NEW helper built the old, leak-prone way, but it + * cannot catch {@link #gitProcessBuilder} itself being gutted: deleting {@code + * pb.environment().putAll(hermeticEnv())} from inside the factory leaves every call site + * unchanged, the count stays 2, and the whole unpoisoned suite stays green — the exact leak + * this ticket fixed would come back silently, with nothing but a human remembering to re-run + * the poison command to catch it. This test instead inspects what the factory actually hands + * to {@link ProcessBuilder#start()}, so it fails the moment the hermetic environment stops + * being applied, on any machine, with no poison needed. + * + *

The property under test: every git subprocess this class starts must run with an + * environment that cannot see the operator's real git configuration. A call-site count is a + * proxy for that; this is the thing itself. + */ + @Test + void gitProcessBuilderCarriesTheFullHermeticEnvironment(@TempDir Path tmp) { + Map env = gitProcessBuilder(tmp, "status", "--porcelain").environment(); + + assertEquals("/dev/null", env.get("GIT_CONFIG_GLOBAL"), + "GIT_CONFIG_GLOBAL must be neutralized, or the operator's real ~/.gitconfig applies"); + assertEquals("/dev/null", env.get("GIT_CONFIG_SYSTEM"), + "GIT_CONFIG_SYSTEM must be neutralized, or the machine's real /etc/gitconfig applies"); + assertEquals("0", env.get("GIT_TERMINAL_PROMPT"), + "GIT_TERMINAL_PROMPT must be disabled, or a credential prompt can hang the subprocess"); + + String xdg = env.get("XDG_CONFIG_HOME"); + assertNotNull(xdg, + "XDG_CONFIG_HOME must be set — left unset, git falls back to the operator's real " + + "$HOME/.config/git/ignore (gitignore(5)), exactly fleetd #369's leak"); + assertFalse(xdg.isBlank(), "XDG_CONFIG_HOME must not be blank — blank behaves like unset"); + assertTrue(Path.of(xdg).startsWith(CLASS_TMP), + "XDG_CONFIG_HOME must point inside this test class's own throwaway directory, " + + "never the operator's real one or the JVM's inherited value — got: " + xdg); + assertFalse(Files.exists(Path.of(xdg, "git", "ignore")), + "the resolved XDG default excludes file must provably not exist, or its contents " + + "would silently apply to every git status this test class runs"); + } }