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

fleetd #638: stop the verdict userinfo mask from crossing / or whitespace #690

Closed
agent wants to merge 0 commits from worker/638-fix-overmask-dbb1bf-11 into main
pull from: worker/638-fix-overmask-dbb1bf-11
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-10-03 22:34:44 +02:00
Member
Copy Link
Copy Source

Supersedes #685 (closing that one as superseded, per the lead's comment 18259).

Builds on PR #685's mask_verdict_userinfo() helper and its six wired print sites, which are correct and kept as-is. Fixes the one remaining defect the lead found in review (ticket comment 18259).

The defect

The pattern was s#://[^@]*@#://<redacted>@#g. [^@]* matches anything that is not @, including spaces and whole words, so the match does not stop at the end of the URI. A verdict line holding a URI with no userinfo, followed later in the same line by an unrelated @ (e.g. an email address in diagnostic prose), had everything between the :// and that later @ destroyed.

The fix

Restrict the character class so it cannot cross a / or whitespace, neither can appear in userinfo:

sed -E 's#://[^@/[:space:]]*@#://<redacted>@#g'

Verified against the five shapes from the lead's comment with a direct sed check: all five produce the expected result (same outcomes as the lead's table).

Test

Added test_uri_without_userinfo_survives_a_later_at_sign to scripts/test-config-edit.sh, using the real refusal shape (scan_region_for_verdict requires the text refused, keeping the running config), with a line matching the lead's measured example:

config reload from <dir>/fleetd.yaml refused, keeping the running config: broker.uri amqp://broker.local/vhost unreachable, contact ops@example.com

RED before the pattern change (bash scripts/test-config-edit.sh, exit 1):

== fleetd #638: a URI with no userinfo survives a later @ in the same line ==
FAIL: a URI with no userinfo plus a later @ in the same line must pass through byte for byte: missing [config reload from .../fleetd.yaml refused, keeping the running config: broker.uri amqp://broker.local/vhost unreachable, contact ops@example.com]

GREEN after the pattern change (bash scripts/test-config-edit.sh, exit 0):

== fleetd #638: a URI with no userinfo survives a later @ in the same line ==
...
PASS: config-edit acceptance criteria

Test count: 24 (from #685) -> 25. Full suite passes, exit 0, both runs.

Comment cleanup

Removed the helper's "Not routed through redact(): ..." sentence per the lead's request, that is a design argument for this description, not a code comment. Kept the first sentence, which states what the helper does.

Also found, not fixed (out of this unit's scope)

redact() itself, at scripts/config-edit.sh:273, has the identical unbounded-class shape: sed -E 's#://[^@]*@#://<redacted>@#g' on diff output. Same bug family as the one fixed here, flagging for a follow-up, not fixing it in this PR.

Scope

scripts/config-edit.sh and scripts/test-config-edit.sh only. No Java touched, no mvn run, no daemon contacted, the test suite only ever runs the real script against throwaway fixtures in a temp directory.

Supersedes #685 (closing that one as superseded, per the lead's comment 18259). Builds on PR #685's `mask_verdict_userinfo()` helper and its six wired print sites, which are correct and kept as-is. Fixes the one remaining defect the lead found in review (ticket comment 18259). ## The defect The pattern was `s#://[^@]*@#://<redacted>@#g`. `[^@]*` matches anything that is not `@`, including spaces and whole words, so the match does not stop at the end of the URI. A verdict line holding a URI with no userinfo, followed later in the same line by an unrelated `@` (e.g. an email address in diagnostic prose), had everything between the `://` and that later `@` destroyed. ## The fix Restrict the character class so it cannot cross a `/` or whitespace, neither can appear in userinfo: ``` sed -E 's#://[^@/[:space:]]*@#://<redacted>@#g' ``` Verified against the five shapes from the lead's comment with a direct sed check: all five produce the expected result (same outcomes as the lead's table). ## Test Added `test_uri_without_userinfo_survives_a_later_at_sign` to `scripts/test-config-edit.sh`, using the real refusal shape (`scan_region_for_verdict` requires the text `refused, keeping the running config`), with a line matching the lead's measured example: ``` config reload from <dir>/fleetd.yaml refused, keeping the running config: broker.uri amqp://broker.local/vhost unreachable, contact ops@example.com ``` RED before the pattern change (`bash scripts/test-config-edit.sh`, exit 1): ``` == fleetd #638: a URI with no userinfo survives a later @ in the same line == FAIL: a URI with no userinfo plus a later @ in the same line must pass through byte for byte: missing [config reload from .../fleetd.yaml refused, keeping the running config: broker.uri amqp://broker.local/vhost unreachable, contact ops@example.com] ``` GREEN after the pattern change (`bash scripts/test-config-edit.sh`, exit 0): ``` == fleetd #638: a URI with no userinfo survives a later @ in the same line == ... PASS: config-edit acceptance criteria ``` Test count: 24 (from #685) -> 25. Full suite passes, exit 0, both runs. ## Comment cleanup Removed the helper's "Not routed through `redact()`: ..." sentence per the lead's request, that is a design argument for this description, not a code comment. Kept the first sentence, which states what the helper does. ## Also found, not fixed (out of this unit's scope) `redact()` itself, at `scripts/config-edit.sh:273`, has the identical unbounded-class shape: `sed -E 's#://[^@]*@#://<redacted>@#g'` on diff output. Same bug family as the one fixed here, flagging for a follow-up, not fixing it in this PR. ## Scope `scripts/config-edit.sh` and `scripts/test-config-edit.sh` only. No Java touched, no `mvn` run, no daemon contacted, the test suite only ever runs the real script against throwaway fixtures in a temp directory.
agent added 3 commits 2026-10-03 22:34:44 +02:00
fleetd #638: mask userinfo in the daemon verdict line before it prints
CI / shell-tests (pull_request) Failing after 7s
Details
CI / contract (pull_request) Successful in 1m7s
Details
CI / build (pull_request) Failing after 2m4s
Details
28b45d97e5 
Apply the userinfo-only rewrite (sed -E 's#://[^@]*@#://<redacted>@#g') to every
path that prints config-edit.sh's $VERDICT_LINE: restore_and_confirm's two
prints, report_outcome's shared local copy (covering its clean/needs-restart/
refused branches), and check_mode's "last verdict in log" line via
last_verdict_line. Deliberately not routed through redact() — that function's
key:value masking does not match this line's prose, and the rest of the line
(e.g. the pattern quoted in a parse-failure refusal) is the detail an operator
needs to fix the refusal.

No current refusal message echoes a URI, token or password, so this is a guard
against a future validator doing so, not a fix for an observed leak.
Merge remote-tracking branch 'refs/remotes/pr/685' into worker/638-fix-overmask-dbb1bf-11 724b35b46e
fleetd #638: stop the verdict userinfo mask from crossing / or whitespace
CI / shell-tests (pull_request) Failing after 10s
Details
CI / contract (pull_request) Successful in 45s
Details
CI / build (pull_request) Failing after 1m57s
Details
5f5d16fbd4 
mask_verdict_userinfo's character class [^@]* crossed a '/' or a space, so a
verdict line with a URI that has no userinfo plus a later @ (e.g. an email
address in diagnostic prose) had everything between them destroyed. Restrict
the class to [^@/[:space:]]* so the match stops at the end of the URI.

Adds the uncovered-direction test: a URI with no userinfo plus a later @ in
the same line must pass through byte for byte. Also drops the design-rationale
sentence from the helper's comment (now in the PR description).
ltms commented 2026-10-03 22:55:11 +02:00
Owner
Copy Link
Copy Source

Merged locally as 7dec74f, pushed. Closing by hand.

We merge locally, so Gitea never closes a PR by itself. main went 2eb2d61..7dec74f. Closes #638 and supersedes #685, which I closed unmerged.

My verification is on #685 and #638, not repeated here. The short version: I probed the old and new patterns against six line shapes in both directions, mutated the merged pattern back to [^@]* and confirmed exactly one test goes RED (test_uri_without_userinfo_survives_a_later_at_sign), re-ran the suite on merged main at RC=0 with 25 tests, and measured that no Java test reads these scripts so the Java total cannot move.

The one finding this PR correctly left alone — redact() at scripts/config-edit.sh:273 carrying the same unbounded class — is filed as #692.

## Merged locally as `7dec74f`, pushed. Closing by hand. We merge locally, so Gitea never closes a PR by itself. `main` went `2eb2d61..7dec74f`. Closes #638 and supersedes #685, which I closed unmerged. **My verification is on #685 and #638**, not repeated here. The short version: I probed the old and new patterns against six line shapes in both directions, mutated the merged pattern back to `[^@]*` and confirmed exactly one test goes RED (`test_uri_without_userinfo_survives_a_later_at_sign`), re-ran the suite on merged `main` at `RC=0` with 25 tests, and measured that no Java test reads these scripts so the Java total cannot move. The one finding this PR correctly left alone — `redact()` at `scripts/config-edit.sh:273` carrying the same unbounded class — is filed as **#692**.
ltms closed this pull request 2026-10-03 22:55:14 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 10s
Details
CI / contract (pull_request) Successful in 45s
Details
CI / build (pull_request) Failing after 1m57s
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#690
Delete Branch "worker/638-fix-overmask-dbb1bf-11"

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?