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

CB-606: auth.mode and two placement fields are accepted unvalidated — a typo in auth.mode silently disables authentication #106

New Issue
Closed
opened 2026-08-16 18:40:17 +02:00 by ltms · 1 comment
ltms commented 2026-08-16 18:40:17 +02:00
Owner
Copy Link
Copy Source

Found by the CB-604 worker when I asked it to check whether any other config field had the same shape. It did not fix them — correctly, since that was a separate question. Two more fields are lower-cased in a compact constructor, then compared against exactly one string, with no membership check.

1. auth.mode — the dangerous one

BridgedConfig.java:901 lower-cases mode; tokenMode() at :906-908 only checks MODE_TOKEN.equals(mode).

So a typo of token — toekn, Token with a trailing space, anything — silently behaves as loopback-trust. No error, no warning.

What makes this worse than an ordinary typo: validateAuthExposure() only fires when the bind is non-loopback. On a loopback bind it never runs, so the typo is masked completely. An operator who set auth.mode: token on purpose, and typed it wrong, gets a daemon that starts cleanly and is running with no authentication — believing it is authenticated. There is no signal anywhere.

That is the exact inverse of what the bind fail-fast was written to guarantee. That check exists because the codebase decided this class of mistake should be unrepresentable rather than documented; a typo walks straight around it.

2. Profile.placement — the per-profile tab/pane field

BridgedConfig.java:293 lower-cases it; tabPlacement() at :467-469 only checks "tab".equals(placement).

An unrecognized value such as placement: tba silently falls back to legacy pane placement. Lower severity — the fleet still works, it just lays out differently from what was asked — but it is the same silent-acceptance shape.

Do not confuse this with the top-level placement: policy field (fixed/round-robin/weighted). That is a different field and it is validated: PlacementPolicies.fromName (PlacementPolicies.java:15-28) throws on an unrecognized name. But note it throws lazily, at first spawn, via placementPolicy.get() in CompositePeerLauncher — not at config load like every other check here. So a bad policy name also starts a daemon that looks healthy and fails later. Worth pulling forward to load time while you are in here.

Why this is one ticket and not three

Three fields, one defect: a config value is accepted, does nothing the operator asked for, and reports no error. CB-604 fixed the first instance and established the pattern — rejectUnknownKind, a raw-YAML scan that throws at load naming the field, the bad value, and the accepted set. This ticket applies that same pattern to the rest.

Acceptance criteria

  1. An unrecognized auth.mode fails at config load, naming the value and the accepted set. This one matters most — say explicitly in your report what happens today with auth.mode: toekn on a loopback bind, before and after.
  2. An unrecognized per-profile placement: fails at config load the same way.
  3. The top-level placement: policy name is validated at config load, not lazily at first spawn.
  4. Every valid value keeps working, case-insensitively, and every absent value keeps its current default. Existing tests covering those must pass unmodified.
  5. A test per field for the rejection, and a test per field for the default.
  6. Sweep for a fourth. Search for the shape — toLowerCase() in a compact constructor followed by a single .equals comparison — and report anything else you find, without fixing it.
  7. mvn -f bridged/pom.xml clean install green, run unpiped.

Follow the existing pattern

BridgedConfig.rejectUnknownKind (CB-604) is the model, alongside rejectNegativeMaxLoad and rejectDuplicateMemberSlots. Throw; do not warn. A warning in a startup log nobody reads is how this defect survives.

Found by the CB-604 worker when I asked it to check whether any other config field had the same shape. It did not fix them — correctly, since that was a separate question. Two more fields are **lower-cased in a compact constructor, then compared against exactly one string, with no membership check.** ## 1. `auth.mode` — the dangerous one `BridgedConfig.java:901` lower-cases `mode`; `tokenMode()` at `:906-908` only checks `MODE_TOKEN.equals(mode)`. So a typo of `token` — `toekn`, `Token ` with a trailing space, anything — silently behaves as **`loopback-trust`**. No error, no warning. What makes this worse than an ordinary typo: `validateAuthExposure()` only fires when the bind is non-loopback. On a loopback bind it never runs, so the typo is masked completely. An operator who set `auth.mode: token` on purpose, and typed it wrong, gets a daemon that starts cleanly and is running with **no authentication** — believing it is authenticated. There is no signal anywhere. That is the exact inverse of what the bind fail-fast was written to guarantee. That check exists because the codebase decided this class of mistake should be unrepresentable rather than documented; a typo walks straight around it. ## 2. `Profile.placement` — the per-profile `tab`/`pane` field `BridgedConfig.java:293` lower-cases it; `tabPlacement()` at `:467-469` only checks `"tab".equals(placement)`. An unrecognized value such as `placement: tba` silently falls back to legacy pane placement. Lower severity — the fleet still works, it just lays out differently from what was asked — but it is the same silent-acceptance shape. **Do not confuse this with the top-level `placement:` policy field** (`fixed`/`round-robin`/`weighted`). That is a *different* field and it **is** validated: `PlacementPolicies.fromName` (`PlacementPolicies.java:15-28`) throws on an unrecognized name. But note it throws **lazily, at first spawn**, via `placementPolicy.get()` in `CompositePeerLauncher` — not at config load like every other check here. So a bad policy name also starts a daemon that looks healthy and fails later. Worth pulling forward to load time while you are in here. ## Why this is one ticket and not three Three fields, one defect: **a config value is accepted, does nothing the operator asked for, and reports no error.** CB-604 fixed the first instance and established the pattern — `rejectUnknownKind`, a raw-YAML scan that throws at load naming the field, the bad value, and the accepted set. This ticket applies that same pattern to the rest. ## Acceptance criteria 1. An unrecognized `auth.mode` fails at config load, naming the value and the accepted set. **This one matters most** — say explicitly in your report what happens today with `auth.mode: toekn` on a loopback bind, before and after. 2. An unrecognized per-profile `placement:` fails at config load the same way. 3. The top-level `placement:` policy name is validated at **config load**, not lazily at first spawn. 4. Every valid value keeps working, case-insensitively, and every absent value keeps its current default. Existing tests covering those must pass **unmodified**. 5. A test per field for the rejection, and a test per field for the default. 6. **Sweep for a fourth.** Search for the shape — `toLowerCase()` in a compact constructor followed by a single `.equals` comparison — and report anything else you find, without fixing it. 7. `mvn -f bridged/pom.xml clean install` green, run unpiped. ## Follow the existing pattern `BridgedConfig.rejectUnknownKind` (CB-604) is the model, alongside `rejectNegativeMaxLoad` and `rejectDuplicateMemberSlots`. Throw; do not warn. A warning in a startup log nobody reads is how this defect survives.
ltms added this to the 1.1 — single-host close-out milestone 2026-08-16 18:40:17 +02:00
ltms closed this issue 2026-08-16 18:58:52 +02:00
ltms commented 2026-08-16 18:59:15 +02:00
Author
Owner
Copy Link
Copy Source

Merged into main as aa4ee64. Verified independently, not taken on the report.

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

Probed the real BridgedConfig.load, eleven values

auth.mode=toekn  (loopback bind) -> REFUSED
auth.mode=token  (loopback bind) -> ACCEPTED, tokenMode=true
auth.mode=TOKEN  (case)          -> ACCEPTED, tokenMode=true
auth.mode=loopback-trust         -> ACCEPTED, tokenMode=false
auth absent (default)            -> ACCEPTED, tokenMode=false
profile placement=tabb           -> REFUSED
profile placement=TAB (case)     -> ACCEPTED
profile placement absent         -> ACCEPTED
policy placement=weightd         -> REFUSED
policy placement=weighted        -> ACCEPTED, policy=weighted
policy placement absent          -> ACCEPTED, policy=fixed

The first line is the one that mattered — a loopback bind, which is where the old code hid the mistake, and it is refused:

refusing to start: auth.mode=toekn is not recognized — accepted values are loopback-trust, token (case-insensitive); an unrecognized mode would otherwise silently fall back to loopback-trust, which authenticates nobody.

The policy check throws out of load(), so criterion 3 holds: it is at config load, not at first spawn.

The check the worker could not run

bridged.yaml is gitignored, so a member cannot see the live config — and a stricter validator is exactly the change that can refuse to start a daemon that was starting fine yesterday. I loaded the real file with the new jar:

LIVE CONFIG ACCEPTED  policy=weighted  tokenMode=false
profiles=[local, local-direct, gx, opus, sonnet, sol, terra]

I also checked that Set.of("tab", "pane") is not a narrowing of documented behaviour. bridged.example.yaml:125-126 documents exactly those two — "Use pane for the legacy behaviour" — so no spelling that was ever documented is now refused.

One expectation of mine that was wrong

I expected the per-profile placement: default to be pane, since tabPlacement() returns "tab".equals(placement). It is tab — Profile's compact constructor defaults it (BridgedConfig.java:293). The worker's test name absentProfilePlacementDefaultsToTab is correct and my reading was not. Checked before raising it.

Criterion 6 — no fourth instance

It swept every toLowerCase() in bridged/src/main/java, not only config/, and classified what it found: enum-to-wire rendering, AgentStatus.fromWire (has an explicit default -> UNKNOWN, and UNKNOWN is not injectable, so it fails safe), pane-text heuristics in StatusRefiner and CompletionResolver, isLoopbackBind, and slug generation. None matches the defect shape. That reads right to me.

Closing.

Merged into `main` as `aa4ee64`. Verified independently, not taken on the report. **Build**, unpiped: `Tests run: 853, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. ## Probed the real `BridgedConfig.load`, eleven values ``` auth.mode=toekn (loopback bind) -> REFUSED auth.mode=token (loopback bind) -> ACCEPTED, tokenMode=true auth.mode=TOKEN (case) -> ACCEPTED, tokenMode=true auth.mode=loopback-trust -> ACCEPTED, tokenMode=false auth absent (default) -> ACCEPTED, tokenMode=false profile placement=tabb -> REFUSED profile placement=TAB (case) -> ACCEPTED profile placement absent -> ACCEPTED policy placement=weightd -> REFUSED policy placement=weighted -> ACCEPTED, policy=weighted policy placement absent -> ACCEPTED, policy=fixed ``` The first line is the one that mattered — **a loopback bind, which is where the old code hid the mistake**, and it is refused: > refusing to start: auth.mode=toekn is not recognized — accepted values are loopback-trust, token (case-insensitive); an unrecognized mode would otherwise silently fall back to loopback-trust, which authenticates nobody. The policy check throws out of `load()`, so criterion 3 holds: it is at config load, not at first spawn. ## The check the worker could not run `bridged.yaml` is gitignored, so a member cannot see the live config — and a stricter validator is exactly the change that can refuse to start a daemon that was starting fine yesterday. I loaded the real file with the new jar: ``` LIVE CONFIG ACCEPTED policy=weighted tokenMode=false profiles=[local, local-direct, gx, opus, sonnet, sol, terra] ``` I also checked that `Set.of("tab", "pane")` is not a narrowing of documented behaviour. `bridged.example.yaml:125-126` documents exactly those two — *"Use `pane` for the legacy behaviour"* — so no spelling that was ever documented is now refused. ## One expectation of mine that was wrong I expected the per-profile `placement:` default to be `pane`, since `tabPlacement()` returns `"tab".equals(placement)`. It is **`tab`** — `Profile`'s compact constructor defaults it (`BridgedConfig.java:293`). The worker's test name `absentProfilePlacementDefaultsToTab` is correct and my reading was not. Checked before raising it. ## Criterion 6 — no fourth instance It swept every `toLowerCase()` in `bridged/src/main/java`, not only `config/`, and classified what it found: enum-to-wire rendering, `AgentStatus.fromWire` (has an explicit `default -> UNKNOWN`, and `UNKNOWN` is not injectable, so it fails safe), pane-text heuristics in `StatusRefiner` and `CompletionResolver`, `isLoopbackBind`, and slug generation. None matches the defect shape. That reads right to me. Closing.
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 1.1 — single-host close-out
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#106
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?