CB-189 review fix: redact the three pre-existing URL-reading exec calls
Review found that execRedacted was applied only to the new remote-enumeration code and left three pre-existing calls reading remote.origin.url through the plain, unredacted exec: removeUserInfoFromHttpsOrigin, requireCredentialFreeHttpsOrigin, and configureHttpsUrlRewriteForSshOrigin. A non-zero exit or timeout on any of those could still have copied the credentialed URL into a WorktreeException message. Switches all three to execRedacted; the set-url write in removeUserInfoFromHttpsOrigin is left on plain exec with a comment explaining why (it writes the already-stripped URL, not a read). Widens the shared exec(Map, boolean, String...) overload to package-private, the same test-seam pattern already used by the afterWorktreeAdded constructor parameter, and adds a test that drives it directly with a synthetic failing command whose stdout carries a marker (passed via env, not argv, so the always-printed command line can't carry it) and asserts the marker never reaches the exception message.
This commit is contained in:
@@ -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<String, String> 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.
|
||||
*
|
||||
* <p>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<String, String> extraEnv, boolean redactOutput, String... command) {
|
||||
String out;
|
||||
int code;
|
||||
Process p;
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user