CB-189: cover every remote, both URLs, and any non-SSH scheme in the credential check
GitWorktrees only ever inspected origin's HTTPS fetch URL for embedded credentials. A credential on any other remote, on a pushurl, or on a plain http:// URL passed through unreported. Adds an additive, reporting-only check that enumerates every remote and both its fetch and push URLs, flagging non-empty user-info on any non-SSH-family scheme. The existing origin/https strip-and-refuse behaviour is untouched. The new check is wrapped so it can never abort a provision, and on failure logs only the exception's class, never its message, since the enumerating `git remote` call is not redacted. Also adds execRedacted, an exec variant that never copies captured stdout into a WorktreeException message, for commands whose stdout may itself be a credentialed URL.
This commit is contained in:
Dai Ha
@@ -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.
|
||||
*
|
||||
* <p>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 <em>class</em> 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<String> 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<String, String> 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<String, String> 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;
|
||||
}
|
||||
|
||||
@@ -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<ILoggingEvent> 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<String> 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<String> 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<String> 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<String> 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 {
|
||||
|
||||
Reference in New Issue
Block a user