CB-157: warn about credentialed remote URLs
This commit is contained in:
@@ -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<String> 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<String, String> 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<String, String> 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;
|
||||
}
|
||||
|
||||
@@ -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<ILoggingEvent> captureWorktreeLogs() {
|
||||
ch.qos.logback.classic.Logger logger =
|
||||
(ch.qos.logback.classic.Logger) LoggerFactory.getLogger(GitWorktrees.class);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.setContext((ch.qos.logback.classic.LoggerContext) LoggerFactory.getILoggerFactory());
|
||||
appender.start();
|
||||
logger.addAppender(appender);
|
||||
return appender;
|
||||
}
|
||||
|
||||
private static void stopCapturingWorktreeLogs(ListAppender<ILoggingEvent> 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<ILoggingEvent> appender = captureWorktreeLogs();
|
||||
|
||||
try {
|
||||
new GitWorktrees(tmp.resolve("wts").toString())
|
||||
.add(repo.toString(), "cb-157-userinfo", "HEAD");
|
||||
|
||||
List<String> 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<ILoggingEvent> 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
|
||||
|
||||
Reference in New Issue
Block a user