diff --git a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java index eacffb9..fb0a5fc 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -140,7 +140,7 @@ public final class GitWorktrees implements Worktrees { if (exitCode("git", "-C", repoRoot, "config", "--get", "remote.origin.url") != 0) { return; } - String origin = exec("git", "-C", repoRoot, "config", "--get", "remote.origin.url").trim(); + String origin = execRedacted("git", "-C", repoRoot, "config", "--get", "remote.origin.url").trim(); URI uri; try { uri = new URI(origin); @@ -156,6 +156,9 @@ public final class GitWorktrees implements Worktrees { throw new WorktreeException("origin URL has invalid HTTPS user info; cannot provision safely"); } String cleanOrigin = origin.substring(0, schemeEnd) + origin.substring(userInfoEnd + 1); + // Plain exec, not execRedacted, is correct here: this call WRITES cleanOrigin (already + // stripped of user-info above) rather than reading a URL back from stdout, so there is + // nothing secret left in either its argv or its stdout to redact. exec("git", "-C", repoRoot, "remote", "set-url", "origin", cleanOrigin); log.info("removed HTTPS user info from forge origin before provisioning worktree"); } @@ -165,7 +168,7 @@ public final class GitWorktrees implements Worktrees { if (exitCode("git", "-C", worktreePath, "remote", "get-url", "--all", "origin") != 0) { return; } - String origins = exec("git", "-C", worktreePath, "remote", "get-url", "--all", "origin"); + String origins = execRedacted("git", "-C", worktreePath, "remote", "get-url", "--all", "origin"); for (String origin : origins.split("\\R")) { try { URI uri = new URI(origin); @@ -328,7 +331,7 @@ public final class GitWorktrees implements Worktrees { if (exitCode("git", "-C", repoRoot, "config", "--get", "remote.origin.url") != 0) { return; } - String origin = exec("git", "-C", repoRoot, "config", "--get", "remote.origin.url").trim(); + String origin = execRedacted("git", "-C", repoRoot, "config", "--get", "remote.origin.url").trim(); URI uri; try { uri = new URI(origin); @@ -711,9 +714,17 @@ public final class GitWorktrees implements Worktrees { return exec(Map.of(), true, command); } - /** Shared implementation for {@link #exec(Map, String...)} and {@link #execRedacted(String...)}. - * {@code redactOutput} suppresses captured stdout from both failure messages below. */ - private String exec(Map extraEnv, boolean redactOutput, String... command) { + /** + * Shared implementation for {@link #exec(Map, String...)} and {@link #execRedacted(String...)}. + * {@code redactOutput} suppresses captured stdout from both failure messages below. + * + *

Package-private, not {@code private}: also a test seam, the same way the + * {@code afterWorktreeAdded} constructor parameter is. It lets a test drive the redaction + * guarantee directly — a synthetic failing command whose stdout carries a test marker passed + * through {@code extraEnv} rather than argv — without depending on finding a real git failure + * mode that happens to echo a URL onto stdout before exiting non-zero. + */ + String exec(Map extraEnv, boolean redactOutput, String... command) { String out; int code; Process p; 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 033e0be..74e99eb 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -17,6 +17,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; @@ -476,6 +477,36 @@ class GitWorktreesTest { + capturedMessages()); } + /** + * CB-189 review fix. Even a FAILING command that read a credential onto its stdout must never + * let that value reach the thrown {@link WorktreeException}'s message — this is gap 4 from the + * CB-189 issue, and the reason {@link GitWorktrees#execRedacted} exists at all. Drives the + * shared {@code exec}/{@code execRedacted} seam directly (it is package-private for exactly this, + * the same way the {@code afterWorktreeAdded} constructor parameter is a test seam) with a + * synthetic, non-git command whose stdout carries a marker — passed through the environment, + * never through argv, so the marker cannot leak via the command line that IS always printed + * unconditionally in the exception message — and which exits non-zero. This isolates the + * redaction guarantee itself rather than depending on a specific git failure mode that happens to + * echo a URL onto stdout before failing: none of the git subcommands this class actually runs was + * found to have one (a corrupted config makes {@code git config --get} fail before it ever reads + * the target key, so its output never carries the URL either). The marker is generated per-test + * run and injected only by the test, never a real-looking credential, so even a failing assertion + * could not itself print a secret. + */ + @Test + void execRedactedNeverCopiesFailingCommandOutputIntoTheExceptionMessage(@TempDir Path tmp) { + String marker = "cb189-marker-" + System.nanoTime(); + GitWorktrees worktrees = new GitWorktrees(tmp.toString()); + + WorktreeException thrown = assertThrows(WorktreeException.class, () -> worktrees.exec( + Map.of("MARKER", marker), true, "sh", "-c", "echo \"$MARKER\"; exit 7")); + + assertNotNull(thrown.getMessage()); + assertFalse(thrown.getMessage().contains(marker), + "a failing command's captured stdout leaked into the exception message: " + + thrown.getMessage()); + } + /** Neutralizing must not look like work in progress, or a worker would commit it into its PR. */ @Test void theNeutralizedConfigIsNotAPendingLocalModification(@TempDir Path tmp) throws Exception {