fleetd #400: {@code eval}'s exit status is not proof that the blank actually happened. + * zsh coerces a bare {@code NAME=} assignment on an integer special parameter (measured on macOS zsh + * 5.9: {@code SECONDS}, {@code RANDOM}, {@code SHLVL}, {@code HISTSIZE}, {@code COLUMNS}, + * {@code LINES}, {@code USERNAME}) to a number instead of failing — {@code eval} returns success, + * the value is untouched, and a status-based classification reports it as blanked when it was not. + * The fix classifies on the observed effect instead: after the attempt, the name's value is read + * back with the {@code (P)} indirection flag and the decision is made from whether that is now + * empty. This one check covers all three shapes a name can take at this point — a genuine blank, a + * fatal read-only error {@code eval} merely contained, and this silent no-op — and the exit status + * plays no part in the decision at all. */ public final class EnvAllowListScrub { @@ -376,6 +387,16 @@ public final class EnvAllowListScrub { # operation this can never fire (the enumeration guard already filtered everything # reaching _cb633_blank), so a name caught here is counted as unblankable rather than # silently dropped — it is real evidence that the upstream guard was bypassed. + # + # fleetd #400: the attempt's own exit status is NOT proof of its effect. zsh coerces + # a bare `NAME=` assignment on an integer special parameter (SECONDS, RANDOM, SHLVL, + # HISTSIZE, COLUMNS, LINES, USERNAME on this host) to a number instead of failing — + # `eval` returns 0, the value is untouched, and the old exit-status check reported it + # as blanked when it was not. Classify on the observed effect instead: attempt the + # export, then read the name's value back with the `(P)` indirection flag and decide + # from whether it is now empty. One check then covers all three shapes a name can + # take here — a genuine blank, a fatal read-only error `eval` merely contained, and + # this silent no-op — without the exit status entering the decision at all. typeset -a _cb633_ok _cb633_unblankable _cb633_ok=() _cb633_unblankable=() @@ -384,7 +405,8 @@ public final class EnvAllowListScrub { _cb633_unblankable+=("$_cb633_n") continue fi - if eval "export ${_cb633_n}=" 2>/dev/null; then + eval "export ${_cb633_n}=" 2>/dev/null + if [[ -z "${(P)_cb633_n}" ]]; then _cb633_ok+=("$_cb633_n") else _cb633_unblankable+=("$_cb633_n") diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java index 8ccdd77..1af5bfe 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java @@ -183,11 +183,85 @@ class EnvAllowListScrubTest { + "silently left untouched"); assertTrue(report.blanked().contains("FLEETD_TEST_AFTER_B"), "the SECOND name after the unblankable one must also still be blanked"); - assertEquals(1, report.failed(), - "exactly one attempted name could not be blanked, and that count must be visible " - + "without reading the member's environment"); - assertEquals(List.of("FLEETD_TEST_UNBLANKABLE"), report.unblankable(), + assertTrue(report.unblankable().contains("FLEETD_TEST_UNBLANKABLE"), "the unblankable name is reported by name, not silently dropped"); + // fleetd #400 note: zsh itself auto-exports SHLVL on every shell start (measured: it appears + // in `command env` even from a fully cleared parent), and a bare assignment to it is coerced + // rather than failing — exactly the shape #400 fixes. Before that fix, eval's exit status + // alone silently misclassified SHLVL as blanked, so this test's old "exactly one" assertion + // passed by accident: it never actually proved SHLVL was absent from the candidates, only + // that the old bug hid it. Now that classification reads the value back, SHLVL and + // FLEETD_TEST_UNBLANKABLE both correctly land in unblankable() — real, unplanted evidence + // the #400 fix works, not just the synthetic case in the dedicated #400 test above. + assertTrue(report.unblankable().contains("SHLVL"), + "fleetd #400: zsh's own auto-exported SHLVL must also be reported unblankable, not " + + "silently miscounted as blanked"); + assertEquals(report.unblankable().size(), report.failed(), + "the failed count must equal the number of names actually reported unblankable"); + } + + /** + * fleetd #400: {@code eval}'s exit status is not proof that a name was actually blanked. zsh + * coerces a bare {@code NAME=} assignment on an integer special parameter to a number instead of + * failing, so {@code eval} reports success while the value stays non-empty — a status-based + * classification calls that "blanked" when it was not. This drives all three shapes a name can + * take through the real {@code scrubScript} in ONE run: a normal, genuinely blankable name; a + * fatal one ({@code LINENO} — deliberately not {@code UID}, so this test does not depend on the + * harness's uid); and the silent-no-op one the ticket is about ({@code SECONDS}, rc 0 but + * unchanged). Under the pre-#400 exit-status check, {@code SECONDS} would land in + * {@code blanked()} — that is the exact false receipt this fix removes. + * + *
Criterion 3: a cleared {@code ProcessBuilder} parent does not, by itself, give the child + * zsh any of these names — {@code SECONDS}/{@code LINENO} are zsh's own built-in parameters and + * only become CANDIDATES the enumeration loop can see (i.e. show up in {@code command env}) when + * they arrive via the process's own environment table, not merely by existing as zsh parameters + * inside the shell. So each is put into {@code pb.environment()} explicitly, after + * {@code clear()} — confirmed empirically first (a throwaway probe piping + * {@code env -i PATH=... SECONDS=999 LINENO=999 FLEETD_TEST_NORMAL=1 zsh -c 'command env | cut + * -d= -f1'}) that all three names really appear in {@code command env}'s output under exactly + * this construction, not relying on whatever the test-runner's own ambient environment happens + * to contain. + */ + @Test + void classifiesByObservedValueNotExitStatusAcrossAllThreeShapes(@TempDir Path tmp) throws Exception { + assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here"); + + String script = EnvAllowListScrub.scrubScript(Set.of("ZDOTDIR")); + + ProcessBuilder pb = new ProcessBuilder("/bin/zsh"); + pb.environment().clear(); + pb.environment().put("PATH", "/usr/bin:/bin"); + pb.environment().put("ZDOTDIR", tmp.toAbsolutePath().toString()); + // Explicitly placed in the child's environment table — see the javadoc above on why a + // cleared parent alone does not put these on the enumeration loop's candidate list. + pb.environment().put("SECONDS", "999"); // rc 0, value coerced/unchanged — the #400 bug + pb.environment().put("LINENO", "999"); // fatal on assignment, eval rc != 0, contained + pb.environment().put("FLEETD_TEST_NORMAL", "1"); // genuinely blankable, the control case + pb.redirectError(ProcessBuilder.Redirect.DISCARD); + Process zsh = pb.start(); + zsh.getOutputStream().write(script.getBytes(StandardCharsets.UTF_8)); + zsh.getOutputStream().flush(); + zsh.getOutputStream().close(); + assertTrue(zsh.waitFor(60, java.util.concurrent.TimeUnit.SECONDS), + "the scrub script did not exit within 60s"); + assertEquals(0, zsh.exitValue(), + "the scrub script must still reach its end with both a fatal name and a silent " + + "no-op name among the candidates"); + + EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(tmp); + assertNotNull(report, "the report must still be written"); + assertTrue(report.blanked().contains("FLEETD_TEST_NORMAL"), + "the control case: an ordinary name is genuinely blankable and must be reported so"); + assertTrue(report.unblankable().contains("LINENO"), + "a name fatal to assign to must be reported unblankable — sanity check that " + + "containment still works under the new classification"); + assertTrue(report.unblankable().contains("SECONDS"), + "the #400 defect: eval returns rc 0 for SECONDS (zsh coerces the assignment instead " + + "of failing) but the value is left non-empty — classifying on the observed " + + "value catches this; classifying on eval's exit status would have called " + + "this \"blanked\" and produced a false receipt"); + assertFalse(report.blanked().contains("SECONDS"), + "SECONDS must never appear as blanked — it was never actually emptied"); } /**