CB-192: fix false credential-gap WARN under allow-list+zsh, split its log guard
CI / contract (pull_request) Successful in 1m5s
CI / build (pull_request) Successful in 1m7s

logCredentialGap(creds) always emitted the WARN wording ("every member pane
inherits them UNBLOCKED"), even under memberCredentials.policy: allow-list on
a zsh login shell, where the generated ZDOTDIR scrub genuinely blanks the
name. The line reported the control working as though it were a hole.

Pass an effectiveAllowed set instead: null keeps the WARN (deny-by-default,
and the allow-list non-zsh fallback, where nothing is ever scrubbed); the
derived allow-list set (only reachable after applyEnvironmentAllowListPolicy's
own zsh gate) selects a new INFO wording that says the scrub will blank the
name instead of claiming it is inherited unblocked.

Also split the single credentialGapLogged AtomicBoolean into two guards
(unprotectedGapLogged / allowListGapLogged) — one per report kind. Since
memberCredentials is a live, re-read-per-spawn supplier, a shared flag let a
harmless allow-list INFO on one spawn permanently suppress a later spawn's
real deny-by-default WARN after a policy reload.

Fixes gitea #192.
This commit is contained in:
Dai Ha
2026-08-31 09:17:20 +07:00
parent 23ada1981e
commit d89ae94a2e
2 changed files with 238 additions and 9 deletions
@@ -1008,6 +1008,13 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
* applied before the login shell runs and a sourced file can (and did) undo it. The control is
* the ZDOTDIR scrub ({@link #applyEnvironmentAllowListPolicy}); {@code known}/{@code allow}
* remain as reporting only via {@link #logCredentialGap}.
*
* <p>CB-633 follow-up (#192): under {@code allow-list} this method does NOT call {@link
* #logCredentialGap} itself — at this point (called from {@link #baseEnv}, before {@link
* #applyEnvironmentAllowListPolicy} runs) we do not yet know whether the pane's shell is zsh, so
* we cannot yet pick correct wording. That decision, and the call, are deferred entirely to
* {@link #applyEnvironmentAllowListPolicy}, which knows by then whether the scrub will actually
* run.
*/
private void applyMemberCredentialPolicy(Map<String, String> workerEnv) {
FleetConfig.MemberCredentials creds = memberCredentials == null ? null : memberCredentials.get();
@@ -1016,8 +1023,8 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
}
if (!creds.isAllowList()) {
overlayBlockedCredentials(workerEnv, creds);
logCredentialGap(creds, null);
}
logCredentialGap(creds);
}
/** Put {@link #BLOCKED_CREDENTIAL_SENTINEL} over every blocked name in the pane-creation env map. */
@@ -1067,12 +1074,15 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
// logCredentialGap's WARN (below) is the only signal for this path.
warnNonZsh(loginShell);
overlayBlockedCredentials(launch.env(), creds);
logCredentialGap(creds);
logCredentialGap(creds, null);
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).
// scrub, so it must not be logged before this gate (see the non-zsh branch above). Same
// reasoning gates logCredentialGap's wording: passing the derived `allowed` set (non-null)
// here, and ONLY here, is what tells it the scrub will really blank an unkept name — #192.
logAllowListCoverage(allowed);
logCredentialGap(creds, 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 "
@@ -1177,18 +1187,46 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
private static final Pattern CREDENTIAL_SHAPED_NAME =
Pattern.compile("(?i).*(TOKEN|SECRET|_KEY|APIKEY|PASSWORD|CREDENTIAL|AUTH).*");
/** Guards {@link #logCredentialGap} to one WARN per launcher instance, not one per spawn. */
private final AtomicBoolean credentialGapLogged = new AtomicBoolean();
/**
* Guards the {@code effectiveAllowed == null} branch of {@link #logCredentialGap} — the
* genuinely-unprotected report (deny-by-default, and the allow-list non-zsh fallback) — to one
* WARN per launcher instance, not one per spawn.
*
* <p>CB-633 follow-up (#192): kept SEPARATE from {@link #allowListGapLogged} on purpose.
* {@code memberCredentials} is a live, re-read-per-spawn supplier, so the policy can change
* between two spawns on the same launcher. A single shared flag would let a harmless allow-list
* INFO on spawn 1 permanently suppress the real deny-by-default WARN a later spawn deserves —
* the report that matters most getting hidden by the report that doesn't. Two flags mean each
* report kind fires exactly once, independent of what the other kind already logged.
*/
private final AtomicBoolean unprotectedGapLogged = new AtomicBoolean();
/**
* Guards the {@code effectiveAllowed != null} branch of {@link #logCredentialGap} — the
* allow-list-scrub-covered report — to one INFO per launcher instance. See {@link
* #unprotectedGapLogged}'s javadoc for why this is a separate flag rather than a shared one.
*/
private final AtomicBoolean allowListGapLogged = new AtomicBoolean();
/**
* CB-596 criterion 4: a credential-shaped host env var name on neither {@code known} nor
* {@code allow} is not silently allowed — it is reported. {@link #hostEnvNames} enumerates the
* daemon's own environment (see that field's javadoc for why the daemon's env is read rather
* than the spawned pane's, which the daemon has no channel to inspect at spawn time); this logs
* every such NAME, at WARN, at most once per launcher instance — never a value, a prefix of a
* value, or a hash of a value, so the log itself cannot leak anything.
* every such NAME — never a value, a prefix of a value, or a hash of a value, so the log itself
* cannot leak anything.
*
* <p>CB-633 follow-up (#192): {@code effectiveAllowed} picks the wording, and it must NOT be
* 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.
*/
private void logCredentialGap(FleetConfig.MemberCredentials creds) {
private void logCredentialGap(FleetConfig.MemberCredentials creds, Set<String> effectiveAllowed) {
Set<String> covered = new HashSet<>(creds.known());
covered.addAll(creds.allow());
List<String> gap = hostEnvNames.get().stream()
@@ -1199,7 +1237,17 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
if (gap.isEmpty()) {
return;
}
if (credentialGapLogged.compareAndSet(false, true)) {
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);
}
return;
}
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 — {}. "
+ "Add each to memberCredentials.known (blocked by default) or .allow "
@@ -1,5 +1,6 @@
package dev.ltms.fleet.member;
import ch.qos.logback.classic.Level;
import ch.qos.logback.classic.Logger;
import ch.qos.logback.classic.spi.ILoggingEvent;
import ch.qos.logback.core.read.ListAppender;
@@ -1055,6 +1056,186 @@ class ClaudeCodeLauncherTest {
"a name already on allow: is covered, not a gap");
}
/**
* CB-633 follow-up (#192): the deny-by-default WARN wording is a promise an operator relies on —
* pinned byte-for-byte so a future edit cannot drift it (e.g. while picking wording for the
* allow-list path) without a test noticing.
*/
@Test
void denyByDefaultKeepsTheExactCredentialGapWarn() {
FakeHerdr herdr = new FakeHerdr();
FleetConfig.Profile cfg = new FleetConfig.Profile(
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN",
List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null);
ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr),
new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(),
_ -> null, 0, System::currentTimeMillis, () -> {}, null, () -> TEST_MEMBER_CREDENTIALS,
() -> Set.of("A_BRAND_NEW_SECRET_TOKEN"));
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
ListAppender<ILoggingEvent> appender = new ListAppender<>();
appender.start();
logger.addAppender(appender);
try {
svc.spawn();
} finally {
logger.detachAppender(appender);
}
assertTrue(appender.list.stream().anyMatch(e ->
("memberCredentials gap: 1 credential-shaped env var name(s) are on neither "
+ "known: nor allow: — every member pane inherits them UNBLOCKED — "
+ "[A_BRAND_NEW_SECRET_TOKEN]. Add each to memberCredentials.known "
+ "(blocked by default) or .allow (if a member legitimately needs it).")
.equals(e.getFormattedMessage())),
"the deny-by-default WARN text must not drift — got: "
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
}
/**
* CB-633 follow-up (#192), defect 1: under {@code policy: allow-list} on a zsh login shell the
* generated ZDOTDIR scrub genuinely blanks an unkept credential-shaped name, so the report must
* not say the pane inherits it UNBLOCKED — that claim is exactly what PR #174 got wrong. This
* goes through the real spawn path (not {@code HerdrPeerLauncherAllowListWiringTest}'s fixture,
* which overrides {@code buildLaunch} and bypasses none of the logic under test here — the
* shell-dependent branch lives in {@code applyEnvironmentAllowListPolicy}, which every spawn
* still passes through).
*/
@Test
void allowListPolicyOnZshReportsTheGapWithoutClaimingItIsUnblocked() {
FakeHerdr herdr = new FakeHerdr();
FleetConfig.Profile cfg = new FleetConfig.Profile(
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_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("AI_GATEWAY_TOKEN"), 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("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN"));
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
Level original = logger.getLevel();
logger.setLevel(Level.INFO);
ListAppender<ILoggingEvent> 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.getFormattedMessage().contains("memberCredentials gap")
&& e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")),
"the unkept name must still be reported — got: "
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
assertFalse(appender.list.stream().anyMatch(e ->
e.getFormattedMessage().contains("memberCredentials gap")
&& e.getFormattedMessage().contains("UNBLOCKED")),
"on zsh the scrub genuinely blanks the name, so the report must not claim it is "
+ "inherited unblocked — got: "
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
}
/**
* CB-633 follow-up (#192): the mirror of the zsh test above. On a non-zsh login shell {@code
* ZDOTDIR} is ignored, so no scrub ever runs — the report must keep the WARN wording (a name here
* really is inherited unblocked) rather than claiming a scrub protects it. This is the trap PR
* #174 fell into the other direction: keying the wording on the shell, not on {@code
* creds.isAllowList()}, is what keeps this branch correct.
*/
@Test
void allowListPolicyOnNonZshKeepsTheWarnWording() {
FakeHerdr herdr = new FakeHerdr();
FleetConfig.Profile cfg = new FleetConfig.Profile(
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_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("AI_GATEWAY_TOKEN"), 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/bash" : null,
0, System::currentTimeMillis, () -> {}, null, () -> creds,
() -> Set.of("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN"));
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
ListAppender<ILoggingEvent> appender = new ListAppender<>();
appender.start();
logger.addAppender(appender);
try {
svc.spawn();
} finally {
logger.detachAppender(appender);
}
assertTrue(appender.list.stream().anyMatch(e ->
e.getFormattedMessage().contains("memberCredentials gap")
&& e.getFormattedMessage().contains("UNBLOCKED")
&& e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")),
"no scrub runs on a non-zsh shell, so the WARN wording must be kept — got: "
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
assertFalse(appender.list.stream().anyMatch(e ->
e.getFormattedMessage().contains("memberCredentials gap")
&& e.getFormattedMessage().toLowerCase(java.util.Locale.ROOT).contains("scrub")),
"nothing is scrubbed on this path, so the report must not claim otherwise — got: "
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
}
/**
* CB-633 follow-up (#192), defect 2: {@code memberCredentials} is a live, re-read-per-spawn
* supplier, so the policy can change between two spawns on the same launcher. Before this fix a
* single {@code AtomicBoolean} guarded both report kinds, so the harmless allow-list INFO on the
* first spawn would permanently suppress the real deny-by-default WARN a later spawn deserves.
* This goes through {@link ClaudeCodeLauncher#buildLaunch}'s real {@code baseEnv()} path — the
* WARN this test pins fires from {@code applyMemberCredentialPolicy}, which {@code
* HerdrPeerLauncherAllowListWiringTest}'s fixture never reaches at all (see its class javadoc).
*/
@Test
void secondSpawnStillWarnsAfterPolicyChangesFromAllowListToDenyByDefault() {
FakeHerdr herdr = new FakeHerdr();
FleetConfig.Profile cfg = new FleetConfig.Profile(
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN",
List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null);
AtomicReference<FleetConfig.MemberCredentials> creds = new AtomicReference<>(
new FleetConfig.MemberCredentials(FleetConfig.MemberCredentials.POLICY_ALLOW_LIST,
List.of("AI_GATEWAY_TOKEN"), 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::get,
() -> Set.of("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN"));
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
Level original = logger.getLevel();
logger.setLevel(Level.INFO);
ListAppender<ILoggingEvent> appender = new ListAppender<>();
appender.start();
try {
logger.addAppender(appender);
svc.spawn(); // allow-list + zsh: harmless INFO, sets the allow-list guard only
creds.set(new FleetConfig.MemberCredentials(FleetConfig.MemberCredentials.POLICY_DENY_BY_DEFAULT,
List.of("AI_GATEWAY_TOKEN"), List.of()));
svc.spawn(); // policy reloaded to deny-by-default: this WARN must NOT be suppressed
} finally {
logger.detachAppender(appender);
logger.setLevel(original);
}
assertTrue(appender.list.stream().anyMatch(e ->
e.getFormattedMessage().contains("memberCredentials gap")
&& e.getFormattedMessage().contains("UNBLOCKED")),
"the second spawn's deny-by-default WARN must still fire even though the first "
+ "spawn's allow-list INFO already logged the same underlying gap — 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