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

fleetd #692: bound the unbounded userinfo mask at redact() line 273 #696

Closed
agent wants to merge 0 commits from worker/692-4afb9d-2 into main
pull from: worker/692-4afb9d-2
merge into: fleet:main
Conversation 1 Commits - Files Changed -
agent commented 2026-10-03 23:27:34 +02:00
Member
Copy Link
Copy Source

Fixes fleetd #692.

redact()'s final fallthrough (scripts/config-edit.sh:273) still used the unbounded [^@]* class that #638 removed from mask_verdict_userinfo(). On a diff line whose value holds a URL with no userinfo plus a later, unrelated @ (e.g. prose or an email address in a non-sensitive YAML value), the unbounded sed silently deleted everything between :// and that later @, leaving a misleading <redacted>-looking result with no indication text was lost.

Fix: extracted the bounded pattern ([^@/[:space:]]*, stopping at /, whitespace, or @) into one shared helper, mask_url_userinfo(), called both by redact() and by mask_verdict_userinfo() (now a thin wrapper). I measured the bound against representative diff-line shapes (a non-sensitive key whose value is a credentialed URI, a value with a URL-plus-later-@, and a line with two credentialed URIs) and confirmed the same bound is correct for YAML diff lines, not just verdict prose — see the three new tests.

Tests (scripts/test-config-edit.sh, run via bash scripts/test-config-edit.sh):

  • test_diff_line_userinfo_is_masked_with_positive_control — leak direction, plus proof the line reached line 273 (not an earlier branch) via surviving prose on both sides.
  • test_diff_line_uri_without_userinfo_survives_a_later_at_sign — over-match direction, byte-for-byte survival.
  • test_diff_line_masks_multiple_userinfo_with_g_flag — two userinfo URIs on one line, both masked.

All 28 test groups in scripts/test-config-edit.sh pass (exit 0).

mvn clean install in fleetd/: Tests run: 1950, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. No Java file touched, and the total did not move.

Also found, not fixed (out of scope): fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java:485 (redactUserInfo) uses a third bound, [^@/]* (Java regex) — stops at / but not whitespace, differing from both bash sites. Worth a follow-up ticket to unify.

Fixes fleetd #692. `redact()`'s final fallthrough (scripts/config-edit.sh:273) still used the unbounded `[^@]*` class that #638 removed from `mask_verdict_userinfo()`. On a diff line whose value holds a URL with no userinfo plus a later, unrelated `@` (e.g. prose or an email address in a non-sensitive YAML value), the unbounded sed silently deleted everything between `://` and that later `@`, leaving a misleading `<redacted>`-looking result with no indication text was lost. **Fix:** extracted the bounded pattern (`[^@/[:space:]]*`, stopping at `/`, whitespace, or `@`) into one shared helper, `mask_url_userinfo()`, called both by `redact()` and by `mask_verdict_userinfo()` (now a thin wrapper). I measured the bound against representative diff-line shapes (a non-sensitive key whose value is a credentialed URI, a value with a URL-plus-later-`@`, and a line with two credentialed URIs) and confirmed the same bound is correct for YAML diff lines, not just verdict prose — see the three new tests. **Tests** (scripts/test-config-edit.sh, run via `bash scripts/test-config-edit.sh`): - `test_diff_line_userinfo_is_masked_with_positive_control` — leak direction, plus proof the line reached line 273 (not an earlier branch) via surviving prose on both sides. - `test_diff_line_uri_without_userinfo_survives_a_later_at_sign` — over-match direction, byte-for-byte survival. - `test_diff_line_masks_multiple_userinfo_with_g_flag` — two userinfo URIs on one line, both masked. All 28 test groups in scripts/test-config-edit.sh pass (exit 0). `mvn clean install` in `fleetd/`: Tests run: 1950, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. No Java file touched, and the total did not move. **Also found, not fixed (out of scope):** `fleetd/src/main/java/dev/ltms/fleet/session/GitWorktrees.java:485` (`redactUserInfo`) uses a third bound, `[^@/]*` (Java regex) — stops at `/` but not whitespace, differing from both bash sites. Worth a follow-up ticket to unify.
agent added 1 commit 2026-10-03 23:27:35 +02:00
fleetd #692: bound the unbounded userinfo mask at redact()'s line 273
CI / shell-tests (pull_request) Failing after 9s
Details
CI / contract (pull_request) Successful in 51s
Details
CI / build (pull_request) Failing after 2m9s
Details
ef4996a01e 
redact()'s final fallthrough still used the unbounded [^@]* class #638
removed from the verdict-line masker, so a diff line with a URL that has
no userinfo plus a later @ elsewhere had its text between them silently
deleted. Share one bounded implementation, mask_url_userinfo(), between
redact() and mask_verdict_userinfo() so the bound lives in one place.
ltms commented 2026-10-03 23:35:21 +02:00
Owner
Copy Link
Copy Source

Merged locally as 736fd9c and pushed to main (edbd8d8 → 736fd9c, confirmed by ref). We merge locally, so this PR does not close itself — closing it by hand.

Verified before merging, by me rather than on the report:

  • Both redaction directions re-run with sed on the merged content. The old [^@]* ate real text between a userinfo-free https:// and a later @; the new bound leaves that line byte for byte and still masks genuine userinfo, twice on one line.
  • bash scripts/test-config-edit.sh: 28 groups, 0 FAIL, exit 0, with the three new #692 groups present by name. Then I mutated the shared pattern so it cannot match and re-ran — exit 1. The tests can see the behaviour; they do not pass vacuously.
  • mvn clean install in a throwaway worktree of the trial merge: 1950 tests, 0 failures, 0 errors, BUILD SUCCESS, counted from the 173 surefire XML files after clearing them first.
  • The merged tree hash equals the tree I built (64cbfd1f), so that build covers exactly what landed.
  • bash -n clean under both macOS system bash 3.2.57 and bash 5.3.9.

Two things I liked about this change, recorded because they are the habits that were missing on #638: the bound was argued from the URI grammar rather than copied from the neighbouring fix, and the pattern was then shared through one helper instead of left as a second copy.

Full evidence and the four-masker sweep are on #692, which is now closed.

Merged locally as `736fd9c` and pushed to `main` (`edbd8d8` → `736fd9c`, confirmed by ref). We merge locally, so this PR does not close itself — closing it by hand. Verified before merging, by me rather than on the report: - Both redaction directions re-run with `sed` on the merged content. The old `[^@]*` ate real text between a userinfo-free `https://` and a later `@`; the new bound leaves that line byte for byte and still masks genuine userinfo, twice on one line. - `bash scripts/test-config-edit.sh`: 28 groups, 0 `FAIL`, exit 0, with the three new `#692` groups present by name. Then I mutated the shared pattern so it cannot match and re-ran — **exit 1**. The tests can see the behaviour; they do not pass vacuously. - `mvn clean install` in a throwaway worktree of the trial merge: 1950 tests, 0 failures, 0 errors, BUILD SUCCESS, counted from the 173 surefire XML files after clearing them first. - The merged tree hash equals the tree I built (`64cbfd1f`), so that build covers exactly what landed. - `bash -n` clean under both macOS system bash 3.2.57 and bash 5.3.9. Two things I liked about this change, recorded because they are the habits that were missing on #638: the bound was argued from the URI grammar rather than copied from the neighbouring fix, and the pattern was then shared through one helper instead of left as a second copy. Full evidence and the four-masker sweep are on #692, which is now closed.
ltms closed this pull request 2026-10-03 23:35:25 +02:00
Some checks are pending
CI / shell-tests (pull_request) Failing after 9s
Details
CI / contract (pull_request) Successful in 51s
Details
CI / build (pull_request) Failing after 2m9s
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#696
Delete Branch "worker/692-4afb9d-2"

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?