diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java index 0e7d8c4..6871dda 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -1049,19 +1049,24 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { return null; } Set allowed = derivedAllowedNames(creds, launch); - logAllowListCoverage(allowed); String loginShell = resolveEnv("SHELL"); boolean zsh = loginShell != null && (loginShell.endsWith("/zsh") || loginShell.equals("zsh")); if (!zsh) { // A non-zsh login shell ignores ZDOTDIR entirely: NO scrub would run, so pretending // otherwise would be worse than saying so. Warn loudly and fall back to the CB-596 // sentinel overlay over the enumerated known: names — weaker (a sourced file can undo - // it), but strictly better than nothing. + // it), but strictly better than nothing. Deliberately no "allowed N of M" line here: the + // scrub this count describes does not run on this path, so printing it would tell an + // operator that a fraction of names were blocked when the real number blocked is zero. + // logCredentialGap's WARN (below) is the only signal for this path. warnNonZsh(loginShell); overlayBlockedCredentials(launch.env(), creds); logCredentialGap(creds); return null; } + // Only reached when the scrub is actually about to run — the count below describes that + // scrub, so it must not be logged before this gate (see the non-zsh branch above). + logAllowListCoverage(allowed); Path dir = EnvAllowListScrub.generate(Path.of(System.getProperty("java.io.tmpdir")), allowed); launch.env().put("ZDOTDIR", dir.toAbsolutePath().toString()); log.info("memberCredentials policy=allow-list: profile={} generated ZDOTDIR {} — derived " @@ -1086,13 +1091,17 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { } /** - * CB-633 follow-up: one INFO line per allow-list spawn, so an operator can read a single log - * line and know the scrub ran and how much of the visible environment it will keep. {@code M} is - * {@link #hostEnvNames}' size (the daemon's own environment — see that field's javadoc for why it - * stands in for the pane's, which the daemon has no channel to inspect at spawn time) and - * {@code N} is how many of those names survive {@code allowed} (including the {@code LC_*} - * prefix rule). Neither number is a constant: both come from the actual derived set and the - * actual environment this spawn sees. Never logs a variable NAME or VALUE — only the counts. + * CB-633 follow-up: one INFO line per allow-list spawn WHOSE SCRUB ACTUALLY RUNS, so an operator + * can read a single log line and know the scrub ran and how much of the visible environment it + * will keep. Callable ONLY from the zsh branch of {@link #applyEnvironmentAllowListPolicy}, after + * the shell gate — logging it before that gate (or on the non-zsh fallback, where nothing is + * scrubbed) would tell an operator a fraction of names were blocked when the real number blocked + * is zero, which is worse than not logging at all. {@code M} is {@link #hostEnvNames}' size (the + * daemon's own environment — see that field's javadoc for why it stands in for the pane's, which + * the daemon has no channel to inspect at spawn time) and {@code N} is how many of those names + * survive {@code allowed} (including the {@code LC_*} prefix rule). Neither number is a constant: + * both come from the actual derived set and the actual environment this spawn sees. Never logs a + * variable NAME or VALUE — only the counts. */ private void logAllowListCoverage(Set allowed) { Set hostNames = hostEnvNames.get(); diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java index b506582..2e926b2 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java @@ -196,6 +196,39 @@ class HerdrPeerLauncherAllowListWiringTest { + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); } + /** + * Lead-review fix: on a NON-zsh shell no scrub ever runs (bash ignores {@code ZDOTDIR}), so the + * "allowed N of M" line — which describes what the scrub does — must not be printed there either. + * Before this fix the line was logged BEFORE the zsh gate, so a non-zsh host printed e.g. + * "allowed 1 of 3" while blocking nothing at all, telling an operator a control ran when it did + * not. Real path: goes through {@link HerdrPeerLauncher#spawn}, same as the sibling test above, + * with the shell fixed to bash so the fallback branch is the one exercised. + */ + @Test + void noAllowedCountLineIsEmittedOnTheNonZshFallbackPath() { + FakeHerdr herdr = new FakeHerdr(); + Set hostEnvNames = Set.of(INJECTED, "SOME_UNRELATED_NAME", "ANOTHER_UNRELATED_NAME"); + WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/bash", () -> hostEnvNames); + + Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); + Level original = logger.getLevel(); + logger.setLevel(Level.INFO); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + } finally { + logger.detachAppender(appender); + logger.setLevel(original); + } + + assertFalse(appender.list.stream() + .anyMatch(e -> e.getFormattedMessage().startsWith("member credentials: allowed ")), + "no scrub runs on a non-zsh shell, so no 'allowed N of M' count may be printed — got: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + } + private static String readAll(Path p) { try { return Files.readString(p);