Compare commits

...

2 Commits

Author SHA1 Message Date
ltms d678783af7 scrub: stop export UID= aborting the allow-list scrub mid-loop
CI / contract (pull_request) Successful in 1m23s
CI / build (pull_request) Successful in 1m32s
CB-633's generated scrub.zsh blanks every non-allow-listed exported name
in one loop wrapped in `{ ... } 2>/dev/null`. In zsh, `export UID=` is
not a failed command: it is a fatal parameter error ("failed to change
user ID: operation not permitted") that aborts the whole sourced file.
The loop stops at UID, every later name is left unscrubbed, and the
report-writing block never runs -- silently, because of the 2>/dev/null.

The inversion is the severity. `env` lists inherited names first and the
names a startup file exports last, so the loop blanked the harmless
inherited half and died immediately before the operator's own exports --
exactly the credentials this policy exists to remove.

Measured on a fleet01 member pane: UID is name 42 of 57, and a decoy
exported from ~/.zshrc sat at 58, non-empty, on 8 of 8 spawns. xtrace
ends at `+scrub.zsh:28> export 'UID='` with rc=126. Reproduced
independently on macOS/zsh 5.9 (fleetd #394).

CONTAINMENT: `eval`, then VERIFY.

Neither `export "$n=" 2>/dev/null || true` nor a `${(t)n}` type guard
contains it -- both still abort, because it is an assignment error and
not a command failure. Only `eval` does. `eval` is safe at that point
because the loop above already rejected every name that is not
[A-Za-z_][A-Za-z0-9_]*, so nothing but a bare identifier reaches it.

Enumerating the specials (UID|EUID|GID|EGID|PPID|LINENO) also works, but
only for the names enumerated: one that turns up exported on another
host brings the abort straight back. `eval` covers all of them.

`eval` alone, though, contains the error WITHOUT blanking the value, so
the name would be reported as blanked having never been blanked --
measured: attempted=59 with UID still 1000. So each name is verified
after the attempt and only counted when it actually blanked. A name that
could not be blanked currently falls into the "allowed" count, which is
imprecise in the safe direction; the honest third count ("tried and could
not blank") needs a report-format change -- readReport treats every
non-first non-blank line as a blanked name -- and belongs with #394.

Why the suite stayed green: EnvAllowListScrubTest starts zsh from a
CLEARED parent (`pb.environment().clear()`), where UID is not an
exported name at all, so the abort cannot reproduce there. The new test
supplies the production shape -- UID present and exported -- and asserts
a report exists, which is written by the last statement in the file and
so proves the script ran to completion. Verified by mutation in both
shapes: reverting the fix fails exactly that test and no other.

Orthogonal to #388, which adds a fifth call site to a script that still
aborts at the same name.
2026-09-10 01:12:27 +00:00
Dai Ha 799014e99d Merge #388: scrub a pane shell that is neither login nor interactive
CI / contract (push) Successful in 1m8s
CI / build (push) Successful in 1m38s
2026-09-10 07:21:48 +07:00
2 changed files with 63 additions and 2 deletions
@@ -334,7 +334,30 @@ public final class EnvAllowListScrub {
_cb633_blank+=("$_cb633_n")
done
{ for _cb633_n in "${_cb633_blank[@]}"; do export "$_cb633_n="; done; } 2>/dev/null
# `export UID=` is not a failed command: it is a FATAL zsh parameter error
# ("failed to change user ID") that aborts this whole sourced file mid-loop,
# leaving every later name unscrubbed and the report below unwritten — silently,
# because of the 2>/dev/null. Neither `|| true` nor a `${(t)n}` type guard
# contains it; only `eval` does. `eval` is safe here precisely because the loop
# above already rejected every name that is not [A-Za-z_][A-Za-z0-9_]*, so
# nothing but a bare identifier can reach it.
#
# Enumerating the special names instead (UID|EUID|GID|EGID|PPID|LINENO) also
# works, but only for the ones enumerated: a special that turns up exported on
# some other host brings the abort straight back. `eval` contains all of them.
#
# Then VERIFY. A contained failure is still a failure, so a name that did not
# actually blank must not be reported as blanked. It currently falls into the
# "allowed" count, which is imprecise in the safe direction; the honest third
# count ("tried and could not blank") needs a report-format change and belongs
# with fleetd #394, not here.
typeset -a _cb633_done
_cb633_done=()
for _cb633_n in "${_cb633_blank[@]}"; do
eval "export ${_cb633_n}=" 2>/dev/null
[[ -z "${(P)_cb633_n}" ]] && _cb633_done+=("$_cb633_n")
done
_cb633_blank=("${_cb633_done[@]}")
integer _cb633_kept=$(( _cb633_total - ${#_cb633_blank} ))
{
@@ -342,7 +365,7 @@ public final class EnvAllowListScrub {
for _cb633_n in "${_cb633_blank[@]}"; do print -r -- "$_cb633_n"; done
} > "$ZDOTDIR/%s" 2>/dev/null
unset _cb633_allowed _cb633_names _cb633_blank _cb633_n _cb633_total _cb633_kept
unset _cb633_done _cb633_allowed _cb633_names _cb633_blank _cb633_n _cb633_total _cb633_kept
""".formatted(names, MemberEnvAllowList.zshCasePattern(), REPORT_FILE);
}
@@ -18,6 +18,7 @@ import java.util.regex.Matcher;
import java.util.regex.Pattern;
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.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -118,6 +119,43 @@ class EnvAllowListScrubTest {
"allowed N of M with N <= M — the denominator is always reported");
}
/**
* A pane inherits {@code UID}; a cleared test parent does not. The scrub must survive it.
*
* <p>Every other test here starts zsh from a CLEARED environment, so {@code UID} is never an
* exported name and never reaches the blanking loop. In a real member pane it is exported and
* it IS reached — and {@code export UID=} is a fatal zsh parameter error that aborts the whole
* sourced file, leaving every later name unscrubbed and writing no report at all. The abort is
* silent: the loop is wrapped in {@code 2>/dev/null}.
*
* <p>The assertion is deliberately "a report exists" rather than "the canary is blanked". The
* report is written by the last statement in the file, so its presence proves the script ran
* to completion; the canary alone would depend on where it happens to sit in {@code env} order.
* Both are checked, but only the first one fails deterministically without the fix.
*/
@Test
void scrubSurvivesAnInheritedUidTheWayARealPaneHasIt(@TempDir Path tmp) throws Exception {
assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here");
Set<String> allowed = MemberEnvAllowList.derive(List.of());
Path zdotdir = EnvAllowListScrub.generate(tmp, allowed);
// The production shape: UID present and exported, as every pane shell inherits it.
Map<String, String> paneLikeParent = Map.of(
"HOME", System.getProperty("user.home"),
"PATH", "/usr/bin:/bin",
"SHELL", "/bin/zsh",
"UID", "1000",
"CB633_CANARY", "must-not-survive-the-scrub");
Set<String> survivors = exportedNamesFromCleanParent(paneLikeParent, zdotdir);
EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(zdotdir);
assertNotNull(report,
"an inherited UID must not abort the scrub — no report means the file died mid-loop "
+ "and every name after UID in `env` order was left unscrubbed");
assertFalse(survivors.contains("CB633_CANARY"),
"a non-allow-listed name must still be blanked when UID is in the environment");
}
/** A group-shared ZDOTDIR still lets the member truncate and write its pre-created receipt. */
@Test
void groupSharedScrubWritesAndReadsItsReport(@TempDir Path tmp) throws Exception {