From 6f968d59a4a0cfd3f8c943c8580aa12ccf772e61 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Sun, 23 Aug 2026 08:17:52 +0200 Subject: [PATCH] CB-633 round 2: the scrub ran on macOS and did nothing on Linux Six review findings, from the peer lead `vms` and a reviewer worker. The first one is a real defect that would have shipped as a dead control. 1. The scrub only ran in a login shell. It lived in the generated `.zlogin`, and zsh reads `.zlogin` only for a login shell. herdr does not open the same kind of shell everywhere: measured on herdr 0.8.0, a macOS pane runs `-zsh` (login) while a Linux pane runs a plain `/usr/bin/zsh`. So on the vhost this was being built for, `.zlogin` never ran and every member kept the whole secret store, in silence. The scrub body now lives in a generated `scrub.zsh` that BOTH `.zshrc` and `.zlogin` source, each after sourcing its own `$HOME` counterpart. Linux runs the first, macOS runs both, and the second pass is not merely harmless -- it re-scrubs anything the operator's `~/.zlogin` exported after `~/.zshrc` had finished. Re-running is idempotent. 2. `INFRASTRUCTURE_PASSTHROUGH` listed names that are not infrastructure: ANTHROPIC_AUTH_TOKEN, GITEA_TOKEN, GITEA_HOST, ANTHROPIC_BASE_URL, ANTHROPIC_MODEL, CLAUDE_CONFIG_DIR, OPENCODE_CONFIG, BRIDGED_MEMBER. I read each injection point and confirmed every one of them reaches `launch.env()` only when actually injected, so `allowed.addAll(launch.env().keySet())` already covers the legitimate case. As static entries they were pure leak surface: a host that happened to export ANTHROPIC_AUTH_TOKEN would have had it passed straight through. 3. A missing `scrub-report.txt` at teardown was logged at debug. The report is the only evidence the scrub ran at all. Its absence has an innocent reading and a serious one, and we cannot tell them apart from the daemon -- so it is now a WARN that says exactly that. Logging it at debug is how a control that quietly stopped working stays unnoticed. 4. Nothing tested that the control was wired in. Deleting the single `applyEnvironmentAllowListPolicy(cfg, launch)` line left all 896 tests green while turning the feature completely off -- the CB-586/CB-611 shape again. `HerdrPeerLauncherAllowListWiringTest` starts a real spawn and asserts on the env that reached herdr. Mutation-checked: unwiring that line fails it. 5. `EnvAllowListScrubTest` now also runs `zsh -i` with no `-l`, which is the Linux pane shape, so finding 1 is tested from a Mac. Mutation-checked: putting the scrub back in `.zlogin` alone fails that test alone, while the login-shell test still passes -- which is exactly the blind spot that let the bug through. 6. Two ZDOTDIR leaks closed. A failed spawn has no pane id, so its directory was never keyed for teardown; it is now removed on the way out. And `deleteOnExit` covers a clean shutdown and nothing else, so `generate` now reaps sibling directories older than 24h left by a killed daemon. Also: `policy:` is lowercased with Locale.ROOT, and the `.zlogin`-only claim is corrected in fleetd.example.yaml, FleetConfig and HerdrPeerLauncher. 901 tests, 0 failures, `mvn clean install` green. --- bridged/fleetd.example.yaml | 17 +- .../dev/ltms/fleet/config/FleetConfig.java | 12 +- .../ltms/fleet/member/EnvAllowListScrub.java | 115 ++++++++++-- .../ltms/fleet/member/HerdrPeerLauncher.java | 45 +++-- .../ltms/fleet/member/MemberEnvAllowList.java | 28 +-- .../fleet/member/EnvAllowListScrubTest.java | 60 ++++++- .../HerdrPeerLauncherAllowListWiringTest.java | 170 ++++++++++++++++++ 7 files changed, 398 insertions(+), 49 deletions(-) create mode 100644 bridged/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java diff --git a/bridged/fleetd.example.yaml b/bridged/fleetd.example.yaml index 0d17170..412390e 100644 --- a/bridged/fleetd.example.yaml +++ b/bridged/fleetd.example.yaml @@ -510,11 +510,14 @@ guard: # where that shell does not re-export the name (see ROUND-2 CORRECTION above). An # unrecognized value refuses to start, naming it. # policy → "allow-list" (CB-633) moves the control to a per-spawn ZDOTDIR directory the daemon -# generates and passes through tab.create's env map. The pane's zsh startup order is -# .zshenv → .zprofile → .zshrc → .zlogin, and the operator's whole chain runs inside the -# first three — so the generated .zlogin, which sources ~/.zlogin first and THEN blanks -# every exported variable not on the derived allow-list, runs after everything the -# operator sourced. No sourced file can undo it. The allow-list is DERIVED, never typed: +# generates and passes through tab.create's env map. Each generated startup file sources +# its ~/ counterpart FIRST and then runs the scrub, so the scrub happens after the +# operator's whole chain and no sourced file can undo it. +# The scrub is sourced from BOTH the generated .zshrc and the generated .zlogin, because +# herdr does not open the same kind of shell everywhere: macOS panes run a LOGIN zsh (so +# .zlogin runs), Linux panes run a plain interactive zsh (so .zlogin never runs at all). +# A scrub in .zlogin alone would be a control that silently does nothing on Linux. +# The allow-list is DERIVED, never typed: # every profile's tokenEnv/gitTokenEnv/gitHostEnv values and env-map keys, plus an # infrastructure set (PATH HOME SHELL TERM LANG LC_* TMPDIR USER LOGNAME PWD SHLVL EDITOR # PAGER JAVA_HOME XDG_* ZDOTDIR), plus whatever keys this spawn's own env overlay carries. @@ -522,6 +525,10 @@ guard: # Under this policy `known`/`allow` below become REPORTING ONLY — they feed the gap WARN, # they are no longer a control. If the member's login shell is NOT zsh, the daemon logs a # loud WARN saying protection is off and falls back to deny-by-default's overlay. +# Each pane writes a scrub-report.txt naming how many variables it kept of how many it +# saw; the daemon logs that "allowed N of M" line when the pane stops. If the report is +# MISSING the daemon logs a WARN instead — the scrub cannot then be confirmed to have +# run, and a silently-dead control is exactly what this policy exists to prevent. # allow → credential names a member legitimately needs. Under deny-by-default, left OUT of the # pane's env overlay entirely, so the value the pane's own (login) shell exports passes # through untouched. Under allow-list: reporting only. diff --git a/bridged/src/main/java/dev/ltms/fleet/config/FleetConfig.java b/bridged/src/main/java/dev/ltms/fleet/config/FleetConfig.java index 455a65d..33ad132 100644 --- a/bridged/src/main/java/dev/ltms/fleet/config/FleetConfig.java +++ b/bridged/src/main/java/dev/ltms/fleet/config/FleetConfig.java @@ -959,8 +959,8 @@ public record FleetConfig( * *

CB-633: allow-list. Deny-by-default's overlay is applied BEFORE the pane's login * shell runs, so any file that chain sources can re-export over it — and did. The allow-list - * policy moves the control to a generated ZDOTDIR whose {@code .zlogin} runs LAST, after the - * whole operator chain, and blanks every exported variable not on an allow-list DERIVED from + * policy moves the control to a generated ZDOTDIR whose startup files run the scrub LAST, after + * the whole operator chain, and blank every exported variable not on an allow-list DERIVED from * what the launcher itself injects (profiles' tokenEnv/gitTokenEnv/gitHostEnv/env keys plus an * infrastructure set) — never hand-typed, so adding a profile cannot break a spawn. Under this * policy {@link #allow} and {@link #known} stop being a control and become reporting only. @@ -1005,8 +1005,10 @@ public record FleetConfig( /** * CB-633: derive the kept-name set from what the launcher itself injects, generate a - * per-spawn ZDOTDIR whose {@code .zlogin} blanks every exported variable not on it AFTER the - * pane's login shell has finished sourcing the operator's chain. + * per-spawn ZDOTDIR whose startup files blank every exported variable not on it AFTER the + * pane's shell has finished sourcing the operator's chain. The scrub is sourced from both + * the generated {@code .zshrc} and {@code .zlogin}, because herdr opens a LOGIN zsh on macOS + * and a plain interactive one on Linux — see {@code EnvAllowListScrub}. */ public static final String POLICY_ALLOW_LIST = "allow-list"; @@ -1017,7 +1019,7 @@ public record FleetConfig( public MemberCredentials { String normalizedPolicy = (policy == null || policy.isBlank()) - ? POLICY_DENY_BY_DEFAULT : policy.toLowerCase(); + ? POLICY_DENY_BY_DEFAULT : policy.toLowerCase(java.util.Locale.ROOT); // deny-list is an alias of deny-by-default, not a third behaviour — normalize to one // spelling so every isDenyList()-style check has one value to compare against. policy = POLICY_DENY_LIST.equals(normalizedPolicy) ? POLICY_DENY_BY_DEFAULT : normalizedPolicy; diff --git a/bridged/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java b/bridged/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java index 5cc4812..be8839c 100644 --- a/bridged/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java +++ b/bridged/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java @@ -1,9 +1,14 @@ package dev.ltms.fleet.member; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Path; +import java.time.Duration; +import java.time.Instant; import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -13,12 +18,27 @@ import java.util.stream.Stream; * CB-633: generates the per-spawn {@code ZDOTDIR} directory whose startup files enforce * {@code memberCredentials.policy: allow-list}. * - *

The seam: a login interactive zsh sources {@code $ZDOTDIR/.zshenv}, then {@code .zprofile}, - * then {@code .zshrc}, then {@code .zlogin} — in that order, LAST first-named-last. The operator's - * whole chain ({@code ~/.zshrc} → secret store) runs inside the first three, so a scrub placed in - * the generated {@code .zlogin} runs after everything the operator sourced, and nothing later can - * re-export over it. This is the property CB-596's env-overlay control lacked: herdr applies that - * overlay BEFORE the login shell starts, so any sourced file can undo it — and did. + *

The seam: zsh reads its startup files from {@code $ZDOTDIR}, and the daemon puts that variable + * in the pane-creation env map. The operator's whole chain ({@code ~/.zshrc} → secret store) runs + * inside those files, so a scrub appended to the LAST one runs after everything the operator + * sourced, and nothing later can re-export over it. This is the property CB-596's env-overlay + * control lacked: herdr applies that overlay BEFORE the shell starts, so any sourced file can undo + * it — and did. + * + *

Which file is last depends on the platform, so the scrub runs from two of them. zsh + * reads {@code .zshenv} always, {@code .zprofile} and {@code .zlogin} only for a LOGIN shell, and + * {@code .zshrc} only for an INTERACTIVE one. herdr does not open the same kind of shell + * everywhere — measured on herdr 0.8.0: macOS panes run {@code -zsh} (login, so {@code .zlogin} + * runs), Linux panes run a plain {@code /usr/bin/zsh} (interactive but NOT login, so + * {@code .zlogin} never runs at all). A scrub in {@code .zlogin} alone is therefore a control that + * silently does nothing on Linux — the exact failure this class exists to remove, one platform + * over. + * + *

So both {@code .zshrc} and {@code .zlogin} source the same generated {@code scrub.zsh} after + * sourcing their {@code $HOME} counterpart. On Linux only the first fires; on macOS both do, and + * the second pass is deliberate rather than merely harmless — it re-scrubs anything the operator's + * own {@code ~/.zlogin} exported after {@code .zshrc} had finished. Re-running is idempotent: a + * name already blank is blanked again, and the report is rewritten with the same counts. * *

Each generated file sources its {@code $HOME} counterpart FIRST, so {@code PATH} and every * toolchain binary still resolve exactly as the operator configured them; only afterwards does @@ -35,9 +55,27 @@ import java.util.stream.Stream; */ public final class EnvAllowListScrub { + private static final Logger log = LoggerFactory.getLogger(EnvAllowListScrub.class); + /** Name of the report file written into the generated directory by the scrub itself. */ static final String REPORT_FILE = "scrub-report.txt"; + /** The scrub body, generated once and sourced from both {@code .zshrc} and {@code .zlogin}. */ + static final String SCRUB_FILE = "scrub.zsh"; + + /** Prefix of every generated directory — also what {@link #reapOrphans} matches on. */ + static final String DIR_PREFIX = "bridged-zdotdir-"; + + /** + * How old an orphan must be before {@link #reapOrphans} removes it. Comfortably longer than any + * spawn takes, so a directory belonging to a pane that is still starting is never removed. + */ + private static final Duration ORPHAN_AGE = Duration.ofHours(24); + + /** Appended to the two startup files that must run the scrub, after their {@code $HOME} source. */ + private static final String SOURCE_SCRUB = + "source \"$ZDOTDIR/" + SCRUB_FILE + "\"\n"; + private EnvAllowListScrub() { } @@ -61,12 +99,17 @@ public final class EnvAllowListScrub { */ public static Path generate(Path parentDir, Set allowedNames) { try { - Path dir = Files.createTempDirectory(parentDir, "bridged-zdotdir-"); + reapOrphans(parentDir); + Path dir = Files.createTempDirectory(parentDir, DIR_PREFIX); dir.toFile().deleteOnExit(); + // The report is written by zsh, after these hooks are registered, so register its path + // too — otherwise the directory is non-empty at JVM exit and cannot be removed at all. + dir.resolve(REPORT_FILE).toFile().deleteOnExit(); + write(dir, SCRUB_FILE, scrubScript(allowedNames)); write(dir, ".zshenv", homeSourcingFile(".zshenv")); write(dir, ".zprofile", homeSourcingFile(".zprofile")); - write(dir, ".zshrc", homeSourcingFile(".zshrc")); - write(dir, ".zlogin", zloginScript(allowedNames)); + write(dir, ".zshrc", homeSourcingFile(".zshrc") + SOURCE_SCRUB); + write(dir, ".zlogin", homeSourcingFile(".zlogin") + SOURCE_SCRUB); return dir; } catch (IOException e) { throw new UncheckedIOException("cannot generate ZDOTDIR scrub files under " + parentDir, e); @@ -83,12 +126,13 @@ public final class EnvAllowListScrub { } /** - * The generated {@code .zlogin}: source the operator's own {@code ~/.zlogin}, then run the scrub. - * Package-private so tests can assert on the exact script handed to zsh — the artefact here IS a - * shell file, and a test that checks only the Java string assembly proves nothing about whether - * zsh accepts it. + * The generated {@code scrub.zsh} — the scrub body on its own, so the two startup files that + * must run it ({@code .zshrc} and {@code .zlogin}) hold one copy between them rather than two + * that can drift. Package-private so tests can assert on the exact script handed to zsh — the + * artefact here IS a shell file, and a test that checks only the Java string assembly proves + * nothing about whether zsh accepts it. */ - static String zloginScript(Set allowedNames) { + static String scrubScript(Set allowedNames) { StringBuilder names = new StringBuilder(); for (String n : allowedNames.stream().sorted().toList()) { if (names.length() > 0) { @@ -100,8 +144,11 @@ public final class EnvAllowListScrub { } return """ # generated by fleetd (CB-633 memberCredentials policy=allow-list) — do not edit. - # Runs LAST in the login-shell order, after everything the operator sourced. - [ -r "$HOME/.zlogin" ] && source "$HOME/.zlogin" + # Sourced from .zshrc and again from .zlogin, each time AFTER that file has sourced + # its $HOME counterpart — so this runs after everything the operator sourced, on a + # login shell (macOS panes) and on a plain interactive one (Linux panes) alike. + # Running twice is idempotent and deliberate: the second pass catches anything + # ~/.zlogin exported after ~/.zshrc had finished. typeset -A _cb633_allowed for _cb633_n in %s; do _cb633_allowed[$_cb633_n]=1; done @@ -174,6 +221,42 @@ public final class EnvAllowListScrub { } /** Best-effort recursive delete; failures are swallowed — JVM-exit cleanup is the backstop. */ + /** + * Remove generated directories left behind by an earlier daemon process. + * + *

{@link #generate} registers each directory for deletion at JVM exit, which covers a clean + * shutdown and covers nothing else. A {@code kill -9}, a crash, or a host reboot leaves the + * directory in the temp dir for good, and the daemon is restarted often enough that these + * accumulate. They hold no secrets — the generated files contain variable NAMES and a report of + * names, never a value — but an unbounded pile of them in {@code /tmp} is still our mess to + * clear. + * + *

Called from {@link #generate}, so it runs on the path that creates them and needs no + * separate wiring or scheduler. Only directories older than {@link #ORPHAN_AGE} are touched, + * which keeps it clear of any pane that is merely still starting, including one belonging to a + * different daemon instance running right now. Best-effort: every failure is ignored, because + * tidying temp files must never be the reason a spawn fails. + */ + static void reapOrphans(Path parentDir) { + Instant cutoff = Instant.now().minus(ORPHAN_AGE); + try (Stream entries = Files.list(parentDir)) { + entries.filter(d -> d.getFileName().toString().startsWith(DIR_PREFIX)) + .filter(Files::isDirectory) + .filter(d -> olderThan(d, cutoff)) + .forEach(EnvAllowListScrub::deleteRecursively); + } catch (IOException | RuntimeException e) { + log.debug("could not scan {} for orphaned ZDOTDIRs: {}", parentDir, e.toString()); + } + } + + private static boolean olderThan(Path dir, Instant cutoff) { + try { + return Files.getLastModifiedTime(dir).toInstant().isBefore(cutoff); + } catch (IOException e) { + return false; // unreadable timestamp ⇒ leave it alone + } + } + static void deleteRecursively(Path dir) { if (dir == null || !Files.exists(dir)) { return; diff --git a/bridged/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java b/bridged/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java index 115ce7d..07e26da 100644 --- a/bridged/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/bridged/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -438,9 +438,20 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { // the exact env-map keys this launch injects (ANTHROPIC_*, OPENCODE_CONFIG, GITEA_TOKEN, …) // — anything the daemon deliberately sets must survive its own control. Path zdotdir = applyEnvironmentAllowListPolicy(cfg, launch); - Agent agent = cfg.tabPlacement() - ? spawnInTab(cfg, launch.env(), launch.argv(), cwd, role, liveFleet) - : spawnAsPane(cfg, launch.env(), launch.argv(), cwd, charter); + Agent agent; + try { + agent = cfg.tabPlacement() + ? spawnInTab(cfg, launch.env(), launch.argv(), cwd, role, liveFleet) + : spawnAsPane(cfg, launch.env(), launch.argv(), cwd, charter); + } catch (RuntimeException e) { + // A failed spawn has no pane id, so nothing would ever key this directory for + // teardown and it would sit in the temp dir until the JVM exits cleanly — which, + // for a daemon, may be never. Remove it on the way out. + if (zdotdir != null) { + EnvAllowListScrub.deleteRecursively(zdotdir); + } + throw e; + } if (zdotdir != null) { zdotdirByPane.put(agent.paneId(), zdotdir); } @@ -1013,9 +1024,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { /** * CB-633: under {@code memberCredentials.policy: allow-list}, generate the per-spawn ZDOTDIR - * directory whose {@code .zlogin} blanks every exported variable not on the DERIVED allow-list — - * running AFTER the pane's login shell has finished sourcing the operator's chain, which is what - * no pre-shell env overlay can achieve. Mutates {@code launch.env()} to carry + * directory whose startup files blank every exported variable not on the DERIVED allow-list — + * running AFTER the pane's shell has finished sourcing the operator's chain, which is what no + * pre-shell env overlay can achieve. {@code EnvAllowListScrub} sources the scrub from both the + * generated {@code .zshrc} and {@code .zlogin}, since a herdr pane is a login shell on macOS + * and a plain interactive one on Linux. Mutates {@code launch.env()} to carry * {@code ZDOTDIR=

}, so both placement paths ({@link #spawnInTab}, {@link #spawnAsPane}) * pass it through {@code tab.create}/{@code pane.split}. Returns the directory for teardown * registration, or {@code null} when the policy does not apply. @@ -1075,9 +1088,17 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { /** * CB-633 teardown half: read the pane's scrub report (the denominator report the generated - * {@code .zlogin} wrote) and delete the directory. Called from {@link #stop}, which is the one - * funnel every teardown exit already goes through. Best-effort throughout: a missing report is - * logged at debug, never an error — the pane may be gone before its shell reached the scrub. + * scrub wrote) and delete the directory. Called from {@link #stop}, which is the one funnel + * every teardown exit already goes through. + * + *

A missing report is a WARN, not a debug line. The report is the only evidence that + * the scrub ran at all in that pane. Its absence has an innocent reading — the pane died before + * its shell finished starting — and a serious one: the shell was not zsh, or it read its + * startup files from somewhere other than the directory we generated, in which case the member + * ran for its whole life with the operator's full secret store in its environment and nothing + * said so. We cannot tell those two apart from here, so the line says what is and is not known + * rather than picking one. Logging this at debug is how a control that silently stopped working + * stays unnoticed — the failure mode this whole class exists to remove. */ private void releaseZdotdir(String paneId) { Path dir = zdotdirByPane.remove(paneId); @@ -1086,7 +1107,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { } EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(dir); if (report == null) { - log.debug("memberCredentials allow-list: pane {} left no scrub report", paneId); + log.warn("memberCredentials allow-list: pane {} left no scrub report in {} — the " + + "environment scrub cannot be confirmed to have run. Either the pane ended " + + "before its shell finished starting, or its shell never read our generated " + + "startup files, in which case that member saw the full host environment.", + paneId, dir); } else { log.info("memberCredentials allow-list: pane {} allowed {} of {} environment variables", paneId, report.allowed(), report.total()); diff --git a/bridged/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java b/bridged/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java index 9e97562..8b7138a 100644 --- a/bridged/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java +++ b/bridged/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java @@ -41,23 +41,27 @@ public final class MemberEnvAllowList { /** * Names that are not credentials and that a login shell or agent binary genuinely needs. * - *

Deliberately conservative beyond the ticket's named set: {@code ZDOTDIR} must survive or - * every later sub-shell loses the scrub; {@code BRIDGED_MEMBER} is the daemon's own marker; - * {@code GITEA_TOKEN}/{@code GITEA_HOST} are what {@code applyGitToken} injects by literal name; - * the {@code ANTHROPIC_*}/{@code CLAUDE_CONFIG_DIR}/{@code OPENCODE_CONFIG} names are what the - * adapters inject by literal name (they are also re-added per-spawn from the env map itself — - * listing them here keeps the derived set self-contained for tests and reporting); {@code - * JAVA_HOME} and the {@code XDG_*} roots are toolchain locations, not secrets. Everything else a - * member needs must arrive via a profile's {@code env:}, which lands on this set automatically. + *

Every name here is a location or a shell setting, never a credential. That rule is load + * bearing, and CB-633's first cut broke it: it also listed {@code ANTHROPIC_AUTH_TOKEN}, + * {@code GITEA_TOKEN}, {@code GITEA_HOST}, {@code ANTHROPIC_BASE_URL}, {@code ANTHROPIC_MODEL}, + * {@code CLAUDE_CONFIG_DIR}, {@code OPENCODE_CONFIG} and {@code BRIDGED_MEMBER} "because the + * launcher injects them". The launcher does — but only on the spawns where it actually sets + * them, and {@code HerdrPeerLauncher} already unions THIS spawn's env-map keys into the + * allow-list. So a static entry adds nothing on a spawn that injects the name, and on a spawn + * that does not it lets the operator's own value through under exactly the name a member reads. + * {@code ANTHROPIC_BASE_URL} is the sharpest case: an inherited one silently moves a member off + * the endpoint the profile chose. + * + *

{@code ZDOTDIR} stays because it is this control's own handle — lose it and every later + * sub-shell loses the scrub. {@code JAVA_HOME} and the {@code XDG_*} roots are toolchain + * locations. Everything else a member needs must arrive via a profile's {@code env:} or the + * launcher's own injection, both of which land on the derived set automatically. */ public static final Set INFRASTRUCTURE_PASSTHROUGH = Set.of( "PATH", "HOME", "SHELL", "TERM", "LANG", "TMPDIR", "USER", "LOGNAME", "PWD", "SHLVL", "EDITOR", "PAGER", "_", - "ZDOTDIR", "BRIDGED_MEMBER", - "GITEA_TOKEN", "GITEA_HOST", - "ANTHROPIC_BASE_URL", "ANTHROPIC_AUTH_TOKEN", "ANTHROPIC_MODEL", - "CLAUDE_CONFIG_DIR", "OPENCODE_CONFIG", + "ZDOTDIR", "JAVA_HOME", "XDG_CONFIG_HOME", "XDG_DATA_HOME", "XDG_CACHE_HOME", "XDG_STATE_HOME"); diff --git a/bridged/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java b/bridged/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java index 46475d9..77323ba 100644 --- a/bridged/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java +++ b/bridged/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java @@ -7,6 +7,7 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -112,6 +113,54 @@ class EnvAllowListScrubTest { assertNull(EnvAllowListScrub.readReport(dir)); } + /** + * The same equality, for a shell that is INTERACTIVE but NOT a login shell — the shape herdr + * opens on Linux. + * + *

Why this test exists. The first version of this control put the scrub in {@code .zlogin} + * alone. zsh reads {@code .zlogin} only for a login shell, and herdr does not open one + * everywhere: measured on herdr 0.8.0, a macOS pane runs {@code -zsh} (login) while a Linux pane + * runs a plain {@code /usr/bin/zsh}. So the control would have passed every test on the + * developer's Mac and protected nothing at all on the vhost it was being built for, in silence. + * + *

This runs {@code zsh -i} — no {@code -l} — so {@code .zprofile} and {@code .zlogin} are + * skipped exactly as they are on Linux. It therefore tests the Linux code path from a Mac, + * which is the only place we can currently run it. Reverting the scrub to {@code .zlogin} only + * makes this test fail while the login-shell test above still passes. + */ + @Test + void scrubAlsoRunsInAnInteractiveNonLoginShell(@TempDir Path tmp) throws Exception { + assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here"); + Path homeZshrc = Path.of(System.getProperty("user.home"), ".zshrc"); + assumeTrue(Files.exists(homeZshrc), "$HOME/.zshrc does not exist — no real chain to test against"); + + Set allowed = MemberEnvAllowList.derive(List.of()); + Path zdotdir = EnvAllowListScrub.generate(tmp, allowed); + + Map cleanParent = Map.of( + "HOME", System.getProperty("user.home"), + "PATH", "/usr/bin:/bin", + "SHELL", "/bin/zsh", + "USER", System.getProperty("user.name", "nobody"), + "TMPDIR", tmp.toString()); + + List interactiveOnly = List.of("-i"); + Set baseline = exportedNamesFromCleanParent(cleanParent, null, interactiveOnly); + Set scrubbed = exportedNamesFromCleanParent(cleanParent, zdotdir, interactiveOnly); + + Set expected = new TreeSet<>(); + for (String name : baseline) { + if (MemberEnvAllowList.keeps(allowed, name)) { + expected.add(name); + } + } + expected.add("ZDOTDIR"); // the harness set it and it is infrastructure, so it must survive + assertEquals(expected, scrubbed, + "a non-login interactive zsh is what a herdr pane runs on Linux; its surviving " + + "exported names must EQUAL baseline \u2229 allow-list, exactly as for a login " + + "shell. A difference here means the scrub is dead on Linux."); + } + /** * Run {@code /bin/zsh -l -i} from a clean parent and return the NAMES it has exported by prompt * time. With {@code zdotdir} non-null, {@code ZDOTDIR} points at a generated scrub directory, so @@ -119,7 +168,16 @@ class EnvAllowListScrubTest { */ private static Set exportedNamesFromCleanParent(Map cleanParent, Path zdotdir) throws IOException, InterruptedException { - ProcessBuilder pb = new ProcessBuilder("/bin/zsh", "-l", "-i"); + return exportedNamesFromCleanParent(cleanParent, zdotdir, List.of("-l", "-i")); + } + + private static Set exportedNamesFromCleanParent(Map cleanParent, + Path zdotdir, List shellFlags) + throws IOException, InterruptedException { + List argv = new ArrayList<>(); + argv.add("/bin/zsh"); + argv.addAll(shellFlags); + ProcessBuilder pb = new ProcessBuilder(argv); pb.environment().clear(); pb.environment().putAll(cleanParent); if (zdotdir != null) { diff --git a/bridged/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java b/bridged/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java new file mode 100644 index 0000000..785597c --- /dev/null +++ b/bridged/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java @@ -0,0 +1,170 @@ +package dev.ltms.fleet.member; + +import dev.ltms.fleet.config.FleetConfig; +import dev.ltms.fleet.herdr.AgentControl; +import dev.ltms.fleet.herdr.FakeHerdr; +import dev.ltms.fleet.herdr.WorkspaceControl; +import dev.ltms.fleet.peer.Capability; +import dev.ltms.fleet.peer.MemberRole; +import dev.ltms.fleet.peer.SpawnRequest; +import org.junit.jupiter.api.Test; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.Supplier; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * CB-633: proves the allow-list scrub is actually WIRED INTO the spawn path — not merely that its + * pieces work when a test calls them directly. + * + *

Why this test exists, and why it is separate from {@link EnvAllowListScrubTest}. Every other + * test of this feature calls {@code EnvAllowListScrub} or {@code MemberEnvAllowList} itself. Those + * prove the scrub is correct. None of them proves anyone runs it: deleting the single + * {@code applyEnvironmentAllowListPolicy(cfg, launch)} line from {@code spawnInternal} left all 896 + * tests green while turning the control completely off. That is the recurring shape in this + * codebase — a feature behind one call, with every test on the far side of it (CB-586, CB-611). + * + *

So this test starts a real spawn through {@link HerdrPeerLauncher#spawn} and asserts on what + * reached herdr. It deliberately checks the pane-creation parameters rather than the launcher's own + * map, because the map is an intermediate: {@code ZDOTDIR} only protects anything if it is in the + * env herdr uses to create the pane, and that is the last point we can observe before the shell + * starts. + */ +class HerdrPeerLauncherAllowListWiringTest { + + /** A name the daemon itself injects — it must survive its own scrub, so it must be allowed. */ + private static final String INJECTED = "ANTHROPIC_BASE_URL"; + + @Test + void spawningUnderAllowListPolicyGivesThePaneAGeneratedZdotdir() { + FakeHerdr herdr = new FakeHerdr(); + WiringLauncher launcher = new WiringLauncher(herdr, allowList()); + + launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + + String paneParams = String.valueOf(herdr.lastCall("pane.split").params()); + assertTrue(paneParams.contains("ZDOTDIR"), + "the spawn must hand herdr a ZDOTDIR so the pane's zsh reads our generated startup " + + "files; without it the scrub never runs and the member inherits the whole " + + "host environment. pane.split params were: " + paneParams); + + Path dir = Path.of(launcher.env.get("ZDOTDIR")); + assertTrue(Files.isDirectory(dir), "ZDOTDIR must point at a directory that exists: " + dir); + // Both startup files must exist and both must source the scrub: .zlogin covers macOS panes + // (login shells), .zshrc covers Linux panes (interactive, NOT login). Checking only one + // would pass on the platform it was written for and ship a dead control on the other. + for (String file : List.of(".zshrc", ".zlogin")) { + Path f = dir.resolve(file); + assertTrue(Files.isRegularFile(f), file + " must be generated: " + f); + assertTrue(readAll(f).contains(EnvAllowListScrub.SCRUB_FILE), + file + " must source " + EnvAllowListScrub.SCRUB_FILE + " — a scrub only one of " + + "them runs is dead on the platform that reads the other"); + } + assertTrue(readAll(dir.resolve(EnvAllowListScrub.SCRUB_FILE)).contains(INJECTED), + "the allow-list must include the names this very launch injects (" + INJECTED + + "), or the daemon's own configuration is blanked by its own control"); + } + + /** The default policy must not generate anything — an upgrade changes nothing until asked. */ + @Test + void spawningUnderTheDefaultPolicyGeneratesNoZdotdir() { + FakeHerdr herdr = new FakeHerdr(); + WiringLauncher launcher = new WiringLauncher(herdr, () -> new FleetConfig.MemberCredentials( + null, List.of(), List.of(), null)); + + launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + + assertFalse(launcher.env.containsKey("ZDOTDIR"), + "policy=deny-by-default is the shipped default; it must not silently start " + + "rewriting members' shell startup files"); + } + + /** + * A non-zsh shell cannot read {@code ZDOTDIR} at all. The launcher must fall back rather than + * generate a directory nothing will ever read — a directory that would look like protection. + */ + @Test + void aNonZshShellGeneratesNothingAndFallsBack() { + FakeHerdr herdr = new FakeHerdr(); + WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/bash"); + + launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + + assertFalse(launcher.env.containsKey("ZDOTDIR"), + "bash ignores ZDOTDIR; setting it would be protection theatre"); + } + + private static Supplier allowList() { + return () -> new FleetConfig.MemberCredentials( + FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of(), null); + } + + private static String readAll(Path p) { + try { + return Files.readString(p); + } catch (java.io.IOException e) { + throw new AssertionError("cannot read " + p, e); + } + } + + private static FleetConfig.Profile profile() { + return new FleetConfig.Profile("test", "http://gx00.gw:8000", null, null, + "BRIDGED_WORKER_TOKEN", List.of("test"), "pane", null, null, null, null, null); + } + + /** + * A minimal peer launcher whose {@code buildLaunch} returns a MUTABLE env map holding one name + * the daemon injects. Mutable on purpose: the policy adds {@code ZDOTDIR} to this very map, so + * an immutable one would throw and the test would pass for the wrong reason. + */ + private static final class WiringLauncher extends HerdrPeerLauncher { + private final Map env = new HashMap<>(Map.of(INJECTED, "http://gateway")); + + WiringLauncher(FakeHerdr herdr, Supplier creds) { + this(herdr, creds, "/bin/zsh"); + } + + WiringLauncher(FakeHerdr herdr, Supplier creds, String shell) { + super("test", new AgentControl(herdr), new WorkspaceControl(herdr), + Map.of("test", profile()), "test", + name -> "SHELL".equals(name) ? shell : null, + 0, () -> 0L, () -> { }, null, creds); + } + + @Override + protected Launch buildLaunch(FleetConfig.Profile cfg, LaunchSpec spec) { + return new Launch(env, List.of("test")); + } + + @Override + public Set capabilities() { + return Set.of(); + } + } + + /** The generated directory is a temp directory; make sure the test does not leave a pile. */ + @Test + void theGeneratedDirectoryIsRemovedWhenThePaneIsStopped() { + FakeHerdr herdr = new FakeHerdr(); + WiringLauncher launcher = new WiringLauncher(herdr, allowList()); + + var spawned = launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + Path dir = Path.of(launcher.env.get("ZDOTDIR")); + assertNotNull(spawned, "spawn returned nothing"); + assertTrue(Files.isDirectory(dir)); + + launcher.stop(spawned.id()); + + assertEquals(false, Files.exists(dir), + "stopping the pane must remove its generated ZDOTDIR: " + dir); + } +}