CB-192 review fix: split the allow-list gap by what the scrub actually keeps
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.
This commit is contained in:
@@ -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<String> effectiveAllowed) {
|
||||
Set<String> 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<String> keptByDerivedList = gap.stream()
|
||||
.filter(name -> MemberEnvAllowList.keeps(effectiveAllowed, name))
|
||||
.toList();
|
||||
List<String> 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<String> 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 — {}. "
|
||||
|
||||
@@ -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<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.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<ILoggingEvent> 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
|
||||
|
||||
Reference in New Issue
Block a user