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

One flag guards two different memberCredentials WARNs, so a later leaking credential name is never reported #341

New Issue
Closed
opened 2026-09-04 10:51:10 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 10:51:10 +02:00
Owner
Copy Link
Copy Source

Found by a hunt over member/, proven by the hunter with a throwaway test. I have re-read all three
sites myself and confirmed the shape.

This is a visibility defect on a credential control. The leak it hides is not new — the WARN
that would tell the operator about it is what goes missing.

What happens

HerdrPeerLauncher has two one-shot flags, and the javadoc says exactly why:

// HerdrPeerLauncher.java:1671-1677
// CB-633 follow-up (#192): kept SEPARATE from allowListGapLogged on purpose.
// memberCredentials is a live, re-read-per-spawn supplier, so the policy can change
// between two spawns on the same launcher. A single shared flag would let a harmless
// allow-list INFO on spawn 1 permanently suppress the real deny-by-default WARN a later
// spawn deserves — the report that matters most getting hidden by the report that doesn't.

That reasoning is right. It was applied to INFO vs WARN and not to WARN vs WARN.

unprotectedGapLogged guards two different WARN branches:

  • HerdrPeerLauncher.java:1819 — the allow-list branch: names are on neither known: nor allow:,
    but the derived allow-list keeps them, "so every member pane inherits them UNBLOCKED".
  • HerdrPeerLauncher.java:1839 (warnGapUnprotected) — the deny-by-default and allow-list-non-zsh
    fallback WARN.

Both do unprotectedGapLogged.compareAndSet(false, true). They report different env var names.

The path in

memberCredentials is a live, re-read-per-spawn supplier, so the policy can change under one
launcher instance with no restart:

  1. Spawn 1 under deny-by-default with an uncovered credential-shaped name — say OLD_LEAK_TOKEN.
    warnGapUnprotected fires and sets the flag.
  2. The operator reloads config, switching to allow-list.
  3. Spawn 2 hits the keptByDerivedList branch with a different name — for example a profile's
    own tokenEnv, which the derived allow-list keeps even though it is on neither known: nor
    allow:.
  4. compareAndSet fails. No WARN. The operator is never told that a second, different variable
    is inherited unblocked by every member pane.

Confirmed

The hunter built one launcher with a mutable memberCredentials supplier and a mutable host-env
supplier, captured logs with a logback ListAppender, and ran the two spawns above. The first WARN
fired naming OLD_LEAK_TOKEN and UNBLOCKED; the second WARN, for FLEETD_WORKER_TOKEN, never
appeared. Its assertion failed as expected. The test was deleted afterwards — I checked the
worktree and it is clean, so that claim holds.

I re-read HerdrPeerLauncher.java:1668-1685, :1812-1830 and :1834-1846 myself and the shared
flag is exactly as described.

Direction of harm

Strictly the wrong direction. The operator sees one WARN, names the variable it lists, fixes that
one, and reasonably believes the gap is closed. A later config change widens the actually-unprotected
set to a different name and the log stays silent.

The whole reason these WARNs name specific variables — rather than saying "a gap exists" — is so an
operator can act on them. Suppressing a WARN that carries a different name, because an earlier
WARN fired for an unrelated one, defeats that. This class's own doc elsewhere says logging this kind
of thing at debug "is how a control that silently stopped working stays unnoticed"; not logging it
at all is the same failure, one step further.

It is also a one-way gate: once the flag trips it never resets for the life of the launcher,
whatever the policy or the names.

Goal and invariants

Goal: every distinct credential-shaped name that ends up unprotected gets reported once, whatever
was reported before it.

Invariants:

  1. Keep the noise control. The reason for a one-shot guard is real — this must not become one
    WARN per spawn for the same names. Once per distinct name, not once per spawn.
  2. allowListGapLogged stays a separate flag. The #192 reasoning that split it is correct; do
    not merge the two while fixing this.
  3. Do not change the wording of either WARN. They are read by operators and one is marked
    "unchanged byte-for-byte by #192" — if you believe a message needs to change, say so and leave it
    to me.

Candidate mechanism, as a candidate only: replace the AtomicBoolean with a set of
already-warned names, so the guard is per name rather than per launcher. OpenCodeLauncher already
uses a Set<String> guard (modelCheckSkippedWarned) one file over, for the same
once-per-distinct-thing reason — follow the existing pattern rather than inventing one. Decide it
yourself and justify it.

Say what bounds the set. An unbounded set keyed on names from the host environment is a slow
leak of its own. Env var names are few and come from a bounded source, so this is probably fine —
but say so explicitly rather than leaving it unexamined.

Why the tests miss it

Every existing test of this WARN (HerdrPeerLauncherAllowListWiringTest) spawns once per launcher
instance, so the flag is only ever seen in its initial false state. The suppression only exists
across spawns, and nothing spans two.

That is the "a test on the seam does not prove the caller" shape: the guard is tested, the
sequence is not.

Found by a hunt over `member/`, proven by the hunter with a throwaway test. I have re-read all three sites myself and confirmed the shape. This is a visibility defect on a **credential** control. The leak it hides is not new — the WARN that would tell the operator about it is what goes missing. ## What happens `HerdrPeerLauncher` has two one-shot flags, and the javadoc says exactly why: ```java // HerdrPeerLauncher.java:1671-1677 // CB-633 follow-up (#192): kept SEPARATE from allowListGapLogged on purpose. // memberCredentials is a live, re-read-per-spawn supplier, so the policy can change // between two spawns on the same launcher. A single shared flag would let a harmless // allow-list INFO on spawn 1 permanently suppress the real deny-by-default WARN a later // spawn deserves — the report that matters most getting hidden by the report that doesn't. ``` That reasoning is right. It was applied to **INFO vs WARN** and not to **WARN vs WARN**. `unprotectedGapLogged` guards *two different* WARN branches: - `HerdrPeerLauncher.java:1819` — the allow-list branch: names are on neither `known:` nor `allow:`, but the derived allow-list keeps them, "so every member pane inherits them UNBLOCKED". - `HerdrPeerLauncher.java:1839` (`warnGapUnprotected`) — the deny-by-default and allow-list-non-zsh fallback WARN. Both do `unprotectedGapLogged.compareAndSet(false, true)`. **They report different env var names.** ## The path in `memberCredentials` is a live, re-read-per-spawn supplier, so the policy can change under one launcher instance with no restart: 1. Spawn 1 under `deny-by-default` with an uncovered credential-shaped name — say `OLD_LEAK_TOKEN`. `warnGapUnprotected` fires and sets the flag. 2. The operator reloads config, switching to `allow-list`. 3. Spawn 2 hits the `keptByDerivedList` branch with a **different** name — for example a profile's own `tokenEnv`, which the derived allow-list keeps even though it is on neither `known:` nor `allow:`. 4. `compareAndSet` fails. **No WARN.** The operator is never told that a second, different variable is inherited unblocked by every member pane. ## Confirmed The hunter built one launcher with a mutable `memberCredentials` supplier and a mutable host-env supplier, captured logs with a logback `ListAppender`, and ran the two spawns above. The first WARN fired naming `OLD_LEAK_TOKEN` and `UNBLOCKED`; the second WARN, for `FLEETD_WORKER_TOKEN`, never appeared. Its assertion failed as expected. The test was deleted afterwards — I checked the worktree and it is clean, so that claim holds. I re-read `HerdrPeerLauncher.java:1668-1685`, `:1812-1830` and `:1834-1846` myself and the shared flag is exactly as described. ## Direction of harm Strictly the wrong direction. The operator sees one WARN, names the variable it lists, fixes that one, and reasonably believes the gap is closed. A later config change widens the actually-unprotected set to a different name and the log stays silent. The whole reason these WARNs name specific variables — rather than saying "a gap exists" — is so an operator can act on them. Suppressing a WARN that carries a **different** name, because an earlier WARN fired for an unrelated one, defeats that. This class's own doc elsewhere says logging this kind of thing at debug "is how a control that silently stopped working stays unnoticed"; not logging it at all is the same failure, one step further. It is also a one-way gate: once the flag trips it never resets for the life of the launcher, whatever the policy or the names. ## Goal and invariants **Goal:** every distinct credential-shaped name that ends up unprotected gets reported once, whatever was reported before it. **Invariants:** 1. **Keep the noise control.** The reason for a one-shot guard is real — this must not become one WARN per spawn for the same names. Once per distinct name, not once per spawn. 2. **`allowListGapLogged` stays a separate flag.** The #192 reasoning that split it is correct; do not merge the two while fixing this. 3. Do not change the wording of either WARN. They are read by operators and one is marked "unchanged byte-for-byte by #192" — if you believe a message needs to change, say so and leave it to me. **Candidate mechanism, as a candidate only:** replace the `AtomicBoolean` with a set of already-warned names, so the guard is per name rather than per launcher. `OpenCodeLauncher` already uses a `Set<String>` guard (`modelCheckSkippedWarned`) one file over, for the same once-per-distinct-thing reason — follow the existing pattern rather than inventing one. Decide it yourself and justify it. **Say what bounds the set.** An unbounded set keyed on names from the host environment is a slow leak of its own. Env var names are few and come from a bounded source, so this is probably fine — but say so explicitly rather than leaving it unexamined. ## Why the tests miss it Every existing test of this WARN (`HerdrPeerLauncherAllowListWiringTest`) spawns once per launcher instance, so the flag is only ever seen in its initial `false` state. The suppression only exists across spawns, and nothing spans two. That is the "a test on the seam does not prove the caller" shape: the guard is tested, the *sequence* is not.
ltms commented 2026-09-04 11:14:30 +02:00
Author
Owner
Copy Link
Copy Source

Merged to main as b32a30f (--no-ff; the branch was behind main). Follow-up commit d057d56
adds two tests — see below.

Build: Tests run: 1362, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS, unpiped.

The caveat you raised — answered by command, not by opinion

You asked me to double-check that both WARN format strings are byte-identical, since only the
argument source was meant to change. I extracted every log.warn string literal from
HerdrPeerLauncher before and after the merge and compared them:

warn call count before/after: 19 19
format strings byte-identical: True

All 19, unchanged. Invariant 3 holds. Raising that rather than asserting it was the right call.

What the fix got right

It reproduced the defect first, against unfixed code, and the reproduction failed exactly as
predicted. It used the Set<String> shape OpenCodeLauncher.modelCheckSkippedWarned already uses
rather than inventing one, left allowListGapLogged alone, and answered both questions I asked in
the PR body instead of leaving them implicit in the code.

The shape survey came back with a real answer — 8 other one-shot guards checked, each gating exactly
one call site, and inject/ has no boolean log guards at all. A negative result stated plainly is
worth as much as a finding.

My own mutation found the other half unpinned

The implementer proved its fix by reverting the whole file. I mutated the guard's other duty
instead.

Mutation X — replace .filter(unprotectedGapNamesWarned::add) with a filter that adds and
always returns true, so every name is logged on every spawn:

Tests run: 1358, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

Nothing failed. Invariant 1 — the noise control, "once per distinct name, not once per spawn" — was
implemented correctly and nothing held it there. The Set behaved; nothing proved this class used
it as a guard rather than as a record.

Two tests added (d057d56)

  1. theSameUnprotectedNameIsWarnedAboutOnlyOnceAcrossSpawns — pins invariant 1. It now fails on
    Mutation X, printing both duplicate WARNs: 
    one unchanged unprotected name across two spawns must produce exactly one WARN — the set is a
guard, not just a record ... expected: <1> but was: <2>
  2. anAllowListWarnDoesNotSuppressALaterDenyByDefaultWarnForADifferentName — the reverse policy
    order    . The defect was found going deny-by-default → allow-list, and a guard fixed in one
    direction is not automatically fixed in the other. Ask which states still open the gate.

Neither needed a production seam, which is why I added them here rather than filing them.

Closing. One fix in, two coverage gaps closed on merge.

Merged to `main` as `b32a30f` (`--no-ff`; the branch was behind main). Follow-up commit `d057d56` adds two tests — see below. Build: `Tests run: 1362, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS`, unpiped. ## The caveat you raised — answered by command, not by opinion You asked me to double-check that both WARN format strings are byte-identical, since only the argument source was meant to change. I extracted every `log.warn` string literal from `HerdrPeerLauncher` before and after the merge and compared them: ``` warn call count before/after: 19 19 format strings byte-identical: True ``` All 19, unchanged. Invariant 3 holds. Raising that rather than asserting it was the right call. ## What the fix got right It reproduced the defect first, against unfixed code, and the reproduction failed exactly as predicted. It used the `Set<String>` shape `OpenCodeLauncher.modelCheckSkippedWarned` already uses rather than inventing one, left `allowListGapLogged` alone, and answered both questions I asked in the PR body instead of leaving them implicit in the code. The shape survey came back with a real answer — 8 other one-shot guards checked, each gating exactly one call site, and `inject/` has no boolean log guards at all. A negative result stated plainly is worth as much as a finding. ## My own mutation found the other half unpinned The implementer proved its fix by reverting the whole file. I mutated the guard's *other* duty instead. **Mutation X** — replace `.filter(unprotectedGapNamesWarned::add)` with a filter that adds and always returns `true`, so every name is logged on every spawn: ``` Tests run: 1358, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` Nothing failed. Invariant 1 — the noise control, "once per distinct name, not once per spawn" — was implemented correctly and nothing held it there. The `Set` behaved; nothing proved this class used it as a *guard* rather than as a *record*. ## Two tests added (`d057d56`) 1. `theSameUnprotectedNameIsWarnedAboutOnlyOnceAcrossSpawns` — pins invariant 1. It now fails on Mutation X, printing both duplicate WARNs: ``` one unchanged unprotected name across two spawns must produce exactly one WARN — the set is a guard, not just a record ... expected: <1> but was: <2> ``` 2. `anAllowListWarnDoesNotSuppressALaterDenyByDefaultWarnForADifferentName` — the **reverse policy order**. The defect was found going deny-by-default → allow-list, and a guard fixed in one direction is not automatically fixed in the other. Ask which states still open the gate. Neither needed a production seam, which is why I added them here rather than filing them. Closing. One fix in, two coverage gaps closed on merge.
ltms closed this issue 2026-09-04 11:14:41 +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#341
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?