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 5bca4cf..eacffb9 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -110,6 +110,7 @@ public final class GitWorktrees implements Worktrees { @Override public String add(String repoRoot, String branch, String baseRef) { + reportRemoteUrlsWithUserInfo(repoRoot); String base = (baseRef == null || baseRef.isBlank()) ? "HEAD" : baseRef; String nonce = nonce(); Path root = resolveRoot(repoRoot); @@ -177,6 +178,99 @@ public final class GitWorktrees implements Worktrees { } } + /** + * Report — never refuse — every remote whose fetch or push URL carries user-info outside SSH. + * A linked worktree shares its parent repository's git config, so a credential on ANY remote + * (not only {@code origin}) or in a {@code pushurl} is just as readable by a member as one on + * {@code origin}'s HTTPS fetch URL — the one case {@link #removeUserInfoFromHttpsOrigin} and + * {@link #requireCredentialFreeHttpsOrigin} already strip and refuse. This check is additive: it + * only logs a warning, it never mutates config and never refuses the provision. + * + *

A reporting-only check must never be able to abort a provision — PR #173 shipped one that + * ran unguarded at the top of {@link #add}, and every git call inside it can throw ({@link #exec} + * turns a non-zero exit or its 30-second timeout into a {@link WorktreeException}). Every git call + * here is therefore wrapped, and on failure only the exception's class is logged, never + * its message: the enumerating {@code git remote} call is not redacted, and its stderr is read + * from the very config that may hold the URL this check exists to find. + */ + private void reportRemoteUrlsWithUserInfo(String repoRoot) { + List remotes; + try { + remotes = exec("git", "-C", repoRoot, "remote").lines() + .map(String::trim) + .filter(r -> !r.isBlank()) + .toList(); + } catch (RuntimeException e) { + log.warn("could not enumerate remotes to check for credentialed URLs in {}: {}", + Path.of(repoRoot).toAbsolutePath().normalize(), e.getClass().getName()); + return; + } + for (String remote : remotes) { + reportOneRemoteUrlsWithUserInfo(repoRoot, remote); + } + } + + private void reportOneRemoteUrlsWithUserInfo(String repoRoot, String remote) { + boolean leaks = remoteUrlsLeakUserInfo(repoRoot, remote, false) + || remoteUrlsLeakUserInfo(repoRoot, remote, true); + if (leaks) { + log.warn("member worktree shares a remote URL containing user-info: remote={} repository={}; " + + "remove credentials from the repository's git config", + remote, Path.of(repoRoot).toAbsolutePath().normalize()); + } + } + + /** + * True when any resolved fetch (or, if {@code push}, push) URL for {@code remote} carries + * non-empty user-info outside SSH. Never throws — a git failure here is caught, logged (its + * class only, per the javadoc above), and treated as "nothing found", so it cannot abort or + * otherwise affect provisioning. Uses {@link #execRedacted} because the command's stdout is + * itself the URL this check exists to find. + */ + private boolean remoteUrlsLeakUserInfo(String repoRoot, String remote, boolean push) { + try { + String out = push + ? execRedacted("git", "-C", repoRoot, "remote", "get-url", "--push", "--all", remote) + : execRedacted("git", "-C", repoRoot, "remote", "get-url", "--all", remote); + return out.lines().anyMatch(url -> !url.isBlank() && urlLeaksUserInfo(url.trim())); + } catch (RuntimeException e) { + log.warn("could not read the {} URL for remote {} to check for credentials: {}", + push ? "push" : "fetch", remote, e.getClass().getName()); + return false; + } + } + + /** + * True when {@code rawUrl} parses as an absolute URI with a non-SSH-family scheme and non-empty + * user-info. An unparsable or scheme-less URL — including the ssh scp-like shorthand + * ({@code user@host:path}) — is not this check's concern and is treated as "no finding", the + * same way {@link #configureHttpsUrlRewriteForSshOrigin} leaves that shorthand untouched. + */ + private static boolean urlLeaksUserInfo(String rawUrl) { + URI uri; + try { + uri = new URI(rawUrl); + } catch (URISyntaxException e) { + return false; + } + String scheme = uri.getScheme(); + if (scheme == null || isSshLikeScheme(scheme)) { + return false; + } + String userInfo = uri.getUserInfo(); + return userInfo != null && !userInfo.isEmpty(); + } + + /** + * SSH-family schemes deliberately excluded from {@link #urlLeaksUserInfo}: there, the user part + * selects an account and authentication itself happens over SSH, so it is not a credential the + * way HTTPS/HTTP user-info is. + */ + private static boolean isSshLikeScheme(String scheme) { + return "ssh".equalsIgnoreCase(scheme) || "git+ssh".equalsIgnoreCase(scheme) + || "ssh+git".equalsIgnoreCase(scheme); + } + /** Configure a per-worktree helper that supplies a token from the member environment at call time. */ private void configureEnvironmentCredentialHelper(String repoRoot, String worktreePath) { exec("git", "-C", repoRoot, "config", "extensions.worktreeConfig", "true"); @@ -603,6 +697,23 @@ public final class GitWorktrees implements Worktrees { /** Same as {@link #exec(String...)}, with extra environment variables set on the child process. */ private String exec(Map extraEnv, String... command) { + return exec(extraEnv, false, command); + } + + /** + * Same as {@link #exec(String...)}, for a command whose stdout may itself carry a credential + * (e.g. {@code git remote get-url}, whose output is a URL). Stdout is still returned normally on + * success — callers still get the URL to inspect — but it is suppressed from BOTH the timeout + * message and the non-zero-exit message, so a failing call here can never copy it into a + * {@link WorktreeException}, and from there into a caller's log. + */ + private String execRedacted(String... command) { + 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) { String out; int code; Process p; @@ -624,7 +735,8 @@ public final class GitWorktrees implements Worktrees { try { if (!p.waitFor(30, TimeUnit.SECONDS)) { p.destroyForcibly(); - throw new WorktreeException("command timed out: " + String.join(" ", command) + "\n" + out); + throw new WorktreeException("command timed out: " + String.join(" ", command) + + (redactOutput || out.isBlank() ? "" : "\n" + out)); } code = p.exitValue(); } catch (InterruptedException e) { @@ -634,7 +746,7 @@ public final class GitWorktrees implements Worktrees { } if (code != 0) { throw new WorktreeException("exit " + code + " for: " + String.join(" ", command) - + (out.isBlank() ? "" : "\n" + out)); + + (redactOutput || out.isBlank() ? "" : "\n" + out)); } return out; } 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 1ddce93..033e0be 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/session/GitWorktreesTest.java @@ -1,7 +1,16 @@ package dev.ltms.fleet.session; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.LoggerContext; +import ch.qos.logback.classic.spi.IThrowableProxy; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.slf4j.LoggerFactory; import java.nio.charset.StandardCharsets; import java.nio.file.Files; @@ -352,6 +361,121 @@ class GitWorktreesTest { assertEquals("worktree origin contains HTTPS user info; refusing provision", error.getMessage()); } + // ---- CB-189: broader remote-URL coverage — every remote, both fetch and push URLs, any + // non-SSH scheme. Reporting only, additive to the origin/https strip-and-refuse tests above. ---- + + private Logger reportingLogger; + private ListAppender reportingAppender; + + /** {@link GitWorktrees}'s own logger, captured fresh for each test so assertions never see a + * message left over from a previous test. */ + @BeforeEach + void attachReportingLogCapture() { + LoggerContext ctx = (LoggerContext) LoggerFactory.getILoggerFactory(); + reportingLogger = ctx.getLogger(GitWorktrees.class); + reportingLogger.setLevel(Level.WARN); + reportingAppender = new ListAppender<>(); + reportingAppender.setContext(ctx); + reportingAppender.start(); + reportingLogger.addAppender(reportingAppender); + } + + @AfterEach + void detachReportingLogCapture() { + reportingLogger.detachAppender(reportingAppender); + } + + private List capturedMessages() { + return reportingAppender.list.stream().map(ILoggingEvent::getFormattedMessage).toList(); + } + + /** Asserts {@code secret} appears in no captured message, and in no attached exception's + * message either — the constraint is that a credential must never reach a log, however it + * would have gotten there. */ + private void assertNoLeak(String secret) { + for (ILoggingEvent event : reportingAppender.list) { + assertFalse(event.getFormattedMessage().contains(secret), + "log message leaked a credential (" + secret + "): " + event.getFormattedMessage()); + IThrowableProxy thrown = event.getThrowableProxy(); + if (thrown != null && thrown.getMessage() != null) { + assertFalse(thrown.getMessage().contains(secret), + "logged exception leaked a credential (" + secret + "): " + thrown.getMessage()); + } + } + } + + /** Gap 1: only {@code origin} was ever inspected. A credential on any other remote's fetch URL + * must now be reported. */ + @Test + void aCredentialedUrlOnANonOriginRemoteIsReported(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + git(repo, "remote", "add", "origin", "https://git.ltms.dev/akb/kb.git"); + git(repo, "remote", "add", "upstream", "https://leaky-upstream-token@git.ltms.dev/akb/kb.git"); + + new GitWorktrees(tmp.resolve("wts").toString()).add(repo.toString(), "cb-189-a", "HEAD"); + + List messages = capturedMessages(); + assertTrue(messages.stream().anyMatch(m -> m.contains("remote=upstream")), + "expected a report naming the leaking non-origin remote:\n" + messages); + assertNoLeak("leaky-upstream-token"); + assertNoLeak("https://leaky-upstream-token@git.ltms.dev/akb/kb.git"); + assertNoLeak("git.ltms.dev"); + } + + /** Gap 2: push URLs were never inspected. A credential visible only on {@code pushurl} — the + * fetch URL for the same remote stays clean — must now be reported. */ + @Test + void aCredentialedPushUrlIsReported(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + git(repo, "remote", "add", "origin", "https://git.ltms.dev/akb/kb.git"); + git(repo, "remote", "add", "mirror", "https://git.ltms.dev/akb/mirror.git"); + git(repo, "remote", "set-url", "--push", "mirror", + "https://leaky-push-token@git.ltms.dev/akb/mirror.git"); + + new GitWorktrees(tmp.resolve("wts").toString()).add(repo.toString(), "cb-189-b", "HEAD"); + + List messages = capturedMessages(); + assertTrue(messages.stream().anyMatch(m -> m.contains("remote=mirror")), + "expected a report naming the remote with the leaking pushurl:\n" + messages); + assertNoLeak("leaky-push-token"); + assertNoLeak("https://leaky-push-token@git.ltms.dev/akb/mirror.git"); + assertNoLeak("git.ltms.dev"); + } + + /** Gap 3: only {@code https} was handled. A plain {@code http://user:pass@…} remote — worse + * than https, not better — must now be reported. */ + @Test + void anHttpUrlWithCredentialsIsReported(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + git(repo, "remote", "add", "origin", "https://git.ltms.dev/akb/kb.git"); + git(repo, "remote", "add", "insecure", "http://plainuser:plainpass@git.ltms.dev/akb/kb.git"); + + new GitWorktrees(tmp.resolve("wts").toString()).add(repo.toString(), "cb-189-c", "HEAD"); + + List messages = capturedMessages(); + assertTrue(messages.stream().anyMatch(m -> m.contains("remote=insecure")), + "expected a report for the credentialed plain-http remote:\n" + messages); + assertNoLeak("plainuser"); + assertNoLeak("plainpass"); + assertNoLeak("plainuser:plainpass"); + assertNoLeak("git.ltms.dev"); + } + + /** A normal {@code ssh://} remote and a credential-free {@code https://} remote must produce no + * report at all — the check must not cry wolf on ordinary, safe configuration. */ + @Test + void anSshRemoteAndACleanHttpsRemoteProduceNoReport(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + git(repo, "remote", "add", "origin", "ssh://git@git.ltms.dev:2224/akb/kb.git"); + git(repo, "remote", "add", "clean", "https://git.ltms.dev/akb/kb.git"); + + new GitWorktrees(tmp.resolve("wts").toString()).add(repo.toString(), "cb-189-d", "HEAD"); + + assertTrue(reportingAppender.list.isEmpty(), + "expected no report for an ssh remote and a credential-free https remote, got:\n" + + capturedMessages()); + } + /** 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 {