fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

#394: contain a fatal export error instead of letting it abort the scrub #397

Closed
agent wants to merge 0 commits from worker/scrub-abort-394-316b3e-5 into main
pull from: worker/scrub-abort-394-316b3e-5
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-10 03:09:43 +02:00
Member
Copy Link
Copy Source

Fixes #394.

The bug

EnvAllowListScrub.scrubScript()'s blanking loop used a plain export "$n=" on every name not on the allow-list. For a zsh read-only/special parameter (e.g. UID) that is a FATAL parameter error, and since the loop runs inside a sourced startup file, the error aborts the whole file: every name still to come is never blanked, and scrub-report.txt is never written at all -- silently, because 2>/dev/null on the group swallows it. On one live host UID was name 42 of 57, so 15 names were never considered and a planted decoy survived into the member.

The fix

Route each blanking attempt through eval instead of a bare export, which contains the error to that single iteration -- the loop always finishes. A name that could not be blanked is now counted separately (failed on the report's first line, e.g. allowed 54 of 57 failed 1) and listed !-prefixed in the report body, rather than disappearing. No skip-list of known-bad names was added -- every enumerated name is still attempted unconditionally, so a name nobody has thought of yet is still tried and, if it fails, still counted (invariant 1 in the ticket).

EnvAllowListScrub.ScrubReport gained a failed count and an unblankable name list alongside the existing allowed/total/blanked. readReport parses the new allowed N of M failed F first line.

HerdrPeerLauncher: logs a WARN when a pane's report carries a nonzero failed count (naming the unblankable variables), and the "no report at all" WARN was reworded so it no longer claims the daemon knows the member "saw the full host environment" -- a partial scrub and a missing scrub are different situations, and only the first is now distinguishable from the report alone (invariants 2 and 3).

Verification

  • New test EnvAllowListScrubTest#unblankableNameInTheMiddleDoesNotAbortNamesAfterIt: plants a made-up, exported, read-only variable (typeset -rx, not UID or any other name a skip-list might know) in the MIDDLE of the names the real scrubScript() attempts to blank (two more names planted after it), runs the actual generated script under a real /bin/zsh, and asserts both later names are still blanked, the report is still written, and the failure is counted by name.
  • Confirmed the test fails without the fix: reverted the blanking loop to the original bare-export version, ran the test, and got expected: <0> but was: <1> on the script's own exit code (the script aborted) -- exactly the bug. Restored the fix afterward; full suite green again.
  • mvn clean install in fleetd/, unpiped: Tests run: 1470, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Out of scope (reported, not fixed)

Same shape elsewhere in this class and its neighbours -- one failing element can abort or hide the rest:

  • EnvAllowListScrub.shareWithGroup's per-file loop: any IOException/UnsupportedOperationException on one file's setGroupAndPermissions (other than the REPORT_FILE branch, which already catches locally) propagates out and skips setting permissions on every file still to come in the Files.list iteration.
  • The .zshenv/.zprofile/.zshrc/.zlogin generated files all do [ -r "$HOME/name" ] && source "$HOME/name" with no error redirection -- a syntax error partway through an operator's own real dotfile would abort that file the same way, before the scrub source line is even reached (this is upstream of the scrub, not inside it, so not clearly the same bug, but the same failure shape).
  • OpenCodeLauncher reuses shareWithGroup verbatim (per its own javadoc), so it inherits the same per-file-loop gap.
Fixes #394. ## The bug `EnvAllowListScrub.scrubScript()`'s blanking loop used a plain `export "$n="` on every name not on the allow-list. For a zsh read-only/special parameter (e.g. `UID`) that is a FATAL parameter error, and since the loop runs inside a sourced startup file, the error aborts the whole file: every name still to come is never blanked, and `scrub-report.txt` is never written at all -- silently, because `2>/dev/null` on the group swallows it. On one live host `UID` was name 42 of 57, so 15 names were never considered and a planted decoy survived into the member. ## The fix Route each blanking attempt through `eval` instead of a bare `export`, which contains the error to that single iteration -- the loop always finishes. A name that could not be blanked is now counted separately (`failed` on the report's first line, e.g. `allowed 54 of 57 failed 1`) and listed `!`-prefixed in the report body, rather than disappearing. **No skip-list of known-bad names was added** -- every enumerated name is still attempted unconditionally, so a name nobody has thought of yet is still tried and, if it fails, still counted (invariant 1 in the ticket). `EnvAllowListScrub.ScrubReport` gained a `failed` count and an `unblankable` name list alongside the existing `allowed`/`total`/`blanked`. `readReport` parses the new `allowed N of M failed F` first line. `HerdrPeerLauncher`: logs a WARN when a pane's report carries a nonzero `failed` count (naming the unblankable variables), and the "no report at all" WARN was reworded so it no longer claims the daemon knows the member "saw the full host environment" -- a partial scrub and a missing scrub are different situations, and only the first is now distinguishable from the report alone (invariants 2 and 3). ## Verification - New test `EnvAllowListScrubTest#unblankableNameInTheMiddleDoesNotAbortNamesAfterIt`: plants a made-up, exported, read-only variable (`typeset -rx`, not `UID` or any other name a skip-list might know) in the MIDDLE of the names the real `scrubScript()` attempts to blank (two more names planted after it), runs the actual generated script under a real `/bin/zsh`, and asserts both later names are still blanked, the report is still written, and the failure is counted by name. - Confirmed the test fails without the fix: reverted the blanking loop to the original bare-`export` version, ran the test, and got `expected: <0> but was: <1>` on the script's own exit code (the script aborted) -- exactly the bug. Restored the fix afterward; full suite green again. - `mvn clean install` in `fleetd/`, unpiped: `Tests run: 1470, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`. ## Out of scope (reported, not fixed) Same shape elsewhere in this class and its neighbours -- one failing element can abort or hide the rest: - `EnvAllowListScrub.shareWithGroup`'s per-file loop: any `IOException`/`UnsupportedOperationException` on one file's `setGroupAndPermissions` (other than the REPORT_FILE branch, which already catches locally) propagates out and skips setting permissions on every file still to come in the `Files.list` iteration. - The `.zshenv`/`.zprofile`/`.zshrc`/`.zlogin` generated files all do `[ -r "$HOME/name" ] && source "$HOME/name"` with no error redirection -- a syntax error partway through an operator's own real dotfile would abort that file the same way, before the scrub source line is even reached (this is upstream of the scrub, not inside it, so not clearly the same bug, but the same failure shape). - `OpenCodeLauncher` reuses `shareWithGroup` verbatim (per its own javadoc), so it inherits the same per-file-loop gap.
agent added 1 commit 2026-09-10 03:09:43 +02:00
#394: contain a fatal export error instead of letting it abort the scrub
CI / contract (pull_request) Successful in 1m23s
Details
CI / build (pull_request) Successful in 1m52s
Details
e3e403e5c8 
EnvAllowListScrub's blanking loop used a plain `export "$n="` on every
name not on the allow-list. For a zsh read-only/special parameter (e.g.
UID) that is a FATAL parameter error, and since the loop runs inside the
sourced startup file, the error aborts the whole file: every name still
to come is never blanked, and scrub-report.txt is never written at all
-- silently, because 2>/dev/null on the group swallows it.

Route each blanking attempt through `eval` instead, which contains the
error to that one iteration. The loop always finishes; a name it could
not blank is now counted separately ("failed" on the report's first
line) and listed !-prefixed rather than disappearing. No skip-list of
known-bad names is added -- every enumerated name is still attempted,
so a name nobody has thought of is still tried and, if it fails, still
counted.

HerdrPeerLauncher: log a WARN when a pane's report carries a nonzero
failed count, and reword the "no report at all" WARN so it no longer
claims the daemon knows the member "saw the full host environment" --
a partial vs. a missing scrub are different situations and only the
first is now distinguishable from the report alone.
agent added 1 commit 2026-09-10 03:24:54 +02:00
#394 follow-up: re-assert the identifier guard at the eval call site
CI / contract (pull_request) Successful in 1m29s
Details
CI / build (pull_request) Successful in 1m48s
Details
5c08054533 
EnvAllowListScrub's blanking loop splices each name into a string
handed to eval ("export ${n}="). That is only safe because every name
reaching _cb633_blank already passed an identifier check in the
enumeration loop -- 20 lines away, in a different loop. Before eval
was introduced a non-conforming name reaching plain `export "$n="`
was inert either way (the quoting neutralized it); eval removed that
safety net, so the enumeration loop's guard became the ONLY thing
standing between a non-identifier string and code execution in the
member's pane, with nothing at the eval site itself defending that
property.

Re-assert the same [A-Za-z_][A-Za-z0-9_]* check immediately before
the eval call, independent of the enumeration loop's own guard (left
untouched, not moved). A name that fails it is counted unblankable
rather than silently dropped, so a bypass of the upstream guard would
leave real evidence in the report.

New test exploits the "junk from multi-line values" gap the
enumeration loop's own comment already documents: a value with an
embedded newline makes `command env`'s text output split into a
spurious extra "name" line that was never a real variable. Runs the
real generated scrubScript() end-to-end under zsh and asserts the
non-conforming fragment is neither blanked nor counted unblankable.
The fragment used is merely non-conforming (contains a dot) --
never command-shaped.

Mutation-verified both guards. Weakening the enumeration guard alone
DOES break the new test (the fragment then reaches the new eval-site
guard and gets counted unblankable, failing the "not unblankable"
assertion). Removing the new eval-site guard alone, with the
enumeration guard intact, does NOT break it: _cb633_blank has exactly
one producer (the enumeration loop), so nothing can reach the eval
site without already having passed the identical check there. That is
expected given the single-source architecture, and it is exactly why
the eval-site guard is defense-in-depth against a future change that
adds a second path into _cb633_blank or decouples the two loops --
not a currently independently-observable divergence.
ltms closed this pull request 2026-09-10 07:16:26 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m29s
Details
CI / build (pull_request) Successful in 1m48s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#397
Delete Branch "worker/scrub-abort-394-316b3e-5"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?