From d89615da0caaaea015d07c4628cf33e214ff938c Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 28 Aug 2026 05:25:38 +0700 Subject: [PATCH] CB-157: warn about credentialed remote URLs --- .../dev/ltms/fleet/session/GitWorktrees.java | 57 +++++++++++++- .../ltms/fleet/session/GitWorktreesTest.java | 74 +++++++++++++++++++ 2 files changed, 129 insertions(+), 2 deletions(-) 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 ceaf80b..7653dc1 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java +++ b/fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java @@ -20,6 +20,7 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; +import java.util.regex.Pattern; import java.util.stream.Collectors; /** @@ -32,6 +33,14 @@ public final class GitWorktrees implements Worktrees { private static final Logger log = LoggerFactory.getLogger(GitWorktrees.class); + /** + * A scheme URL whose authority starts with non-empty userinfo followed by {@code @}. SSH URLs + * are excluded because their user part selects the account while authentication stays in SSH. + */ + private static final Pattern URL_WITH_USERINFO = + Pattern.compile("^(?!(?:ssh|git\\+ssh|ssh\\+git)://)[A-Za-z][A-Za-z0-9+.-]*://[^/?#@]+@.*$", + Pattern.CASE_INSENSITIVE); + /** Project-level MCP config. Present in the repo, so every worktree would otherwise inherit the * primary's IDE server mounts (a CB-523 worker edited the primary checkout; see the isolation * javadoc). Neutralized unconditionally. */ @@ -96,6 +105,7 @@ public final class GitWorktrees implements Worktrees { @Override public String add(String repoRoot, String branch, String baseRef) { + warnAboutRemoteUrlUserInfo(repoRoot); String base = (baseRef == null || baseRef.isBlank()) ? "HEAD" : baseRef; String nonce = nonce(); Path root = resolveRoot(repoRoot); @@ -112,6 +122,39 @@ public final class GitWorktrees implements Worktrees { return wt; } + /** + * Check the fetch and push URLs that git resolves at provisioning time. A linked worktree shares + * these settings with its parent repository, so URL userinfo is readable by every member. The + * warning names only the remote and repository. It must never include the URL or userinfo. + */ + private void warnAboutRemoteUrlUserInfo(String repoRoot) { + String remotes = exec("git", "-C", repoRoot, "remote"); + for (String remote : remotes.split("\\R")) { + if (remote.isBlank()) { + continue; + } + boolean leaksUserInfo = effectiveRemoteUrls(repoRoot, remote, false).stream() + .anyMatch(URL_WITH_USERINFO.asMatchPredicate()); + if (!leaksUserInfo) { + leaksUserInfo = effectiveRemoteUrls(repoRoot, remote, true).stream() + .anyMatch(URL_WITH_USERINFO.asMatchPredicate()); + } + if (leaksUserInfo) { + log.warn("member worktree inherits a remote URL containing userinfo: remote={} repository={}; " + + "remove credentials from the repository's git config", + remote, Path.of(repoRoot).toAbsolutePath().normalize()); + } + } + } + + /** Read git's effective URLs while ensuring a command failure cannot copy them into an exception. */ + private List effectiveRemoteUrls(String repoRoot, String remote, boolean push) { + 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().filter(url -> !url.isBlank()).toList(); + } + /** * Neutralize the worktree's worktree-hostile project configs so a worker inherits only the tools * and environment its launcher mounts (the bridge via {@code --mcp-config}, the opencode config @@ -452,6 +495,15 @@ 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); + } + + /** Run a command whose stdout may be secret and omit that output from every failure message. */ + private String execRedacted(String... command) { + return exec(Map.of(), true, command); + } + + private String exec(Map extraEnv, boolean redactOutput, String... command) { String out; int code; Process p; @@ -473,7 +525,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) { @@ -483,7 +536,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 3b40911..fb6ac93 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,11 @@ package dev.ltms.fleet.session; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; 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; @@ -186,6 +190,76 @@ class GitWorktreesTest { return !forEachRef(cwd, ref).trim().isEmpty(); } + private static ListAppender captureWorktreeLogs() { + ch.qos.logback.classic.Logger logger = + (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(GitWorktrees.class); + ListAppender appender = new ListAppender<>(); + appender.setContext((ch.qos.logback.classic.LoggerContext) LoggerFactory.getILoggerFactory()); + appender.start(); + logger.addAppender(appender); + return appender; + } + + private static void stopCapturingWorktreeLogs(ListAppender appender) { + ((ch.qos.logback.classic.Logger) LoggerFactory.getLogger(GitWorktrees.class)).detachAppender(appender); + appender.stop(); + } + + /** + * CB-157. Provisioning must inspect the real remote URL that git resolves, then warn without + * copying any part of the userinfo or URL into the log. + */ + @Test + void provisioningWarnsOnceAboutRemoteUrlUserInfoWithoutLeakingIt(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + String secretUser = "cb157-secret-user"; + String secretPassword = "cb157-secret-password"; + String remoteUrl = "https://" + secretUser + ":" + secretPassword + "@git.example.invalid/team/repo.git"; + git(repo, "remote", "add", "origin", remoteUrl); + ListAppender appender = captureWorktreeLogs(); + + try { + new GitWorktrees(tmp.resolve("wts").toString()) + .add(repo.toString(), "cb-157-userinfo", "HEAD"); + + List warnings = appender.list.stream() + .filter(event -> event.getLevel() == Level.WARN) + .map(ILoggingEvent::getFormattedMessage) + .toList(); + assertEquals(1, warnings.size(), "the unsafe remote must produce one WARN: " + warnings); + String warning = warnings.getFirst(); + assertTrue(warning.contains("origin"), "the WARN must name the remote: " + warning); + assertTrue(warning.contains(repo.toAbsolutePath().normalize().toString()), + "the WARN must name the repository path: " + warning); + assertFalse(warning.contains(remoteUrl), "the WARN must not contain the remote URL"); + assertFalse(warning.contains(secretUser), "the WARN must not contain the credential user part"); + assertFalse(warning.contains(secretPassword), "the WARN must not contain the credential password"); + assertFalse(warning.contains("git.example.invalid"), + "the WARN must not contain a fragment copied from the remote URL"); + } finally { + stopCapturingWorktreeLogs(appender); + } + } + + /** CB-157. Common SSH and credential-free HTTPS remotes must not produce a false warning. */ + @Test + void provisioningDoesNotWarnForNormalSshOrHttpsRemotes(@TempDir Path tmp) throws Exception { + Path repo = initRepo(tmp.resolve("repo")); + git(repo, "remote", "add", "origin", "ssh://git@git.example.invalid:2224/team/repo.git"); + git(repo, "remote", "add", "mirror", "https://git.example.invalid/team/repo.git"); + ListAppender appender = captureWorktreeLogs(); + + try { + new GitWorktrees(tmp.resolve("wts").toString()) + .add(repo.toString(), "cb-157-safe-remotes", "HEAD"); + + assertFalse(appender.list.stream().anyMatch(event -> event.getLevel() == Level.WARN), + "safe remotes must produce no WARN at all: " + appender.list); + } finally { + stopCapturingWorktreeLogs(appender); + } + } + /** * The heart of CB-525: a provisioned worktree must not inherit the primary's MCP servers. Without * the isolation step the checked-out {@code .mcp.json} carries them in, and a worker navigating -- 2.52.0