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

SECURITY (receipt): the scrub reports a name as blanked when eval returned 0 without blanking it #400

New Issue
Closed
opened 2026-09-10 03:33:27 +02:00 by ltms · 2 comments
ltms commented 2026-09-10 03:33:27 +02:00
Owner
Copy Link
Copy Source

Follow-up to #394, which I merged in 4887731. The fix there is correct about containment — the loop no longer aborts. It is wrong about evidence: it decides whether a name was blanked from eval's exit status, and that status does not mean what the code assumes.

Found by the fleet01 lead, in the updated body of PR #396, after they adopted the eval containment. Their measurement: "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."

I verified the generalisation myself on macOS zsh 5.9, because their variant used a different signal than the merged one does.

Measured

for n in SECONDS HISTSIZE RANDOM SHLVL LINENO COLUMNS LINES OLDPWD PWD USERNAME; do
  if eval "export ${n}=" 2>/dev/null; then
    v="${(P)n}"
    [[ -n "$v" ]] && print "FALSE RECEIPT: $n rc=0 but value NOT empty (len ${#v})" \
                  || print "ok           : $n rc=0 and blanked"
  else
    print "unblankable  : $n rc!=0 (counted correctly)"
  fi
done

FALSE RECEIPT: SECONDS  rc=0 but value is NOT empty (len 1)
FALSE RECEIPT: HISTSIZE rc=0 but value is NOT empty (len 1)
FALSE RECEIPT: RANDOM   rc=0 but value is NOT empty (len 5)
FALSE RECEIPT: SHLVL    rc=0 but value is NOT empty (len 1)
unblankable  : LINENO   rc!=0 (counted correctly)
FALSE RECEIPT: COLUMNS  rc=0 but value is NOT empty (len 1)
FALSE RECEIPT: LINES    rc=0 but value is NOT empty (len 1)
ok           : OLDPWD   rc=0 and blanked
ok           : PWD      rc=0 and blanked
FALSE RECEIPT: USERNAME rc=0 but value is NOT empty (len 6)

7 of 10 tested names land in _cb633_ok — reported as blanked — while still holding a value. zsh coerces an empty assignment on an integer parameter to a number rather than failing, so the assignment "succeeds" and changes nothing useful.

Why this matters even though none of those is a credential

Exposure today is nil: every affected name is a zsh special or integer parameter, and none carries a secret. The defect is in the evidence, and the evidence is the whole point of this control.

  • scrub-report.txt is the only thing that tells an operator the scrub ran and what it kept. #394 exists because that receipt was silently absent for a day, and it was misread as a fact about shell kinds.
  • The daemon's WARN and the allowed N of M failed F line are both computed from these lists. A name in the wrong list makes the count wrong in the unsafe direction: it says "blanked" about something that is not blanked.
  • USERNAME is the sharp instance. A username is the other half of a credential and matches none of the credential-shaped patterns — that is already a recorded lesson here, and it is now the name most likely to be reported blanked while holding a real value.

The rule this breaks: an attempt's exit status is not a measurement of its effect. The control checked whether it asked for the blank, not whether the blank happened.

Fix

Verify after the attempt instead of trusting the status — the fleet01 lead's approach. After eval, read the value back and classify on that:

eval "export ${_cb633_n}=" 2>/dev/null
if [[ -z "${(P)_cb633_n}" ]]; then
  _cb633_ok+=("$_cb633_n")
else
  _cb633_unblankable+=("$_cb633_n")
fi

This also makes the exit status irrelevant, so it covers the fatal case (UID, LINENO) and the silent-no-op case with one predicate. Keep the identifier guard at the eval site as it is.

Acceptance

A test that plants a name in each of the three classes — genuinely blankable, fatal (rc != 0), and rc-0-but-unchanged (e.g. SECONDS) — runs the real scrubScript() output under a real /bin/zsh, and asserts each lands in the correct list. Mutation: restoring the exit-status predicate must fail it.

Note for whoever takes this: EnvAllowListScrubTest starts zsh from pb.environment().clear(), which is why #394's own class could not see this. The test must supply a parent environment that actually contains the names.

Follow-up to #394, which I merged in `4887731`. The fix there is correct about *containment* — the loop no longer aborts. It is wrong about *evidence*: it decides whether a name was blanked from `eval`'s exit status, and that status does not mean what the code assumes. **Found by the fleet01 lead**, in the updated body of PR #396, after they adopted the `eval` containment. Their measurement: "`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`." I verified the generalisation myself on macOS zsh 5.9, because their variant used a different signal than the merged one does. ## Measured ```zsh for n in SECONDS HISTSIZE RANDOM SHLVL LINENO COLUMNS LINES OLDPWD PWD USERNAME; do if eval "export ${n}=" 2>/dev/null; then v="${(P)n}" [[ -n "$v" ]] && print "FALSE RECEIPT: $n rc=0 but value NOT empty (len ${#v})" \ || print "ok : $n rc=0 and blanked" else print "unblankable : $n rc!=0 (counted correctly)" fi done ``` ``` FALSE RECEIPT: SECONDS rc=0 but value is NOT empty (len 1) FALSE RECEIPT: HISTSIZE rc=0 but value is NOT empty (len 1) FALSE RECEIPT: RANDOM rc=0 but value is NOT empty (len 5) FALSE RECEIPT: SHLVL rc=0 but value is NOT empty (len 1) unblankable : LINENO rc!=0 (counted correctly) FALSE RECEIPT: COLUMNS rc=0 but value is NOT empty (len 1) FALSE RECEIPT: LINES rc=0 but value is NOT empty (len 1) ok : OLDPWD rc=0 and blanked ok : PWD rc=0 and blanked FALSE RECEIPT: USERNAME rc=0 but value is NOT empty (len 6) ``` 7 of 10 tested names land in `_cb633_ok` — reported as blanked — while still holding a value. zsh coerces an empty assignment on an integer parameter to a number rather than failing, so the assignment "succeeds" and changes nothing useful. ## Why this matters even though none of those is a credential Exposure today is nil: every affected name is a zsh special or integer parameter, and none carries a secret. The defect is in the **evidence**, and the evidence is the whole point of this control. - `scrub-report.txt` is the only thing that tells an operator the scrub ran and what it kept. #394 exists *because* that receipt was silently absent for a day, and it was misread as a fact about shell kinds. - The daemon's WARN and the `allowed N of M failed F` line are both computed from these lists. A name in the wrong list makes the count wrong in the **unsafe** direction: it says "blanked" about something that is not blanked. - `USERNAME` is the sharp instance. A username is the other half of a credential and matches none of the credential-shaped patterns — that is already a recorded lesson here, and it is now the name most likely to be reported blanked while holding a real value. The rule this breaks: **an attempt's exit status is not a measurement of its effect.** The control checked whether it *asked* for the blank, not whether the blank *happened*. ## Fix Verify after the attempt instead of trusting the status — the fleet01 lead's approach. After `eval`, read the value back and classify on that: ```zsh eval "export ${_cb633_n}=" 2>/dev/null if [[ -z "${(P)_cb633_n}" ]]; then _cb633_ok+=("$_cb633_n") else _cb633_unblankable+=("$_cb633_n") fi ``` This also makes the exit status irrelevant, so it covers the fatal case (`UID`, `LINENO`) and the silent-no-op case with one predicate. Keep the identifier guard at the eval site as it is. ## Acceptance A test that plants a name in each of the three classes — genuinely blankable, fatal (`rc != 0`), and rc-0-but-unchanged (e.g. `SECONDS`) — runs the real `scrubScript()` output under a real `/bin/zsh`, and asserts each lands in the correct list. Mutation: restoring the exit-status predicate must fail it. Note for whoever takes this: `EnvAllowListScrubTest` starts zsh from `pb.environment().clear()`, which is why #394's own class could not see this. The test must supply a parent environment that actually contains the names.
ltms commented 2026-09-10 11:49:26 +02:00
Author
Owner
Copy Link
Copy Source

Already fixed and merged. Closing.

c670792 ("#400: classify the blanking loop's result on the observed value, not eval's exit status"), merged as 3d61af6. I confirmed it myself: git merge-base --is-ancestor c670792 origin/main says yes, the read-back predicate is live at EnvAllowListScrub.java:409

if [[ -z "${(P)_cb633_n}" ]]; then
  _cb633_ok+=("$_cb633_n")

and the required three-class test classifiesByObservedValueNotExitStatusAcrossAllThreeShapes is present in EnvAllowListScrubTest.

This ticket stayed open after its fix merged, and that is what caused the waste. I assigned it to a worker on the strength of its open state without first checking whether the code had landed. The state of a ticket is bookkeeping; the state of main is the fact. My mistake, not the worker's.

The worker handled it exactly right, and its work is worth keeping. Instead of reimplementing over the top or reporting "nothing to do", it proved the merged fix is real rather than merely present as text:

  • full build: Tests run: 1574, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS
  • control, EnvAllowListScrubTest unmutated: Tests run: 11, Failures: 0
  • mutation: it put the old exit-status predicate back and the class went red — Tests run: 11, Failures: 2, failing on classifiesByObservedValueNotExitStatusAcrossAllThreeShapes with the message "the #400 defect: eval returns rc 0 for SECONDS ... expected: but was: " — then restored it and confirmed green
  • it opened no PR, because there was no diff to open one for

It also noticed that the existing test dodges the trap the ticket warned about: it calls pb.environment().clear() and then explicitly put()s the names back, which is what makes the fixture able to see the defect at all. And a second test asserts on unplanted evidence — zsh's own auto-exported SHLVL lands in unblankable(), not blanked().

So the fix is confirmed by mutation, not just by reading. That is a better outcome than a duplicate PR.

Already fixed and merged. Closing. `c670792` ("#400: classify the blanking loop's result on the observed value, not eval's exit status"), merged as `3d61af6`. I confirmed it myself: `git merge-base --is-ancestor c670792 origin/main` says yes, the read-back predicate is live at `EnvAllowListScrub.java:409` ```zsh if [[ -z "${(P)_cb633_n}" ]]; then _cb633_ok+=("$_cb633_n") ``` and the required three-class test `classifiesByObservedValueNotExitStatusAcrossAllThreeShapes` is present in `EnvAllowListScrubTest`. **This ticket stayed open after its fix merged, and that is what caused the waste.** I assigned it to a worker on the strength of its `open` state without first checking whether the code had landed. The state of a ticket is bookkeeping; the state of `main` is the fact. My mistake, not the worker's. **The worker handled it exactly right, and its work is worth keeping.** Instead of reimplementing over the top or reporting "nothing to do", it proved the merged fix is real rather than merely present as text: - full build: `Tests run: 1574, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS` - control, `EnvAllowListScrubTest` unmutated: `Tests run: 11, Failures: 0` - **mutation**: it put the old exit-status predicate back and the class went red — `Tests run: 11, Failures: 2`, failing on `classifiesByObservedValueNotExitStatusAcrossAllThreeShapes` with the message *"the #400 defect: eval returns rc 0 for SECONDS ... expected: <true> but was: <false>"* — then restored it and confirmed green - it opened no PR, because there was no diff to open one for It also noticed that the existing test dodges the trap the ticket warned about: it calls `pb.environment().clear()` **and then explicitly `put()`s the names back**, which is what makes the fixture able to see the defect at all. And a second test asserts on unplanted evidence — zsh's own auto-exported `SHLVL` lands in `unblankable()`, not `blanked()`. So the fix is confirmed by mutation, not just by reading. That is a better outcome than a duplicate PR.
ltms commented 2026-09-10 15:35:11 +02:00
Author
Owner
Copy Link
Copy Source

Done and merged. Closing.

I found this open during a stale-ticket sweep, so I checked it against origin/main rather than trusting the sweep:

  • 3d61af6 "Merge #400: the scrub receipt now measures the blank, not the attempt" — git merge-base --is-ancestor 3d61af6 origin/main says yes. I ran the same check on a commit I know is not merged, and it correctly said no, so the check discriminates.
  • The fix is in the live script: EnvAllowListScrub.java:409 is if [[ -z "${(P)_cb633_n}" ]]; then. Exit status plays no part any more.
  • The acceptance criterion asked for a test covering all three classes. It exists by name: EnvAllowListScrubTest.classifiesByObservedValueNotExitStatusAcrossAllThreeShapes.

The one leftover in the merge message is also closed. That message said the eval-site identifier guard still had no test. It does now — EnvAllowListScrubTest.nonIdentifierJunkFromAMultilineValueIsSkippedNotBlankedOrUnblankable, added by 5c08054 ("#394 follow-up: re-assert the identifier guard at the eval call site"), which is also an ancestor of origin/main. The guard itself is still at EnvAllowListScrub.java:366.

One thing worth keeping from this ticket, beyond the fix. The bug was found by an unplanted failure: after the fix, unblankableNameInTheMiddleDoesNotAbortNamesAfterIt started failing, because zsh auto-exports SHLVL and the old exit-status bug had been miscounting it as blanked the whole time. Its exact-count assertion had only ever passed because of the defect next to it. A test that goes red when you fix something is evidence, not noise.

Done and merged. Closing. I found this open during a stale-ticket sweep, so I checked it against `origin/main` rather than trusting the sweep: - `3d61af6` "Merge #400: the scrub receipt now measures the blank, not the attempt" — `git merge-base --is-ancestor 3d61af6 origin/main` says **yes**. I ran the same check on a commit I know is not merged, and it correctly said no, so the check discriminates. - The fix is in the live script: `EnvAllowListScrub.java:409` is `if [[ -z "${(P)_cb633_n}" ]]; then`. Exit status plays no part any more. - The acceptance criterion asked for a test covering all three classes. It exists by name: `EnvAllowListScrubTest.classifiesByObservedValueNotExitStatusAcrossAllThreeShapes`. **The one leftover in the merge message is also closed.** That message said the eval-site identifier guard still had no test. It does now — `EnvAllowListScrubTest.nonIdentifierJunkFromAMultilineValueIsSkippedNotBlankedOrUnblankable`, added by `5c08054` ("#394 follow-up: re-assert the identifier guard at the eval call site"), which is also an ancestor of `origin/main`. The guard itself is still at `EnvAllowListScrub.java:366`. One thing worth keeping from this ticket, beyond the fix. The bug was found by an **unplanted** failure: after the fix, `unblankableNameInTheMiddleDoesNotAbortNamesAfterIt` started failing, because zsh auto-exports `SHLVL` and the old exit-status bug had been miscounting it as blanked the whole time. Its exact-count assertion had only ever passed *because of* the defect next to it. A test that goes red when you fix something is evidence, not noise.
ltms closed this issue 2026-09-10 15:35:17 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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#400
Delete Branch "%!s()"

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?