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

scrub: stop export UID= aborting the allow-list scrub mid-loop #396

Closed
agent wants to merge 1 commits from fix/scrub-uid-abort into main
pull from: fix/scrub-uid-abort
merge into: fleet:main
Conversation 2 Commits 1 Files Changed 2
+63 -2
agent commented 2026-09-10 03:06:16 +02:00
Member
Copy Link
Copy Source

Fixes the CB-633 allow-list scrub, which has been aborting mid-loop on every member pane on fleet01 and saying nothing.

What happens

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 that terminates the whole sourced file:

$ zsh -c 'export UID='
zsh:1: failed to change user ID: operation not permitted

xtrace against a live pane's own generated ZDOTDIR ends exactly there:

+scrub.zsh:28> _cb633_n=UID
+scrub.zsh:28> export 'UID='
+zsh:1> rc=126          <- file aborted; the report block never runs

Why it matters more than the count suggests

env lists inherited names first and the names a startup file exports last. So the loop blanks the harmless inherited half and dies immediately before the operator's own exports — exactly the credentials the policy exists to remove. The selection is inverted, not merely partial.

Measured on a fleet01 member pane: UID is name 42 of 57, and a decoy exported from ~/.zshrc sits at 58, non-empty, on 8 of 8 spawns.

The missing scrub-report.txt is a consequence of the abort — the report block is lines 30-34, after the blanking loop — so its absence has been read as evidence about which shell ran, when it is really evidence that the script died.

Also fatal: EUID, GID, EGID, PPID, LINENO. Not fatal: USERNAME, SHLVL, PWD, OLDPWD, SECONDS, HISTSIZE.

The fix: contain with eval, then verify

Three shapes were tested against a live pane's own generated ZDOTDIR:

export "$n=" 2>/dev/null           ->  ABORTS
export "$n=" 2>/dev/null || true   ->  ABORTS  (assignment error, not command failure)
[[ ${(t)n} == *readonly* ]] guard  ->  ABORTS  (the type guard never fires)
case ... UID|EUID|GID|EGID|PPID|LINENO) continue  ->  works, but only for names enumerated
eval "export ${n}=" 2>/dev/null    ->  loop completes

Only eval contains it, and it is safe at that point precisely because the loop above has already
rejected every name that is not [A-Za-z_][A-Za-z0-9_]* — verified: a name X; echo INJECTED is
refused before it reaches eval.

But eval alone 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 counted only if it actually blanked:

REACHED-END  blanked=57  unblankable=1
  unblankable: UID
  decoy: []

A name that could not be blanked currently falls into the "allowed" count — imprecise in the safe
direction. The honest third count ("tried and could not blank") needs a report-format change, because
readReport treats every non-first non-blank line as a blanked name, and that belongs with #394.

Why the suite stayed green

EnvAllowListScrubTest starts zsh from pb.environment().clear(). Under a cleared parent UID is not an exported name at all, so the abort structurally cannot reproduce in that harness. Same script, two parents:

-- under env -i (test harness shape):      REACHED-END blanked=1
-- under a normal environment (pane shape): (no output — died mid-loop)

The new test supplies the production shape — UID present and exported — and asserts a report exists. The report is written by the last statement in the file, so its presence proves the script ran to completion; that assertion fails deterministically without the fix, independent of env ordering.

Mutation-verified in both shapes: reverting the fix fails exactly scrubSurvivesAnInheritedUidTheWayARealPaneHasIt and no other test in the class.

Relationship to #388

Orthogonal. #388 adds the scrub to .zshenv for panes that are neither login nor interactive. That is a fifth call site for a script that still aborts at the same name, so it does not fix a host exhibiting this. Both changes are wanted.

Test status — please read before merging

mvn test on this branch: 1470 run, 1 failure. The failure is MessageServiceTest.aFinishedTicketIsStillPrunedOnceTheTtlPassesSinceItFinished, and it is pre-existing on main: I checked out 799014e with no changes and ran that test in isolation, and it fails identically there. It is unrelated to this diff — my change touches only EnvAllowListScrub. Flagging it rather than folding a fix for it into this PR.

Independently reproduced on macOS/zsh 5.9 — see #394. The eval containment is @mac's finding; the verify-after-attempt half keeps the receipt from claiming a blank that did not happen.

Fixes the CB-633 allow-list scrub, which has been aborting mid-loop on every member pane on fleet01 and saying nothing. ## What happens `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** that terminates the whole sourced file: ``` $ zsh -c 'export UID=' zsh:1: failed to change user ID: operation not permitted ``` xtrace against a live pane's own generated ZDOTDIR ends exactly there: ``` +scrub.zsh:28> _cb633_n=UID +scrub.zsh:28> export 'UID=' +zsh:1> rc=126 <- file aborted; the report block never runs ``` ## Why it matters more than the count suggests `env` lists inherited names first and the names a startup file exports last. So the loop blanks the harmless inherited half and dies **immediately before the operator's own exports** — exactly the credentials the policy exists to remove. The selection is inverted, not merely partial. Measured on a fleet01 member pane: `UID` is name **42 of 57**, and a decoy exported from `~/.zshrc` sits at 58, non-empty, on 8 of 8 spawns. The missing `scrub-report.txt` is a *consequence* of the abort — the report block is lines 30-34, after the blanking loop — so its absence has been read as evidence about which shell ran, when it is really evidence that the script died. Also fatal: `EUID`, `GID`, `EGID`, `PPID`, `LINENO`. Not fatal: `USERNAME`, `SHLVL`, `PWD`, `OLDPWD`, `SECONDS`, `HISTSIZE`. ## The fix: contain with `eval`, then verify Three shapes were tested against a live pane's own generated ZDOTDIR: ``` export "$n=" 2>/dev/null -> ABORTS export "$n=" 2>/dev/null || true -> ABORTS (assignment error, not command failure) [[ ${(t)n} == *readonly* ]] guard -> ABORTS (the type guard never fires) case ... UID|EUID|GID|EGID|PPID|LINENO) continue -> works, but only for names enumerated eval "export ${n}=" 2>/dev/null -> loop completes ``` Only `eval` contains it, and it is safe at that point precisely because the loop above has already rejected every name that is not `[A-Za-z_][A-Za-z0-9_]*` — verified: a name `X; echo INJECTED` is refused before it reaches `eval`. But `eval` **alone** 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 counted only if it actually blanked: ``` REACHED-END blanked=57 unblankable=1 unblankable: UID decoy: [] ``` A name that could not be blanked currently falls into the "allowed" count — imprecise in the safe direction. The honest third count ("tried and could not blank") needs a report-format change, because `readReport` treats every non-first non-blank line as a blanked name, and that belongs with #394. ## Why the suite stayed green `EnvAllowListScrubTest` starts zsh from `pb.environment().clear()`. Under a cleared parent `UID` is not an exported name at all, so the abort **structurally cannot reproduce** in that harness. Same script, two parents: ``` -- under env -i (test harness shape): REACHED-END blanked=1 -- under a normal environment (pane shape): (no output — died mid-loop) ``` The new test supplies the production shape — `UID` present and exported — and asserts a report exists. The report is written by the last statement in the file, so its presence proves the script ran to completion; that assertion fails deterministically without the fix, independent of `env` ordering. **Mutation-verified in both shapes**: reverting the fix fails exactly `scrubSurvivesAnInheritedUidTheWayARealPaneHasIt` and no other test in the class. ## Relationship to #388 Orthogonal. #388 adds the scrub to `.zshenv` for panes that are neither login nor interactive. That is a fifth call site for a script that still aborts at the same name, so it does not fix a host exhibiting this. Both changes are wanted. ## Test status — please read before merging `mvn test` on this branch: **1470 run, 1 failure**. The failure is `MessageServiceTest.aFinishedTicketIsStillPrunedOnceTheTtlPassesSinceItFinished`, and it is **pre-existing on `main`**: I checked out `799014e` with no changes and ran that test in isolation, and it fails identically there. It is unrelated to this diff — my change touches only `EnvAllowListScrub`. Flagging it rather than folding a fix for it into this PR. --- *Independently reproduced on macOS/zsh 5.9 — see #394. The `eval` containment is @mac's finding; the verify-after-attempt half keeps the receipt from claiming a blank that did not happen.*
agent added 1 commit 2026-09-10 03:12:29 +02:00
scrub: stop export UID= aborting the allow-list scrub mid-loop
CI / contract (pull_request) Successful in 1m23s
Details
CI / build (pull_request) Successful in 1m32s
Details
d678783af7 
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.
agent force-pushed fix/scrub-uid-abort from 0d07a0f056 to d678783af7 2026-09-10 03:12:29 +02:00 Compare
ltms commented 2026-09-10 03:16:25 +02:00
Owner
Copy Link
Copy Source

Thank you for this, and sorry for the collision — I had a worker on #394 before your root cause landed and did not say so. That is my miss.

Decision: I am taking #397 and will close this once #397 merges. Not on ownership — on the shape of the fix. Two reasons, one of which is a correction to a measurement here.

eval does contain the abort

This PR says:

if ! export "$_cb633_n=" 2>/dev/null; then ...; fi still aborts — measured before writing this. The abort happens however the assignment is wrapped.

The first sentence is right; the generalisation is not. Measured on macOS zsh, UID in the middle of four names:

plain:  for n in AAA UID BBB CCC; do export "$n="; done
        ./probe.zsh:2: failed to change user ID: operation not permitted
        after: AAA=[] BBB=[b] CCC=[c]      <- died at UID, later names SURVIVED

eval:   for n in AAA UID BBB CCC; do eval "export ${n}=" 2>/dev/null; done
        reached-end rc=0
        after: AAA=[] BBB=[] CCC=[]        <- loop completed, every name blanked

eval reparses in a nested context, so the fatal parameter error kills the eval rather than the sourced file. A direct if ! export ... has no such boundary, which is why the test of that form failed.

The skip-list has to be complete; eval needs no list

UID EUID GID EGID PPID LINENO has to be the whole set, forever. #394 is a security control, so the version that does not depend on an enumeration being right is the one to keep. This PR's own best argument points the same way: the reason the bug was invisible for so long is that the test harness enumerated a parent environment which structurally could not contain the trigger.

What from this PR survives into the merge

The inverted-selection finding, which is the most valuable thing in either PR and which the other worker did not find:

env lists inherited names first and the names a startup file exports last. So the loop blanks the harmless inherited half and dies immediately before the operator's own exports — exactly the credentials the policy exists to remove. The selection is inverted, not merely partial.

Measured there as UID = name 42 of 57, with a ~/.zshrc decoy at 58 non-empty on 8 of 8 spawns. "Partial scrub" reads as "we got most of it"; the truth is it got precisely the wrong half. That is what makes #394 urgent rather than untidy, and it will be quoted in the merge commit with credit to fleet01.

Also carried over: the explanation of why the suite stayed green (pb.environment().clear() in EnvAllowListScrubTest), and the note that #388 is orthogonal rather than a fix for this.

Test status

The MessageServiceTest failure flagged in the PR body is real on fleet01 and is not caused by this diff — filed as #399. It is a race on the completedNanos stamp, not a regression: it passes 12 of 12 here at the same commit, and the mechanism is a happens-before hole the test never closes. Nothing to fold into this PR.

Thank you for this, and sorry for the collision — I had a worker on #394 before your root cause landed and did not say so. That is my miss. **Decision: I am taking #397 and will close this once #397 merges.** Not on ownership — on the shape of the fix. Two reasons, one of which is a correction to a measurement here. ### `eval` does contain the abort This PR says: > `if ! export "$_cb633_n=" 2>/dev/null; then ...; fi` still aborts — measured before writing this. The abort happens however the assignment is wrapped. The first sentence is right; the generalisation is not. Measured on macOS zsh, `UID` in the middle of four names: ``` plain: for n in AAA UID BBB CCC; do export "$n="; done ./probe.zsh:2: failed to change user ID: operation not permitted after: AAA=[] BBB=[b] CCC=[c] <- died at UID, later names SURVIVED eval: for n in AAA UID BBB CCC; do eval "export ${n}=" 2>/dev/null; done reached-end rc=0 after: AAA=[] BBB=[] CCC=[] <- loop completed, every name blanked ``` `eval` reparses in a nested context, so the fatal parameter error kills the eval rather than the sourced file. A direct `if ! export ...` has no such boundary, which is why the test of that form failed. ### The skip-list has to be complete; `eval` needs no list `UID EUID GID EGID PPID LINENO` has to be the whole set, forever. #394 is a security control, so the version that does not depend on an enumeration being right is the one to keep. This PR's own best argument points the same way: the reason the bug was invisible for so long is that the test harness enumerated a parent environment which structurally could not contain the trigger. ### What from this PR survives into the merge The **inverted-selection finding**, which is the most valuable thing in either PR and which the other worker did not find: > `env` lists inherited names first and the names a startup file exports last. So the loop blanks the harmless inherited half and dies immediately before the operator's own exports — exactly the credentials the policy exists to remove. The selection is inverted, not merely partial. Measured there as `UID` = name 42 of 57, with a `~/.zshrc` decoy at 58 non-empty on 8 of 8 spawns. "Partial scrub" reads as "we got most of it"; the truth is it got precisely the wrong half. That is what makes #394 urgent rather than untidy, and it will be quoted in the merge commit with credit to fleet01. Also carried over: the explanation of why the suite stayed green (`pb.environment().clear()` in `EnvAllowListScrubTest`), and the note that #388 is orthogonal rather than a fix for this. ### Test status The `MessageServiceTest` failure flagged in the PR body is real on fleet01 and is **not** caused by this diff — filed as #399. It is a race on the `completedNanos` stamp, not a regression: it passes 12 of 12 here at the same commit, and the mechanism is a happens-before hole the test never closes. Nothing to fold into this PR.
ltms closed this pull request 2026-09-10 03:32:01 +02:00
agent commented 2026-09-10 03:38:50 +02:00
Author
Member
Copy Link
Copy Source

Closing in favour of #400, at @mac's call and with my agreement.

EnvAllowListScrub.java on main has moved past what this diff is written against (#394 merged as 4887731), a worker is mid-implementation on the same reconciled predicate — eval then [[ -z "${(P)n}" ]] — and two people writing one generator is the waste this pair of tickets exists to avoid.

Nothing here is lost. What carries forward:

  • The inverted-selection finding. env lists inherited names first and a startup file's own exports last, so the abort blanked the harmless half and died at name 42 of 57 — immediately before the operator's own exports. "Partial scrub" reads as "we got most of it"; it got precisely the wrong half. That is what makes this urgent rather than untidy.
  • The verify-after-attempt predicate. eval contains the fatal parameter error without blanking the value, so a count of attempts reports a blank that never happened. Reading the value back makes the exit status irrelevant, which matters more than it looked: eval "export SECONDS=" exits 0 and changes nothing, because zsh coerces an empty assignment on an integer parameter instead of failing. Fatal, silent-no-op, and genuine success all resolve under one predicate.
  • The behaviour survey. UID EUID GID EGID PPID LINENO fatal; USERNAME SHLVL PWD OLDPWD SECONDS HISTSIZE not. The two sets are two different failure modes, and the second is the one that produces a false receipt.
  • Why the test suite was blind. EnvAllowListScrubTest starts zsh from pb.environment().clear(), so no test in the class could inherit the trigger. A replacement test has to prove the names were actually present in the parent, and say how it proved it.

The report-format half I deferred here landed with #394 already (!-prefixed names, 5-part first line), so there is no parser change left to design around.

Superseded by #400.

Closing in favour of #400, at @mac's call and with my agreement. `EnvAllowListScrub.java` on main has moved past what this diff is written against (#394 merged as `4887731`), a worker is mid-implementation on the same reconciled predicate — `eval` then `[[ -z "${(P)n}" ]]` — and two people writing one generator is the waste this pair of tickets exists to avoid. Nothing here is lost. What carries forward: - **The inverted-selection finding.** `env` lists inherited names first and a startup file's own exports last, so the abort blanked the harmless half and died at name 42 of 57 — immediately before the operator's own exports. "Partial scrub" reads as "we got most of it"; it got precisely the wrong half. That is what makes this urgent rather than untidy. - **The verify-after-attempt predicate.** `eval` contains the fatal parameter error without blanking the value, so a count of *attempts* reports a blank that never happened. Reading the value back makes the exit status irrelevant, which matters more than it looked: `eval "export SECONDS="` exits 0 and changes nothing, because zsh coerces an empty assignment on an integer parameter instead of failing. Fatal, silent-no-op, and genuine success all resolve under one predicate. - **The behaviour survey.** `UID EUID GID EGID PPID LINENO` fatal; `USERNAME SHLVL PWD OLDPWD SECONDS HISTSIZE` not. The two sets are two different failure modes, and the second is the one that produces a false receipt. - **Why the test suite was blind.** `EnvAllowListScrubTest` starts zsh from `pb.environment().clear()`, so no test in the class could inherit the trigger. A replacement test has to prove the names were actually present in the parent, and say how it proved it. The report-format half I deferred here landed with #394 already (`!`-prefixed names, 5-part first line), so there is no parser change left to design around. Superseded by #400.
Some checks are pending
CI / contract (pull_request) Successful in 1m23s
Details
CI / build (pull_request) Successful in 1m32s
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#396
Delete Branch "fix/scrub-uid-abort"

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?