From 615af4ed0a5130831f849f328702393112cd4929 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 31 Aug 2026 09:28:10 +0700 Subject: [PATCH] CB-192 review fix: split the allow-list gap by what the scrub actually keeps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lead review on PR #194 found that the allow-list INFO wording claimed the whole gap ("credential-shaped names on neither known: nor allow:") is blanked by the scrub, without checking that against effectiveAllowed. effectiveAllowed is a SUPERSET of known+allow — MemberEnvAllowList.derive also unions in every profile's gitTokenEnv/gitHostEnv/tokenEnv/env: keys, and derivedAllowedNames further unions in the spawn's own env keys — so a gap name can still be kept by the derived list (e.g. a profile's tokenEnv names it) and reach the member unblocked while the INFO said "no member pane keeps them". That inversion is exactly what #192 exists to remove. logCredentialGap now splits the gap with MemberEnvAllowList.keeps (the same predicate the generated scrub itself evaluates, so this cannot drift from what the scrub does): names it keeps get a WARN, guarded by the same unprotectedGapLogged flag as the deny-by-default case (same severity — a name reaching a member unprotected is equally serious either way); names it blanks keep the existing INFO, guarded by allowListGapLogged. The deny-by-default WARN text and the non-zsh fallback are untouched. Added allowListWarnsWhenTheDerivedAllowListKeepsAnUncoveredName and allowListSplitsAMixedGapBetweenTheWarnAndTheInfo to ClaudeCodeLauncherTest. --- .../ltms/fleet/member/HerdrPeerLauncher.java | 57 +++++++--- .../fleet/member/ClaudeCodeLauncherTest.java | 102 ++++++++++++++++++ 2 files changed, 146 insertions(+), 13 deletions(-) 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 fda9c4d..113fb22 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -1220,11 +1220,22 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { * picked from {@code creds.isAllowList()} — see {@link #applyEnvironmentAllowListPolicy}'s * javadoc for the reasoning this mirrors. {@code null} means no scrub-derived allow-list was * computed for this call — true on the deny-by-default path AND on the allow-list non-zsh - * fallback, where nothing is ever scrubbed — so the gap is real and gets the WARN, unchanged - * from before this fix. Non-null means this call came from the zsh branch of {@link - * #applyEnvironmentAllowListPolicy}, reachable ONLY after that method's own zsh gate — so the - * generated scrub genuinely will blank these names, and the INFO says so instead of claiming - * they are inherited unblocked. + * fallback, where nothing is ever scrubbed — so the whole gap is real and gets the WARN, + * unchanged from before this fix. Non-null means this call came from the zsh branch of {@link + * #applyEnvironmentAllowListPolicy}, reachable ONLY after that method's own zsh gate — but + * {@code effectiveAllowed} is a SUPERSET of {@code known ∪ allow}: {@link MemberEnvAllowList#derive} + * also unions in every profile's {@code gitTokenEnv}/{@code gitHostEnv}/{@code tokenEnv}/ + * {@code env:} keys, and {@link #derivedAllowedNames} further unions in this very spawn's own + * env keys — so a name can be in the gap (uncovered by {@code known}/{@code allow}) AND still be + * kept by the derived allow-list, in which case the scrub does NOT blank it and the member DOES + * inherit it. Lead review on #192 caught this: the first cut of this fix reported the WHOLE gap + * as scrub-blanked without checking that, which reported a real leak as safe — the exact + * inversion #192 exists to remove. So on this path the gap is split with {@link + * MemberEnvAllowList#keeps}, the SAME predicate the generated scrub itself evaluates, so this + * split cannot drift from what the scrub actually does: the names it says are kept get the WARN + * (same severity, and same guard, as the deny-by-default case — a name genuinely reaching a + * member unprotected is equally serious whichever path put it there), and the names it says are + * blanked keep the INFO. */ private void logCredentialGap(FleetConfig.MemberCredentials creds, Set effectiveAllowed) { Set covered = new HashSet<>(creds.known()); @@ -1237,16 +1248,36 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { if (gap.isEmpty()) { return; } - if (effectiveAllowed != null) { - if (allowListGapLogged.compareAndSet(false, true)) { - log.info("memberCredentials gap: {} credential-shaped env var name(s) are on neither " - + "known: nor allow: — {}. The allow-list scrub blanks them anyway (they " - + "are not on the derived allow-list), so no member pane keeps them; add " - + "each to memberCredentials.known or .allow to make that explicit.", - gap.size(), gap); - } + if (effectiveAllowed == null) { + warnGapUnprotected(gap); return; } + List keptByDerivedList = gap.stream() + .filter(name -> MemberEnvAllowList.keeps(effectiveAllowed, name)) + .toList(); + List blankedByScrub = gap.stream() + .filter(name -> !MemberEnvAllowList.keeps(effectiveAllowed, name)) + .toList(); + if (!keptByDerivedList.isEmpty() && unprotectedGapLogged.compareAndSet(false, true)) { + log.warn("memberCredentials gap: {} credential-shaped env var name(s) are on neither " + + "known: nor allow: — the derived allow-list keeps them anyway (a profile's " + + "gitTokenEnv/gitHostEnv/tokenEnv/env: names one, or this spawn injects it), " + + "so every member pane inherits them UNBLOCKED — {}. Add each to " + + "memberCredentials.known (or .allow if a member legitimately needs it), or " + + "remove it from whatever profile setting derives it in.", + keptByDerivedList.size(), keptByDerivedList); + } + if (!blankedByScrub.isEmpty() && allowListGapLogged.compareAndSet(false, true)) { + log.info("memberCredentials gap: {} credential-shaped env var name(s) are on neither " + + "known: nor allow: — {}. The allow-list scrub blanks them anyway (they " + + "are not on the derived allow-list), so no member pane keeps them; add " + + "each to memberCredentials.known or .allow to make that explicit.", + blankedByScrub.size(), blankedByScrub); + } + } + + /** The deny-by-default (and allow-list non-zsh fallback) WARN — unchanged byte-for-byte by #192. */ + private void warnGapUnprotected(List gap) { if (unprotectedGapLogged.compareAndSet(false, true)) { log.warn("memberCredentials gap: {} credential-shaped env var name(s) are on neither " + "known: nor allow: — every member pane inherits them UNBLOCKED — {}. " diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java index 209d938..6b6be77 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java @@ -1236,6 +1236,108 @@ class ClaudeCodeLauncherTest { + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); } + /** + * CB-633 follow-up (#192), lead-review fix: {@code effectiveAllowed} is a SUPERSET of + * {@code known ∪ allow} — {@code MemberEnvAllowList.derive} also unions in every profile's + * {@code tokenEnv} (among other fields), so a credential-shaped name can be uncovered by + * {@code known:}/{@code allow:} and STILL survive the scrub because a profile's own + * {@code tokenEnv} names it. Here {@code tokenEnv} is deliberately set to a credential-shaped + * name the operator forgot to list — the misconfiguration this report exists to catch. The scrub + * genuinely keeps it, so the report must WARN, not claim (as the pre-lead-review cut of this fix + * did) that "no member pane keeps them". + */ + @Test + void allowListWarnsWhenTheDerivedAllowListKeepsAnUncoveredName() { + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile cfg = new FleetConfig.Profile( + "ltms-local", "http://gx00.gw:8000", "coder", null, "SOME_LEAKY_TOKEN", + List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null); + FleetConfig.MemberCredentials creds = new FleetConfig.MemberCredentials( + FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of()); + ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), + name -> "SHELL".equals(name) ? "/bin/zsh" : null, + 0, System::currentTimeMillis, () -> {}, null, () -> creds, + () -> Set.of("SOME_LEAKY_TOKEN")); + + 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 { + svc.spawn(); + } finally { + logger.detachAppender(appender); + logger.setLevel(original); + } + + assertTrue(appender.list.stream().anyMatch(e -> + e.getLevel() == Level.WARN + && e.getFormattedMessage().contains("memberCredentials gap") + && e.getFormattedMessage().contains("SOME_LEAKY_TOKEN") + && e.getFormattedMessage().contains("UNBLOCKED")), + "a name kept by the derived allow-list (via this profile's tokenEnv) must still WARN " + + "— got: " + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + assertFalse(appender.list.stream().anyMatch(e -> + e.getFormattedMessage().contains("memberCredentials gap") + && e.getFormattedMessage().contains("SOME_LEAKY_TOKEN") + && e.getFormattedMessage().contains("blanks them anyway")), + "the scrub does NOT blank this name, so the INFO wording must not claim it does — got: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + } + + /** + * CB-633 follow-up (#192), lead-review fix: a mixed gap — one name the derived allow-list keeps + * (this profile's {@code tokenEnv}), one it does not — must split cleanly: the WARN names only + * the kept one, the INFO names only the blanked one. Proves the split uses {@code + * MemberEnvAllowList.keeps} per-name rather than an all-or-nothing decision for the whole gap. + */ + @Test + void allowListSplitsAMixedGapBetweenTheWarnAndTheInfo() { + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile cfg = new FleetConfig.Profile( + "ltms-local", "http://gx00.gw:8000", "coder", null, "SOME_LEAKY_TOKEN", + List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null); + FleetConfig.MemberCredentials creds = new FleetConfig.MemberCredentials( + FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of()); + ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), + name -> "SHELL".equals(name) ? "/bin/zsh" : null, + 0, System::currentTimeMillis, () -> {}, null, () -> creds, + () -> Set.of("SOME_LEAKY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN")); + + 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 { + svc.spawn(); + } finally { + logger.detachAppender(appender); + logger.setLevel(original); + } + + boolean warnNamesOnlyKept = appender.list.stream().anyMatch(e -> + e.getLevel() == Level.WARN + && e.getFormattedMessage().contains("memberCredentials gap") + && e.getFormattedMessage().contains("SOME_LEAKY_TOKEN") + && !e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")); + boolean infoNamesOnlyBlanked = appender.list.stream().anyMatch(e -> + e.getLevel() == Level.INFO + && e.getFormattedMessage().contains("memberCredentials gap") + && e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN") + && !e.getFormattedMessage().contains("SOME_LEAKY_TOKEN")); + + assertTrue(warnNamesOnlyKept, "the WARN must name the derived-list-kept variable and only it " + + "— got: " + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + assertTrue(infoNamesOnlyBlanked, "the INFO must name the scrub-blanked variable and only it " + + "— got: " + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + } + /** * The half of CB-592 that can actually survive the pane's login shell. BRIDGED_MEMBER is a name * secrets.sh never exports, so nothing overwrites it — measured: GITEA_TOKEN is injected the