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

redact() carries the same unbounded userinfo class that #638 fixed in the verdict line #692

New Issue
Closed
opened 2026-10-03 22:41:36 +02:00 by ltms · 1 comment
ltms commented 2026-10-03 22:41:36 +02:00
Owner
Copy Link
Copy Source

Found by the #638 fix worker, reported and deliberately not fixed because it sat outside its scope. I confirmed it myself on refs/pull/690/head before filing.

The defect

scripts/config-edit.sh:273, inside redact():

printf '%s\n' "$line" | sed -E 's#://[^@]*@#://<redacted>@#g'

[^@]* is unbounded. It crosses /, whitespace and clause boundaries, so it does not stop at the end of a URI. That is the identical shape #638 just removed from the verdict-line masker, which is now s#://[^@/[:space:]]*@#://<redacted>@#g at config-edit.sh:586 on main at 7dec74f.

Why it matters, measured

I ran six line shapes through the old class. The two that matter for this path:

input result under [^@]*
see https://docs.local/guide and mail ops@example.com see https://<redacted>@example.com
amqp://u:p@h1/ and amqp://h2/x then ops@example.com amqp://<redacted>@h1/ and amqp://<redacted>@example.com

So this is not over-redaction of a secret. It silently deletes real text between a :// and a later @ anywhere on the line. The operator reading the output cannot tell that anything was removed, because <redacted> looks like a deliberate mask sitting where their content used to be.

This is the same failure direction that made #638 hard to see: a masker has two failure directions, and a leak test passes while the over-match destroys good text.

Why it is lower severity than #638 was

redact() runs on diff output, not on the daemon's verdict line. Two things reduce the blast radius, and I checked both in the code:

  1. The lines reaching that sed have already passed a key-name filter at config-edit.sh:265-272. A line whose key matches TOKEN|SECRET|PASSWORD|PASSWD|PASSPHRASE|CREDENTIAL|URI|_KEY is replaced wholesale with <redacted> and continues, so it never reaches the sed at all.
  2. What does reach it is a YAML diff line, which is usually one key: value pair rather than prose with two URIs in it.

So the realistic damage is a diff line that holds a URL plus a later @, printed with its middle removed. Annoying and misleading rather than a disclosure.

What I have not measured

I did not construct a real --set run whose diff output contains a line that triggers this. I read the filter above and reasoned about which lines survive it; I did not enumerate them from a live diff. So "usually one key-value pair" is my reading of the code, not a measurement of real output.

I also did not check whether the narrowed class from #638 is the right fix here. A diff line is a different shape from a prose verdict line, and the correct bound may differ. Whoever takes this should decide that rather than copying [^@/[:space:]]* on the assumption it transfers.

Suggested direction

Share one masker instead of having two. The project rule is one fact in one place, and there are now two userinfo-masking regexes in this file that have already drifted once. If the correct bound turns out to be the same, mask_verdict_userinfo and this line should become one function. If the bound genuinely differs, say why at the definition so the next reader does not "unify" them.

Needs a test in each direction: a real userinfo is masked, and a userinfo-free line with a later @ survives byte for byte. A test covering only the first direction is what let the original pass review.

Found by the #638 fix worker, reported and deliberately not fixed because it sat outside its scope. I confirmed it myself on `refs/pull/690/head` before filing. ## The defect `scripts/config-edit.sh:273`, inside `redact()`: ```bash printf '%s\n' "$line" | sed -E 's#://[^@]*@#://<redacted>@#g' ``` `[^@]*` is unbounded. It crosses `/`, whitespace and clause boundaries, so it does not stop at the end of a URI. That is the identical shape #638 just removed from the verdict-line masker, which is now `s#://[^@/[:space:]]*@#://<redacted>@#g` at `config-edit.sh:586` on `main` at `7dec74f`. ## Why it matters, measured I ran six line shapes through the old class. The two that matter for this path: | input | result under `[^@]*` | |---|---| | `see https://docs.local/guide and mail ops@example.com` | `see https://<redacted>@example.com` | | `amqp://u:p@h1/ and amqp://h2/x then ops@example.com` | `amqp://<redacted>@h1/ and amqp://<redacted>@example.com` | So this is **not** over-redaction of a secret. It silently deletes real text between a `://` and a later `@` anywhere on the line. The operator reading the output cannot tell that anything was removed, because `<redacted>` looks like a deliberate mask sitting where their content used to be. This is the same failure direction that made #638 hard to see: a masker has two failure directions, and a leak test passes while the over-match destroys good text. ## Why it is lower severity than #638 was `redact()` runs on **diff output**, not on the daemon's verdict line. Two things reduce the blast radius, and I checked both in the code: 1. The lines reaching that `sed` have already passed a key-name filter at `config-edit.sh:265-272`. A line whose key matches `TOKEN|SECRET|PASSWORD|PASSWD|PASSPHRASE|CREDENTIAL|URI|_KEY` is replaced wholesale with `<redacted>` and `continue`s, so it never reaches the `sed` at all. 2. What does reach it is a YAML diff line, which is usually one `key: value` pair rather than prose with two URIs in it. So the realistic damage is a diff line that holds a URL plus a later `@`, printed with its middle removed. Annoying and misleading rather than a disclosure. ## What I have not measured I did not construct a real `--set` run whose diff output contains a line that triggers this. I read the filter above and reasoned about which lines survive it; I did not enumerate them from a live diff. So "usually one key-value pair" is my reading of the code, not a measurement of real output. I also did not check whether the narrowed class from #638 is the right fix *here*. A diff line is a different shape from a prose verdict line, and the correct bound may differ. Whoever takes this should decide that rather than copying `[^@/[:space:]]*` on the assumption it transfers. ## Suggested direction Share one masker instead of having two. The project rule is one fact in one place, and there are now two userinfo-masking regexes in this file that have already drifted once. If the correct bound turns out to be the same, `mask_verdict_userinfo` and this line should become one function. If the bound genuinely differs, say why at the definition so the next reader does not "unify" them. Needs a test in each direction: a real userinfo is masked, **and** a userinfo-free line with a later `@` survives byte for byte. A test covering only the first direction is what let the original pass review.
ltms commented 2026-10-03 23:34:58 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged as 736fd9c

PR #696 merged locally and pushed; main went edbd8d8 → 736fd9c, confirmed by ref (git ls-remote origin refs/heads/main equals local HEAD).

redact()'s final line no longer carries its own regex. Both maskers now call one shared helper, mask_url_userinfo(), holding the single bound [^@/[:space:]]*.

The chosen bound, and why the "it may not transfer" caveat resolved to "it does"

This ticket warned against copying #638's pattern on the assumption it transfers, because redact() takes a YAML diff line while mask_verdict_userinfo() takes a prose verdict line. The implementer engaged with that and concluded the same bound is correct, with a reason that does not depend on the input being prose: a URI's userinfo component cannot legally contain /, whitespace, or @, so the stop set is a property of the URI grammar, not of the surrounding text. The YAML-vs-prose distinction changes what else is on the line; it does not change what a userinfo can hold.

Because the answer was "same bound", the pattern was shared through one helper rather than duplicated. Two copies of one literal is what caused this ticket.

Verified by the lead, not taken on report

I re-ran all three directions myself with sed, on main's merged content:

input old [^@]* new bound
note2: see https://docs.local/guide and mail ops@example.com note2: see https://<redacted>@example.com — ate the good text between unchanged, byte for byte
note: see amqp://alice:wonderland@rabbit.local:5672/vhost for details — amqp://<redacted>@rabbit.local:5672/vhost
n3: amqp://u1:p1@host1/v1 and amqp://u2:p2@host2/v2 — both masked (g flag holds)

So the over-match defect was real and reachable, and the fix closes it without regressing the leak direction.

The tests are not vacuous, and I proved that rather than assuming it. bash scripts/test-config-edit.sh reports 28 groups, 0 FAIL, exit 0, and the three new #692 groups appear by name in the output. Then I mutated the shared pattern to something that cannot match and re-ran: exit 1. A runtime kill, which is the only kind that is behavioural evidence. Restored afterwards.

Both failure directions now have their own test, which is the thing #638 lacked: two tests and a reviewer there drove only the leak side, and that is how the greedy pattern survived review.

Build

mvn clean install in a throwaway worktree of the trial merge: 1950 tests, 0 failures, 0 errors, 0 skipped, BUILD SUCCESS. I counted that from the 173 surefire XML files directly, after rm -rf target/surefire-reports, rather than reading a log line — and the 173 report-file count is the control that the count saw a full run.

The merged tree hash equals the tree I built (64cbfd1f), so that build covers exactly what landed.

The total did not move, as expected: grep -rl "config-edit" fleetd/src/test/java fleetd/src/main/java is empty. I paired that zero with a positive control — the same grep against scripts/ returns both script files — so the zero is a real absence, not a broken pattern reading as clean.

Tested under both /bin/bash 3.2.57 (macOS system) and bash 5.3.9; bash -n clean on both. The change is printf/function-call/sed only, with nothing version-dependent.

The sweep: four maskers, three bounds, and all three are correct

The brief asked for other userinfo maskers, reported not fixed. The implementer found three. My own wider sweep found four, and the extra one matters for anyone tempted to unify them:

site bound input it receives correct?
scripts/config-edit.sh:224 (shared helper) [^@/[:space:]]* a line of text yes, as of this fix
fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java:485 redactUserInfo [^@/]* a bare URL yes
fleetd/src/main/java/dev/ltms/fleet/Fleetd.java:1411 stripCredentials [^@/]* a bare AMQP URI yes

The two Java sites take a single URI, never a line of prose, and a URI cannot contain whitespace — so the narrower bound is right there, and widening it would be a change with no defect behind it. The tempting conclusion was "three bounds, unify them"; the measured conclusion is "three bounds, each correct for its own input". Recording that so it is not re-litigated.

One cosmetic observation, deliberately not filed as a defect: Fleetd.stripCredentials replaces the userinfo with :// and no marker, so its log line cannot tell "credentials were present and removed" from "there were none". That is the conflated-sentinel shape this project tracks elsewhere, but it is a log message with no leak, and no caller decision rests on it.

Nothing further is owed here. Closing.

## Fixed and merged as `736fd9c` PR #696 merged locally and pushed; `main` went `edbd8d8` → `736fd9c`, confirmed by ref (`git ls-remote origin refs/heads/main` equals local `HEAD`). `redact()`'s final line no longer carries its own regex. Both maskers now call one shared helper, `mask_url_userinfo()`, holding the single bound `[^@/[:space:]]*`. ## The chosen bound, and why the "it may not transfer" caveat resolved to "it does" This ticket warned against copying #638's pattern on the assumption it transfers, because `redact()` takes a YAML diff line while `mask_verdict_userinfo()` takes a prose verdict line. The implementer engaged with that and concluded the same bound is correct, with a reason that does not depend on the input being prose: **a URI's userinfo component cannot legally contain `/`, whitespace, or `@`**, so the stop set is a property of the URI grammar, not of the surrounding text. The YAML-vs-prose distinction changes what else is on the line; it does not change what a userinfo can hold. Because the answer was "same bound", the pattern was shared through one helper rather than duplicated. Two copies of one literal is what caused this ticket. ## Verified by the lead, not taken on report I re-ran all three directions myself with `sed`, on `main`'s merged content: | input | old `[^@]*` | new bound | |---|---|---| | `note2: see https://docs.local/guide and mail ops@example.com` | `note2: see https://<redacted>@example.com` — ate the good text between | **unchanged, byte for byte** | | `note: see amqp://alice:wonderland@rabbit.local:5672/vhost for details` | — | `amqp://<redacted>@rabbit.local:5672/vhost` | | `n3: amqp://u1:p1@host1/v1 and amqp://u2:p2@host2/v2` | — | both masked (`g` flag holds) | So the over-match defect was real and reachable, and the fix closes it without regressing the leak direction. **The tests are not vacuous, and I proved that rather than assuming it.** `bash scripts/test-config-edit.sh` reports 28 groups, 0 `FAIL`, exit 0, and the three new `#692` groups appear by name in the output. Then I mutated the shared pattern to something that cannot match and re-ran: **exit 1**. A runtime kill, which is the only kind that is behavioural evidence. Restored afterwards. Both failure directions now have their own test, which is the thing #638 lacked: two tests and a reviewer there drove only the leak side, and that is how the greedy pattern survived review. ## Build `mvn clean install` in a throwaway worktree of the trial merge: **1950 tests, 0 failures, 0 errors, 0 skipped, BUILD SUCCESS**. I counted that from the 173 surefire XML files directly, after `rm -rf target/surefire-reports`, rather than reading a log line — and the 173 report-file count is the control that the count saw a full run. The merged tree hash equals the tree I built (`64cbfd1f`), so that build covers exactly what landed. The total did not move, as expected: `grep -rl "config-edit" fleetd/src/test/java fleetd/src/main/java` is empty. I paired that zero with a positive control — the same grep against `scripts/` returns both script files — so the zero is a real absence, not a broken pattern reading as clean. Tested under both `/bin/bash` 3.2.57 (macOS system) and bash 5.3.9; `bash -n` clean on both. The change is `printf`/function-call/`sed` only, with nothing version-dependent. ## The sweep: four maskers, three bounds, and all three are correct The brief asked for other userinfo maskers, reported not fixed. The implementer found three. **My own wider sweep found four**, and the extra one matters for anyone tempted to unify them: | site | bound | input it receives | correct? | |---|---|---|---| | `scripts/config-edit.sh:224` (shared helper) | `[^@/[:space:]]*` | a **line of text** | yes, as of this fix | | `fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java:485` `redactUserInfo` | `[^@/]*` | a bare URL | yes | | `fleetd/src/main/java/dev/ltms/fleet/Fleetd.java:1411` `stripCredentials` | `[^@/]*` | a bare AMQP URI | yes | The two Java sites take a single URI, never a line of prose, and a URI cannot contain whitespace — so the narrower bound is right there, and widening it would be a change with no defect behind it. **The tempting conclusion was "three bounds, unify them"; the measured conclusion is "three bounds, each correct for its own input".** Recording that so it is not re-litigated. One cosmetic observation, deliberately **not** filed as a defect: `Fleetd.stripCredentials` replaces the userinfo with `://` and no marker, so its log line cannot tell "credentials were present and removed" from "there were none". That is the conflated-sentinel shape this project tracks elsewhere, but it is a log message with no leak, and no caller decision rests on it. Nothing further is owed here. Closing.
ltms closed this issue 2026-10-03 23:35:02 +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#692
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?